fix(auth): avoid out-of-bounds read in auth_nvctr()

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 <demiobenour@gmail.com>

diff --git a/drivers/auth/auth_mod.c b/drivers/auth/auth_mod.c
index fa9509a0c19336b1fbc1df3a04ca923eb9a90d30..1bf03d40925b44da04ca1d18e41c613c1b59ed1a 100644 (file)
--- 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++;