Merge pull request #7303 from daverodgman/msan_bzero_testcase
Add tests that cover msan explicit_bzero issue
diff --git a/ChangeLog.d/fix-oid-to-string-bugs.txt b/ChangeLog.d/fix-oid-to-string-bugs.txt
index 799f444..3cf02c3 100644
--- a/ChangeLog.d/fix-oid-to-string-bugs.txt
+++ b/ChangeLog.d/fix-oid-to-string-bugs.txt
@@ -3,4 +3,8 @@
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 86214b2..63b3df3 100644
--- a/library/oid.c
+++ b/library/oid.c
@@ -813,65 +813,26 @@
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 @@
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/library/platform_util.c b/library/platform_util.c
index 6d4759c..f891cd4 100644
--- a/library/platform_util.c
+++ b/library/platform_util.c
@@ -57,6 +57,15 @@
#endif
#if !defined(MBEDTLS_PLATFORM_ZEROIZE_ALT)
+
+#undef HAVE_MEMORY_SANITIZER
+#if defined(__has_feature)
+#if __has_feature(memory_sanitizer)
+#include <sanitizer/msan_interface.h>
+#define HAVE_MEMORY_SANITIZER
+#endif
+#endif
+
/*
* Where possible, we try to detect the presence of a platform-provided
* secure memset, such as explicit_bzero(), that is safe against being optimized
@@ -100,6 +109,15 @@
if (len > 0) {
#if defined(MBEDTLS_PLATFORM_HAS_EXPLICIT_BZERO)
explicit_bzero(buf, len);
+#if defined(HAVE_MEMORY_SANITIZER)
+ /* You'd think that Msan would recognize explicit_bzero() as
+ * equivalent to bzero(), but it actually doesn't on several
+ * platforms, including Linux (Ubuntu 20.04).
+ * https://github.com/google/sanitizers/issues/1507
+ * https://github.com/openssh/openssh-portable/commit/74433a19bb6f4cef607680fa4d1d7d81ca3826aa
+ */
+ __msan_unpoison(buf, len);
+#endif
#elif defined(__STDC_LIB_EXT1__)
memset_s(buf, len, 0, len);
#elif defined(_WIN32)
diff --git a/tests/suites/test_suite_oid.data b/tests/suites/test_suite_oid.data
index b9fa654..75213e9 100644
--- a/tests/suites/test_suite_oid.data
+++ b/tests/suites/test_suite_oid.data
@@ -101,12 +101,30 @@
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 3004b65..5fbc9b5 100644
--- a/tests/suites/test_suite_oid.function
+++ b/tests/suites/test_suite_oid.function
@@ -105,13 +105,16 @@
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);
}