Merge pull request #10791 from nvxbug/tls12-session-id-bounds

Reject out-of-bounds session ID length in ssl_tls12_session_load

ChangeLog.d/serialized-data-load-hardening.txt

@@ -0,0 +1,4 @@
+Bugfix
+   * Reject serialized TLS 1.2 sessions whose session ID length exceeds 32,
+     instead of accepting an out-of-range length that is later used to read
+     past the end of the 32-byte session ID buffer.

framework

@@ -1 +1 @@
-Subproject commit dde0c4a0e448a0552f18817dcea633bb851fd288
+Subproject commit a3079552234917a9831dd70693983af5e62c70de

library/ssl_tls.c

@@ -3254,6 +3254,9 @@ static int ssl_tls12_session_load(mbedtls_ssl_session *session,
     }
 
     session->id_len = *p++;
+    if (session->id_len > sizeof(session->id)) {
+        return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
+    }
     memcpy(session->id, p, 32);
     p += 32;
 

library/ssl_tls13_generic.c

@@ -341,11 +341,12 @@ static int ssl_tls13_parse_certificate_verify(mbedtls_ssl_context *ssl,
 
     MBEDTLS_SSL_DEBUG_BUF(3, "verify hash", verify_hash, verify_hash_len);
 
-    if ((ret = mbedtls_pk_verify_ext((mbedtls_pk_sigalg_t) sig_alg,
+    ret = mbedtls_pk_verify_ext((mbedtls_pk_sigalg_t) sig_alg,
-                                     &ssl->session_negotiate->peer_cert->pk,
+                                &ssl->session_negotiate->peer_cert->pk,
-                                     md_alg, verify_hash, verify_hash_len,
+                                md_alg, verify_hash, verify_hash_len,
-                                     p, signature_len)) == 0) {
+                                p, signature_len);
-        return 0;
+    if (ret == 0) {
+        return ret;
     }
     MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_pk_verify_ext", ret);
 

scripts/generate_errors.pl

@@ -188,7 +188,7 @@ foreach my $match (@matches)
             $first = 0;
             foreach my $dep (split(/,/, ${$old_define_name}))
             {
-                ${$code_check} .= " || \n          " if ($first++);
+                ${$code_check} .= " ||\n          " if ($first++);
                 ${$code_check} .= "${$old_define_prefix}${dep}${$old_define_suffix}";
             }
             ${$code_check} .= " */\n\n";
@@ -229,7 +229,7 @@ if ($ll_old_define[0] ne "")
     my $first = 0;
     foreach my $dep (split(/,/, $ll_old_define[0]))
     {
-        $ll_code_check .= " || \n          " if ($first++);
+        $ll_code_check .= " ||\n          " if ($first++);
         $ll_code_check .= "${ll_old_define[1]}${dep}${ll_old_define[2]}";
     }
     $ll_code_check .= " */\n";
@@ -240,7 +240,7 @@ if ($hl_old_define[0] ne "")
     my $first = 0;
     foreach my $dep (split(/,/, $hl_old_define[0]))
     {
-        $hl_code_check .= " || \n          " if ($first++);
+        $hl_code_check .= " ||\n          " if ($first++);
         $hl_code_check .= "${hl_old_define[1]}${dep}${hl_old_define[2]}";
     }
     $hl_code_check .= " */\n";

tests/suites/test_suite_ssl.data

@@ -3043,21 +3043,29 @@ depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_SRV_
 ssl_serialize_session_load_buf_size:0:"":MBEDTLS_SSL_IS_SERVER:MBEDTLS_SSL_VERSION_TLS1_3
 
 TLS 1.3: Session serialization rejects trailing data
-depends_on:MBEDTLS_SSL_CLI_C
+depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C
-ssl_tls13_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_IS_CLIENT:TEST_TLS13_SESSION_LOAD_TRAILING_DATA
+ssl_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_VERSION_TLS1_3:MBEDTLS_SSL_IS_CLIENT:TEST_SESSION_LOAD_TRAILING_DATA
 
 TLS 1.3: Session serialization rejects hostname without terminator
-depends_on:MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SERVER_NAME_INDICATION
+depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C:MBEDTLS_SSL_SERVER_NAME_INDICATION
-ssl_tls13_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_IS_CLIENT:TEST_TLS13_SESSION_LOAD_HOSTNAME_NO_NUL
+ssl_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_VERSION_TLS1_3:MBEDTLS_SSL_IS_CLIENT:TEST_TLS13_SESSION_LOAD_HOSTNAME_NO_NUL
 
 TLS 1.3: Session serialization rejects ALPN without terminator
-depends_on:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_EARLY_DATA:MBEDTLS_SSL_ALPN
+depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_EARLY_DATA:MBEDTLS_SSL_ALPN
-ssl_tls13_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_IS_SERVER:TEST_TLS13_SESSION_LOAD_ALPN_NO_NUL
+ssl_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_VERSION_TLS1_3:MBEDTLS_SSL_IS_SERVER:TEST_TLS13_SESSION_LOAD_ALPN_NO_NUL
 
 SSL context serialization rejects out-of-bounds DTLS CID length
 depends_on:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_PROTO_DTLS:MBEDTLS_SSL_DTLS_CONNECTION_ID:MBEDTLS_SSL_CONTEXT_SERIALIZATION:PSA_HAVE_ALG_SOME_RSA_SIGN:PSA_WANT_ECC_SECP_R1_384:PSA_WANT_ALG_SHA_256:MBEDTLS_CAN_HANDLE_RSA_TEST_KEY:TEST_GCM_OR_CHACHAPOLY_ENABLED
 ssl_context_load_rejects_oob_cid_length:
 
+TLS 1.2: Session serialization rejects out-of-bounds session ID length (client)
+depends_on:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_CLI_C
+ssl_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_VERSION_TLS1_2:MBEDTLS_SSL_IS_CLIENT:TEST_TLS12_SESSION_LOAD_OOB_ID_LEN
+
+TLS 1.2: Session serialization rejects out-of-bounds session ID length (server)
+depends_on:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_SRV_C
+ssl_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_VERSION_TLS1_2:MBEDTLS_SSL_IS_SERVER:TEST_TLS12_SESSION_LOAD_OOB_ID_LEN
+
 Test configuration of EC groups through mbedtls_ssl_conf_groups()
 conf_group:
 

tests/suites/test_suite_ssl.function

@@ -26,10 +26,11 @@
 #define TEST_EARLY_DATA_NO_INITIAL_ALPN 6
 #define TEST_EARLY_DATA_NO_LATER_ALPN 7
 
-/* Mutations for ssl_tls13_session_load_rejects_bad_serialized_data */
+/* Mutations for ssl_session_load_rejects_bad_serialized_data */
-#define TEST_TLS13_SESSION_LOAD_TRAILING_DATA    0
+#define TEST_SESSION_LOAD_TRAILING_DATA          0
 #define TEST_TLS13_SESSION_LOAD_HOSTNAME_NO_NUL  1
 #define TEST_TLS13_SESSION_LOAD_ALPN_NO_NUL      2
+#define TEST_TLS12_SESSION_LOAD_OOB_ID_LEN       3
 
 #if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \
     defined(MBEDTLS_SSL_SESSION_TICKETS) && \
@@ -2918,9 +2919,10 @@ exit:
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS */
+/* BEGIN_CASE */
-void ssl_tls13_session_load_rejects_bad_serialized_data(int endpoint_type,
+void ssl_session_load_rejects_bad_serialized_data(int tls_version,
-                                                        int mutation)
+                                                  int endpoint_type,
+                                                  int mutation)
 {
     mbedtls_ssl_session session, restored;
     unsigned char *buf = NULL;
@@ -2931,12 +2933,34 @@ void ssl_tls13_session_load_rejects_bad_serialized_data(int endpoint_type,
     const unsigned char *string_to_corrupt = NULL;
     size_t string_len = 0;
 
+    (void) hostname;
+    (void) alpn;
+
     mbedtls_ssl_session_init(&session);
     mbedtls_ssl_session_init(&restored);
     USE_PSA_INIT();
 
-    TEST_EQUAL(mbedtls_test_ssl_tls13_populate_session(
+    switch (tls_version) {
-                   &session, 42, endpoint_type),  0);
+#if defined(MBEDTLS_SSL_PROTO_TLS1_2)
+        case MBEDTLS_SSL_VERSION_TLS1_2:
+            TEST_EQUAL(mbedtls_test_ssl_tls12_populate_session(
+                           &session, 0, endpoint_type, NULL),  0);
+            /* Sentinel session ID so we can locate the length byte later.
+             * The detection code below relies on id_len being exactly
+             * sizeof(session.id), so force it. */
+            memset(session.id, 0x5A, sizeof(session.id));
+            session.id_len = sizeof(session.id);
+            break;
+#endif
+#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && defined(MBEDTLS_SSL_SESSION_TICKETS)
+        case MBEDTLS_SSL_VERSION_TLS1_3:
+            TEST_EQUAL(mbedtls_test_ssl_tls13_populate_session(
+                           &session, 42, endpoint_type),  0);
+            break;
+#endif
+        default:
+            TEST_FAIL("unsupported TLS version");
+    }
 
     TEST_EQUAL(mbedtls_ssl_session_save(&session, NULL, 0, &len),
                MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL);
@@ -2951,7 +2975,7 @@ void ssl_tls13_session_load_rejects_bad_serialized_data(int endpoint_type,
 
     bad_len = len;
     switch (mutation) {
-        case TEST_TLS13_SESSION_LOAD_TRAILING_DATA:
+        case TEST_SESSION_LOAD_TRAILING_DATA:
             bad_len = len + 1;
             buf[len] = 0;
             break;
@@ -2966,6 +2990,28 @@ void ssl_tls13_session_load_rejects_bad_serialized_data(int endpoint_type,
             string_len = sizeof(alpn);
             break;
 
+        case TEST_TLS12_SESSION_LOAD_OOB_ID_LEN:
+            /* The format writes [id_len][32 id bytes]; find the length byte
+             * that precedes the sentinel run and push it out of range. */
+            for (i = 1; i + sizeof(session.id) <= len; i++) {
+                size_t k = 0;
+                if (buf[i - 1] != (unsigned char) sizeof(session.id)) {
+                    continue;
+                }
+                while (k < sizeof(session.id) && buf[i + k] == 0x5A) {
+                    k++;
+                }
+                if (k == sizeof(session.id)) {
+                    field = buf + i - 1;
+                    break;
+                }
+            }
+            TEST_ASSERT(field != NULL);
+            /* Smallest value strictly greater than sizeof(id). */
+            *field = (unsigned char) (sizeof(session.id) + 1);
+            field = NULL;
+            break;
+
         default:
             TEST_ASSERT(0);
     }

tests/suites/test_suite_x509parse.data

@@ -1769,15 +1769,15 @@ x509parse_crt:"307d3068a0030201008204deadbeef300d06092a864886f70d01010b0500300c3
 
 X509 CRT ASN1 (TBS, inv SubPubKeyInfo, inv internal bitstring length)
 depends_on:PSA_HAVE_ALG_SOME_RSA_VERIFY:PSA_WANT_ALG_SHA_256
-x509parse_crt:"308180306ba0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743015300d06092A864886F70D0101010500030400300000300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_PUBKEY
+x509parse_crt:"308180306ba0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743015300d06092A864886F70D0101010500030400300000300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_ALG
 
 X509 CRT ASN1 (TBS, inv SubPubKeyInfo, inv internal bitstring tag)
 depends_on:PSA_HAVE_ALG_SOME_RSA_VERIFY:PSA_WANT_ALG_SHA_256
-x509parse_crt:"308180306ba0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743015300d06092A864886F70D0101010500030400310000300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_PUBKEY
+x509parse_crt:"308180306ba0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743015300d06092A864886F70D0101010500030400310000300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_ALG
 
 X509 CRT ASN1 (TBS, inv SubPubKeyInfo, inv RSA modulus)
 depends_on:PSA_HAVE_ALG_SOME_RSA_VERIFY:PSA_WANT_ALG_SHA_256
-x509parse_crt:"3081873072a0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374301c300d06092A864886F70D0101010500030b0030080202ffff0302ffff300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_PUBKEY
+x509parse_crt:"3081873072a0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374301c300d06092A864886F70D0101010500030b0030080202ffff0302ffff300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_ALG
 
 X509 CRT ASN1 (TBS, inv SubPubKeyInfo, total length mismatch)
 depends_on:PSA_HAVE_ALG_SOME_RSA_VERIFY:PSA_WANT_ALG_SHA_256
@@ -1785,11 +1785,11 @@ x509parse_crt:"3081893074a0030201008204deadbeef300d06092a864886f70d01010b0500300
 
 X509 CRT ASN1 (TBS, inv SubPubKeyInfo, check failed)
 depends_on:PSA_HAVE_ALG_SOME_RSA_VERIFY:PSA_WANT_ALG_SHA_256
-x509parse_crt:"3081873072a0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374301c300d06092A864886F70D0101010500030b0030080202ffff0202ffff300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_PUBKEY
+x509parse_crt:"3081873072a0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374301c300d06092A864886F70D0101010500030b0030080202ffff0202ffff300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_ALG
 
 X509 CRT ASN1 (TBS, inv SubPubKeyInfo, check failed, expanded length notation)
 depends_on:PSA_HAVE_ALG_SOME_RSA_VERIFY:PSA_WANT_ALG_SHA_256
-x509parse_crt:"308196308180a0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092A864886F70D010101050003190030160210fffffffffffffffffffffffffffffffe0202ffff300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_PUBKEY
+x509parse_crt:"308196308180a0030201008204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092A864886F70D010101050003190030160210fffffffffffffffffffffffffffffffe0202ffff300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_PK_INVALID_ALG
 
 # We expect an extension parsing error here because the IssuerID is optional.
 # Hence, if we find an ASN.1 tag doesn't match the IssuerID, we assume the

tests/suites/test_suite_x509parse.function

@@ -1136,12 +1136,15 @@ void x509parse_crt(data_t *buf, char *result_str, int result)
 #else
     ((void) result_str);
 #endif
-    /* Tests whose result is MBEDTLS_ERR_PK_INVALID_PUBKEY might return
-     * MBEDTLS_ERR_ASN1_UNEXPECTED_TAG until psa#308 is merged. This variable
-     * is therefore used for backward compatiblity and will be removed in
-     * mbedtls#10229. */
-    int result_back_comp = result;
     int res;
+    /* After psa#827 some failing tests that were returning MBEDTLS_ERR_PK_INVALID_PUBKEY
+     * will return MBEDTLS_ERR_PK_INVALID_ALG instead. We need a temporary
+     * backward compatibility fix until psa#827 is merged.
+     */
+    int result_back_comp = result;
+    if (result == MBEDTLS_ERR_PK_INVALID_ALG) {
+        result_back_comp = MBEDTLS_ERR_PK_INVALID_PUBKEY;
+    }
 
     mbedtls_x509_crt_init(&crt);
     USE_PSA_INIT();

tf-psa-crypto

@@ -1 +1 @@
-Subproject commit 73c5da561c8e5253db7b1fb440eda86fde8d8024
+Subproject commit a92cb6a97d84762d2f4355eae99af7b9ff5c6ae1