From c98ceccfc4d6c10fbbe37b702de256eecd4cb63f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E6=B1=9F=E6=B5=B7?= Date: Fri, 8 Aug 2025 19:05:24 +0800 Subject: [PATCH] fix bug CVE-2025-47917 CVE-2025-48965 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 李江海 --- ChangeLog.d/fix-asnn1write-raw-buffer.txt | 5 +++ .../fix-string-to-names-memory-management.txt | 18 ++++++++++ .../fix-string-to-names-store-named-data.txt | 12 +++++++ include/mbedtls/x509.h | 3 +- library/asn1write.c | 4 ++- library/x509_create.c | 8 +++-- library/x509write_crt.c | 2 ++ library/x509write_csr.c | 1 + programs/x509/cert_req.c | 32 ++++++++++++++---- programs/x509/cert_write.c | 33 ++++++++++++++++--- 10 files changed, 103 insertions(+), 15 deletions(-) create mode 100644 ChangeLog.d/fix-asnn1write-raw-buffer.txt create mode 100644 ChangeLog.d/fix-string-to-names-memory-management.txt create mode 100644 ChangeLog.d/fix-string-to-names-store-named-data.txt diff --git a/ChangeLog.d/fix-asnn1write-raw-buffer.txt b/ChangeLog.d/fix-asnn1write-raw-buffer.txt new file mode 100644 index 000000000..4786a8c36 --- /dev/null +++ b/ChangeLog.d/fix-asnn1write-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 anlyzers. \ No newline at end of file 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 000000000..0bbce364a --- /dev/null +++ b/ChangeLog.d/fix-string-to-names-memory-management.txt @@ -0,0 +1,18 @@ +Security + * Fix possible usr-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 documentd 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. + Inparticular, 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_string_to_names() now requires its head argument + to point to NULL on entry. This makes it likely that existing risky uses of + this function (see the entry in the Security seciton) will be detected and + fixed. \ No newline at end of file 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 000000000..b42282e7e --- /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 onr of the + mbedtls_x509write_{crt, csr}_set_{subject,issuer}_name() functions. The + inconsistent state of the outputcould 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 creat (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. \ No newline at end of file diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h index 453f598c7..efb3ff974 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/asn1write.c b/library/asn1write.c index 415357b7b..97f9db039 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; } diff --git a/library/x509_create.c b/library/x509_create.c index 839b5df22..420e36b81 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 == '=') { diff --git a/library/x509write_crt.c b/library/x509write_crt.c index 72f5a10a1..ed30f85cc 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -80,12 +80,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 d3ddbcc03..b4416ac1e 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -62,6 +62,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); } diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index dcfd1765c..032f33136 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,15 @@ 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. (Note mbedtls_x509_name and + * mbedtls_ans1_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) { mbedtls_strerror(ret, buf, sizeof(buf)); mbedtls_printf( @@ -283,7 +290,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 +501,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); @@ -502,9 +510,19 @@ exit: cur = opt.san_list; while (cur != NULL) { - prev = cur; - cur = cur->next; - mbedtls_free(prev); + 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; } diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index 0b2575e84..82c079d12 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,16 @@ 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, + cur->node.type = MBEDTLS_X509_SAN_DIRECTORY_NAME; + /* 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. (Note mbedtls_x509_name and + * mbedtls_ans1_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) { mbedtls_strerror(ret, buf, sizeof(buf)); mbedtls_printf( @@ -604,7 +612,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; @@ -995,10 +1005,25 @@ 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 */ - 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); -- Gitee