Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -38436,13 +38436,21 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)

#if defined(HAVE_TLS_EXTENSIONS) && defined(HAVE_ENCRYPT_THEN_MAC) && \
!defined(WOLFSSL_AEAD_ONLY)
if (ssl->options.encThenMac && ssl->specs.cipher_type == block) {
ret = TLSX_EncryptThenMac_Respond(ssl);
if (ret != 0)
goto out;
/* Only respond to ETM here when resumption actually succeeded;
* HandleTlsResumption populates ssl->specs via SetCipherSpecs only
* on the success path. If resumption failed, resuming has been
* cleared and ssl->specs.cipher_type is still zero-initialized,
* so we must defer the ETM decision until after MatchSuite. */
if (ssl->options.resuming) {
if (ssl->options.encThenMac &&
ssl->specs.cipher_type == block) {
ret = TLSX_EncryptThenMac_Respond(ssl);
if (ret != 0)
goto out;
}
else
ssl->options.encThenMac = 0;
}
else
ssl->options.encThenMac = 0;
#endif
if (ssl->options.clientState == CLIENT_KEYEXCHANGE_COMPLETE) {
WOLFSSL_LEAVE("DoClientHello", ret);
Expand Down
89 changes: 89 additions & 0 deletions tests/api/test_tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,95 @@ int test_tls12_no_null_compression(void)
* uppercase names like "SECP384R1" do not match the lowercase "secp384r1"
* entry; they fall through to the wolfCrypt ECC look-up which uses
* XSTRCASECMP. */
/* Regression test for the encrypt-then-MAC silent-disable bug.
*
* Before the fix, when a client sent a 32-byte session ID in its ClientHello
* (so the server set ssl->options.resuming = 1) but the server's session
* cache did not contain that session, DoClientHello would run an
* encrypt_then_mac decision *before* MatchSuite/SetCipherSpecs had populated
* ssl->specs.cipher_type. Because cipher_type was zero-initialized
* (== stream, not block), the ETM block cleared encThenMac to 0, and the
* post-MatchSuite block could not re-enable it. The connection then
* silently negotiated MAC-then-encrypt instead of encrypt-then-MAC.
*
* This test forces a stale-resumption ClientHello against a server with an
* empty session cache, using a CBC-mode cipher suite, and asserts that the
* server still negotiates encrypt-then-MAC. */
int test_tls12_etm_failed_resumption(void)
{
EXPECT_DECLS;
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
!defined(WOLFSSL_NO_TLS12) && defined(HAVE_ENCRYPT_THEN_MAC) && \
!defined(WOLFSSL_AEAD_ONLY) && !defined(NO_RSA) && !defined(NO_AES) && \
defined(HAVE_AES_CBC) && !defined(NO_SHA256) && \
defined(HAVE_SESSION_TICKET) && defined(HAVE_ECC)
Comment thread
embhorn marked this conversation as resolved.
/* TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 - a CBC suite, where ETM applies. */
const char* cbcSuite = "ECDHE-RSA-AES128-SHA256";
WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
WOLFSSL_SESSION *sess = NULL;
struct test_memio_ctx test_ctx;

/* First handshake: establish a session-ID-based session on the client.
* Disable TLS 1.2 session tickets on both sides so resumption uses the
* session ID path (not tickets), which is the path the bug lives on. */
XMEMSET(&test_ctx, 0, sizeof(test_ctx));
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_c), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_s), WOLFSSL_SUCCESS);
Comment thread
embhorn marked this conversation as resolved.
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_c, cbcSuite), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_s, cbcSuite), WOLFSSL_SUCCESS);
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
/* Sanity: the first handshake itself must use ETM. */
ExpectIntEQ(ssl_s->options.encThenMac, 1);
ExpectNotNull(sess = wolfSSL_get1_session(ssl_c));
Comment thread
embhorn marked this conversation as resolved.

wolfSSL_free(ssl_c); ssl_c = NULL;
wolfSSL_free(ssl_s); ssl_s = NULL;
wolfSSL_CTX_free(ctx_c); ctx_c = NULL;
wolfSSL_CTX_free(ctx_s); ctx_s = NULL;
Comment thread
embhorn marked this conversation as resolved.

/* Second handshake against a *fresh* server context (empty cache). The
* client offers the saved session, so the server's ClientHello parser
* sets options.resuming = 1, but HandleTlsResumption then fails to find
* the session and clears resuming. Pre-fix, ETM was silently dropped
* here. */
XMEMSET(&test_ctx, 0, sizeof(test_ctx));
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
Comment thread
embhorn marked this conversation as resolved.
/* The internal session cache is process-global, so the saved session is
* still findable via the cache. Disable lookups on this server SSL
* directly so that HandleTlsResumption hits its "session lookup failed"
* path - exactly the scenario the bug fix targets. */
if (ssl_s != NULL)
ssl_s->options.sessionCacheOff = 1;
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_c), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_NoTicketTLSv12(ssl_s), WOLFSSL_SUCCESS);
Comment thread
embhorn marked this conversation as resolved.
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_c, cbcSuite), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_set_cipher_list(ssl_s, cbcSuite), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_set_session(ssl_c, sess), WOLFSSL_SUCCESS);
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
if (ssl_s != NULL) {
/* The server should NOT have actually resumed (fresh ctx, empty
* cache). */
ExpectIntEQ(ssl_s->options.resuming, 0);
/* And - the regression check - encrypt-then-MAC must still be
* active. */
ExpectIntEQ(ssl_s->options.encThenMac, 1);
}
Comment thread
embhorn marked this conversation as resolved.
if (ssl_c != NULL)
ExpectIntEQ(ssl_c->options.encThenMac, 1);

wolfSSL_SESSION_free(sess);
Comment thread
embhorn marked this conversation as resolved.
wolfSSL_free(ssl_c);
wolfSSL_free(ssl_s);
wolfSSL_CTX_free(ctx_c);
wolfSSL_CTX_free(ctx_s);
#endif
return EXPECT_RESULT();
}

int test_tls_set_curves_list_ecc_fallback(void)
{
EXPECT_DECLS;
Expand Down
2 changes: 2 additions & 0 deletions tests/api/test_tls.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ int test_tls13_curve_intersection(void);
int test_tls_certreq_order(void);
int test_tls12_bad_cv_sig_alg(void);
int test_tls12_no_null_compression(void);
int test_tls12_etm_failed_resumption(void);
int test_tls_set_curves_list_ecc_fallback(void);

#define TEST_TLS_DECLS \
Expand All @@ -41,6 +42,7 @@ int test_tls_set_curves_list_ecc_fallback(void);
TEST_DECL_GROUP("tls", test_tls_certreq_order), \
TEST_DECL_GROUP("tls", test_tls12_bad_cv_sig_alg), \
TEST_DECL_GROUP("tls", test_tls12_no_null_compression), \
TEST_DECL_GROUP("tls", test_tls12_etm_failed_resumption), \
TEST_DECL_GROUP("tls", test_tls_set_curves_list_ecc_fallback)

#endif /* TESTS_API_TEST_TLS_H */
Loading