Merge 83d132b47d720ec04d4d14daa8510ded4414b8e3 into 03e1438089654fb884ac48e0f3cd0049cfe2c102

Fix resource leak in populate transform

ChangeLog.d/tls13-empty-psk-kex-modes.txt

@@ -0,0 +1,6 @@
+Bugfix
+   * Reject a TLS 1.3 ClientHello whose psk_key_exchange_modes extension has
+     a zero-length ke_modes vector, instead of accepting it and recording an
+     empty set of PSK key exchange modes, which could lead to issuing
+     session tickets that can never be used for resumption. Reported in
+     #10467 and #10888.

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

@@ -0,0 +1,4 @@
+Bugfix
+   * Fix a resource leak in TLS 1.3 transform setup when importing the
+     decryption traffic key fails after the encryption traffic key has already
+     been imported. Fixes #10917.

library/ssl_tls13_keys.c

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

library/ssl_tls13_server.c

@@ -116,6 +116,13 @@ static int ssl_tls13_parse_key_exchange_modes_ext(mbedtls_ssl_context *ssl,
     /* Read ke_modes length (1 Byte) */
     MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 1);
     ke_modes_len = *p++;
+
+    if (ke_modes_len == 0) {
+        MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR,
+                                     MBEDTLS_ERR_SSL_DECODE_ERROR);
+        return MBEDTLS_ERR_SSL_DECODE_ERROR;
+    }
+
     /* Currently, there are only two PSK modes, so even without looking
      * at the content, something's wrong if the list has more than 2 items. */
     if (ke_modes_len > 2) {

tests/suites/test_suite_ssl.data

@@ -2856,6 +2856,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
@@ -3650,3 +3653,6 @@ tls13_record_boundary_alignment:MBEDTLS_SSL_IS_SERVER:MBEDTLS_SSL_HS_FINISHED
 TLS 1.3 EndOfEarlyData record boundary alignment enforcement
 depends_on:MBEDTLS_SSL_EARLY_DATA:MBEDTLS_SSL_SESSION_TICKETS
 tls13_record_boundary_alignment:MBEDTLS_SSL_IS_SERVER:MBEDTLS_SSL_HS_END_OF_EARLY_DATA
+
+TLS 1.3: SRV: reject zero-length psk_key_exchange_modes
+tls13_malformed_psk_kex_modes:

tests/suites/test_suite_ssl.function

@@ -8,6 +8,7 @@
 #endif /* MBEDTLS_PK_HAVE_PRIVATE_HEADER */
 #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>
 
@@ -36,6 +37,35 @@
 #define TEST_SSL_CONTEXT_LOAD_OOB_IN_CID   0
 #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)
+#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 */
+#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) && \
@@ -607,7 +637,90 @@ static void tls_tweak_in_msglen(void *ctx, int level,
 }
 #endif
 
+#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && defined(MBEDTLS_DEBUG_C) && \
+    defined(MBEDTLS_SSL_CLI_C) && defined(MBEDTLS_SSL_SRV_C) && \
+    defined(MBEDTLS_TEST_AT_LEAST_ONE_TLS1_3_CIPHERSUITE) && \
+    defined(MBEDTLS_TEST_HAS_DEFAULT_EC_GROUP) && \
+    defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED) && \
+    defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED) && \
+    defined(PSA_WANT_ALG_SHA_256) && \
+    defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT) && \
+    defined(PSA_WANT_ALG_RSA_PKCS1V15_SIGN)
+static unsigned char *tls13_find_psk_kex_modes_len_byte(unsigned char *hs,
+                                                        size_t hs_len)
+{
+    unsigned char *p = hs;
+    unsigned char *end = hs + hs_len;
+    size_t len;
 
+    /* legacy_version (2) + random (32) */
+    if ((size_t) (end - p) < 34) {
+        return NULL;
+    }
+    p += 34;
+
+    /* session_id */
+    if ((size_t) (end - p) < 1) {
+        return NULL;
+    }
+    len = p[0];
+    p += 1;
+    if ((size_t) (end - p) < len) {
+        return NULL;
+    }
+    p += len;
+
+    /* cipher_suites */
+    if ((size_t) (end - p) < 2) {
+        return NULL;
+    }
+    len = ((size_t) p[0] << 8) | p[1];
+    p += 2;
+    if ((size_t) (end - p) < len) {
+        return NULL;
+    }
+    p += len;
+
+    /* compression_methods */
+    if ((size_t) (end - p) < 1) {
+        return NULL;
+    }
+    len = p[0];
+    p += 1;
+    if ((size_t) (end - p) < len) {
+        return NULL;
+    }
+    p += len;
+
+    /* extensions */
+    if ((size_t) (end - p) < 2) {
+        return NULL;
+    }
+    len = ((size_t) p[0] << 8) | p[1];
+    p += 2;
+    if ((size_t) (end - p) < len) {
+        return NULL;
+    }
+    end = p + len;
+
+    while ((size_t) (end - p) >= 4) {
+        unsigned ext_type = ((unsigned) p[0] << 8) | p[1];
+        size_t ext_len = ((size_t) p[2] << 8) | p[3];
+        if ((size_t) (end - p) < 4 + ext_len) {
+            return NULL;
+        }
+        if (ext_type == MBEDTLS_TLS_EXT_PSK_KEY_EXCHANGE_MODES) {
+            /* extension data starts with the 1-byte ke_modes vector length */
+            if (ext_len < 1) {
+                return NULL;
+            }
+            return p + 4;
+        }
+        p += 4 + ext_len;
+    }
+    return NULL;
+}
+#endif
 
 /* END_HEADER */
 
@@ -2489,6 +2602,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 */
+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,
@@ -7488,3 +7680,123 @@ exit:
     PSA_DONE();
 }
 /* END_CASE */
+
+/* BEGIN_CASE depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_DEBUG_C:MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SRV_C:MBEDTLS_TEST_AT_LEAST_ONE_TLS1_3_CIPHERSUITE:MBEDTLS_TEST_HAS_DEFAULT_EC_GROUP:MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED:MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED:PSA_WANT_ALG_SHA_256:MBEDTLS_X509_RSASSA_PSS_SUPPORT:PSA_WANT_ALG_RSA_PKCS1V15_SIGN */
+void tls13_malformed_psk_kex_modes()
+{
+    int ret = -1;
+    mbedtls_test_ssl_endpoint client_ep, server_ep;
+    memset(&client_ep, 0, sizeof(client_ep));
+    memset(&server_ep, 0, sizeof(server_ep));
+
+    mbedtls_test_handshake_test_options client_options, server_options;
+    mbedtls_test_init_handshake_options(&client_options);
+    mbedtls_test_init_handshake_options(&server_options);
+
+    /* TLS alert 50 = decode_error */
+    mbedtls_test_ssl_log_pattern server_alert_pattern =
+    { "send alert level=2 message=50", 0 };
+
+    unsigned char *ke_modes_len_byte = NULL;
+
+    PSA_INIT();
+
+    mbedtls_debug_set_threshold(3);
+    server_options.srv_log_fun = mbedtls_test_ssl_log_analyzer;
+    server_options.srv_log_obj = &server_alert_pattern;
+
+    TEST_EQUAL(mbedtls_test_ssl_endpoint_init(&client_ep,
+                                              MBEDTLS_SSL_IS_CLIENT,
+                                              &client_options), 0);
+    TEST_EQUAL(mbedtls_test_ssl_endpoint_init(&server_ep,
+                                              MBEDTLS_SSL_IS_SERVER,
+                                              &server_options), 0);
+
+    TEST_EQUAL(mbedtls_test_mock_socket_connect(&client_ep.socket,
+                                                &server_ep.socket, 1024), 0);
+
+    /*
+     * Let the client write its ClientHello into its output socket buffer.
+     * The client then waits for the ServerHello. (Without early data, no
+     * CCS follows the ClientHello even in compatibility mode, see
+     * RFC 8446 appendix D.4.)
+     */
+    TEST_EQUAL(mbedtls_test_move_handshake_to_state(
+                   &client_ep.ssl, &server_ep.ssl,
+                   MBEDTLS_SSL_SERVER_HELLO), 0);
+    TEST_EQUAL(mbedtls_ssl_flush_output(&client_ep.ssl), 0);
+
+    /*
+     * Walk the records and handshake messages in the client output buffer,
+     * find the psk_key_exchange_modes extension in the ClientHello and
+     * corrupt the ke_modes vector length to zero. RFC 8446 4.2.9 requires
+     * ke_modes<1..255>, so a zero-length vector is a syntax error.
+     */
+    {
+        unsigned char *buf = client_ep.socket.output->buffer;
+        size_t buf_len = client_ep.socket.output->content_length;
+        size_t rec_pos = 0;
+
+        while (rec_pos + 5 <= buf_len && ke_modes_len_byte == NULL) {
+            unsigned char rec_type = buf[rec_pos];
+            size_t rec_len = MBEDTLS_GET_UINT16_BE(buf, rec_pos + 3);
+            unsigned char *rec = buf + rec_pos + 5;
+            size_t hs_pos = 0;
+
+            TEST_ASSERT(rec_pos + 5 + rec_len <= buf_len);
+
+            if (rec_type != MBEDTLS_SSL_MSG_HANDSHAKE) {
+                rec_pos += 5 + rec_len;
+                continue;
+            }
+
+            while (hs_pos + 4 <= rec_len && ke_modes_len_byte == NULL) {
+                unsigned char hs_type = rec[hs_pos];
+                size_t hs_len = ((size_t) rec[hs_pos + 1] << 16) |
+                                ((size_t) rec[hs_pos + 2] << 8) |
+                                (size_t) rec[hs_pos + 3];
+
+                TEST_ASSERT(hs_pos + 4 + hs_len <= rec_len);
+
+                if (hs_type == MBEDTLS_SSL_HS_CLIENT_HELLO) {
+                    ke_modes_len_byte =
+                        tls13_find_psk_kex_modes_len_byte(rec + hs_pos + 4,
+                                                          hs_len);
+                }
+                hs_pos += 4 + hs_len;
+            }
+
+            rec_pos += 5 + rec_len;
+        }
+    }
+
+    /* The client must have offered at least one mode; zero it. */
+    TEST_ASSERT(ke_modes_len_byte != NULL);
+    TEST_ASSERT(*ke_modes_len_byte >= 1);
+    *ke_modes_len_byte = 0;
+
+    /*
+     * The server must reject the malformed extension with
+     * MBEDTLS_ERR_SSL_DECODE_ERROR and send a decode_error alert.
+     */
+    ret = mbedtls_test_move_handshake_to_state(&server_ep.ssl,
+                                               &client_ep.ssl,
+                                               MBEDTLS_SSL_SERVER_HELLO);
+    TEST_EQUAL(ret, MBEDTLS_ERR_SSL_DECODE_ERROR);
+    TEST_ASSERT(server_alert_pattern.counter > 0);
+
+    ret = mbedtls_ssl_handshake_step(&client_ep.ssl);
+    TEST_EQUAL(ret, MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE);
+    TEST_EQUAL(mbedtls_ssl_get_fatal_alert(&client_ep.ssl),
+               MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR);
+
+
+exit:
+    mbedtls_test_ssl_endpoint_free(&client_ep);
+    mbedtls_test_ssl_endpoint_free(&server_ep);
+    mbedtls_test_free_handshake_options(&client_options);
+    mbedtls_test_free_handshake_options(&server_options);
+    mbedtls_debug_set_threshold(0);
+    PSA_DONE();
+}
+/* END_CASE */