Merge pull request #7270 from DemiMarie/oid-fix

Fix segfault in mbedtls_oid_get_numeric_string

ChangeLog.d/fix-oid-to-string-bugs.txt

@@ -3,4 +3,8 @@ Bugfix
      mbedtls_oid_get_numeric_string(). OIDs such as 2.40.0.25 are now printed
      correctly.
    * Reject OIDs with overlong-encoded subidentifiers when converting
-     OID-to-string.
+     them to a string.
+   * Reject OIDs with subidentifier values exceeding UINT_MAX.  Such
+     subidentifiers can be valid, but Mbed TLS cannot currently handle them.
+   * Reject OIDs that have unterminated subidentifiers, or (equivalently)
+     have the most-significant bit set in their last byte.

library/oid.c

@@ -813,65 +813,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;
+    char *p = buf;
-    unsigned int value;
+    size_t n = size;
-    char *p;
+    unsigned int value = 0;
 
-    p = buf;
+    if (size > INT_MAX) {
-    n = size;
+        /* Avoid overflow computing return value */
-
+        return MBEDTLS_ERR_ASN1_INVALID_LENGTH;
-    /* 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)) {
+    if (oid->len <= 0) {
-        /* Prevent overflow in value. */
+        /* OID must not be empty */
-        if (value > (UINT_MAX >> 7)) {
-            return MBEDTLS_ERR_ASN1_INVALID_DATA;
-        }
-
-        value |= oid->p[i] & 0x7F;
-        value <<= 7;
-        i++;
-    }
-    if (i >= oid->len) {
         return MBEDTLS_ERR_ASN1_OUT_OF_DATA;
     }
-    /* Last byte of first subidentifier */
-    value |= oid->p[i] & 0x7F;
-    i++;
 
-    unsigned int component1 = value / 40;
+    for (size_t i = 0; i < oid->len; i++) {
-    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++) {
         /* Prevent overflow in value. */
         if (value > (UINT_MAX >> 7)) {
             return MBEDTLS_ERR_ASN1_INVALID_DATA;
@@ -886,12 +847,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);
+            if (n == size) {
-            OID_SAFE_SNPRINTF;
+                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

@@ -105,13 +105,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);
     }