From abb8f936fd0ad085b1966bdc2cddf040ba3865e3 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 9 Dec 2022 18:21:47 -0500 Subject: [PATCH] fix(auth): avoid out-of-bounds read in auth_nvctr() MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit auth_nvctr() does not check that the buffer provided is long enough to hold an ASN.1 INTEGER, or even that the buffer is non-empty. Since auth_nvctr() will only ever read 6 bytes, it is possible to read up to 6 bytes past the end of the buffer. This out-of-bounds read turns out to be harmless. The only caller of auth_nvctr() always passes a pointer into an X.509 TBSCertificate, and all in-tree chains of trust require that the certificate’s signature has already been validated. This means that the signature algorithm identifier is at least 4 bytes and the signature itself more than that. Therefore, the data read will be from the certificate itself. Even if the certificate signature has not been validated, an out-of-bounds read is still not possible. Since there are at least two bytes (tag and length) in both the signature algorithm ID and the signature itself, an out-of-bounds read would require that the tag byte of the signature algorithm ID would need to be either the tag or length byte of the DER-encoded nonvolatile counter. However, this byte must be (MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE) (0x30), which is greater than 4 and not equal to MBEDTLS_ASN1_INTEGER (2). Therefore, auth_nvctr() will error out before reading the integer itself, preventing an out-of-bounds read. Change-Id: Ibdf1af702fbeb98a94c0c96456ebddd3d392ad44 Signed-off-by: Demi Marie Obenour --- drivers/auth/auth_mod.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/auth/auth_mod.c b/drivers/auth/auth_mod.c index fa9509a0c..1bf03d409 100644 --- a/drivers/auth/auth_mod.c +++ b/drivers/auth/auth_mod.c @@ -243,7 +243,7 @@ static int auth_nvctr(const auth_method_param_nv_ctr_t *param, unsigned int *cert_nv_ctr, bool *need_nv_ctr_upgrade) { - char *p; + unsigned char *p; void *data_ptr = NULL; unsigned int data_len, len, i; unsigned int plat_nv_ctr; @@ -258,16 +258,24 @@ static int auth_nvctr(const auth_method_param_nv_ctr_t *param, /* Parse the DER encoded integer */ assert(data_ptr); - p = (char *)data_ptr; - if (*p != ASN1_INTEGER) { + p = (unsigned char *)data_ptr; + + /* + * Integers must be at least 3 bytes: 1 for tag, 1 for length, and 1 + * for value. The first byte (tag) must be ASN1_INTEGER. + */ + if ((data_len < 3) || (*p != ASN1_INTEGER)) { /* Invalid ASN.1 integer */ return 1; } p++; - /* NV-counters are unsigned integers up to 32-bit */ - len = (unsigned int)(*p & 0x7f); - if ((*p & 0x80) || (len > 4)) { + /* + * NV-counters are unsigned integers up to 31 bits. Trailing + * padding is not allowed. + */ + len = (unsigned int)*p; + if ((len > 4) || (data_len - 2 != len)) { return 1; } p++; -- 2.39.5