Skip to content

Commit af53696

Browse files
embhornembhorn
committed
Fix ETM on resumption
1 parent 1d363f3 commit af53696

3 files changed

Lines changed: 99 additions & 6 deletions

File tree

‎src/internal.c‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38436,13 +38436,21 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
3843638436

3843738437
#if defined(HAVE_TLS_EXTENSIONS) && defined(HAVE_ENCRYPT_THEN_MAC) && \
3843838438
!defined(WOLFSSL_AEAD_ONLY)
38439-
if (ssl->options.encThenMac && ssl->specs.cipher_type == block) {
38440-
ret = TLSX_EncryptThenMac_Respond(ssl);
38441-
if (ret != 0)
38442-
goto out;
38439+
/* Only respond to ETM here when resumption actually succeeded;
38440+
* HandleTlsResumption populates ssl->specs via SetCipherSpecs only
38441+
* on the success path. If resumption failed, resuming has been
38442+
* cleared and ssl->specs.cipher_type is still zero-initialized,
38443+
* so we must defer the ETM decision until after MatchSuite. */
38444+
if (ssl->options.resuming) {
38445+
if (ssl->options.encThenMac &&
38446+
ssl->specs.cipher_type == block) {
38447+
ret = TLSX_EncryptThenMac_Respond(ssl);
38448+
if (ret != 0)
38449+
goto out;
38450+
}
38451+
else
38452+
ssl->options.encThenMac = 0;
3844338453
}
38444-
else
38445-
ssl->options.encThenMac = 0;
3844638454
#endif
3844738455
if (ssl->options.clientState == CLIENT_KEYEXCHANGE_COMPLETE) {
3844838456
WOLFSSL_LEAVE("DoClientHello", ret);

‎tests/api/test_tls.c‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -730,6 +730,89 @@ int test_tls12_no_null_compression(void)
730730
* uppercase names like "SECP384R1" do not match the lowercase "secp384r1"
731731
* entry; they fall through to the wolfCrypt ECC look-up which uses
732732
* XSTRCASECMP. */
733+
/* Regression test for the encrypt-then-MAC silent-disable bug.
734+
*
735+
* Before the fix, when a client sent a 32-byte session ID in its ClientHello
736+
* (so the server set ssl->options.resuming = 1) but the server's session
737+
* cache did not contain that session, DoClientHello would run an
738+
* encrypt_then_mac decision *before* MatchSuite/SetCipherSpecs had populated
739+
* ssl->specs.cipher_type. Because cipher_type was zero-initialized
740+
* (== stream, not block), the ETM block cleared encThenMac to 0, and the
741+
* post-MatchSuite block could not re-enable it. The connection then
742+
* silently negotiated MAC-then-encrypt instead of encrypt-then-MAC.
743+
*
744+
* This test forces a stale-resumption ClientHello against a server with an
745+
* empty session cache, using a CBC-mode cipher suite, and asserts that the
746+
* server still negotiates encrypt-then-MAC. */
747+
int test_tls12_etm_failed_resumption(void)
748+
{
749+
EXPECT_DECLS;
750+
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
751+
!defined(WOLFSSL_NO_TLS12) && defined(HAVE_ENCRYPT_THEN_MAC) && \
752+
!defined(WOLFSSL_AEAD_ONLY) && !defined(NO_RSA) && !defined(NO_AES) && \
753+
defined(HAVE_AES_CBC) && !defined(NO_SHA256) && \
754+
defined(HAVE_SESSION_TICKET) && defined(HAVE_ECC)
755+
/* TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 — a CBC suite, where ETM applies. */
756+
const char* cbcSuite = "ECDHE-RSA-AES128-SHA256";
757+
WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
758+
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
759+
WOLFSSL_SESSION *sess = NULL;
760+
struct test_memio_ctx test_ctx;
761+
762+
/* First handshake: establish a session-ID-based session on the client.
763+
* Disable TLS 1.2 session tickets on both sides so resumption uses the
764+
* session ID path (not tickets), which is the path the bug lives on. */
765+
XMEMSET(&test_ctx, 0, sizeof(test_ctx));
766+
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
767+
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
768+
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_c), WOLFSSL_SUCCESS);
769+
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_s), WOLFSSL_SUCCESS);
770+
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_c, cbcSuite), WOLFSSL_SUCCESS);
771+
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_s, cbcSuite), WOLFSSL_SUCCESS);
772+
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
773+
/* Sanity: the first handshake itself must use ETM. */
774+
ExpectIntEQ(ssl_s->options.encThenMac, 1);
775+
ExpectNotNull(sess = wolfSSL_get1_session(ssl_c));
776+
777+
wolfSSL_free(ssl_c); ssl_c = NULL;
778+
wolfSSL_free(ssl_s); ssl_s = NULL;
779+
wolfSSL_CTX_free(ctx_c); ctx_c = NULL;
780+
wolfSSL_CTX_free(ctx_s); ctx_s = NULL;
781+
782+
/* Second handshake against a *fresh* server context (empty cache). The
783+
* client offers the saved session, so the server's ClientHello parser
784+
* sets options.resuming = 1, but HandleTlsResumption then fails to find
785+
* the session and clears resuming. Pre-fix, ETM was silently dropped
786+
* here. */
787+
XMEMSET(&test_ctx, 0, sizeof(test_ctx));
788+
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
789+
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
790+
/* The internal session cache is process-global, so the saved session is
791+
* still findable via the cache. Disable lookups on this server SSL
792+
* directly so that HandleTlsResumption hits its "session lookup failed"
793+
* path — exactly the scenario the bug fix targets. */
794+
ssl_s->options.sessionCacheOff = 1;
795+
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_c), WOLFSSL_SUCCESS);
796+
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_s), WOLFSSL_SUCCESS);
797+
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_c, cbcSuite), WOLFSSL_SUCCESS);
798+
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_s, cbcSuite), WOLFSSL_SUCCESS);
799+
ExpectIntEQ(wolfSSL_set_session(ssl_c, sess), WOLFSSL_SUCCESS);
800+
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
801+
/* The server should NOT have actually resumed (fresh ctx, empty cache). */
802+
ExpectIntEQ(ssl_s->options.resuming, 0);
803+
/* And — the regression check — encrypt-then-MAC must still be active. */
804+
ExpectIntEQ(ssl_s->options.encThenMac, 1);
805+
ExpectIntEQ(ssl_c->options.encThenMac, 1);
806+
807+
wolfSSL_SESSION_free(sess);
808+
wolfSSL_free(ssl_c);
809+
wolfSSL_free(ssl_s);
810+
wolfSSL_CTX_free(ctx_c);
811+
wolfSSL_CTX_free(ctx_s);
812+
#endif
813+
return EXPECT_RESULT();
814+
}
815+
733816
int test_tls_set_curves_list_ecc_fallback(void)
734817
{
735818
EXPECT_DECLS;

‎tests/api/test_tls.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ int test_tls13_curve_intersection(void);
3030
int test_tls_certreq_order(void);
3131
int test_tls12_bad_cv_sig_alg(void);
3232
int test_tls12_no_null_compression(void);
33+
int test_tls12_etm_failed_resumption(void);
3334
int test_tls_set_curves_list_ecc_fallback(void);
3435

3536
#define TEST_TLS_DECLS \
@@ -41,6 +42,7 @@ int test_tls_set_curves_list_ecc_fallback(void);
4142
TEST_DECL_GROUP("tls", test_tls_certreq_order), \
4243
TEST_DECL_GROUP("tls", test_tls12_bad_cv_sig_alg), \
4344
TEST_DECL_GROUP("tls", test_tls12_no_null_compression), \
45+
TEST_DECL_GROUP("tls", test_tls12_etm_failed_resumption), \
4446
TEST_DECL_GROUP("tls", test_tls_set_curves_list_ecc_fallback)
4547

4648
#endif /* TESTS_API_TEST_TLS_H */

0 commit comments

Comments
 (0)