From 13f86e689e64cc121b469822ea957d5f61c31900 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 21 May 2025 14:35:42 +0200 Subject: [PATCH 1/5] Add tests for bug in mbedtls_x509_string_to_names() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commented out tests cause crashes (in different ways) until the bug is fixed; the first two test are passing already and are here mostly to provide a reference point. The bug report was using programs/x509/cert_write, but string_to_names() is what it was really targetting, which is better for automated tests. The strings used are a minor adapation of those from the report. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.data | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index 0cbad4bcee..7bd5e74e1f 100644 --- a/tests/suites/test_suite_x509write.data +++ b/tests/suites/test_suite_x509write.data @@ -254,6 +254,27 @@ mbedtls_x509_string_to_names:"C=NL, O=Of\\CCspark, OU=PolarSSL":"C=NL, O=Of\\CCs X509 String to Names #20 (Reject empty AttributeValue) mbedtls_x509_string_to_names:"C=NL, O=, OU=PolarSSL":"":MBEDTLS_ERR_X509_INVALID_NAME:0 +# Note: the behaviour is incorrect, output from string->names->string should be +# the same as the input, rather than just the last component, see +# https://github.com/Mbed-TLS/mbedtls/issues/10189 +# Still including tests for the current incorrect behaviour because of the +# variants below where we want to ensure at least that no memory corruption +# happens (which would be a lot worse than just a functional bug). +X509 String to Names (repeated OID) +mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=ef":"CN=ef":0:0 + +# Note: when a value starts with a # sign, it's treated as the hex encoding of +# the DER encoding of the value. Here, 0400 is a zero-length OCTET STRING. +# The tag actually doesn't matter for our purposes, only the length. +X509 String to Names (repeated OID, 1st is zero-length) +mbedtls_x509_string_to_names:"CN=#0400,CN=cd,CN=ef":"CN=ef":0:0 + +#X509 String to Names (repeated OID, middle is zero-length) +#mbedtls_x509_string_to_names:"CN=ab,CN=#0400,CN=ef":"CN=ef":0:0 + +#X509 String to Names (repeated OID, last is zero-length) +#mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=#0400":"CN=ef":0:0 + X509 Round trip test (Escaped characters) mbedtls_x509_string_to_names:"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":0:0 From 2df7ab7c0c3d5bb8a31481073c494521d10d4eba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 26 May 2025 10:42:14 +0200 Subject: [PATCH 2/5] Fix bug in mbedtls_asn1_store_named_data() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When passed a zero-length val, the function was free-ing the buffer as the documentation suggests: * \param val_len The minimum length of the data buffer needed. * If this is 0, do not allocate a buffer for the associated * data. * If the OID was already present, enlarge, shrink or free * the existing buffer to fit \p val_len. However it kept the previous length, leaving the val structure in the corresponding item in the output list in an inconsistent state: p == NULL but len != 0 As a result, functions that would try using this item in the list (including the same function!) afterwards would trip an dereference the NULL pointer. Signed-off-by: Manuel Pégourié-Gonnard --- library/asn1write.c | 1 + tests/suites/test_suite_x509write.data | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/library/asn1write.c b/library/asn1write.c index 775a9ef530..415357b7b5 100644 --- a/library/asn1write.c +++ b/library/asn1write.c @@ -412,6 +412,7 @@ mbedtls_asn1_named_data *mbedtls_asn1_store_named_data( } else if (val_len == 0) { mbedtls_free(cur->val.p); cur->val.p = NULL; + cur->val.len = 0; } else if (cur->val.len != val_len) { /* * Enlarge existing value buffer if needed diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index 7bd5e74e1f..56af0ce8a8 100644 --- a/tests/suites/test_suite_x509write.data +++ b/tests/suites/test_suite_x509write.data @@ -269,11 +269,11 @@ mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=ef":"CN=ef":0:0 X509 String to Names (repeated OID, 1st is zero-length) mbedtls_x509_string_to_names:"CN=#0400,CN=cd,CN=ef":"CN=ef":0:0 -#X509 String to Names (repeated OID, middle is zero-length) -#mbedtls_x509_string_to_names:"CN=ab,CN=#0400,CN=ef":"CN=ef":0:0 +X509 String to Names (repeated OID, middle is zero-length) +mbedtls_x509_string_to_names:"CN=ab,CN=#0400,CN=ef":"CN=ef":0:0 -#X509 String to Names (repeated OID, last is zero-length) -#mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=#0400":"CN=ef":0:0 +X509 String to Names (repeated OID, last is zero-length) +mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=#0400":"CN=#0000":0:MAY_FAIL_GET_NAME X509 Round trip test (Escaped characters) mbedtls_x509_string_to_names:"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":0:0 From 12df5f3a16a2bc53881b9d88556192dcbc2f6774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 26 May 2025 10:55:59 +0200 Subject: [PATCH 3/5] Improve unit tests for mbedtls_asn1_store_named_data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every time we check found->val.p we should also check found->val.len. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_asn1write.function | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/suites/test_suite_asn1write.function b/tests/suites/test_suite_asn1write.function index f5fc025f7d..b1e66ed901 100644 --- a/tests/suites/test_suite_asn1write.function +++ b/tests/suites/test_suite_asn1write.function @@ -550,6 +550,9 @@ void store_named_data_val_found(int old_len, int new_len) } if (new_len == 0) { TEST_ASSERT(found->val.p == NULL); + /* If new_len != 0, then new_val != NULL and the length has been checked + * above by TEST_MEMORY_COMPARE(). But not here, so we need to check. */ + TEST_EQUAL(found->val.len, 0); } else if (new_len == old_len) { TEST_ASSERT(found->val.p == old_val); } else { @@ -583,8 +586,10 @@ void store_named_data_val_new(int new_len, int set_new_val) TEST_MEMORY_COMPARE(found->oid.p, found->oid.len, oid, oid_len); if (new_len == 0) { TEST_ASSERT(found->val.p == NULL); + TEST_EQUAL(found->val.len, 0); } else if (new_val == NULL) { TEST_ASSERT(found->val.p != NULL); + TEST_EQUAL(found->val.len, new_len); } else { TEST_ASSERT(found->val.p != new_val); TEST_MEMORY_COMPARE(found->val.p, found->val.len, From 04fe95d95b1609b7a28e5665bd86066f58f00484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 26 May 2025 12:38:52 +0200 Subject: [PATCH 4/5] Add ChangeLog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/fix-string-to-names-store-named-data.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 ChangeLog.d/fix-string-to-names-store-named-data.txt diff --git a/ChangeLog.d/fix-string-to-names-store-named-data.txt b/ChangeLog.d/fix-string-to-names-store-named-data.txt new file mode 100644 index 0000000000..422ce07f85 --- /dev/null +++ b/ChangeLog.d/fix-string-to-names-store-named-data.txt @@ -0,0 +1,12 @@ +Security + * Fix a bug in mbedtls_asn1_store_named_data() where it would sometimes leave + an item in the output list in an inconsistent state with val.p == NULL but + val.len > 0. This impacts applications that call this function directly, + or indirectly via mbedtls_x509_string_to_names() or one of the + mbedtls_x509write_{crt,csr}_set_{subject,issuer}_name() functions. The + inconsistent state of the output could then cause a NULL dereference either + inside the same call to mbedtls_x509_string_to_names(), or in subsequent + users of the output structure, such as mbedtls_x509_write_names(). This + only affects applications that create (as opposed to consume) X.509 + certificates, CSRs or CRLS, or that call mbedtls_asn1_store_named_data() + directly. Found by Linh Le and Ngan Nguyen from Calif. From e51bde06daff0ac92f0545ebe4406bda260542e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 3 Jun 2025 11:22:55 +0200 Subject: [PATCH 5/5] Fix possible UB in mbedtls_asn1_write_raw_buffer() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is mostly unrelated to other commits in this PR, except for the fact that one of the added X.509 tests revealed that with UBSan. Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/fix-asn1write-raw-buffer.txt | 5 +++++ library/asn1write.c | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/fix-asn1write-raw-buffer.txt diff --git a/ChangeLog.d/fix-asn1write-raw-buffer.txt b/ChangeLog.d/fix-asn1write-raw-buffer.txt new file mode 100644 index 0000000000..292631aabc --- /dev/null +++ b/ChangeLog.d/fix-asn1write-raw-buffer.txt @@ -0,0 +1,5 @@ +Bugfix + * When calling mbedtls_asn1_write_raw_buffer() with NULL, 0 as the last two + arguments, undefined behaviour would be triggered, in the form of a call to + memcpy(..., NULL, 0). This was harmless in practice, but could trigger + complains from sanitizers or static analyzers. diff --git a/library/asn1write.c b/library/asn1write.c index 415357b7b5..97f9db039b 100644 --- a/library/asn1write.c +++ b/library/asn1write.c @@ -90,7 +90,9 @@ int mbedtls_asn1_write_raw_buffer(unsigned char **p, const unsigned char *start, len = size; (*p) -= len; - memcpy(*p, buf, len); + if (len != 0) { + memcpy(*p, buf, len); + } return (int) len; }