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..87bc59694f --- /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 makes it likely that existing risky uses of + this function (see the entry in the Security section) will be detected and + fixed. 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..e517cbb72a --- /dev/null +++ b/ChangeLog.d/fix-string-to-names-store-named-data.txt @@ -0,0 +1,8 @@ +Security + * 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. Found by Linh Le and Ngan Nguyen from Calif. diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h index 2afcfb2f9f..b1a80e3011 100644 --- a/include/mbedtls/x509.h +++ b/include/mbedtls/x509.h @@ -329,7 +329,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 e5ade5d997..17fc8fbeb5 100644 --- a/library/x509_create.c +++ b/library/x509_create.c @@ -468,8 +468,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 e530ae8dbe..09c2328b1a 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -82,12 +82,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 b353d37de5..88adf794f7 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -64,6 +64,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 f09e93863a..e59772ffda 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_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) { 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; @@ -490,7 +499,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); @@ -500,12 +508,21 @@ 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; } - mbedtls_exit(exit_code); } #endif /* MBEDTLS_X509_CSR_WRITE_C && MBEDTLS_PK_PARSE_C && MBEDTLS_FS_IO && diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index 9776dc1c37..3cabff4b5a 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,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_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) { mbedtls_strerror(ret, buf, sizeof(buf)); mbedtls_printf( @@ -602,7 +609,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; @@ -991,10 +1000,26 @@ 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); diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index e4e08dafc0..4dcd967226 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=#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 diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index 51a5d37584..224768ab4e 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -670,6 +670,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); diff --git a/tf-psa-crypto b/tf-psa-crypto index a07506eab0..5ff707caa3 160000 --- a/tf-psa-crypto +++ b/tf-psa-crypto @@ -1 +1 @@ -Subproject commit a07506eab0b693152d5a522273b812d222ddd87c +Subproject commit 5ff707caa307bf738128030bfe7d014b65b7eb3e