From 43a1e733d8453dc77518c514626e8234c2abb59b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 16:41:52 +0200 Subject: [PATCH 01/16] Fix undocumented free() in x509_string_to_names() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now programs/x509/cert_write san="DN:CN=#0000;DN:CN=#0000" is no longer crashing with use-after-free, instead it's now failing cleanly: failed ! mbedtls_x509_string_to_names returned -0x2800 - X509 - Input invalid That's better of course but still not great, will be fixed by future commits. Signed-off-by: Manuel Pégourié-Gonnard --- .../fix-string-to-names-memory-management.txt | 18 ++++++++++++++++++ include/mbedtls/x509.h | 3 ++- library/x509_create.c | 8 ++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 ChangeLog.d/fix-string-to-names-memory-management.txt diff --git a/ChangeLog.d/fix-string-to-names-memory-management.txt b/ChangeLog.d/fix-string-to-names-memory-management.txt new file mode 100644 index 0000000000..1b2198287d --- /dev/null +++ b/ChangeLog.d/fix-string-to-names-memory-management.txt @@ -0,0 +1,18 @@ +Security + * Fix possible use-after-free or double-free in code calling + mbedtls_x509_string_to_names(). This was caused by the function calling + mbedtls_asn1_free_named_data_list() on its head argument, while the + documentation did no suggest it did, making it likely for callers relying + on the documented behaviour to still hold pointers to memory blocks after + they were free()d, resulting in high risk of use-after-free or double-free, + with consequences ranging up to arbitrary code execution. + In particular, the two sample programs x509/cert_write and x509/cert_req + were affected (use-after-free if the san string contains more than one DN). + Code that does not call mbedtls_string_to_names() directly is not affected. + Found by Linh Le and Ngan Nguyen from Calif. + +Changes + * The function mbedtls_x509_string_to_names() now requires its head argument + to point to NULL on entry. This make it likely that existing risky uses of + this function (see the entry in the Security section) will be detected and + fixed. diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h index 18df19ce6c..081acff9ad 100644 --- a/include/mbedtls/x509.h +++ b/include/mbedtls/x509.h @@ -332,7 +332,8 @@ int mbedtls_x509_dn_gets(char *buf, size_t size, const mbedtls_x509_name *dn); * call to mbedtls_asn1_free_named_data_list(). * * \param[out] head Address in which to store the pointer to the head of the - * allocated list of mbedtls_x509_name + * allocated list of mbedtls_x509_name. Must point to NULL on + * entry. * \param[in] name The string representation of a DN to convert * * \return 0 on success, or a negative error code. diff --git a/library/x509_create.c b/library/x509_create.c index 48ac080cbe..093cf88ed9 100644 --- a/library/x509_create.c +++ b/library/x509_create.c @@ -467,8 +467,12 @@ int mbedtls_x509_string_to_names(mbedtls_asn1_named_data **head, const char *nam unsigned char data[MBEDTLS_X509_MAX_DN_NAME_SIZE]; size_t data_len = 0; - /* Clear existing chain if present */ - mbedtls_asn1_free_named_data_list(head); + /* Ensure the output parameter is not already populated. + * (If it were, overwriting it would likely cause a memory leak.) + */ + if (*head != NULL) { + return MBEDTLS_ERR_X509_BAD_INPUT_DATA; + } while (c <= end) { if (in_attr_type && *c == '=') { From 2dc6b583acde7dfe99e920e7c41edec49de54da5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 16:49:45 +0200 Subject: [PATCH 02/16] Restore behaviour of mbedtls_x509write_set_foo_name() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documentation doesn't say you can't call these functions more than once on the same context, and if you do it shouldn't result in a memory leak. Historically, the call to mbedtls_asn1_free_named_data_list() in mbedtls_x509_string_to_names() (that was removed in the previous commit) was ensuring that. Let's restore it where it makes sense. (These are the only 3 places calling mbedtls_x509_string_to_names() in the library.) Signed-off-by: Manuel Pégourié-Gonnard --- library/x509write_crt.c | 2 ++ library/x509write_csr.c | 1 + 2 files changed, 3 insertions(+) diff --git a/library/x509write_crt.c b/library/x509write_crt.c index 7d207481c2..932d28d435 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -81,12 +81,14 @@ void mbedtls_x509write_crt_set_issuer_key(mbedtls_x509write_cert *ctx, int mbedtls_x509write_crt_set_subject_name(mbedtls_x509write_cert *ctx, const char *subject_name) { + mbedtls_asn1_free_named_data_list(&ctx->subject); return mbedtls_x509_string_to_names(&ctx->subject, subject_name); } int mbedtls_x509write_crt_set_issuer_name(mbedtls_x509write_cert *ctx, const char *issuer_name) { + mbedtls_asn1_free_named_data_list(&ctx->issuer); return mbedtls_x509_string_to_names(&ctx->issuer, issuer_name); } diff --git a/library/x509write_csr.c b/library/x509write_csr.c index e65ddb07f4..65403055c6 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -63,6 +63,7 @@ void mbedtls_x509write_csr_set_key(mbedtls_x509write_csr *ctx, mbedtls_pk_contex int mbedtls_x509write_csr_set_subject_name(mbedtls_x509write_csr *ctx, const char *subject_name) { + mbedtls_asn1_free_named_data_list(&ctx->subject); return mbedtls_x509_string_to_names(&ctx->subject, subject_name); } From 6b1147993c3a28fc05807db338ece7ae8f881770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 17:09:14 +0200 Subject: [PATCH 03/16] Fix runtime error in cert_write & cert_req MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runtime error was introduced two commits ago (while avoiding a use-after-free). Now the programs run cleanly but still leak memory. The memory leak is long pre-existing and larger than just DN components (which are made temporarily slightly worse by this commit) and will be fixed properly in the next commit. Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 13 +++++++++---- programs/x509/cert_write.c | 13 +++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index f09e93863a..8677cbb04f 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -150,7 +150,6 @@ int main(int argc, char *argv[]) mbedtls_ctr_drbg_context ctr_drbg; const char *pers = "csr example app"; mbedtls_x509_san_list *cur, *prev; - mbedtls_asn1_named_data *ext_san_dirname = NULL; #if defined(MBEDTLS_X509_CRT_PARSE_C) uint8_t ip[4] = { 0 }; #endif @@ -274,7 +273,12 @@ usage: cur->node.san.unstructured_name.len = sizeof(ip); } else if (strcmp(q, "DN") == 0) { cur->node.type = MBEDTLS_X509_SAN_DIRECTORY_NAME; - if ((ret = mbedtls_x509_string_to_names(&ext_san_dirname, + /* Work around an API mismatch between string_to_names() and + * mbedtls_x509_subject_alternative_name, which holds an + * actual mbedtls_x509_name while a pointer to one would be + * more convenient here. */ + mbedtls_asn1_named_data *tmp_san_dirname = NULL; + if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { mbedtls_strerror(ret, buf, sizeof(buf)); mbedtls_printf( @@ -283,7 +287,9 @@ usage: (unsigned int) -ret, buf); goto exit; } - cur->node.san.directory_name = *ext_san_dirname; + cur->node.san.directory_name = *tmp_san_dirname; + mbedtls_free(tmp_san_dirname); + tmp_san_dirname = NULL; } else { mbedtls_free(cur); goto usage; @@ -490,7 +496,6 @@ exit: } mbedtls_x509write_csr_free(&req); - mbedtls_asn1_free_named_data_list(&ext_san_dirname); mbedtls_pk_free(&key); mbedtls_ctr_drbg_free(&ctr_drbg); mbedtls_entropy_free(&entropy); diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index 9776dc1c37..aa70a17549 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -310,7 +310,6 @@ int main(int argc, char *argv[]) mbedtls_ctr_drbg_context ctr_drbg; const char *pers = "crt example app"; mbedtls_x509_san_list *cur, *prev; - mbedtls_asn1_named_data *ext_san_dirname = NULL; uint8_t ip[4] = { 0 }; /* * Set to sane values @@ -593,7 +592,12 @@ usage: cur->node.san.unstructured_name.len = sizeof(ip); } else if (strcmp(q, "DN") == 0) { cur->node.type = MBEDTLS_X509_SAN_DIRECTORY_NAME; - if ((ret = mbedtls_x509_string_to_names(&ext_san_dirname, + /* Work around an API mismatch between string_to_names() and + * mbedtls_x509_subject_alternative_name, which holds an + * actual mbedtls_x509_name while a pointer to one would be + * more convenient here. */ + mbedtls_asn1_named_data *tmp_san_dirname = NULL; + if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { mbedtls_strerror(ret, buf, sizeof(buf)); mbedtls_printf( @@ -602,7 +606,9 @@ usage: (unsigned int) -ret, buf); goto exit; } - cur->node.san.directory_name = *ext_san_dirname; + cur->node.san.directory_name = *tmp_san_dirname; + mbedtls_free(tmp_san_dirname); + tmp_san_dirname = NULL; } else { mbedtls_free(cur); goto usage; @@ -994,7 +1000,6 @@ exit: #if defined(MBEDTLS_X509_CSR_PARSE_C) mbedtls_x509_csr_free(&csr); #endif /* MBEDTLS_X509_CSR_PARSE_C */ - mbedtls_asn1_free_named_data_list(&ext_san_dirname); mbedtls_x509_crt_free(&issuer_crt); mbedtls_x509write_crt_free(&crt); mbedtls_pk_free(&loaded_subject_key); From b0958627224ed9c9f767f06bc5f803b755d5d035 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 17:31:35 +0200 Subject: [PATCH 04/16] Fix memory leak in cert_write & cert_req MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit That memory leak had been present ever since the san command-line argument has been added. Tested that the following invocation is now fully valgrind clean: programs/x509/cert_write san=DN:C=NL,CN=#0000,CN=foo;DN:CN=#0000,O=foo,OU=bar,C=UK;IP:1.2.3.4;IP:4.3.2.1;URI:http\\://example.org/;URI:foo;DNS:foo.example.org;DNS:bar.example.org Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 17 +++++++++++++++++ programs/x509/cert_write.c | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index 8677cbb04f..605d78c578 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -495,6 +495,23 @@ exit: #endif } + cur = opt.san_list; + while (cur != NULL) { + mbedtls_x509_san_list *next = cur->next; + /* Note: mbedtls_x509_free_subject_alt_name() is not what we want here. + * It's the right thing for entries that were parsed from a certificate, + * where pointers are to the raw certificate, but here all the + * pointers were allocated while parsing from a user-provided string. */ + if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { + mbedtls_x509_name dn = cur->node.san.directory_name; + mbedtls_free(dn.oid.p); + mbedtls_free(dn.val.p); + mbedtls_asn1_free_named_data_list(&dn.next); + } + mbedtls_free(cur); + cur = next; + } + mbedtls_x509write_csr_free(&req); mbedtls_pk_free(&key); mbedtls_ctr_drbg_free(&ctr_drbg); diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index aa70a17549..268036147d 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -997,6 +997,23 @@ usage: exit_code = MBEDTLS_EXIT_SUCCESS; exit: + cur = opt.san_list; + while (cur != NULL) { + mbedtls_x509_san_list *next = cur->next; + /* Note: mbedtls_x509_free_subject_alt_name() is not what we want here. + * It's the right thing for entries that were parsed from a certificate, + * where pointers are to the raw certificate, but here all the + * pointers were allocated while parsing from a user-provided string. */ + if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { + mbedtls_x509_name dn = cur->node.san.directory_name; + mbedtls_free(dn.oid.p); + mbedtls_free(dn.val.p); + mbedtls_asn1_free_named_data_list(&dn.next); + } + mbedtls_free(cur); + cur = next; + } + #if defined(MBEDTLS_X509_CSR_PARSE_C) mbedtls_x509_csr_free(&csr); #endif /* MBEDTLS_X509_CSR_PARSE_C */ From bda3ab927826ae7603a46d8073790cc051848976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 18:25:26 +0200 Subject: [PATCH 05/16] Add unit test for new behaviour of string_to_names() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.function | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index f3a161ca52..6893c8bc7d 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -669,6 +669,11 @@ void mbedtls_x509_string_to_names(char *name, char *parsed_name, TEST_LE_S(1, ret); TEST_ASSERT(strcmp((char *) out, parsed_name) == 0); + /* Check that calling a 2nd time with the same param (now non-NULL) + * returns an error as expected. */ + ret = mbedtls_x509_string_to_names(&names, name); + TEST_EQUAL(ret, MBEDTLS_ERR_X509_BAD_INPUT_DATA); + exit: mbedtls_asn1_free_named_data_list(&names); From 8de781d99d5059bc6abbe5e9fbd618a6075dee68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 19 May 2025 12:21:32 +0200 Subject: [PATCH 06/16] Remove redundant free loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This version is incomplete. I failed to noticed it when adding a more complete version, making the existing one redundant. Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index 605d78c578..89ab181be6 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -495,6 +495,14 @@ exit: #endif } + mbedtls_x509write_csr_free(&req); + mbedtls_pk_free(&key); + mbedtls_ctr_drbg_free(&ctr_drbg); + mbedtls_entropy_free(&entropy); +#if defined(MBEDTLS_USE_PSA_CRYPTO) + mbedtls_psa_crypto_free(); +#endif /* MBEDTLS_USE_PSA_CRYPTO */ + cur = opt.san_list; while (cur != NULL) { mbedtls_x509_san_list *next = cur->next; @@ -512,22 +520,6 @@ exit: cur = next; } - mbedtls_x509write_csr_free(&req); - mbedtls_pk_free(&key); - mbedtls_ctr_drbg_free(&ctr_drbg); - mbedtls_entropy_free(&entropy); -#if defined(MBEDTLS_USE_PSA_CRYPTO) - mbedtls_psa_crypto_free(); -#endif /* MBEDTLS_USE_PSA_CRYPTO */ - - cur = opt.san_list; - while (cur != NULL) { - prev = cur; - cur = cur->next; - mbedtls_free(prev); - } - - mbedtls_exit(exit_code); } #endif /* MBEDTLS_X509_CSR_WRITE_C && MBEDTLS_PK_PARSE_C && MBEDTLS_FS_IO && From bb8c0aba74c2e6d7b4ab76887b3cf8fb0c6db1bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 19 May 2025 12:28:42 +0200 Subject: [PATCH 07/16] Add comment on apparent type mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 5 ++++- programs/x509/cert_write.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index 89ab181be6..c16ec34987 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -276,7 +276,10 @@ usage: /* Work around an API mismatch between string_to_names() and * mbedtls_x509_subject_alternative_name, which holds an * actual mbedtls_x509_name while a pointer to one would be - * more convenient here. */ + * more convenient here. (Note mbedtls_x509_name and + * mbedtls_asn1_named_data are synonymous, again + * string_to_names() uses one while + * cur->node.san.directory_name is nominally the other.) */ mbedtls_asn1_named_data *tmp_san_dirname = NULL; if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index 268036147d..f29eef0eb0 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -595,7 +595,10 @@ usage: /* Work around an API mismatch between string_to_names() and * mbedtls_x509_subject_alternative_name, which holds an * actual mbedtls_x509_name while a pointer to one would be - * more convenient here. */ + * more convenient here. (Note mbedtls_x509_name and + * mbedtls_asn1_named_data are synonymous, again + * string_to_names() uses one while + * cur->node.san.directory_name is nominally the other.) */ mbedtls_asn1_named_data *tmp_san_dirname = NULL; if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { From 38317281e91477b6f2b9198fff83579640811473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 19 May 2025 12:29:11 +0200 Subject: [PATCH 08/16] Fix type in ChangeLog 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-memory-management.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/fix-string-to-names-memory-management.txt b/ChangeLog.d/fix-string-to-names-memory-management.txt index 1b2198287d..87bc59694f 100644 --- a/ChangeLog.d/fix-string-to-names-memory-management.txt +++ b/ChangeLog.d/fix-string-to-names-memory-management.txt @@ -13,6 +13,6 @@ Security Changes * The function mbedtls_x509_string_to_names() now requires its head argument - to point to NULL on entry. This make it likely that existing risky uses of + to point to NULL on entry. This makes it likely that existing risky uses of this function (see the entry in the Security section) will be detected and fixed. From 6b8f517e4d3e4c5f1860cc8bd11d146d5bc1b6df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 21 May 2025 11:17:39 +0200 Subject: [PATCH 09/16] Avoid a useless copy in cert_{req,write} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I'm just trying to have a shorter name to avoid repeating a long expression. This is a job for a pointer, not copying a struct. Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 8 ++++---- programs/x509/cert_write.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index c16ec34987..e59772ffda 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -514,10 +514,10 @@ exit: * where pointers are to the raw certificate, but here all the * pointers were allocated while parsing from a user-provided string. */ if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { - mbedtls_x509_name dn = cur->node.san.directory_name; - mbedtls_free(dn.oid.p); - mbedtls_free(dn.val.p); - mbedtls_asn1_free_named_data_list(&dn.next); + mbedtls_x509_name *dn = &cur->node.san.directory_name; + mbedtls_free(dn->oid.p); + mbedtls_free(dn->val.p); + mbedtls_asn1_free_named_data_list(&dn->next); } mbedtls_free(cur); cur = next; diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index f29eef0eb0..3cabff4b5a 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -1008,10 +1008,10 @@ exit: * where pointers are to the raw certificate, but here all the * pointers were allocated while parsing from a user-provided string. */ if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { - mbedtls_x509_name dn = cur->node.san.directory_name; - mbedtls_free(dn.oid.p); - mbedtls_free(dn.val.p); - mbedtls_asn1_free_named_data_list(&dn.next); + mbedtls_x509_name *dn = &cur->node.san.directory_name; + mbedtls_free(dn->oid.p); + mbedtls_free(dn->val.p); + mbedtls_asn1_free_named_data_list(&dn->next); } mbedtls_free(cur); cur = next; From 5989da22a9d32cd314411f3f79df4ae580d7d285 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 10/16] 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 e4e08dafc0..e5224218c5 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 67f63821a5f6027213f99e7e7f29c09a67a773c2 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Thu, 29 May 2025 17:25:21 +0100 Subject: [PATCH 11/16] Updated tf-psa-crypto pointer Signed-off-by: Minos Galanakis --- tf-psa-crypto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tf-psa-crypto b/tf-psa-crypto index 35ae18cf89..9af7c0e7ba 160000 --- a/tf-psa-crypto +++ b/tf-psa-crypto @@ -1 +1 @@ -Subproject commit 35ae18cf891d3675584da41f7e830f1de5f87f07 +Subproject commit 9af7c0e7ba4d6bf2a9c3e56a3e3f04b4b053ce47 From d1090d70ffd084b8750b64334a32b8b6d473ee19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 28 May 2025 13:06:27 +0200 Subject: [PATCH 12/16] Update crypto submodule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tf-psa-crypto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tf-psa-crypto b/tf-psa-crypto index 35ae18cf89..9af7c0e7ba 160000 --- a/tf-psa-crypto +++ b/tf-psa-crypto @@ -1 +1 @@ -Subproject commit 35ae18cf891d3675584da41f7e830f1de5f87f07 +Subproject commit 9af7c0e7ba4d6bf2a9c3e56a3e3f04b4b053ce47 From d2262f23049356528e7a7849dcd18928f484255e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 28 May 2025 13:07:42 +0200 Subject: [PATCH 13/16] Uncomment tests now that crypto is fixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.data | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index e5224218c5..96311f3b56 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=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 5f6310b65f6ad3cf2faa62b9c8a2109ecf0bedb3 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 14/16] 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 dc82fa67c5cfab62010d4d642015c267b0739307 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 28 May 2025 13:10:44 +0200 Subject: [PATCH 15/16] Keep only the X.509 part from the Changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- .../fix-string-to-names-store-named-data.txt | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/ChangeLog.d/fix-string-to-names-store-named-data.txt b/ChangeLog.d/fix-string-to-names-store-named-data.txt index 422ce07f85..e517cbb72a 100644 --- a/ChangeLog.d/fix-string-to-names-store-named-data.txt +++ b/ChangeLog.d/fix-string-to-names-store-named-data.txt @@ -1,12 +1,8 @@ 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 + * Fix a bug in mbedtls_x509_string_to_names() and the + mbedtls_x509write_{crt,csr}_set_{subject,issuer}_name() functions, + where some inputs would cause an inconsistent state to be reached, causing + a NULL dereference either in the function itself, 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. + certificates, CSRs or CRLs. Found by Linh Le and Ngan Nguyen from Calif. From f5a63d1456f109c369500d89f605ea308ea14f1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 10 Jun 2025 09:56:40 +0200 Subject: [PATCH 16/16] Fix invalid test data by aligning with 3.6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index 96311f3b56..4dcd967226 100644 --- a/tests/suites/test_suite_x509write.data +++ b/tests/suites/test_suite_x509write.data @@ -273,7 +273,7 @@ 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 +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