Merge pull request #1349 from felixc-arm/pem-integer-underflow-3.6
[3.6] Fix Integer Underflow when Decoding PEM Keys
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 0000000..87bc596
--- /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/pkcs7-padding-side-channel-fix.txt b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt
index b813b84..c5cbc75 100644
--- a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt
+++ b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt
@@ -2,3 +2,5 @@
* Fix a timing side channel in the implementation of PKCS#7 padding
which would allow an attacker who can request decryption of arbitrary
ciphertexts to recover the plaintext through a timing oracle attack.
+ Reported by Ka Lok Wu from Stony Brook University and Doria Tang from
+ The Chinese University of Hong Kong.
diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h
index 453f598..6b10461 100644
--- a/include/mbedtls/x509.h
+++ b/include/mbedtls/x509.h
@@ -332,7 +332,8 @@
* 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 839b5df..420e36b 100644
--- a/library/x509_create.c
+++ b/library/x509_create.c
@@ -292,8 +292,12 @@
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 56f23c9..d829ffe 100644
--- a/library/x509write_crt.c
+++ b/library/x509write_crt.c
@@ -84,12 +84,14 @@
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 0d6f6bb..75bd8f0 100644
--- a/library/x509write_csr.c
+++ b/library/x509write_csr.c
@@ -66,6 +66,7 @@
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 995ee49..38b7af0 100644
--- a/programs/x509/cert_req.c
+++ b/programs/x509/cert_req.c
@@ -150,7 +150,6 @@
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 @@
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 @@
(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 @@
}
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,12 +510,21 @@
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 6fd1dce..95f08b9 100644
--- a/programs/x509/cert_write.c
+++ b/programs/x509/cert_write.c
@@ -312,7 +312,6 @@
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,15 @@
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(
@@ -604,7 +611,9 @@
(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 +1004,26 @@
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.function b/tests/suites/test_suite_x509write.function
index 81816fe..a22c486 100644
--- a/tests/suites/test_suite_x509write.function
+++ b/tests/suites/test_suite_x509write.function
@@ -731,6 +731,11 @@
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);