From 15bd0bb84182a76e09fc7884474d1330c1beda84 Mon Sep 17 00:00:00 2001 From: Kareem Date: Fri, 7 Aug 2026 13:55:42 -0700 Subject: [PATCH] Code review feedback --- .github/configs/os-check-linux.json | 3 + src/internal.c | 39 ++++++----- src/keys.c | 10 +++ src/ssl_api_ext.c | 104 +++++++++++++++++----------- src/ssl_sess.c | 2 +- src/tls.c | 13 ---- tests/api/test_tls_ext.c | 2 +- wolfssl/ssl.h | 2 + 8 files changed, 102 insertions(+), 73 deletions(-) diff --git a/.github/configs/os-check-linux.json b/.github/configs/os-check-linux.json index b6cf004d50..fdab96471c 100644 --- a/.github/configs/os-check-linux.json +++ b/.github/configs/os-check-linux.json @@ -40,6 +40,9 @@ {"name": "all-faultharden-pk-privkey", "minutes": 7.8, "comment": "Test holding private key in the PK callback with fault harden. Based on existing customer use case.", "configure": ["--enable-all", "--enable-faultharden", "--enable-pkcallbacks", "CPPFLAGS=-DTEST_PK_PRIVKEY"]}, +{"name": "all-no-ticket-expire", "minutes": 7.8, + "comment": "Nothing else in CI compiles WOLFSSL_NO_TICKET_EXPIRE; it removes the session expiry check that heads the resumption control flow in HandleTlsResumption.", + "configure": ["--enable-all", "CPPFLAGS=-DWOLFSSL_NO_TICKET_EXPIRE"]}, {"name": "all-secure-renegotiation", "minutes": 7.8, "configure": ["--enable-all", "--enable-secure-renegotiation"]}, {"name": "all-debug-certs", "minutes": 7.8, diff --git a/src/internal.c b/src/internal.c index 2930a30336..7df5b72e8e 100644 --- a/src/internal.c +++ b/src/internal.c @@ -25022,11 +25022,11 @@ static int CheckResumptionConsistency(WOLFSSL* ssl) #ifdef HAVE_SECRET_CALLBACK /* Skip the EMS checks for EAP-FAST (session-secret callback): the master * secret comes from the callback rather than the cached session. */ - skipEmsCheck = ssl->sessionSecretCb != NULL + skipEmsCheck = (ssl->sessionSecretCb != NULL #ifdef HAVE_SESSION_TICKET && ssl->session->ticketLen > 0 #endif - ; + ) ? 1 : 0; #endif /* EMS must match (RFC 7627 5.3). */ if (!skipEmsCheck && ssl->session->haveEMS != ssl->options.haveEMS) { @@ -25037,7 +25037,7 @@ static int CheckResumptionConsistency(WOLFSSL* ssl) return EXT_MASTER_SECRET_NEEDED_E; } #ifdef HAVE_EXTENDED_MASTER - /* Resumption skips MakeTlsMasterSecret, so enforce required EMS here. */ + /* Resumption skips MakeMasterSecret, so enforce required EMS here. */ if (!skipEmsCheck && ssl->options.requireEMS && !ssl->options.haveEMS) { WOLFSSL_MSG("EMS required but not negotiated with peer"); SendAlert(ssl, alert_fatal, handshake_failure); @@ -41356,6 +41356,19 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl) #endif } #endif /* HAVE_SESSION_TICKET && (HAVE_SNI || HAVE_ALPN) */ + +#ifdef HAVE_EXTENDED_MASTER + /* Resumption skips MakeMasterSecret, so enforce required EMS here. */ + if (ssl->options.requireEMS && !ssl->options.haveEMS) { + WOLFSSL_MSG("EMS required but not negotiated with peer"); + #ifdef WOLFSSL_EXTRA_ALERTS + SendAlert(ssl, alert_fatal, handshake_failure); + #endif + WOLFSSL_ERROR_VERBOSE(EXT_MASTER_SECRET_NEEDED_E); + return EXT_MASTER_SECRET_NEEDED_E; + } +#endif /* HAVE_EXTENDED_MASTER */ + #if !defined(WOLFSSL_NO_TICKET_EXPIRE) && !defined(NO_ASN_TIME) /* check if the ticket is valid */ if (LowResTimer() > session->bornOn + ssl->timeout) { @@ -41364,19 +41377,12 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl) } #endif /* !WOLFSSL_NO_TICKET_EXPIRE && !NO_ASN_TIME */ -#ifdef HAVE_EXTENDED_MASTER - else if (ssl->options.requireEMS && !ssl->options.haveEMS) { - /* Resumption skips MakeTlsMasterSecret, so enforce required EMS - * here. */ - WOLFSSL_MSG("EMS required but not negotiated with peer"); - #ifdef WOLFSSL_EXTRA_ALERTS - SendAlert(ssl, alert_fatal, handshake_failure); - #endif - ret = EXT_MASTER_SECRET_NEEDED_E; - WOLFSSL_ERROR_VERBOSE(ret); + if (!ssl->options.resuming) { + /* Expired above: DoClientHello falls back to a full handshake. */ + return ret; } -#endif /* HAVE_EXTENDED_MASTER */ - else if (session->haveEMS != ssl->options.haveEMS) { + + if (session->haveEMS != ssl->options.haveEMS) { /* RFC 7627, 5.3, server-side */ /* if old sess didn't have EMS, but new does, full handshake */ if (!session->haveEMS && ssl->options.haveEMS) { @@ -42096,7 +42102,8 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl) #ifdef HAVE_EXTENDED_MASTER /* Honor a user request to disable EMS on the server by * ignoring the peer's extension. */ - else if (extId == HELLO_EXT_EXTMS && !ssl->options.disableEMS) + else if (extId == HELLO_EXT_EXTMS && + !ssl->options.disableEMS) ssl->options.haveEMS = 1; #endif else diff --git a/src/keys.c b/src/keys.c index f9e7d3b7b3..744da03ce6 100644 --- a/src/keys.c +++ b/src/keys.c @@ -4231,6 +4231,16 @@ static int MakeSslMasterSecret(WOLFSSL* ssl) /* Master wrapper, doesn't use SSL stack space in TLS mode */ int MakeMasterSecret(WOLFSSL* ssl) { +#ifdef HAVE_EXTENDED_MASTER + /* User requires EMS but it was not negotiated: abort rather than derive + * a standard master secret (RFC 7627). */ + if (ssl->options.requireEMS && !ssl->options.haveEMS) { + WOLFSSL_MSG("EMS required but not negotiated with peer"); + SendAlert(ssl, alert_fatal, handshake_failure); + WOLFSSL_ERROR_VERBOSE(EXT_MASTER_SECRET_NEEDED_E); + return EXT_MASTER_SECRET_NEEDED_E; + } +#endif /* append secret to premaster : premaster | SerSi | CliSi */ #ifndef NO_OLD_TLS if (ssl->options.tls) return MakeTlsMasterSecret(ssl); diff --git a/src/ssl_api_ext.c b/src/ssl_api_ext.c index d063b75623..f2cae5c437 100644 --- a/src/ssl_api_ext.c +++ b/src/ssl_api_ext.c @@ -1631,18 +1631,22 @@ int wolfSSL_DisableExtendedMasterSecret(WOLFSSL* ssl) */ int wolfSSL_CTX_EnableExtendedMasterSecret(WOLFSSL_CTX* ctx) { - if (ctx == NULL) - return BAD_FUNC_ARG; + int ret = WOLFSSL_SUCCESS; - ctx->disableEMS = 0; - ctx->requireEMS = 0; - /* Re-arm client advertising. A side-less (wolfSSLv23) object is armed by - * InitSSL_Side instead; arming it here would make a server echo an - * unsolicited extension. */ - if (ctx->method != NULL && ctx->method->side == WOLFSSL_CLIENT_END) - ctx->haveEMS = 1; + if (ctx == NULL) { + ret = BAD_FUNC_ARG; + } + else { + ctx->disableEMS = 0; + ctx->requireEMS = 0; + /* Re-arm client advertising. A side-less (wolfSSLv23) object is + * armed by InitSSL_Side instead; arming it here would make a server + * echo an unsolicited extension. */ + if (ctx->method != NULL && ctx->method->side == WOLFSSL_CLIENT_END) + ctx->haveEMS = 1; + } - return WOLFSSL_SUCCESS; + return ret; } @@ -1657,21 +1661,27 @@ int wolfSSL_CTX_EnableExtendedMasterSecret(WOLFSSL_CTX* ctx) */ int wolfSSL_EnableExtendedMasterSecret(WOLFSSL* ssl) { - if (ssl == NULL) - return BAD_FUNC_ARG; + int ret = WOLFSSL_SUCCESS; - ssl->options.disableEMS = 0; - ssl->options.requireEMS = 0; - /* Re-arm client advertising. A side-less (wolfSSLv23) object is armed by - * InitSSL_Side instead; arming it here would make a server echo an - * unsolicited extension. */ - if (ssl->options.side == WOLFSSL_CLIENT_END) - ssl->options.haveEMS = 1; + if (ssl == NULL) { + ret = BAD_FUNC_ARG; + } + else { + ssl->options.disableEMS = 0; + ssl->options.requireEMS = 0; + /* Re-arm client advertising. A side-less (wolfSSLv23) object is + * armed by InitSSL_Side instead; arming it here would make a server + * echo an unsolicited extension. */ + if (ssl->options.side == WOLFSSL_CLIENT_END) + ssl->options.haveEMS = 1; + } - return WOLFSSL_SUCCESS; + return ret; } +#ifndef WOLFSSL_NO_TLS12 + /* Require the Extended Master Secret extension on the context. * * If EMS (RFC 7627) is not negotiated the connection is aborted with @@ -1684,19 +1694,23 @@ int wolfSSL_EnableExtendedMasterSecret(WOLFSSL* ssl) */ int wolfSSL_CTX_RequireExtendedMasterSecret(WOLFSSL_CTX* ctx) { - if (ctx == NULL) - return BAD_FUNC_ARG; + int ret = WOLFSSL_SUCCESS; - ctx->requireEMS = 1; - /* Mutually exclusive with disabling EMS. */ - ctx->disableEMS = 0; - /* Re-arm client advertising. A side-less (wolfSSLv23) object is armed by - * InitSSL_Side instead; arming it here would make a server echo an - * unsolicited extension. */ - if (ctx->method != NULL && ctx->method->side == WOLFSSL_CLIENT_END) - ctx->haveEMS = 1; + if (ctx == NULL) { + ret = BAD_FUNC_ARG; + } + else { + ctx->requireEMS = 1; + /* Mutually exclusive with disabling EMS. */ + ctx->disableEMS = 0; + /* Re-arm client advertising. A side-less (wolfSSLv23) object is + * armed by InitSSL_Side instead; arming it here would make a server + * echo an unsolicited extension. */ + if (ctx->method != NULL && ctx->method->side == WOLFSSL_CLIENT_END) + ctx->haveEMS = 1; + } - return WOLFSSL_SUCCESS; + return ret; } @@ -1712,22 +1726,28 @@ int wolfSSL_CTX_RequireExtendedMasterSecret(WOLFSSL_CTX* ctx) */ int wolfSSL_RequireExtendedMasterSecret(WOLFSSL* ssl) { - if (ssl == NULL) - return BAD_FUNC_ARG; + int ret = WOLFSSL_SUCCESS; - ssl->options.requireEMS = 1; - /* Mutually exclusive with disabling EMS. */ - ssl->options.disableEMS = 0; - /* Re-arm client advertising. A side-less (wolfSSLv23) object is armed by - * InitSSL_Side instead; arming it here would make a server echo an - * unsolicited extension. */ - if (ssl->options.side == WOLFSSL_CLIENT_END) - ssl->options.haveEMS = 1; + if (ssl == NULL) { + ret = BAD_FUNC_ARG; + } + else { + ssl->options.requireEMS = 1; + /* Mutually exclusive with disabling EMS. */ + ssl->options.disableEMS = 0; + /* Re-arm client advertising. A side-less (wolfSSLv23) object is + * armed by InitSSL_Side instead; arming it here would make a server + * echo an unsolicited extension. */ + if (ssl->options.side == WOLFSSL_CLIENT_END) + ssl->options.haveEMS = 1; + } return ret; } -#endif +#endif /* !WOLFSSL_NO_TLS12 */ + +#endif /* HAVE_EXTENDED_MASTER */ #endif /* !NO_TLS */ /* ---- OpenSSL-compatibility TLS extension APIs (moved from ssl.c) ---- */ diff --git a/src/ssl_sess.c b/src/ssl_sess.c index cb5416a5d3..a32a5bb23f 100644 --- a/src/ssl_sess.c +++ b/src/ssl_sess.c @@ -1612,7 +1612,7 @@ int wolfSSL_SetSession(WOLFSSL* ssl, WOLFSSL_SESSION* session) ssl->options.resuming = 1; #ifdef HAVE_EXTENDED_MASTER /* A user EMS override takes precedence over the session's EMS state. */ - if (ssl->options.requireEMS) + if (ssl->options.requireEMS && ssl->options.side == WOLFSSL_CLIENT_END) ssl->options.haveEMS = 1; else if (ssl->options.disableEMS) ssl->options.haveEMS = 0; diff --git a/src/tls.c b/src/tls.c index dcfc05c01b..5211b93b7d 100644 --- a/src/tls.c +++ b/src/tls.c @@ -706,19 +706,6 @@ int MakeTlsMasterSecret(WOLFSSL* ssl) { int ret; -#ifdef HAVE_EXTENDED_MASTER - /* The user disabled the standard master secret and requires the Extended - * Master Secret extension (RFC 7627). If it was not negotiated with the - * peer, abort rather than derive a standard master secret. Only reachable - * for TLS 1.2 and earlier; TLS 1.3 uses a separate key schedule. */ - if (ssl->options.requireEMS && !ssl->options.haveEMS) { - WOLFSSL_MSG("EMS required but not negotiated with peer"); - SendAlert(ssl, alert_fatal, handshake_failure); - WOLFSSL_ERROR_VERBOSE(EXT_MASTER_SECRET_NEEDED_E); - return EXT_MASTER_SECRET_NEEDED_E; - } -#endif - #if defined(WOLFSSL_SNIFFER) && defined(WOLFSSL_SNIFFER_KEYLOGFILE) /* If this is called from a sniffer session with keylog file support, obtain * the master secret from the callback */ diff --git a/tests/api/test_tls_ext.c b/tests/api/test_tls_ext.c index 399e431bba..5de92af1c7 100644 --- a/tests/api/test_tls_ext.c +++ b/tests/api/test_tls_ext.c @@ -1161,7 +1161,7 @@ int test_wolfSSL_RequireExtendedMasterSecret(void) { EXPECT_DECLS; #if defined(HAVE_EXTENDED_MASTER) && !defined(NO_WOLFSSL_CLIENT) && \ - !defined(NO_TLS) + !defined(NO_TLS) && !defined(WOLFSSL_NO_TLS12) WOLFSSL_CTX *ctx = NULL; WOLFSSL *ssl = NULL; diff --git a/wolfssl/ssl.h b/wolfssl/ssl.h index 9337141d7b..972123d860 100644 --- a/wolfssl/ssl.h +++ b/wolfssl/ssl.h @@ -5205,8 +5205,10 @@ WOLFSSL_API int wolfSSL_DisableExtendedMasterSecret(WOLFSSL* ssl); WOLFSSL_API int wolfSSL_CTX_DisableExtendedMasterSecret(WOLFSSL_CTX* ctx); WOLFSSL_API int wolfSSL_EnableExtendedMasterSecret(WOLFSSL* ssl); WOLFSSL_API int wolfSSL_CTX_EnableExtendedMasterSecret(WOLFSSL_CTX* ctx); +#ifndef WOLFSSL_NO_TLS12 WOLFSSL_API int wolfSSL_RequireExtendedMasterSecret(WOLFSSL* ssl); WOLFSSL_API int wolfSSL_CTX_RequireExtendedMasterSecret(WOLFSSL_CTX* ctx); +#endif #define WOLFSSL_CRL_MONITOR 0x01 /* monitor this dir flag */