diff --git a/ChangeLog.d/fix-oid-to-string-bugs.txt b/ChangeLog.d/fix-oid-to-string-bugs.txt index 799f444747ea..3cf02c39c3ee 100644 --- a/ChangeLog.d/fix-oid-to-string-bugs.txt +++ b/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. diff --git a/library/oid.c b/library/oid.c index 4ec752fb9ccc..12a96503bd07 100644 --- a/library/oid.c +++ b/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); } diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data index 38d8b7e1c6fe..2d331418b0df 100644 --- a/tests/suites/test_suite_oid.data +++ b/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:"" diff --git a/tests/suites/test_suite_oid.function b/tests/suites/test_suite_oid.function index 7759baf363bf..c06e3373e3a5 100644 --- a/tests/suites/test_suite_oid.function +++ b/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); }