Merge 327fca77cbd154e31efb602ce5511dba4e0b90cc into 4fdb917aae2fb5876e04f88621630712a906a067

Backport 3.6: Fix resource leak in populate transform

ChangeLog.d/tls13-transform-resource-leak.txt

@@ -0,0 +1,4 @@
+Bugfix
+   * Fix resource leaks in TLS 1.3 transform setup when a setup step fails
+     after allocating cipher contexts or importing the encryption traffic key.
+     Fixes #10917.

ChangeLog.d/x509-programs-ip-san.txt

@@ -0,0 +1,6 @@
+Bugfix
+   * Fix a stack buffer overflow in the cert_write and cert_req sample
+     programs when an IPv6 address is given in the san parameter. IPv6
+     addresses are now written correctly.
+   * Fix the cert_write and cert_req sample programs writing the last IP
+     address for every IP entry when the san parameter has more than one.

library/ssl_tls13_keys.c

@@ -977,6 +977,7 @@ int mbedtls_ssl_tls13_populate_transform(
     if ((ret = mbedtls_cipher_setup(&transform->cipher_ctx_dec,
                                     cipher_info)) != 0) {
         MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_cipher_setup", ret);
+        mbedtls_ssl_transform_free(transform);
         return ret;
     }
 #endif /* MBEDTLS_USE_PSA_CRYPTO */
@@ -999,6 +1000,9 @@ int mbedtls_ssl_tls13_populate_transform(
 #endif /* MBEDTLS_SSL_CLI_C */
     {
         /* should not happen */
+#if !defined(MBEDTLS_USE_PSA_CRYPTO)
+        mbedtls_ssl_transform_free(transform);
+#endif
         return MBEDTLS_ERR_SSL_INTERNAL_ERROR;
     }
 
@@ -1010,6 +1014,7 @@ int mbedtls_ssl_tls13_populate_transform(
                                      key_enc, (int) mbedtls_cipher_info_get_key_bitlen(cipher_info),
                                      MBEDTLS_ENCRYPT)) != 0) {
         MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_cipher_setkey", ret);
+        mbedtls_ssl_transform_free(transform);
         return ret;
     }
 
@@ -1017,6 +1022,7 @@ int mbedtls_ssl_tls13_populate_transform(
                                      key_dec, (int) mbedtls_cipher_info_get_key_bitlen(cipher_info),
                                      MBEDTLS_DECRYPT)) != 0) {
         MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_cipher_setkey", ret);
+        mbedtls_ssl_transform_free(transform);
         return ret;
     }
 #endif /* MBEDTLS_USE_PSA_CRYPTO */
@@ -1081,6 +1087,8 @@ int mbedtls_ssl_tls13_populate_transform(
                                      &transform->psa_key_dec)) != PSA_SUCCESS) {
             MBEDTLS_SSL_DEBUG_RET(
                 1, "psa_import_key", PSA_TO_MBEDTLS_ERR(status));
+            psa_destroy_key(transform->psa_key_enc);
+            transform->psa_key_enc = MBEDTLS_SVC_KEY_ID_INIT;
             return PSA_TO_MBEDTLS_ERR(status);
         }
     }

programs/x509/cert_req.c

@@ -59,7 +59,7 @@ int main(void)
     "                           DNS:value\n"            \
     "                           URI:value\n"            \
     "                           RFC822:value\n"         \
-    "                           IP:value (Only IPv4 is supported)\n" \
+    "                           IP:value (IPv4 or IPv6)\n" \
     "                           DN:list of comma separated key=value pairs\n" \
     "    key_usage=%%s        default: (empty)\n"       \
     "                        Comma-separated-list of values:\n"     \
@@ -150,9 +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;
-#if defined(MBEDTLS_X509_CRT_PARSE_C)
-    uint8_t ip[4] = { 0 };
-#endif
     /*
      * Set to sane values
      */
@@ -262,15 +259,24 @@ usage:
                     cur->node.type = MBEDTLS_X509_SAN_DNS_NAME;
                 } else if (strcmp(q, "IP") == 0) {
                     size_t ip_addr_len = 0;
+                    /* Each entry needs its own buffer, because the list is
+                     * only written once all entries have been parsed.
+                     * mbedtls_x509_crt_parse_cn_inet_pton() needs 16 bytes. */
+                    unsigned char *ip = mbedtls_calloc(1, 16);
+                    if (ip == NULL) {
+                        mbedtls_printf("Not enough memory for subjectAltName list\n");
+                        goto exit;
+                    }
                     cur->node.type = MBEDTLS_X509_SAN_IP_ADDRESS;
                     ip_addr_len = mbedtls_x509_crt_parse_cn_inet_pton(subtype_value, ip);
                     if (ip_addr_len == 0) {
                         mbedtls_printf("mbedtls_x509_crt_parse_cn_inet_pton failed to parse %s\n",
                                        subtype_value);
+                        mbedtls_free(ip);
                         goto exit;
                     }
-                    cur->node.san.unstructured_name.p = (unsigned char *) ip;
-                    cur->node.san.unstructured_name.len = sizeof(ip);
+                    cur->node.san.unstructured_name.p = ip;
+                    cur->node.san.unstructured_name.len = ip_addr_len;
                 } else if (strcmp(q, "DN") == 0) {
                     cur->node.type = MBEDTLS_X509_SAN_DIRECTORY_NAME;
                     /* Work around an API mismatch between string_to_names() and
@@ -521,6 +527,9 @@ exit:
             mbedtls_free(dn->val.p);
             mbedtls_asn1_free_named_data_list(&dn->next);
         }
+        if (cur->node.type == MBEDTLS_X509_SAN_IP_ADDRESS) {
+            mbedtls_free(cur->node.san.unstructured_name.p);
+        }
         mbedtls_free(cur);
         cur = next;
     }

programs/x509/cert_write.c

@@ -128,7 +128,7 @@ int main(void)
     "                             DNS:value\n"            \
     "                             URI:value\n"            \
     "                             RFC822:value\n"         \
-    "                             IP:value (Only IPv4 is supported)\n" \
+    "                             IP:value (IPv4 or IPv6)\n" \
     "                             DN:list of comma separated key=value pairs\n" \
     "    authority_identifier=%%s default: 1\n"             \
     "                            Possible values: 0, 1\n"   \
@@ -316,7 +316,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;
-    uint8_t ip[4] = { 0 };
     /*
      * Set to sane values
      */
@@ -587,15 +586,24 @@ usage:
                     cur->node.type = MBEDTLS_X509_SAN_DNS_NAME;
                 } else if (strcmp(q, "IP") == 0) {
                     size_t ip_addr_len = 0;
+                    /* Each entry needs its own buffer, because the list is
+                     * only written once all entries have been parsed.
+                     * mbedtls_x509_crt_parse_cn_inet_pton() needs 16 bytes. */
+                    unsigned char *ip = mbedtls_calloc(1, 16);
+                    if (ip == NULL) {
+                        mbedtls_printf("Not enough memory for subjectAltName list\n");
+                        goto exit;
+                    }
                     cur->node.type = MBEDTLS_X509_SAN_IP_ADDRESS;
                     ip_addr_len = mbedtls_x509_crt_parse_cn_inet_pton(subtype_value, ip);
                     if (ip_addr_len == 0) {
                         mbedtls_printf("mbedtls_x509_crt_parse_cn_inet_pton failed to parse %s\n",
                                        subtype_value);
+                        mbedtls_free(ip);
                         goto exit;
                     }
-                    cur->node.san.unstructured_name.p = (unsigned char *) ip;
-                    cur->node.san.unstructured_name.len = sizeof(ip);
+                    cur->node.san.unstructured_name.p = ip;
+                    cur->node.san.unstructured_name.len = ip_addr_len;
                 } else if (strcmp(q, "DN") == 0) {
                     cur->node.type = MBEDTLS_X509_SAN_DIRECTORY_NAME;
                     /* Work around an API mismatch between string_to_names() and
@@ -1021,6 +1029,9 @@ exit:
             mbedtls_free(dn->val.p);
             mbedtls_asn1_free_named_data_list(&dn->next);
         }
+        if (cur->node.type == MBEDTLS_X509_SAN_IP_ADDRESS) {
+            mbedtls_free(cur->node.san.unstructured_name.p);
+        }
         mbedtls_free(cur);
         cur = next;
     }

tests/suites/test_suite_ssl.data

@@ -3003,6 +3003,9 @@ SSL TLS 1.3 Record Encryption RFC 8448 Example #2
 depends_on:PSA_WANT_KEY_TYPE_AES:PSA_WANT_ALG_GCM:PSA_WANT_ALG_SHA_256
 ssl_tls13_record_protection:MBEDTLS_TLS1_3_AES_128_GCM_SHA256:MBEDTLS_SSL_IS_SERVER:1:1:"9f02283b6c9c07efc26bb9f2ac92e356":"cf782b88dd83549aadf1e984":"17422dda596ed5d9acd890e3c63f5051":"5b78923dee08579033e523d9":"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031":"2e937e11ef4ac740e538ad36005fc4a46932fc3225d05f82aa1b36e30efaf97d90e6dffc602dcb501a59a8fcc49c4bf2e5f0a21c0047c2abf332540dd032e167c2955d"
 
+TLS 1.3 transform frees encryption key if decryption key import fails
+ssl_tls13_populate_transform_second_key_import_failure
+
 SSL TLS 1.3 Key schedule: Application secrets derivation helper
 # Vector from RFC 8448
 depends_on:PSA_WANT_ALG_SHA_256

tests/suites/test_suite_ssl.function

@@ -5,6 +5,7 @@
 #include <mbedtls/pk.h>
 #include <ssl_tls13_keys.h>
 #include <ssl_tls13_invasive.h>
+#include <psa_crypto_slot_management.h>
 #include <test/ssl_helpers.h>
 #include <test/ssl_helpers_internal.h>
 
@@ -34,6 +35,39 @@
 #define TEST_SSL_CONTEXT_LOAD_OOB_OUT_CID  1
 #define TEST_SSL_CONTEXT_LOAD_OOB_ALPN     2
 
+#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC)
+#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_SSL_CLI_C) && \
+    defined(MBEDTLS_PSA_CRYPTO_C) && defined(MBEDTLS_SSL_PROTO_TLS1_3) && \
+    defined(MBEDTLS_USE_PSA_CRYPTO)
+#define SSL_TEST_KEY_SLICE_1_LENGTH 4
+#define SSL_TEST_KEY_SLICE_2_LENGTH 10
+static size_t ssl_test_tiny_key_slice_length(size_t slice_idx)
+{
+    switch (slice_idx) {
+        case 1: return SSL_TEST_KEY_SLICE_1_LENGTH;
+        case 2: return SSL_TEST_KEY_SLICE_2_LENGTH;
+        default: return 1;
+    }
+}
+
+/* In the below definition SSL_TEST_KEY_SLICE_1_LENGTH has 4 slots and
+ * SSL_TEST_KEY_SLICE_2_LENGTH has 10 all other slices have 1.
+ * psa_key_slot_volatile_slice_count() returns the total number of slices.
+ * However we have already countes slices 1 and 2, so we mustr subtract 2 from
+ * this number to avoid double counting.
+ */
+
+#define SSL_TEST_MAX_VOLATILE_KEYS                         \
+    (SSL_TEST_KEY_SLICE_1_LENGTH +                         \
+     SSL_TEST_KEY_SLICE_2_LENGTH +                         \
+     psa_key_slot_volatile_slice_count() - 2)
+#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_SSL_CLI_C &&
+          MBEDTLS_PSA_CRYPTO_C && MBEDTLS_SSL_PROTO_TLS1_3 &&
+          MBEDTLS_USE_PSA_CRYPTO */
+#else /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */
+#define SSL_TEST_MAX_VOLATILE_KEYS MBEDTLS_PSA_KEY_SLOT_COUNT
+#endif /* MBEDTLS_PSA_KEY_STORE_DYNAMIC */
+
 #if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \
     defined(MBEDTLS_SSL_SESSION_TICKETS) && \
     defined(MBEDTLS_SSL_CLI_C) && defined(MBEDTLS_SSL_SRV_C) && \
@@ -2461,6 +2495,85 @@ exit:
 }
 /* END_CASE */
 
+/* BEGIN_CASE depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_CLI_C:SSL_TEST_MAX_VOLATILE_KEYS:MBEDTLS_PSA_CRYPTO_C:MBEDTLS_USE_PSA_CRYPTO */
+void ssl_tls13_populate_transform_second_key_import_failure(void)
+{
+    mbedtls_ssl_key_set traffic_keys;
+    mbedtls_ssl_transform transform;
+    mbedtls_svc_key_id_t *keys = NULL;
+    psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
+    mbedtls_psa_stats_t stats_before, stats_after;
+    psa_status_t status = PSA_SUCCESS;
+    size_t i = 0;
+
+    memset(&traffic_keys, 0, sizeof(traffic_keys));
+    mbedtls_ssl_transform_init(&transform);
+
+#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) && defined(MBEDTLS_TEST_HOOKS)
+    mbedtls_test_hook_psa_volatile_key_slice_length =
+        ssl_test_tiny_key_slice_length;
+#endif
+
+    PSA_INIT();
+    TEST_CALLOC(keys, SSL_TEST_MAX_VOLATILE_KEYS + 1);
+
+    psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_EXPORT);
+    psa_set_key_type(&attributes, PSA_KEY_TYPE_RAW_DATA);
+
+    /* Allow one more attempt than the maximum number of volatile keys so
+     * that the loop ends because the key store is full. */
+    while (i <= SSL_TEST_MAX_VOLATILE_KEYS) {
+        status = psa_import_key(&attributes,
+                                (const uint8_t *) &i, sizeof(i),
+                                &keys[i]);
+        if (status == PSA_ERROR_INSUFFICIENT_MEMORY) {
+            break;
+        }
+        PSA_ASSERT(status);
+        i++;
+    }
+    TEST_EQUAL(status, PSA_ERROR_INSUFFICIENT_MEMORY);
+    TEST_ASSERT(i > 0);
+
+    /* Leave exactly one slot free: the encryption key import consumes it,
+     * then the decryption key import fails. */
+    PSA_ASSERT(psa_destroy_key(keys[--i]));
+    keys[i] = MBEDTLS_SVC_KEY_ID_INIT;
+    mbedtls_psa_get_stats(&stats_before);
+
+    traffic_keys.key_len = 16;
+    traffic_keys.iv_len = 12;
+    TEST_ASSERT(mbedtls_ssl_tls13_populate_transform(
+                    &transform, MBEDTLS_SSL_IS_CLIENT,
+                    MBEDTLS_TLS1_3_AES_128_GCM_SHA256,
+                    &traffic_keys, NULL) != 0);
+
+    /* The first import must be rolled back when the second one fails. */
+    TEST_ASSERT(mbedtls_svc_key_id_is_null(transform.psa_key_enc));
+
+    /* Also check the key store itself: the failed operation must neither
+     * consume a slot nor leave the only free slot unusable. */
+    mbedtls_psa_get_stats(&stats_after);
+    TEST_EQUAL(stats_after.volatile_slots, stats_before.volatile_slots);
+    TEST_EQUAL(stats_after.empty_slots, stats_before.empty_slots);
+    PSA_ASSERT(psa_import_key(&attributes,
+                              (const uint8_t *) &i, sizeof(i),
+                              &keys[i]));
+    i++;
+
+exit:
+    mbedtls_ssl_transform_free(&transform);
+    while (i > 0) {
+        psa_destroy_key(keys[--i]);
+    }
+    mbedtls_free(keys);
+    PSA_DONE();
+#if defined(MBEDTLS_PSA_KEY_STORE_DYNAMIC) && defined(MBEDTLS_TEST_HOOKS)
+    mbedtls_test_hook_psa_volatile_key_slice_length = NULL;
+#endif
+}
+/* END_CASE */
+
 /* BEGIN_CASE depends_on:MBEDTLS_SSL_PROTO_TLS1_3 */
 void ssl_tls13_key_evolution(int hash_alg,
                              data_t *secret,