From c2c8d1e142fb265b876a0c83104bc5f248955568 Mon Sep 17 00:00:00 2001 From: Naveed Date: Thu, 25 Jun 2026 13:05:46 +0530 Subject: [PATCH 1/7] Reject out-of-bounds ALPN length in ssl_context_load The serialized ALPN length was passed to memcmp() against each configured protocol without checking it against the remaining serialized data, so a malformed context could read past the end of the buffer. Bound it before the comparison, like the adjacent CID fields. Signed-off-by: Naveed --- ChangeLog.d/ssl-context-load-alpn-bounds.txt | 4 + library/ssl_tls.c | 4 + tests/suites/test_suite_ssl.data | 4 + tests/suites/test_suite_ssl.function | 83 ++++++++++++++++++++ 4 files changed, 95 insertions(+) create mode 100644 ChangeLog.d/ssl-context-load-alpn-bounds.txt diff --git a/ChangeLog.d/ssl-context-load-alpn-bounds.txt b/ChangeLog.d/ssl-context-load-alpn-bounds.txt new file mode 100644 index 0000000000..77f21d6834 --- /dev/null +++ b/ChangeLog.d/ssl-context-load-alpn-bounds.txt @@ -0,0 +1,4 @@ +Bugfix + * Reject serialized SSL contexts whose ALPN protocol length exceeds the + remaining serialized data, instead of reading past the end of the buffer + in mbedtls_ssl_context_load(). diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6a15d8bace..1f44fc71a7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5101,6 +5101,10 @@ static int ssl_context_load(mbedtls_ssl_context *ssl, alpn_len = *p++; + if ((size_t) (end - p) < alpn_len) { + return MBEDTLS_ERR_SSL_BAD_INPUT_DATA; + } + if (alpn_len != 0 && ssl->conf->alpn_list != NULL) { /* alpn_chosen should point to an item in the configured list */ for (cur = ssl->conf->alpn_list; *cur != NULL; cur++) { diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index a2703e1027..a421f4dbbf 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3058,6 +3058,10 @@ 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: +SSL context serialization rejects out-of-bounds ALPN length +depends_on:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_PROTO_DTLS:MBEDTLS_SSL_ALPN: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_alpn_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 diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 8a463c5472..24970af069 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -3128,6 +3128,89 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_PROTO_DTLS:MBEDTLS_SSL_ALPN: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_alpn_length(void) +{ + /* The negotiated protocol (alpn[0]) is serialized; alpn[1] is longer and is + * what the corrupted length will match against on load, so the unbounded + * memcmp() reads past the end of the buffer. */ + const char negotiated[] = "alpnsentinel"; + const char *alpn[] = { negotiated, "alpnsentinel-padding", NULL }; + + 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 *alpn_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); + + /* Offer the same protocol list on both peers so alpn[0] is negotiated and + * serialized into the context. */ + TEST_EQUAL(mbedtls_ssl_conf_alpn_protocols(&client_ep.conf, alpn), 0); + TEST_EQUAL(mbedtls_ssl_conf_alpn_protocols(&server_ep.conf, alpn), 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)); + + TEST_ASSERT(server_ep.ssl.alpn_chosen != NULL); + TEST_EQUAL(strcmp(server_ep.ssl.alpn_chosen, negotiated), 0); + + /* 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 [alpn_len][alpn bytes]; locate the length + * byte by searching for the negotiated protocol string. */ + for (i = 1; i + strlen(negotiated) <= context_buf_len; i++) { + if (context_buf[i - 1] == (unsigned char) strlen(negotiated) && + memcmp(context_buf + i, negotiated, strlen(negotiated)) == 0) { + alpn_len_p = context_buf + i - 1; + break; + } + } + TEST_ASSERT(alpn_len_p != NULL); + + /* Claim the length of the longer configured protocol, which is more bytes + * than remain in the buffer. */ + *alpn_len_p = (unsigned char) strlen(alpn[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, From 8b670951ebf0b92871f5c174273bed62f4f7b8c7 Mon Sep 17 00:00:00 2001 From: Naveed Date: Thu, 9 Jul 2026 13:17:05 +0530 Subject: [PATCH 2/7] Merge the two ssl_context_load OOB length tests Fold ssl_context_load_rejects_oob_cid_length and ssl_context_load_rejects_oob_alpn_length into a single parameterized test function, since they shared most of their code. Signed-off-by: Naveed --- tests/suites/test_suite_ssl.data | 8 +- tests/suites/test_suite_ssl.function | 174 +++++++++++---------------- 2 files changed, 76 insertions(+), 106 deletions(-) diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index a421f4dbbf..5e99c801db 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3055,12 +3055,12 @@ depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_SRV_ 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: +depends_on:MBEDTLS_SSL_DTLS_CONNECTION_ID +ssl_context_load_rejects_oob_length:TEST_SSL_CONTEXT_LOAD_OOB_CID SSL context serialization rejects out-of-bounds ALPN length -depends_on:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_PROTO_DTLS:MBEDTLS_SSL_ALPN: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_alpn_length: +depends_on:MBEDTLS_SSL_ALPN +ssl_context_load_rejects_oob_length:TEST_SSL_CONTEXT_LOAD_OOB_ALPN TLS 1.2: Session serialization rejects out-of-bounds session ID length (client) depends_on:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_CLI_C diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 24970af069..6381041c16 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -32,6 +32,10 @@ #define TEST_TLS13_SESSION_LOAD_ALPN_NO_NUL 2 #define TEST_TLS12_SESSION_LOAD_OOB_ID_LEN 3 +/* Corrupted length fields for ssl_context_load_rejects_oob_length */ +#define TEST_SSL_CONTEXT_LOAD_OOB_CID 0 +#define TEST_SSL_CONTEXT_LOAD_OOB_ALPN 1 + #if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_CLI_C) && defined(MBEDTLS_SSL_SRV_C) && \ @@ -3038,9 +3042,10 @@ exit: } /* 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) +/* BEGIN_CASE depends_on:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_PROTO_DTLS: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_length(int field) { +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) /* 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 @@ -3048,100 +3053,23 @@ void ssl_context_load_rejects_oob_cid_length(void) 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 depends_on:MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_SSL_PROTO_DTLS:MBEDTLS_SSL_ALPN: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_alpn_length(void) -{ +#endif +#if defined(MBEDTLS_SSL_ALPN) /* The negotiated protocol (alpn[0]) is serialized; alpn[1] is longer and is * what the corrupted length will match against on load, so the unbounded * memcmp() reads past the end of the buffer. */ const char negotiated[] = "alpnsentinel"; const char *alpn[] = { negotiated, "alpnsentinel-padding", NULL }; +#endif 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 *alpn_len_p = NULL; + const unsigned char *sentinel = NULL; + size_t sentinel_len = 0; + unsigned char corrupt_len = 0; + unsigned char *len_p = NULL; memset(&client_ep, 0, sizeof(client_ep)); memset(&server_ep, 0, sizeof(server_ep)); @@ -3157,18 +3085,62 @@ void ssl_context_load_rejects_oob_alpn_length(void) TEST_EQUAL(mbedtls_test_ssl_endpoint_init( &server_ep, MBEDTLS_SSL_IS_SERVER, &options), 0); - /* Offer the same protocol list on both peers so alpn[0] is negotiated and - * serialized into the context. */ - TEST_EQUAL(mbedtls_ssl_conf_alpn_protocols(&client_ep.conf, alpn), 0); - TEST_EQUAL(mbedtls_ssl_conf_alpn_protocols(&server_ep.conf, alpn), 0); + switch (field) { +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + case TEST_SSL_CONTEXT_LOAD_OOB_CID: + /* 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); + sentinel = server_cid; + sentinel_len = sizeof(server_cid); + /* The smallest value strictly greater than sizeof(in_cid), so we + * exercise the bounds check rather than the pre-existing + * buffer-size check that would also trip on huge values. */ + corrupt_len = (unsigned char) (MBEDTLS_SSL_CID_IN_LEN_MAX + 1); + break; +#endif +#if defined(MBEDTLS_SSL_ALPN) + case TEST_SSL_CONTEXT_LOAD_OOB_ALPN: + /* Offer the same protocol list on both peers so alpn[0] is + * negotiated and serialized into the context. */ + TEST_EQUAL(mbedtls_ssl_conf_alpn_protocols(&client_ep.conf, alpn), + 0); + TEST_EQUAL(mbedtls_ssl_conf_alpn_protocols(&server_ep.conf, alpn), + 0); + sentinel = (const unsigned char *) negotiated; + sentinel_len = strlen(negotiated); + /* The length of the longer configured protocol, which is more + * bytes than remain in the buffer. */ + corrupt_len = (unsigned char) strlen(alpn[1]); + break; +#endif + default: + TEST_ASSERT(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)); - TEST_ASSERT(server_ep.ssl.alpn_chosen != NULL); - TEST_EQUAL(strcmp(server_ep.ssl.alpn_chosen, negotiated), 0); +#if defined(MBEDTLS_SSL_ALPN) + if (field == TEST_SSL_CONTEXT_LOAD_OOB_ALPN) { + TEST_ASSERT(server_ep.ssl.alpn_chosen != NULL); + TEST_EQUAL(strcmp(server_ep.ssl.alpn_chosen, negotiated), 0); + } +#endif /* Serialize the post-handshake server context. */ TEST_EQUAL(mbedtls_ssl_context_save(&server_ep.ssl, NULL, 0, @@ -3178,20 +3150,18 @@ void ssl_context_load_rejects_oob_alpn_length(void) TEST_EQUAL(mbedtls_ssl_context_save(&server_ep.ssl, context_buf, context_buf_len, &context_buf_len), 0); - /* The serialized format writes [alpn_len][alpn bytes]; locate the length - * byte by searching for the negotiated protocol string. */ - for (i = 1; i + strlen(negotiated) <= context_buf_len; i++) { - if (context_buf[i - 1] == (unsigned char) strlen(negotiated) && - memcmp(context_buf + i, negotiated, strlen(negotiated)) == 0) { - alpn_len_p = context_buf + i - 1; + /* Both fields are serialized as [length byte][data bytes]; locate the + * length byte by searching for the sentinel data. */ + for (i = 1; i + sentinel_len <= context_buf_len; i++) { + if (context_buf[i - 1] == (unsigned char) sentinel_len && + memcmp(context_buf + i, sentinel, sentinel_len) == 0) { + len_p = context_buf + i - 1; break; } } - TEST_ASSERT(alpn_len_p != NULL); + TEST_ASSERT(len_p != NULL); - /* Claim the length of the longer configured protocol, which is more bytes - * than remain in the buffer. */ - *alpn_len_p = (unsigned char) strlen(alpn[1]); + *len_p = corrupt_len; /* Reinitialise the server SSL context so we can load into a fresh one. */ mbedtls_ssl_free(&server_ep.ssl); From 467e053210cc9b54c825cb6502ebd82c49054898 Mon Sep 17 00:00:00 2001 From: Naveed Date: Sat, 11 Jul 2026 10:55:10 +0530 Subject: [PATCH 3/7] Address review: verify pristine load, corrupt ALPN length by exactly one Load the serialized context once before corrupting it, to check the data is well-formed to begin with. Shorten the second ALPN protocol so the corrupted length overruns the serialized data by exactly one byte, and assert that property at runtime. Document that the corrupted load usually fails even without the bounds check, so the regression is only caught when the suite runs under ASan or Valgrind. Signed-off-by: Naveed --- tests/suites/test_suite_ssl.function | 42 ++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 6381041c16..c43a96d7f0 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -3055,11 +3055,17 @@ void ssl_context_load_rejects_oob_length(int field) }; #endif #if defined(MBEDTLS_SSL_ALPN) - /* The negotiated protocol (alpn[0]) is serialized; alpn[1] is longer and is - * what the corrupted length will match against on load, so the unbounded - * memcmp() reads past the end of the buffer. */ + /* The negotiated protocol (alpn[0]) is the last field in the serialized + * context, so exactly strlen(alpn[0]) bytes remain after its length byte. + * On load, ssl_context_load() only memcmp()s configured protocols whose + * strlen() equals the length byte, so the corrupted length has to match a + * configured protocol to reach the memcmp(): alpn[1] is one character + * longer than alpn[0], which makes the corrupted length point exactly one + * byte past the end of the serialized data. It shares alpn[0]'s prefix so + * that all in-bounds bytes match and the memcmp() cannot stop before + * reading past the end of the buffer. */ const char negotiated[] = "alpnsentinel"; - const char *alpn[] = { negotiated, "alpnsentinel-padding", NULL }; + const char *alpn[] = { negotiated, "alpnsentinel2", NULL }; #endif mbedtls_test_handshake_test_options options; @@ -3121,8 +3127,8 @@ void ssl_context_load_rejects_oob_length(int field) 0); sentinel = (const unsigned char *) negotiated; sentinel_len = strlen(negotiated); - /* The length of the longer configured protocol, which is more - * bytes than remain in the buffer. */ + /* One byte past the end of the serialized data; see the + * declaration of alpn[] above. */ corrupt_len = (unsigned char) strlen(alpn[1]); break; #endif @@ -3150,6 +3156,14 @@ void ssl_context_load_rejects_oob_length(int field) TEST_EQUAL(mbedtls_ssl_context_save(&server_ep.ssl, context_buf, context_buf_len, &context_buf_len), 0); + /* Double-check that the serialized data loads cleanly before it gets + * corrupted. */ + 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), 0); + /* Both fields are serialized as [length byte][data bytes]; locate the * length byte by searching for the sentinel data. */ for (i = 1; i + sentinel_len <= context_buf_len; i++) { @@ -3161,6 +3175,15 @@ void ssl_context_load_rejects_oob_length(int field) } TEST_ASSERT(len_p != NULL); +#if defined(MBEDTLS_SSL_ALPN) + if (field == TEST_SSL_CONTEXT_LOAD_OOB_ALPN) { + /* Double-check that the corrupted length overruns the serialized data + * by exactly one byte, as the ALPN field is the last one saved. */ + TEST_EQUAL((size_t) corrupt_len, + (size_t) (context_buf + context_buf_len - (len_p + 1)) + 1); + } +#endif + *len_p = corrupt_len; /* Reinitialise the server SSL context so we can load into a fresh one. */ @@ -3168,6 +3191,13 @@ void ssl_context_load_rejects_oob_length(int field) mbedtls_ssl_init(&server_ep.ssl); TEST_EQUAL(mbedtls_ssl_setup(&server_ep.ssl, &server_ep.conf), 0); + /* Note that this rejection alone does not prove the bounds check works: + * without it the ALPN case usually still fails with + * MBEDTLS_ERR_SSL_BAD_INPUT_DATA, because the out-of-bounds bytes are + * unlikely to match a configured protocol so the alpn_chosen lookup + * fails afterwards. What the missing check leaks is the out-of-bounds + * read in memcmp() itself, so this test case only catches a regression + * when the test suite runs under ASan or Valgrind. */ TEST_EQUAL(mbedtls_ssl_context_load(&server_ep.ssl, context_buf, context_buf_len), MBEDTLS_ERR_SSL_BAD_INPUT_DATA); From 05f02b1e40cc07f793de5dab618f60f1a6a9ce64 Mon Sep 17 00:00:00 2001 From: Naveed Date: Fri, 17 Jul 2026 14:52:08 +0530 Subject: [PATCH 4/7] Fix the CID case of the OOB length regression test The corrupted in_cid length exercised the pre-existing size-cap check, not the bounds check: the in_cid field is not the last one serialized, and the MBEDTLS_SSL_CID_IN_LEN_MAX cap is smaller than the size of the fields that follow it, so no corrupted length could reach past the end of the full buffer, and reverting the bounds check did not change the test's outcome. Truncate the loaded data right after the in_cid bytes, load from a tightly allocated copy, and corrupt the length to one byte past that end, so that reverting the bounds check now trips ASan with an out-of-bounds read in the memcpy(), like the ALPN case. Signed-off-by: Naveed --- tests/suites/test_suite_ssl.function | 60 ++++++++++++++++++---------- 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index c43a96d7f0..8ea73180af 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -3071,7 +3071,8 @@ void ssl_context_load_rejects_oob_length(int field) 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 *load_buf = NULL; + size_t context_buf_len, load_len, i; const unsigned char *sentinel = NULL; size_t sentinel_len = 0; unsigned char corrupt_len = 0; @@ -3111,10 +3112,16 @@ void ssl_context_load_rejects_oob_length(int field) server_cid, sizeof(server_cid)), 0); sentinel = server_cid; sentinel_len = sizeof(server_cid); - /* The smallest value strictly greater than sizeof(in_cid), so we - * exercise the bounds check rather than the pre-existing - * buffer-size check that would also trip on huge values. */ - corrupt_len = (unsigned char) (MBEDTLS_SSL_CID_IN_LEN_MAX + 1); + /* The in_cid field sits in the middle of the serialized data and + * its length is capped at MBEDTLS_SSL_CID_IN_LEN_MAX by a + * separate check, which is less than the size of the fields that + * follow it, so no corrupted length can reach past the end of + * the full buffer. Instead the loaded data is truncated right + * after the in_cid bytes (see the computation of load_len below) + * and the length is corrupted to reach one byte past that end. + * Staying under the cap ensures the bounds check, not the cap, + * is what must reject the corrupted length. */ + corrupt_len = (unsigned char) (sizeof(server_cid) + 1); break; #endif #if defined(MBEDTLS_SSL_ALPN) @@ -3175,31 +3182,43 @@ void ssl_context_load_rejects_oob_length(int field) } TEST_ASSERT(len_p != NULL); -#if defined(MBEDTLS_SSL_ALPN) - if (field == TEST_SSL_CONTEXT_LOAD_OOB_ALPN) { - /* Double-check that the corrupted length overruns the serialized data - * by exactly one byte, as the ALPN field is the last one saved. */ - TEST_EQUAL((size_t) corrupt_len, - (size_t) (context_buf + context_buf_len - (len_p + 1)) + 1); + /* The ALPN field is the last one saved, so the buffer already ends right + * after it. The in_cid field is not last, so the loaded data is truncated + * right after the in_cid bytes to bring the end of the buffer within + * reach of the corrupted length. */ + if (field == TEST_SSL_CONTEXT_LOAD_OOB_CID) { + load_len = (size_t) (len_p + 1 - context_buf) + sentinel_len; + } else { + load_len = context_buf_len; } -#endif + + /* Double-check that the corrupted length overruns the loaded data by + * exactly one byte. */ + TEST_EQUAL((size_t) corrupt_len, + load_len - (size_t) (len_p + 1 - context_buf) + 1); *len_p = corrupt_len; + /* Load from a tightly allocated copy, so that reading even one byte past + * the end of the data overruns the allocation and gets caught by ASan or + * Valgrind. */ + TEST_CALLOC(load_buf, load_len); + memcpy(load_buf, context_buf, load_len); + /* 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); - /* Note that this rejection alone does not prove the bounds check works: - * without it the ALPN case usually still fails with + /* Note that this rejection alone does not prove the bounds checks work: + * without them the load usually still fails with * MBEDTLS_ERR_SSL_BAD_INPUT_DATA, because the out-of-bounds bytes are - * unlikely to match a configured protocol so the alpn_chosen lookup - * fails afterwards. What the missing check leaks is the out-of-bounds - * read in memcmp() itself, so this test case only catches a regression - * when the test suite runs under ASan or Valgrind. */ - TEST_EQUAL(mbedtls_ssl_context_load(&server_ep.ssl, context_buf, - context_buf_len), + * unlikely to match a configured ALPN protocol, and the fields parsed + * after the overrun in_cid fail their own checks. What the missing + * checks leak is the out-of-bounds read itself, so this test case only + * catches a regression when the test suite runs under ASan or + * Valgrind. */ + TEST_EQUAL(mbedtls_ssl_context_load(&server_ep.ssl, load_buf, load_len), MBEDTLS_ERR_SSL_BAD_INPUT_DATA); exit: @@ -3207,6 +3226,7 @@ exit: mbedtls_test_ssl_endpoint_free(&server_ep); mbedtls_test_free_handshake_options(&options); mbedtls_free(context_buf); + mbedtls_free(load_buf); MD_OR_USE_PSA_DONE(); } /* END_CASE */ From 13bbd7ec0bb5a947921fbd502446c70190182de1 Mon Sep 17 00:00:00 2001 From: Naveed Date: Sun, 26 Jul 2026 17:40:51 +0530 Subject: [PATCH 5/7] Keep the out_cid length byte in the CID case's truncated data With the loaded data cut right after the in_cid bytes, even the pristine in_cid length tripped the (end - p) < in_cid_len + 1u bounds check, since the out_cid length byte it accounts for was missing: the truncation, not the corruption, was what got rejected. Keep the out_cid length byte in the loaded data, so the pristine length passes the check and only the corrupted length trips it. Also document why this pre-existing check is the only one in the in_cid path that a regression test can guard: the in_cid_len > sizeof(in_cid) cap protects the memcpy() destination, and without it the overflow lands in the out_cid field inside the transform structure, where sanitizers cannot see it. Signed-off-by: Naveed --- tests/suites/test_suite_ssl.function | 31 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 8ea73180af..60d985d188 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -3112,16 +3112,21 @@ void ssl_context_load_rejects_oob_length(int field) server_cid, sizeof(server_cid)), 0); sentinel = server_cid; sentinel_len = sizeof(server_cid); - /* The in_cid field sits in the middle of the serialized data and - * its length is capped at MBEDTLS_SSL_CID_IN_LEN_MAX by a - * separate check, which is less than the size of the fields that - * follow it, so no corrupted length can reach past the end of - * the full buffer. Instead the loaded data is truncated right - * after the in_cid bytes (see the computation of load_len below) - * and the length is corrupted to reach one byte past that end. - * Staying under the cap ensures the bounds check, not the cap, - * is what must reject the corrupted length. */ - corrupt_len = (unsigned char) (sizeof(server_cid) + 1); + /* Corrupting the in_cid length can only exercise the + * (end - p) < in_cid_len + 1u bounds check in + * ssl_context_load(). The in_cid_len > sizeof(in_cid) cap that + * guards the memcpy() destination cannot be regression-tested: + * without it the overflowing bytes land in the adjacent out_cid + * field of the transform structure, out of sight of ASan, while + * the source read stays inside the buffer, since more than + * sizeof(in_cid) bytes are always serialized after in_cid. So + * the loaded data is truncated right after the out_cid length + * byte (see the computation of load_len below) and the in_cid + * length is corrupted to reach one byte past that end. Keeping + * the out_cid length byte in the loaded data ensures the + * pristine in_cid length still passes the bounds check: the + * corrupted length, not the truncation, is what it rejects. */ + corrupt_len = (unsigned char) (sizeof(server_cid) + 2); break; #endif #if defined(MBEDTLS_SSL_ALPN) @@ -3184,10 +3189,10 @@ void ssl_context_load_rejects_oob_length(int field) /* The ALPN field is the last one saved, so the buffer already ends right * after it. The in_cid field is not last, so the loaded data is truncated - * right after the in_cid bytes to bring the end of the buffer within - * reach of the corrupted length. */ + * right after the out_cid length byte that follows the in_cid bytes, to + * bring the end of the buffer within reach of the corrupted length. */ if (field == TEST_SSL_CONTEXT_LOAD_OOB_CID) { - load_len = (size_t) (len_p + 1 - context_buf) + sentinel_len; + load_len = (size_t) (len_p + 1 - context_buf) + sentinel_len + 1; } else { load_len = context_buf_len; } From a39d101718c00e195b5472bdd0280d35f98dc413 Mon Sep 17 00:00:00 2001 From: Naveed Date: Sun, 26 Jul 2026 17:49:42 +0530 Subject: [PATCH 6/7] Guard the in_cid size check in the CID case The previous approach documented the in_cid_len > sizeof(in_cid) cap as untestable, since the overflowing memcpy() lands inside the transform structure where sanitizers cannot see it. It can be guarded without a sanitizer: grow the in_cid field in the loaded data (keep its bytes, pad to MBEDTLS_SSL_CID_IN_LEN_MAX + 1) and append the rest of the serialized data untouched, so the in-buffer bounds check passes and the cap is what must reject the length. Without the cap the remaining data parses correctly aligned and the load succeeds, so the test catches a regression by the return value alone. Signed-off-by: Naveed --- tests/suites/test_suite_ssl.function | 88 +++++++++++++++------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 60d985d188..132218ce6b 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -3072,7 +3072,7 @@ void ssl_context_load_rejects_oob_length(int field) mbedtls_test_ssl_endpoint client_ep, server_ep; unsigned char *context_buf = NULL; unsigned char *load_buf = NULL; - size_t context_buf_len, load_len, i; + size_t context_buf_len, load_len, field_off, i; const unsigned char *sentinel = NULL; size_t sentinel_len = 0; unsigned char corrupt_len = 0; @@ -3112,21 +3112,13 @@ void ssl_context_load_rejects_oob_length(int field) server_cid, sizeof(server_cid)), 0); sentinel = server_cid; sentinel_len = sizeof(server_cid); - /* Corrupting the in_cid length can only exercise the - * (end - p) < in_cid_len + 1u bounds check in - * ssl_context_load(). The in_cid_len > sizeof(in_cid) cap that - * guards the memcpy() destination cannot be regression-tested: - * without it the overflowing bytes land in the adjacent out_cid - * field of the transform structure, out of sight of ASan, while - * the source read stays inside the buffer, since more than - * sizeof(in_cid) bytes are always serialized after in_cid. So - * the loaded data is truncated right after the out_cid length - * byte (see the computation of load_len below) and the in_cid - * length is corrupted to reach one byte past that end. Keeping - * the out_cid length byte in the loaded data ensures the - * pristine in_cid length still passes the bounds check: the - * corrupted length, not the truncation, is what it rejects. */ - corrupt_len = (unsigned char) (sizeof(server_cid) + 2); + /* The smallest length that overflows the in_cid array in the SSL + * transform, so the size check on that array, not the in-buffer + * bounds check before it, is what must reject the corrupted + * length. The loaded data gets the in_cid field grown to this + * many bytes (see the construction of load_buf below) so that + * the in-buffer bounds check indeed passes. */ + corrupt_len = (unsigned char) (MBEDTLS_SSL_CID_IN_LEN_MAX + 1); break; #endif #if defined(MBEDTLS_SSL_ALPN) @@ -3187,42 +3179,56 @@ void ssl_context_load_rejects_oob_length(int field) } TEST_ASSERT(len_p != NULL); - /* The ALPN field is the last one saved, so the buffer already ends right - * after it. The in_cid field is not last, so the loaded data is truncated - * right after the out_cid length byte that follows the in_cid bytes, to - * bring the end of the buffer within reach of the corrupted length. */ + field_off = (size_t) (len_p + 1 - context_buf); + if (field == TEST_SSL_CONTEXT_LOAD_OOB_CID) { - load_len = (size_t) (len_p + 1 - context_buf) + sentinel_len + 1; + /* The in_cid field sits in the middle of the serialized data, so a + * corrupted length cannot reach past the end of the buffer. Instead + * grow the field itself: keep its bytes, pad it to corrupt_len bytes + * and append the untouched rest of the serialized data, so that the + * in-buffer bounds check passes and the size check on the in_cid + * array is reached. */ + load_len = context_buf_len + corrupt_len - sentinel_len; + TEST_CALLOC(load_buf, load_len); + memcpy(load_buf, context_buf, field_off + sentinel_len); + memset(load_buf + field_off + sentinel_len, 0xA5, + (size_t) corrupt_len - sentinel_len); + memcpy(load_buf + field_off + corrupt_len, + context_buf + field_off + sentinel_len, + context_buf_len - field_off - sentinel_len); + /* Double-check that corrupt_len bytes of field data plus the next + * length byte are present, which is what the in-buffer bounds check + * requires. */ + TEST_ASSERT(load_len - field_off >= (size_t) corrupt_len + 1); } else { + /* The ALPN field is the last one saved, so the buffer already ends + * right after it and the corrupted length reaches exactly one byte + * past the end. Load from a tightly allocated copy, so that reading + * even one byte past the end of the data overruns the allocation and + * gets caught by ASan or Valgrind. */ + TEST_EQUAL((size_t) corrupt_len, context_buf_len - field_off + 1); load_len = context_buf_len; + TEST_CALLOC(load_buf, load_len); + memcpy(load_buf, context_buf, load_len); } - /* Double-check that the corrupted length overruns the loaded data by - * exactly one byte. */ - TEST_EQUAL((size_t) corrupt_len, - load_len - (size_t) (len_p + 1 - context_buf) + 1); - - *len_p = corrupt_len; - - /* Load from a tightly allocated copy, so that reading even one byte past - * the end of the data overruns the allocation and gets caught by ASan or - * Valgrind. */ - TEST_CALLOC(load_buf, load_len); - memcpy(load_buf, context_buf, load_len); + load_buf[field_off - 1] = corrupt_len; /* 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); - /* Note that this rejection alone does not prove the bounds checks work: - * without them the load usually still fails with - * MBEDTLS_ERR_SSL_BAD_INPUT_DATA, because the out-of-bounds bytes are - * unlikely to match a configured ALPN protocol, and the fields parsed - * after the overrun in_cid fail their own checks. What the missing - * checks leak is the out-of-bounds read itself, so this test case only - * catches a regression when the test suite runs under ASan or - * Valgrind. */ + /* How a regression is caught differs between the two cases. In the ALPN + * case, without the bounds check the load still fails, because the + * out-of-bounds bytes are unlikely to match a configured protocol; what + * the missing check leaks is the out-of-bounds read in memcmp() itself, + * so that case catches a regression only when the test suite runs under + * ASan or Valgrind. In the CID case, without the size check the + * memcpy() overflows the in_cid array inside the SSL transform (which + * stays within the enclosing struct, invisible to ASan), the rest of + * the data then parses correctly aligned and the load succeeds, so the + * check below catches a regression on its own. */ TEST_EQUAL(mbedtls_ssl_context_load(&server_ep.ssl, load_buf, load_len), MBEDTLS_ERR_SSL_BAD_INPUT_DATA); From fbc06d7909ee618b7263fdc2a4eb15070de05c33 Mon Sep 17 00:00:00 2001 From: Naveed Date: Tue, 22 Sep 2026 17:27:35 +0530 Subject: [PATCH 7/7] Add an out_cid case to the ssl_context_load OOB length test Cover the serialized out_cid length with the same construction as the in_cid one, and rename the existing case to IN_CID accordingly. Give the client and server sentinel CIDs different lengths so the two fields can be told apart in the serialized data. Skip the CID cases under configurations of MBEDTLS_SSL_CID_IN_LEN_MAX and MBEDTLS_SSL_CID_OUT_LEN_MAX that the test does not support. Signed-off-by: Naveed --- tests/suites/test_suite_ssl.data | 8 ++- tests/suites/test_suite_ssl.function | 77 ++++++++++++++++++---------- 2 files changed, 56 insertions(+), 29 deletions(-) diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 5e99c801db..a5d932200c 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3054,9 +3054,13 @@ TLS 1.3: Session serialization rejects ALPN without terminator depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_EARLY_DATA:MBEDTLS_SSL_ALPN 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 +SSL context serialization rejects out-of-bounds DTLS in_cid length depends_on:MBEDTLS_SSL_DTLS_CONNECTION_ID -ssl_context_load_rejects_oob_length:TEST_SSL_CONTEXT_LOAD_OOB_CID +ssl_context_load_rejects_oob_length:TEST_SSL_CONTEXT_LOAD_OOB_IN_CID + +SSL context serialization rejects out-of-bounds DTLS out_cid length +depends_on:MBEDTLS_SSL_DTLS_CONNECTION_ID +ssl_context_load_rejects_oob_length:TEST_SSL_CONTEXT_LOAD_OOB_OUT_CID SSL context serialization rejects out-of-bounds ALPN length depends_on:MBEDTLS_SSL_ALPN diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 132218ce6b..e651ebc4ac 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -33,8 +33,9 @@ #define TEST_TLS12_SESSION_LOAD_OOB_ID_LEN 3 /* Corrupted length fields for ssl_context_load_rejects_oob_length */ -#define TEST_SSL_CONTEXT_LOAD_OOB_CID 0 -#define TEST_SSL_CONTEXT_LOAD_OOB_ALPN 1 +#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_SSL_PROTO_TLS1_2) && \ defined(MBEDTLS_SSL_SESSION_TICKETS) && \ @@ -3046,12 +3047,16 @@ exit: void ssl_context_load_rejects_oob_length(int field) { #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - /* Sentinel CID so we can find its location inside the serialized buffer. */ + /* Sentinel CIDs so we can find their location inside the serialized + * buffer. The server's own CID is serialized as in_cid and the client's + * as out_cid; they have different lengths so that the two fields can be + * told apart. */ 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 + 0x9A, 0x78, 0x56, 0x34, 0x12, 0xEF, + 0xCD, 0xAB, 0x10, 0x32, 0x54, 0x76 }; #endif #if defined(MBEDTLS_SSL_ALPN) @@ -3094,10 +3099,17 @@ void ssl_context_load_rejects_oob_length(int field) switch (field) { #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - case TEST_SSL_CONTEXT_LOAD_OOB_CID: + case TEST_SSL_CONTEXT_LOAD_OOB_IN_CID: + case TEST_SSL_CONTEXT_LOAD_OOB_OUT_CID: + /* Each peer's own CID has to fit in MBEDTLS_SSL_CID_IN_LEN_MAX and + * its peer's CID in MBEDTLS_SSL_CID_OUT_LEN_MAX. */ + TEST_ASSUME(MBEDTLS_SSL_CID_IN_LEN_MAX >= sizeof(server_cid) && + MBEDTLS_SSL_CID_IN_LEN_MAX >= sizeof(client_cid)); + TEST_ASSUME(MBEDTLS_SSL_CID_OUT_LEN_MAX >= sizeof(server_cid) && + MBEDTLS_SSL_CID_OUT_LEN_MAX >= sizeof(client_cid)); /* 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. */ + * bytes in the serialized context. On each peer the length passed + * to conf_cid() must match the one passed to set_cid(). */ TEST_EQUAL(mbedtls_ssl_conf_cid( &client_ep.conf, sizeof(client_cid), MBEDTLS_SSL_UNEXPECTED_CID_FAIL), 0); @@ -3110,15 +3122,24 @@ void ssl_context_load_rejects_oob_length(int field) TEST_EQUAL(mbedtls_ssl_set_cid( &server_ep.ssl, MBEDTLS_SSL_CID_ENABLED, server_cid, sizeof(server_cid)), 0); - sentinel = server_cid; - sentinel_len = sizeof(server_cid); - /* The smallest length that overflows the in_cid array in the SSL - * transform, so the size check on that array, not the in-buffer - * bounds check before it, is what must reject the corrupted - * length. The loaded data gets the in_cid field grown to this - * many bytes (see the construction of load_buf below) so that - * the in-buffer bounds check indeed passes. */ - corrupt_len = (unsigned char) (MBEDTLS_SSL_CID_IN_LEN_MAX + 1); + /* The corrupted length is the smallest one that overflows the + * in_cid or out_cid array in the SSL transform, so the size check + * on that array, not the in-buffer bounds check before it, is + * what must reject it. The loaded data gets the field grown to + * this many bytes (see the construction of load_buf below) so + * that the in-buffer bounds check indeed passes. A one-byte + * serialized length cannot overflow a 255-byte CID array. */ + if (field == TEST_SSL_CONTEXT_LOAD_OOB_IN_CID) { + TEST_ASSUME(MBEDTLS_SSL_CID_IN_LEN_MAX < 255); + sentinel = server_cid; + sentinel_len = sizeof(server_cid); + corrupt_len = (unsigned char) (MBEDTLS_SSL_CID_IN_LEN_MAX + 1); + } else { + TEST_ASSUME(MBEDTLS_SSL_CID_OUT_LEN_MAX < 255); + sentinel = client_cid; + sentinel_len = sizeof(client_cid); + corrupt_len = (unsigned char) (MBEDTLS_SSL_CID_OUT_LEN_MAX + 1); + } break; #endif #if defined(MBEDTLS_SSL_ALPN) @@ -3181,13 +3202,14 @@ void ssl_context_load_rejects_oob_length(int field) field_off = (size_t) (len_p + 1 - context_buf); - if (field == TEST_SSL_CONTEXT_LOAD_OOB_CID) { - /* The in_cid field sits in the middle of the serialized data, so a + if (field == TEST_SSL_CONTEXT_LOAD_OOB_IN_CID || + field == TEST_SSL_CONTEXT_LOAD_OOB_OUT_CID) { + /* The CID fields sit in the middle of the serialized data, so a * corrupted length cannot reach past the end of the buffer. Instead * grow the field itself: keep its bytes, pad it to corrupt_len bytes * and append the untouched rest of the serialized data, so that the - * in-buffer bounds check passes and the size check on the in_cid - * array is reached. */ + * in-buffer bounds check passes and the size check on the CID array + * is reached. */ load_len = context_buf_len + corrupt_len - sentinel_len; TEST_CALLOC(load_buf, load_len); memcpy(load_buf, context_buf, field_off + sentinel_len); @@ -3197,8 +3219,9 @@ void ssl_context_load_rejects_oob_length(int field) context_buf + field_off + sentinel_len, context_buf_len - field_off - sentinel_len); /* Double-check that corrupt_len bytes of field data plus the next - * length byte are present, which is what the in-buffer bounds check - * requires. */ + * byte are present, which is the most the in-buffer bounds checks + * require (the in_cid one also accounts for the out_cid length + * byte). */ TEST_ASSERT(load_len - field_off >= (size_t) corrupt_len + 1); } else { /* The ALPN field is the last one saved, so the buffer already ends @@ -3224,11 +3247,11 @@ void ssl_context_load_rejects_oob_length(int field) * out-of-bounds bytes are unlikely to match a configured protocol; what * the missing check leaks is the out-of-bounds read in memcmp() itself, * so that case catches a regression only when the test suite runs under - * ASan or Valgrind. In the CID case, without the size check the - * memcpy() overflows the in_cid array inside the SSL transform (which - * stays within the enclosing struct, invisible to ASan), the rest of - * the data then parses correctly aligned and the load succeeds, so the - * check below catches a regression on its own. */ + * ASan or Valgrind. In the CID cases, without the size check the + * memcpy() overflows the in_cid or out_cid array inside the SSL + * transform (which stays within the enclosing struct, invisible to + * ASan), the rest of the data then parses correctly aligned and the + * load succeeds, so the check below catches a regression on its own. */ TEST_EQUAL(mbedtls_ssl_context_load(&server_ep.ssl, load_buf, load_len), MBEDTLS_ERR_SSL_BAD_INPUT_DATA);