Fix segfault in mbedtls_oid_get_numeric_string

When passed an empty OID, mbedtls_oid_get_numeric_string would read one
byte from the zero-sized buffer and return an error code that depends on
its value.  This is demonstrated by the test suite changes, which
check that an OID with length zero and an invalid buffer pointer does
not cause Mbed TLS to segfault.

Also check that second and subsequent subidentifiers are terminated, and
add a test case for that.  Furthermore, stop relying on integer division
by 40, use the same loop for both the first and subsequent
subidentifiers, and add additional tests.

Signed-off-by: Demi Marie Obenour <[email protected]>

ChangeLog.d/oid-oob-read.txt

@@ -0,0 +1,14 @@
+Security:
+    * Fix a 1-byte out-of-bounds read in mbedtls_oid_get_numeric_string() when
+      called on the empty input.  The only information returned to the caller
+      is whether byte 1 (of the 0-length OID) is 0x80 or not.  This bug
+      appeared in the development and 2.28 branches, but was caught by Demi
+      Marie Obenour before it appeared in git master or in any release tags, so
+      no CVE has been assigned.
+
+Bugfix:
+    * If mbedtls_oid_get_numeric_string() is called with a length greater than
+      INT_MAX, fail instead of returning an incorrect length.
+    * Fail (with MBEDTLS_ERR_ASN1_OUT_OF_DATA) if the second or subsequent
+      subidentifier in an OID passed to mbedtls_oid_get_numeric_string() is not
+      terminated.

library/oid.c

@@ -775,65 +775,26 @@ FN_OID_GET_ATTR2(mbedtls_oid_get_pkcs12_pbe_alg,
                  cipher_alg)
 #endif /* MBEDTLS_PKCS12_C */
 
-#define OID_SAFE_SNPRINTF                               \
-    do {                                                \
-        if (ret < 0 || (size_t) ret >= n)              \
-        return MBEDTLS_ERR_OID_BUF_TOO_SMALL;    \
-                                                      \
-        n -= (size_t) ret;                              \
-        p += (size_t) ret;                              \
-    } while (0)
-
 /* Return the x.y.z.... style numeric string for the given OID */
 int mbedtls_oid_get_numeric_string(char *buf, size_t size,
                                    const mbedtls_asn1_buf *oid)
 {
     int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
-    size_t i, n;
-    unsigned int value;
-    char *p;
-
-    p = buf;
-    n = size;
-
-    /* First subidentifier contains first two OID components */
-    i = 0;
-    value = 0;
-    if ((oid->p[0]) == 0x80) {
-        /* Overlong encoding is not allowed */
-        return MBEDTLS_ERR_ASN1_INVALID_DATA;
-    }
-
-    while (i < oid->len && ((oid->p[i] & 0x80) != 0)) {
-        /* Prevent overflow in value. */
-        if (value > (UINT_MAX >> 7)) {
-            return MBEDTLS_ERR_ASN1_INVALID_DATA;
-        }
+    char *p = buf;
+    size_t n = size;
+    unsigned int value = 0;
 
-        value |= oid->p[i] & 0x7F;
-        value <<= 7;
-        i++;
+    if (size > INT_MAX) {
+        /* Avoid overflow computing return value */
+        return MBEDTLS_ERR_ASN1_INVALID_LENGTH;
     }
-    if (i >= oid->len) {
+
+    if (oid->len <= 0) {
+        /* OID must not be empty */
         return MBEDTLS_ERR_ASN1_OUT_OF_DATA;
     }
-    /* Last byte of first subidentifier */
-    value |= oid->p[i] & 0x7F;
-    i++;
-
-    unsigned int component1 = value / 40;
-    if (component1 > 2) {
-        /* The first component can only be 0, 1 or 2.
-         * If oid->p[0] / 40 is greater than 2, the leftover belongs to
-         * the second component. */
-        component1 = 2;
-    }
-    unsigned int component2 = value - (40 * component1);
-    ret = mbedtls_snprintf(p, n, "%u.%u", component1, component2);
-    OID_SAFE_SNPRINTF;
 
-    value = 0;
-    for (; i < oid->len; i++) {
+    for (size_t i = 0; i < oid->len; i++) {
         /* Prevent overflow in value. */
         if (value > (UINT_MAX >> 7)) {
             return MBEDTLS_ERR_ASN1_INVALID_DATA;
@@ -848,12 +809,38 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size,
 
         if (!(oid->p[i] & 0x80)) {
             /* Last byte */
-            ret = mbedtls_snprintf(p, n, ".%u", value);
-            OID_SAFE_SNPRINTF;
+            if (n == size) {
+                int component1;
+                unsigned int component2;
+                /* First subidentifier contains first two OID components */
+                if (value >= 80) {
+                    component1 = '2';
+                    component2 = value - 80;
+                } else if (value >= 40) {
+                    component1 = '1';
+                    component2 = value - 40;
+                } else {
+                    component1 = '0';
+                    component2 = value;
+                }
+                ret = mbedtls_snprintf(p, n, "%c.%u", component1, component2);
+            } else {
+                ret = mbedtls_snprintf(p, n, ".%u", value);
+            }
+            if (ret < 2 || (size_t) ret >= n) {
+                return MBEDTLS_ERR_OID_BUF_TOO_SMALL;
+            }
+            n -= (size_t) ret;
+            p += ret;
             value = 0;
         }
     }
 
+    if (value != 0) {
+        /* Unterminated subidentifier */
+        return MBEDTLS_ERR_ASN1_OUT_OF_DATA;
+    }
+
     return (int) (size - n);
 }
 

tests/suites/test_suite_oid.data

@@ -101,12 +101,30 @@ oid_get_numeric_string:"81010000863A00":0:"2.49.0.0.826.0"
 OID get numeric string - multi-byte first subidentifier
 oid_get_numeric_string:"8837":0:"2.999"
 
+OID get numeric string - second subidentifier not terminated
+oid_get_numeric_string:"0081":MBEDTLS_ERR_ASN1_OUT_OF_DATA:""
+
 OID get numeric string - empty oid buffer
 oid_get_numeric_string:"":MBEDTLS_ERR_ASN1_OUT_OF_DATA:""
 
 OID get numeric string - no final / all bytes have top bit set
 oid_get_numeric_string:"818181":MBEDTLS_ERR_ASN1_OUT_OF_DATA:""
 
+OID get numeric string - 0.39
+oid_get_numeric_string:"27":0:"0.39"
+
+OID get numeric string - 1.0
+oid_get_numeric_string:"28":0:"1.0"
+
+OID get numeric string - 1.39
+oid_get_numeric_string:"4f":0:"1.39"
+
+OID get numeric string - 2.0
+oid_get_numeric_string:"50":0:"2.0"
+
+OID get numeric string - 1 byte first subidentifier beyond 2.39
+oid_get_numeric_string:"7f":0:"2.47"
+
 # Encodes the number 0x0400000000 as a subidentifier which overflows 32-bits
 OID get numeric string - 32-bit overflow
 oid_get_numeric_string:"C080808000":MBEDTLS_ERR_ASN1_INVALID_DATA:""

tests/suites/test_suite_oid.function

@@ -104,13 +104,16 @@ void oid_get_numeric_string(data_t *oid, int error_ret, char *result_str)
     int ret;
 
     input_oid.tag = MBEDTLS_ASN1_OID;
-    input_oid.p = oid->x;
+    /* Test that an empty OID is not dereferenced */
+    input_oid.p = oid->len ? oid->x : (void *) 1;
     input_oid.len = oid->len;
 
     ret = mbedtls_oid_get_numeric_string(buf, sizeof(buf), &input_oid);
 
     if (error_ret == 0) {
-        TEST_ASSERT(strcmp(buf, result_str) == 0);
+        TEST_EQUAL(ret, strlen(result_str));
+        TEST_ASSERT(ret >= 3);
+        TEST_EQUAL(strcmp(buf, result_str), 0);
     } else {
         TEST_EQUAL(ret, error_ret);
     }