Merge pull request #7105 from davidhorstmann-arm/fix-oid-printing-bug

Fix bugs in OID to string conversion

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

@@ -0,0 +1,6 @@
+Bugfix
+   * Fix bug in conversion from OID to string in
+     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.

library/oid.c

@@ -834,21 +834,55 @@ int mbedtls_oid_get_numeric_string(char *buf, size_t size,
     p = buf;
     n = size;
 
-    /* First byte contains first two dots */
+    /* First subidentifier contains first two OID components */
-    if (oid->len > 0) {
+    i = 0;
-        ret = mbedtls_snprintf(p, n, "%d.%d", oid->p[0] / 40, oid->p[0] % 40);
+    value = 0;
-        OID_SAFE_SNPRINTF;
+    if ((oid->p[0]) == 0x80) {
+        /* Overlong encoding is not allowed */
+        return MBEDTLS_ERR_ASN1_INVALID_DATA;
     }
 
-    value = 0;
+    while (i < oid->len && ((oid->p[i] & 0x80) != 0)) {
-    for (i = 1; i < oid->len; i++) {
         /* Prevent overflow in value. */
-        if (((value << 7) >> 7) != value) {
+        if (value > (UINT_MAX >> 7)) {
-            return MBEDTLS_ERR_OID_BUF_TOO_SMALL;
+            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;
+    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;
+        }
+        if ((value == 0) && ((oid->p[i]) == 0x80)) {
+            /* Overlong encoding is not allowed */
+            return MBEDTLS_ERR_ASN1_INVALID_DATA;
         }
 
         value <<= 7;
-        value += oid->p[i] & 0x7F;
+        value |= oid->p[i] & 0x7F;
 
         if (!(oid->p[i] & 0x80)) {
             /* Last byte */

tests/suites/test_suite_oid.data

@@ -89,3 +89,33 @@ oid_get_md_alg_id:"2b24030201":MBEDTLS_MD_RIPEMD160
 OID hash id - invalid oid
 oid_get_md_alg_id:"2B864886f70d0204":-1
 
+OID get numeric string - hardware module name
+oid_get_numeric_string:"2B06010505070804":0:"1.3.6.1.5.5.7.8.4"
+
+OID get numeric string - multi-byte subidentifier
+oid_get_numeric_string:"29903C":0:"1.1.2108"
+
+OID get numeric string - second component greater than 39
+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 - 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:""
+
+# 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:""
+
+OID get numeric string - 32-bit overflow, second subidentifier
+oid_get_numeric_string:"2BC080808000":MBEDTLS_ERR_ASN1_INVALID_DATA:""
+
+OID get numeric string - overlong encoding
+oid_get_numeric_string:"8001":MBEDTLS_ERR_ASN1_INVALID_DATA:""
+
+OID get numeric string - overlong encoding, second subidentifier
+oid_get_numeric_string:"2B8001":MBEDTLS_ERR_ASN1_INVALID_DATA:""

tests/suites/test_suite_oid.function

@@ -96,3 +96,24 @@ void oid_get_md_alg_id(data_t *oid, int exp_md_id)
     }
 }
 /* END_CASE */
+
+/* BEGIN_CASE */
+void oid_get_numeric_string(data_t *oid, int error_ret, char *result_str)
+{
+    char buf[256];
+    mbedtls_asn1_buf input_oid = { 0, 0, NULL };
+    int ret;
+
+    input_oid.tag = MBEDTLS_ASN1_OID;
+    input_oid.p = oid->x;
+    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);
+    } else {
+        TEST_EQUAL(ret, error_ret);
+    }
+}
+/* END_CASE */

tests/suites/test_suite_x509parse.data

@@ -2558,7 +2558,7 @@ X509 OID numstring #4 (larger number)
 x509_oid_numstr:"2a864886f70d":"1.2.840.113549":15:14
 
 X509 OID numstring #5 (arithmetic overflow)
-x509_oid_numstr:"2a8648f9f8f7f6f5f4f3f2f1f001":"":100:MBEDTLS_ERR_OID_BUF_TOO_SMALL
+x509_oid_numstr:"2a8648f9f8f7f6f5f4f3f2f1f001":"":100:MBEDTLS_ERR_ASN1_INVALID_DATA
 
 X509 CRT keyUsage #1 (no extension, expected KU)
 depends_on:MBEDTLS_RSA_C:MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA_BASED_ON_USE_PSA