diff --git a/ChangeLog.d/fix-asnn1write-raw-buffer.txt b/ChangeLog.d/fix-asnn1write-raw-buffer.txt new file mode 100644 index 0000000000000000000000000000000000000000..4786a8c36e3e4bd81f1a16f7c8b93f2eeb079139 --- /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 0000000000000000000000000000000000000000..0bbce364a97c1d100015914422f3b545bab263c2 --- /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 0000000000000000000000000000000000000000..b42282e7efc6fff2c193da102bef41aa8fc13831 --- /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 453f598c746bbedf4b42ac361f3bae450fddef1b..efb3ff9749c60f702f9dfb6194aaef47d1531a3e 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 415357b7b5923a1feaddd213cc2dd9f3214a5d11..97f9db039beb2e9cbe0f9ec1c6c46e3f8d785a74 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 839b5df226ffb6b4c5ff1278895959f4ece3b74e..420e36b81b5f5e773d5d9f3588541ab688af8348 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 72f5a10a17b9f9b804379615f164e347e4209a32..ed30f85cc58ff42d62e76fc91efc7bb3c7b35f40 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 d3ddbcc03d2187da23ede1742df60d6ae7b00027..b4416ac1e5fe61a80850f7b985665b5ef2923ddf 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 dcfd1765c39e20ce8dcc7be3d807c4a02a7fea99..032f33136badeada9960dcc2f2b017820957680d 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 0b2575e84a9d6ebc9da35b3405f3a5bcd38c79f3..82c079d12e2ead45c896a4ac06398e8498b37f84 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);