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] 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 == '=') {