Merge pull request #10715 from Nadav0077/harden-tls13-session-load

Harden TLS 1.3 serialized session loading

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

@@ -0,0 +1,5 @@
+Bugfix
+   * Reject malformed TLS 1.3 serialized sessions instead of accepting
+     unterminated strings or extra trailing data.
+   * Reject serialized SSL contexts whose DTLS Connection ID length exceeds
+     the maximum supported size, instead of overflowing the internal buffer.

library/ssl_tls.c

@@ -3634,6 +3634,12 @@ static int ssl_tls13_session_load(mbedtls_ssl_session *session,
         }
 
         if (alpn_len > 0) {
+            /* The data is about to be used as a null-terminated string, so
+             * check that it actually is one. */
+            if (p[alpn_len - 1] != '\0') {
+                return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
+            }
+
             int ret = mbedtls_ssl_session_set_ticket_alpn(session, (char *) p);
             if (ret != 0) {
                 return ret;
@@ -3659,11 +3665,16 @@ static int ssl_tls13_session_load(mbedtls_ssl_session *session,
             return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
         }
         if (hostname_len > 0) {
-            session->hostname = mbedtls_calloc(1, hostname_len);
-            if (session->hostname == NULL) {
-                return MBEDTLS_ERR_SSL_ALLOC_FAILED;
+            /* The data is about to be used as a null-terminated string, so
+             * check that it actually is one. */
+            if (p[hostname_len - 1] != '\0') {
+                return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
+            }
+
+            int ret = mbedtls_ssl_session_set_hostname(session, (const char *) p);
+            if (ret != 0) {
+                return ret;
             }
-            memcpy(session->hostname, p, hostname_len);
             p += hostname_len;
         }
 #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */
@@ -3701,6 +3712,10 @@ static int ssl_tls13_session_load(mbedtls_ssl_session *session,
     }
 #endif /* MBEDTLS_SSL_CLI_C */
 
+    if (p != end) {
+        return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
+    }
+
     return 0;
 
 }
@@ -4999,6 +5014,10 @@ static int ssl_context_load(mbedtls_ssl_context *ssl,
         return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
     }
 
+    if (ssl->transform->in_cid_len > sizeof(ssl->transform->in_cid)) {
+        return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
+    }
+
     memcpy(ssl->transform->in_cid, p, ssl->transform->in_cid_len);
     p += ssl->transform->in_cid_len;
 
@@ -5008,6 +5027,10 @@ static int ssl_context_load(mbedtls_ssl_context *ssl,
         return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
     }
 
+    if (ssl->transform->out_cid_len > sizeof(ssl->transform->out_cid)) {
+        return MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
+    }
+
     memcpy(ssl->transform->out_cid, p, ssl->transform->out_cid_len);
     p += ssl->transform->out_cid_len;
 #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */

tests/suites/test_suite_ssl.data

@@ -3028,6 +3028,22 @@ TLS 1.3: SRV: Session serialization, load buffer size
 depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_SRV_C
 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
+ssl_tls13_session_load_rejects_bad_serialized_data:MBEDTLS_SSL_IS_CLIENT:TEST_TLS13_SESSION_LOAD_TRAILING_DATA
+
+TLS 1.3: Session serialization rejects hostname without terminator
+depends_on: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
+
+TLS 1.3: Session serialization rejects ALPN without terminator
+depends_on: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 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:
+
 Test configuration of EC groups through mbedtls_ssl_conf_groups()
 conf_group:
 

tests/suites/test_suite_ssl.function

@@ -25,6 +25,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 */
+#define TEST_TLS13_SESSION_LOAD_TRAILING_DATA    0
+#define TEST_TLS13_SESSION_LOAD_HOSTNAME_NO_NUL  1
+#define TEST_TLS13_SESSION_LOAD_ALPN_NO_NUL      2
+
 #if (!defined(MBEDTLS_SSL_PROTO_TLS1_2)) && \
     defined(MBEDTLS_SSL_EARLY_DATA) && defined(MBEDTLS_SSL_CLI_C) && \
     defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_DEBUG_C) && \
@@ -2733,6 +2738,170 @@ exit:
 }
 /* END_CASE */
 
+/* BEGIN_CASE depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS */
+void ssl_tls13_session_load_rejects_bad_serialized_data(int endpoint_type,
+                                                        int mutation)
+{
+    mbedtls_ssl_session session, restored;
+    unsigned char *buf = NULL;
+    size_t len, bad_len, i;
+    unsigned char *field = NULL;
+    const unsigned char hostname[] = "hostname example";
+    const unsigned char alpn[] = "ALPNExample";
+    const unsigned char *string_to_corrupt = NULL;
+    size_t string_len = 0;
+
+    mbedtls_ssl_session_init(&session);
+    mbedtls_ssl_session_init(&restored);
+    USE_PSA_INIT();
+
+    TEST_EQUAL(mbedtls_test_ssl_tls13_populate_session(
+                   &session, 42, endpoint_type),  0);
+
+    TEST_EQUAL(mbedtls_ssl_session_save(&session, NULL, 0, &len),
+               MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL);
+    TEST_CALLOC(buf, len + 1);
+    TEST_EQUAL(mbedtls_ssl_session_save(&session, buf, len, &len),  0);
+
+    /* The serialized data must load successfully before we corrupt it,
+     * otherwise the test below would not prove anything. */
+    TEST_EQUAL(mbedtls_ssl_session_load(&restored, buf, len), 0);
+    mbedtls_ssl_session_free(&restored);
+    mbedtls_ssl_session_init(&restored);
+
+    bad_len = len;
+    switch (mutation) {
+        case TEST_TLS13_SESSION_LOAD_TRAILING_DATA:
+            bad_len = len + 1;
+            buf[len] = 0;
+            break;
+
+        case TEST_TLS13_SESSION_LOAD_HOSTNAME_NO_NUL:
+            string_to_corrupt = hostname;
+            string_len = sizeof(hostname);
+            break;
+
+        case TEST_TLS13_SESSION_LOAD_ALPN_NO_NUL:
+            string_to_corrupt = alpn;
+            string_len = sizeof(alpn);
+            break;
+
+        default:
+            TEST_ASSERT(0);
+    }
+
+    if (string_to_corrupt != NULL) {
+        for (i = 0; i + string_len <= len; i++) {
+            if (memcmp(buf + i, string_to_corrupt, string_len) == 0) {
+                field = buf + i;
+                break;
+            }
+        }
+        TEST_ASSERT(field != NULL);
+        field[string_len - 1] = 'X';
+    }
+
+    TEST_EQUAL(mbedtls_ssl_session_load(&restored, buf, bad_len),
+               MBEDTLS_ERR_SSL_BAD_INPUT_DATA);
+
+exit:
+    mbedtls_ssl_session_free(&session);
+    mbedtls_ssl_session_free(&restored);
+    mbedtls_free(buf);
+    USE_PSA_DONE();
+}
+/* END_CASE */
+
+/* BEGIN_CASE 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 */
+void ssl_context_load_rejects_oob_cid_length(void)
+{
+    /* Sentinel CID so we can find its location inside the serialized buffer. */
+    const unsigned char server_cid[] = {
+        0xAB, 0xCD, 0xEF, 0x12, 0x34, 0x56, 0x78, 0x9A
+    };
+    const unsigned char client_cid[] = {
+        0x9A, 0x78, 0x56, 0x34, 0x12, 0xEF, 0xCD, 0xAB
+    };
+
+    mbedtls_test_handshake_test_options options;
+    mbedtls_test_ssl_endpoint client_ep, server_ep;
+    unsigned char *context_buf = NULL;
+    size_t context_buf_len, i;
+    unsigned char *cid_len_p = NULL;
+
+    memset(&client_ep, 0, sizeof(client_ep));
+    memset(&server_ep, 0, sizeof(server_ep));
+
+    MD_OR_USE_PSA_INIT();
+    mbedtls_test_init_handshake_options(&options);
+
+    options.dtls = 1;
+    options.expected_negotiated_version = MBEDTLS_SSL_VERSION_TLS1_2;
+
+    TEST_EQUAL(mbedtls_test_ssl_endpoint_init(
+                   &client_ep, MBEDTLS_SSL_IS_CLIENT, &options), 0);
+    TEST_EQUAL(mbedtls_test_ssl_endpoint_init(
+                   &server_ep, MBEDTLS_SSL_IS_SERVER, &options), 0);
+
+    /* Configure CID with unique patterns so we can locate the length byte in
+     * the serialized context. Both peers must agree on the CID length via
+     * conf_cid() before set_cid() will accept the value. */
+    TEST_EQUAL(mbedtls_ssl_conf_cid(&client_ep.conf, sizeof(client_cid),
+                                    MBEDTLS_SSL_UNEXPECTED_CID_FAIL), 0);
+    TEST_EQUAL(mbedtls_ssl_conf_cid(&server_ep.conf, sizeof(server_cid),
+                                    MBEDTLS_SSL_UNEXPECTED_CID_FAIL), 0);
+    TEST_EQUAL(mbedtls_ssl_set_cid(&client_ep.ssl, MBEDTLS_SSL_CID_ENABLED,
+                                   client_cid, sizeof(client_cid)), 0);
+    TEST_EQUAL(mbedtls_ssl_set_cid(&server_ep.ssl, MBEDTLS_SSL_CID_ENABLED,
+                                   server_cid, sizeof(server_cid)), 0);
+
+    TEST_EQUAL(mbedtls_test_ssl_dtls_join_endpoints(&client_ep, &server_ep), 0);
+
+    TEST_ASSERT(mbedtls_test_ssl_perform_connection(&options, &client_ep,
+                                                    &server_ep));
+
+    /* Serialize the post-handshake server context. */
+    TEST_EQUAL(mbedtls_ssl_context_save(&server_ep.ssl, NULL, 0,
+                                        &context_buf_len),
+               MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL);
+    TEST_CALLOC(context_buf, context_buf_len);
+    TEST_EQUAL(mbedtls_ssl_context_save(&server_ep.ssl, context_buf,
+                                        context_buf_len, &context_buf_len), 0);
+
+    /* The serialized format writes [in_cid_len][in_cid bytes]; locate the
+     * length byte by searching for our sentinel CID. */
+    for (i = 1; i + sizeof(server_cid) <= context_buf_len; i++) {
+        if (context_buf[i - 1] == (unsigned char) sizeof(server_cid) &&
+            memcmp(context_buf + i, server_cid, sizeof(server_cid)) == 0) {
+            cid_len_p = context_buf + i - 1;
+            break;
+        }
+    }
+    TEST_ASSERT(cid_len_p != NULL);
+
+    /* Corrupt the length to the smallest value strictly greater than
+     * sizeof(in_cid), so we exercise the new bounds check rather than the
+     * pre-existing buffer-size check that would also trip on huge values. */
+    *cid_len_p = (unsigned char) (MBEDTLS_SSL_CID_IN_LEN_MAX + 1);
+
+    /* Reinitialise the server SSL context so we can load into a fresh one. */
+    mbedtls_ssl_free(&server_ep.ssl);
+    mbedtls_ssl_init(&server_ep.ssl);
+    TEST_EQUAL(mbedtls_ssl_setup(&server_ep.ssl, &server_ep.conf), 0);
+
+    TEST_EQUAL(mbedtls_ssl_context_load(&server_ep.ssl, context_buf,
+                                        context_buf_len),
+               MBEDTLS_ERR_SSL_BAD_INPUT_DATA);
+
+exit:
+    mbedtls_test_ssl_endpoint_free(&client_ep);
+    mbedtls_test_ssl_endpoint_free(&server_ep);
+    mbedtls_test_free_handshake_options(&options);
+    mbedtls_free(context_buf);
+    MD_OR_USE_PSA_DONE();
+}
+/* END_CASE */
+
 /* BEGIN_CASE */
 void ssl_session_serialize_version_check(int corrupt_major,
                                          int corrupt_minor,