From 19d2c9165a13decf754177adda2bf59fd0e32aa1 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 1/9] 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 453f598c74..6b104613d7 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 839b5df226..420e36b81b 100644 --- a/library/x509_create.c +++ b/library/x509_create.c @@ -292,8 +292,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 acdcb7fcd1aaca3050431b9034cc4ec3f8f7c7de 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 2/9] 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 56f23c9fab..d829ffe602 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -84,12 +84,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 0d6f6bb1d3..75bd8f044e 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -66,6 +66,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 4dd52b7cfedc9d42aa144d0f5e4350fa07001226 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 3/9] 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 995ee499d5..c399021916 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; @@ -492,7 +498,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 6fd1dce1fc..63872a953f 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -312,7 +312,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 @@ -595,7 +594,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( @@ -604,7 +608,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; @@ -998,7 +1004,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 0803df29fc5f69628eac03ffc7b2274542b6bcd3 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 4/9] 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 c399021916..26aed93e4f 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -497,6 +497,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 63872a953f..d46470274a 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -1001,6 +1001,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 f9ac5e772832010aad14e7ed0f7da7dd515002e9 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 5/9] 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 81816fe63f..a22c486def 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -731,6 +731,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 35f2220e37cce029fa7f22f5dd579bd61b2a59b0 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 6/9] 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 26aed93e4f..97b2cccf2d 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -497,6 +497,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; @@ -514,22 +522,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 8a6fc08607f538c5a753ab6b58b228264fe92a73 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 7/9] 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 97b2cccf2d..cefdf179a9 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 d46470274a..478dab71d6 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -597,7 +597,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 8429619a921e3ea2e074015009ac153b03724681 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 8/9] 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 8ac3eb9833f4447489b77c92d7f995a63739f6f4 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 9/9] 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 cefdf179a9..38b7af0925 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -516,10 +516,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 478dab71d6..95f08b9127 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -1012,10 +1012,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;