Code review feedback

.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,

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

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);

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) ---- */

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;

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 */

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;
 

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 */