From 889534a4d27900f8b51c554e051496f45a7c76c5 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Mar 2023 17:45:28 -0500 Subject: [PATCH] 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 --- ChangeLog.d/fix-oid-to-string-bugs.txt | 6 +- library/oid.c | 87 +++++++++++--------------- tests/suites/test_suite_oid.data | 18 ++++++ tests/suites/test_suite_oid.function | 7 ++- 4 files changed, 65 insertions(+), 53 deletions(-) diff --git a/ChangeLog.d/fix-oid-to-string-bugs.txt b/ChangeLog.d/fix-oid-to-string-bugs.txt index 799f444747..3cf02c39c3 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 86214b23a0..63b3df3880 100644 --- a/library/oid.c +++ b/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; - unsigned int value; - char *p; + char *p = buf; + size_t n = size; + unsigned int value = 0; - 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; + if (size > INT_MAX) { + /* Avoid overflow computing return value */ + return MBEDTLS_ERR_ASN1_INVALID_LENGTH; } - while (i < oid->len && ((oid->p[i] & 0x80) != 0)) { - /* Prevent overflow in value. */ - if (value > (UINT_MAX >> 7)) { - return MBEDTLS_ERR_ASN1_INVALID_DATA; - } - - value |= oid->p[i] & 0x7F; - value <<= 7; - i++; - } - 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; @@ -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); - 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 b9fa6543d9..75213e97b5 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 3004b65fe1..5fbc9b5223 100644 --- a/tests/suites/test_suite_oid.function +++ b/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); }