Bug 1789410 - ECH client: Send ech_required alert on server negotiating TLS 1.2. Fixed misleading Gtest, enabled corresponding BoGo test. r=djackson

Differential Revision: https://phabricator.services.mozilla.com/D156565

4 files changed, 30 insertions, 29 deletions

diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 58cb82dab..e05dc0612 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -12372,9 +12372,7 @@ ssl3_FinishHandshake(sslSocket *ss)
     ss->ssl3.hs.canFalseStart = PR_FALSE; /* False Start phase is complete */
     ss->ssl3.hs.ws = idle_handshake;
 
-    ssl_FinishHandshake(ss);
-
-    return SECSuccess;
+    return ssl_FinishHandshake(ss);
 }
 
 SECStatus
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 8d8be7c56..af61823c1 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1268,7 +1268,7 @@ extern void ssl3_SetAlwaysBlock(sslSocket *ss);
 
 extern SECStatus ssl_EnableNagleDelay(sslSocket *ss, PRBool enabled);
 
-extern void ssl_FinishHandshake(sslSocket *ss);
+extern SECStatus ssl_FinishHandshake(sslSocket *ss);
 
 extern SECStatus ssl_CipherPolicySet(PRInt32 which, PRInt32 policy);
 
diff --git a/lib/ssl/sslsecur.c b/lib/ssl/sslsecur.c
index 4d2b2070c..e1f53108c 100644
--- a/lib/ssl/sslsecur.c
+++ b/lib/ssl/sslsecur.c
@@ -15,6 +15,7 @@
 #include "pk11func.h" /* for PK11_GenerateRandom */
 #include "nss.h"      /* for NSS_RegisterShutdown */
 #include "prinit.h"   /* for PR_CallOnceWithArg */
+#include "tls13ech.h"
 #include "tls13psk.h"
 
 /* Step through the handshake functions.
@@ -49,11 +50,34 @@ ssl_Do1stHandshake(sslSocket *ss)
     return rv;
 }
 
-void
+SECStatus
 ssl_FinishHandshake(sslSocket *ss)
 {
     PORT_Assert(ss->opt.noLocks || ssl_Have1stHandshakeLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
+    PORT_Assert(ss->ssl3.hs.echAccepted ||
+                (ss->opt.enableTls13BackendEch &&
+                 ss->xtnData.ech &&
+                 ss->xtnData.ech->receivedInnerXtn) ==
+                    ssl3_ExtensionNegotiated(ss, ssl_tls13_encrypted_client_hello_xtn));
+
+    /* If ECH was OFFERED to (echHpkeCtx is set on the client) DISABLED by the
+     * server through negotiation of a TLS version < 1.3, an 'ech_required'
+     * alert MUST be sent to inform the server about the intention / possible
+     * misconfiguration. */
+    if (!ss->sec.isServer && ss->ssl3.hs.echHpkeCtx && !ss->ssl3.hs.echAccepted) {
+        SSL3_SendAlert(ss, alert_fatal, ech_required);
+        /* "If [one, none] of the retry_configs contains a supported version,
+         * the client can regard ECH as securely [replaced, disabled] by the
+         * server." */
+        if (ss->xtnData.ech && ss->xtnData.ech->retryConfigs.len) {
+            PORT_SetError(SSL_ERROR_ECH_RETRY_WITH_ECH);
+            ss->xtnData.ech->retryConfigsValid = PR_TRUE;
+        } else {
+            PORT_SetError(SSL_ERROR_ECH_RETRY_WITHOUT_ECH);
+        }
+        return SECFailure;
+    }
 
     SSL_TRC(3, ("%d: SSL[%d]: handshake is completed", SSL_GETPID(), ss->fd));
 
@@ -69,6 +93,8 @@ ssl_FinishHandshake(sslSocket *ss)
     }
 
     ssl_FreeEphemeralKeyPairs(ss);
+
+    return SECSuccess;
 }
 
 /*
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index 5f24f4d43..23fe279a8 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -5063,8 +5063,6 @@ loser:
 static SECStatus
 tls13_FinishHandshake(sslSocket *ss)
 {
-    /* If |!echHpkeCtx|, any advertised ECH was GREASE ECH. */
-    PRBool offeredEch = !ss->sec.isServer && ss->ssl3.hs.echHpkeCtx;
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
     PORT_Assert(ss->ssl3.hs.restartTarget == NULL);
@@ -5080,28 +5078,7 @@ tls13_FinishHandshake(sslSocket *ss)
 
     TLS13_SET_HS_STATE(ss, idle_handshake);
 
-    PORT_Assert(ss->ssl3.hs.echAccepted ||
-                (ss->opt.enableTls13BackendEch &&
-                 ss->xtnData.ech &&
-                 ss->xtnData.ech->receivedInnerXtn) ==
-                    ssl3_ExtensionNegotiated(ss, ssl_tls13_encrypted_client_hello_xtn));
-    if (offeredEch && !ss->ssl3.hs.echAccepted) {
-        SSL3_SendAlert(ss, alert_fatal, ech_required);
-
-        /* "If [one, none] of the retry_configs contains a supported version, the client can
-         * regard ECH as securely [replaced, disabled] by the server." */
-        if (ss->xtnData.ech && ss->xtnData.ech->retryConfigs.len) {
-            PORT_SetError(SSL_ERROR_ECH_RETRY_WITH_ECH);
-            ss->xtnData.ech->retryConfigsValid = PR_TRUE;
-        } else {
-            PORT_SetError(SSL_ERROR_ECH_RETRY_WITHOUT_ECH);
-        }
-        return SECFailure;
-    }
-
-    ssl_FinishHandshake(ss);
-
-    return SECSuccess;
+    return ssl_FinishHandshake(ss);
 }
 
 /* Do the parts of sending the client's second round that require