EMS code review feedback:

Check as early in the connection as possible
Abort resumption rather than fully aborting connection when EMS is disabled
Make version check around haveEMS consistent
Clear premaster secret when EMS check fails
Update documentation

Author: kareem-wolfssl

doc/dox_comments/header_files/ssl.h

@@ -17287,8 +17287,10 @@ int wolfSSL_set_scr_check_enabled(WOLFSSL* ssl, byte enabled);
     \ingroup Setup
     \brief Disables the TLS Extended Master Secret extension (RFC 7627) on
     the context: a client stops advertising it and a server ignores the
-    peer's request, so a standard master secret is negotiated. TLS 1.2 and
-    earlier only. Requires HAVE_EXTENDED_MASTER.
+    peer's request, so a standard master secret is negotiated. A server also
+    declines resumption of sessions or tickets that used EMS and does a full
+    handshake instead. TLS 1.2 and earlier only. Requires
+    HAVE_EXTENDED_MASTER.
 
     \return WOLFSSL_SUCCESS on success.
     \return BAD_FUNC_ARG if ctx is NULL.
@@ -17311,8 +17313,10 @@ int wolfSSL_CTX_DisableExtendedMasterSecret(WOLFSSL_CTX* ctx);
     \ingroup Setup
     \brief Disables the TLS Extended Master Secret extension (RFC 7627) on
     the SSL object: a client stops advertising it and a server ignores the
-    peer's request, so a standard master secret is negotiated. TLS 1.2 and
-    earlier only. Requires HAVE_EXTENDED_MASTER.
+    peer's request, so a standard master secret is negotiated. A server also
+    declines resumption of sessions or tickets that used EMS and does a full
+    handshake instead. TLS 1.2 and earlier only. Requires
+    HAVE_EXTENDED_MASTER.
 
     \return WOLFSSL_SUCCESS on success.
     \return BAD_FUNC_ARG if ssl is NULL.
@@ -17382,7 +17386,8 @@ int wolfSSL_EnableExtendedMasterSecret(WOLFSSL* ssl);
     \brief Makes the TLS Extended Master Secret extension (RFC 7627)
     mandatory on the context: if it is not negotiated, the connection
     is aborted with EXT_MASTER_SECRET_NEEDED_E. A client advertises
-    the extension even after a previous disable. TLS 1.2 and earlier
+    the extension even after a previous disable. Sessions using a
+    session-secret callback (EAP-FAST) are exempt. TLS 1.2 and earlier
     only. Requires HAVE_EXTENDED_MASTER.
 
     \return WOLFSSL_SUCCESS on success.
@@ -17407,8 +17412,9 @@ int wolfSSL_CTX_RequireExtendedMasterSecret(WOLFSSL_CTX* ctx);
     \brief Makes the TLS Extended Master Secret extension (RFC 7627)
     mandatory on the SSL object: if it is not negotiated, including on
     resumption, the connection is aborted with EXT_MASTER_SECRET_NEEDED_E. A
-    client advertises the extension even after a previous disable. TLS 1.2
-    and earlier only. Requires HAVE_EXTENDED_MASTER.
+    client advertises the extension even after a previous disable. Sessions
+    using a session-secret callback (EAP-FAST) are exempt. TLS 1.2 and
+    earlier only. Requires HAVE_EXTENDED_MASTER.
 
     \return WOLFSSL_SUCCESS on success.
     \return BAD_FUNC_ARG if ssl is NULL.

src/internal.c

@@ -24554,15 +24554,6 @@ static int CheckResumptionConsistency(WOLFSSL* ssl)
         WOLFSSL_ERROR_VERBOSE(EXT_MASTER_SECRET_NEEDED_E);
         return EXT_MASTER_SECRET_NEEDED_E;
     }
-#ifdef HAVE_EXTENDED_MASTER
-    /* 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);
-        WOLFSSL_ERROR_VERBOSE(EXT_MASTER_SECRET_NEEDED_E);
-        return EXT_MASTER_SECRET_NEEDED_E;
-    }
-#endif /* HAVE_EXTENDED_MASTER */
 #ifndef NO_RESUME_SUITE_CHECK
     /* Suite must match (RFC 5246 7.4.1.3), tickets included. Skip when no suite
      * was retained (both zero = TLS_NULL_WITH_NULL_NULL, e.g. EAP-FAST PAC). */
@@ -34553,6 +34544,31 @@ static void MakePSKPreMasterSecret(Arrays* arrays, byte use_psk_key)
         }
 #endif /* HAVE_TLS_EXTENSIONS */
 
+#ifdef HAVE_EXTENDED_MASTER
+        /* The negotiated EMS state is final once the ServerHello extensions
+         * are parsed: abort a requiring client here, before any key material
+         * is computed or sent. */
+        if (ssl->options.requireEMS && !ssl->options.haveEMS) {
+            byte skipEmsCheck = 0;
+#ifdef HAVE_SECRET_CALLBACK
+            /* Skip for EAP-FAST (session-secret callback): the master secret
+             * comes from the callback. */
+            skipEmsCheck = (ssl->sessionSecretCb != NULL
+#ifdef HAVE_SESSION_TICKET
+                            && ssl->session != NULL
+                            && ssl->session->ticketLen > 0
+#endif
+                           ) ? 1 : 0;
+#endif
+            if (!skipEmsCheck) {
+                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 /* HAVE_EXTENDED_MASTER */
+
 #if !defined(NO_WOLFSSL_CLIENT) && !defined(WOLFSSL_NO_TLS12) && \
     defined(HAVE_SERVER_RENEGOTIATION_INFO) && \
     !defined(WOLFSSL_HARDEN_TLS_NO_SCR_CHECK)
@@ -40665,7 +40681,7 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
 #endif /* !WOLFSSL_NO_TICKET_EXPIRE && !NO_ASN_TIME */
 
         if (!ssl->options.resuming) {
-            /* Expired above: DoClientHello falls back to a full handshake. */
+            /* Resumption abandoned: DoClientHello runs a full handshake. */
             return ret;
         }
 
@@ -40680,13 +40696,25 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
             }
             /* if old sess used EMS, but new doesn't, MUST abort */
             else if (session->haveEMS && !ssl->options.haveEMS) {
-                WOLFSSL_MSG("Trying to resume a session with EMS without "
-                            "using EMS");
-            #ifdef WOLFSSL_EXTRA_ALERTS
-                SendAlert(ssl, alert_fatal, handshake_failure);
-            #endif
-                ret = EXT_MASTER_SECRET_NEEDED_E;
-                WOLFSSL_ERROR_VERBOSE(ret);
+#ifdef HAVE_EXTENDED_MASTER
+                if (ssl->options.disableEMS) {
+                    /* Local disable, not a client downgrade: decline the
+                     * resumption and do a full handshake. */
+                    WOLFSSL_MSG("EMS disabled locally, declining resumption "
+                                "of an EMS session. Do full handshake.");
+                    ssl->options.resuming = 0;
+                }
+                else
+#endif
+                {
+                    WOLFSSL_MSG("Trying to resume a session with EMS without "
+                                "using EMS");
+                #ifdef WOLFSSL_EXTRA_ALERTS
+                    SendAlert(ssl, alert_fatal, handshake_failure);
+                #endif
+                    ret = EXT_MASTER_SECRET_NEEDED_E;
+                    WOLFSSL_ERROR_VERBOSE(ret);
+                }
             }
         }
         else {

src/keys.c

@@ -3935,6 +3935,28 @@ int StoreKeys(WOLFSSL* ssl, const byte* keyData, int side)
     return 0;
 }
 
+#if !defined(NO_OLD_TLS) || defined(HAVE_EXTENDED_MASTER)
+static void CleanPreMaster(WOLFSSL* ssl)
+{
+    int sz = (int)(ssl->arrays->preMasterSz);
+
+#ifdef WOLFSSL_CHECK_MEM_ZERO
+    wc_MemZero_Add("CleanPreMaster preMasterSecret",
+                   ssl->arrays->preMasterSecret, sz);
+#endif
+
+    ForceZero(ssl->arrays->preMasterSecret, sz);
+
+#ifdef WOLFSSL_CHECK_MEM_ZERO
+    wc_MemZero_Check(ssl->arrays->preMasterSecret, sz);
+#endif
+
+    XFREE(ssl->arrays->preMasterSecret, ssl->heap, DYNAMIC_TYPE_SECRET);
+    ssl->arrays->preMasterSecret = NULL;
+    ssl->arrays->preMasterSz = 0;
+}
+#endif /* !NO_OLD_TLS || HAVE_EXTENDED_MASTER */
+
 #ifndef NO_OLD_TLS
 int DeriveKeys(WOLFSSL* ssl)
 {
@@ -4062,27 +4084,6 @@ int DeriveKeys(WOLFSSL* ssl)
 }
 
 
-static void CleanPreMaster(WOLFSSL* ssl)
-{
-    int sz = (int)(ssl->arrays->preMasterSz);
-
-#ifdef WOLFSSL_CHECK_MEM_ZERO
-    wc_MemZero_Add("CleanPreMaster preMasterSecret",
-                   ssl->arrays->preMasterSecret, sz);
-#endif
-
-    ForceZero(ssl->arrays->preMasterSecret, sz);
-
-#ifdef WOLFSSL_CHECK_MEM_ZERO
-    wc_MemZero_Check(ssl->arrays->preMasterSecret, sz);
-#endif
-
-    XFREE(ssl->arrays->preMasterSecret, ssl->heap, DYNAMIC_TYPE_SECRET);
-    ssl->arrays->preMasterSecret = NULL;
-    ssl->arrays->preMasterSz = 0;
-}
-
-
 /* Create and store the master secret see page 32, 6.1 */
 static int MakeSslMasterSecret(WOLFSSL* ssl)
 {
@@ -4238,6 +4239,8 @@ int MakeMasterSecret(WOLFSSL* ssl)
         WOLFSSL_MSG("EMS required but not negotiated with peer");
         SendAlert(ssl, alert_fatal, handshake_failure);
         WOLFSSL_ERROR_VERBOSE(EXT_MASTER_SECRET_NEEDED_E);
+        if (ssl->arrays->preMasterSecret != NULL)
+            CleanPreMaster(ssl);
         return EXT_MASTER_SECRET_NEEDED_E;
     }
 #endif

src/ssl_api_ext.c

@@ -1563,6 +1563,23 @@ int wolfSSL_set_SessionTicket_cb(WOLFSSL* ssl,
 
 #ifdef HAVE_EXTENDED_MASTER
 
+/* EMS applies to (D)TLS 1.0-1.2 only; the same version gate is applied by
+ * InitSSL_Ctx and InitSSL_Side when arming the default advertisement. */
+static int EmsAllowedForVersion(ProtocolVersion pv)
+{
+    int allowed = 0;
+
+    if (pv.major == SSLv3_MAJOR && pv.minor >= TLSv1_MINOR)
+        allowed = 1;
+#ifdef WOLFSSL_DTLS
+    if (pv.major == DTLS_MAJOR)
+        allowed = 1;
+#endif
+
+    return allowed;
+}
+
+
 /* Disable the Extended Master Secret extension on the context.
  *
  * For a client this stops the extension being advertised. For a server this
@@ -1641,7 +1658,8 @@ int wolfSSL_CTX_EnableExtendedMasterSecret(WOLFSSL_CTX* ctx)
         /* 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)
+        if (ctx->method != NULL && ctx->method->side == WOLFSSL_CLIENT_END &&
+                EmsAllowedForVersion(ctx->method->version))
             ctx->haveEMS = 1;
     }
 
@@ -1671,7 +1689,8 @@ int wolfSSL_EnableExtendedMasterSecret(WOLFSSL* ssl)
         /* 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)
+        if (ssl->options.side == WOLFSSL_CLIENT_END &&
+                EmsAllowedForVersion(ssl->ctx->method->version))
             ssl->options.haveEMS = 1;
     }
 
@@ -1705,7 +1724,8 @@ int wolfSSL_CTX_RequireExtendedMasterSecret(WOLFSSL_CTX* ctx)
         /* 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)
+        if (ctx->method != NULL && ctx->method->side == WOLFSSL_CLIENT_END &&
+                EmsAllowedForVersion(ctx->method->version))
             ctx->haveEMS = 1;
     }
 
@@ -1737,7 +1757,8 @@ int wolfSSL_RequireExtendedMasterSecret(WOLFSSL* ssl)
         /* 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)
+        if (ssl->options.side == WOLFSSL_CLIENT_END &&
+                EmsAllowedForVersion(ssl->ctx->method->version))
             ssl->options.haveEMS = 1;
     }
 

tests/api.c

@@ -39474,6 +39474,7 @@ TEST_CASE testCases[] = {
     TEST_DECL(test_tls_ems_resumption_downgrade),
     TEST_DECL(test_tls_ems_resumption_server_downgrade),
     TEST_DECL(test_tls_ems_server_disable),
+    TEST_DECL(test_tls_ems_server_disable_resumption),
     TEST_DECL(test_tls_ems_disable_v23),
     TEST_DECL(test_tls_require_ems),
     TEST_DECL(test_tls_require_ems_resumption),

tests/api/test_tls_ext.c

@@ -352,6 +352,59 @@ int test_tls_ems_server_disable(void)
 }
 
 
+/* A server that disables EMS declines resumption of a session that used EMS
+ * from an EMS-offering client: full handshake instead of a fatal alert. */
+int test_tls_ems_server_disable_resumption(void)
+{
+    EXPECT_DECLS;
+#if !defined(WOLFSSL_NO_TLS12) && defined(HAVE_EXTENDED_MASTER) && \
+        !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) && \
+        defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
+        !defined(NO_SESSION_CACHE)
+    struct test_memio_ctx test_ctx;
+    WOLFSSL_CTX *ctx_c = NULL;
+    WOLFSSL_CTX *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL;
+    WOLFSSL *ssl_s = NULL;
+    WOLFSSL_SESSION *session = NULL;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+
+    /* Establish a session that uses EMS. */
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+            wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+    ExpectNotNull(session = wolfSSL_get1_session(ssl_c));
+    ExpectTrue(session->haveEMS);
+
+    wolfSSL_free(ssl_c);
+    ssl_c = NULL;
+    wolfSSL_free(ssl_s);
+    ssl_s = NULL;
+    test_memio_clear_buffer(&test_ctx, 0);
+    test_memio_clear_buffer(&test_ctx, 1);
+
+    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_DisableExtendedMasterSecret(ssl_s), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_set_session(ssl_c, session), WOLFSSL_SUCCESS);
+
+    /* The handshake must complete as a full handshake without EMS. */
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+    ExpectIntEQ(ssl_s->options.resuming, 0);
+    ExpectIntEQ(ssl_c->options.haveEMS, 0);
+    ExpectIntEQ(ssl_s->options.haveEMS, 0);
+
+    wolfSSL_SESSION_free(session);
+    wolfSSL_free(ssl_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_CTX_free(ctx_s);
+#endif
+    return EXPECT_RESULT();
+}
+
+
 #if !defined(WOLFSSL_NO_TLS12) && defined(HAVE_EXTENDED_MASTER) && \
         !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) && \
         defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES)

tests/api/test_tls_ext.h

@@ -26,6 +26,7 @@ int test_tls_ems_downgrade(void);
 int test_tls_ems_resumption_downgrade(void);
 int test_tls_ems_resumption_server_downgrade(void);
 int test_tls_ems_server_disable(void);
+int test_tls_ems_server_disable_resumption(void);
 int test_tls_ems_disable_v23(void);
 int test_tls_require_ems(void);
 int test_tls_require_ems_resumption(void);