Merge pull request #1340 from mpg/fix-string-to-names-uaf-3.6

[3.6] Fix string to names memory management

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.

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.

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 */
+    /* Ensure the output parameter is not already populated.
-    mbedtls_asn1_free_named_data_list(head);
+     * (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 == '=') {

library/x509write_crt.c

@@ -84,12 +84,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);
 }
 

library/x509write_csr.c

@@ -66,6 +66,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);
 }
 

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;
@@ -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,12 +510,21 @@ exit:
 
     cur = opt.san_list;
     while (cur != NULL) {
-        prev = cur;
+        mbedtls_x509_san_list *next = cur->next;
-        cur = cur->next;
+        /* Note: mbedtls_x509_free_subject_alt_name() is not what we want here.
-        mbedtls_free(prev);
+         * 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 &&

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,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(
@@ -604,7 +611,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 +1004,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);

tests/suites/test_suite_x509write.function

@@ -731,6 +731,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);