Bug 1779234 - Added check for server only sending ECH extension with retry configs in EncryptedExtensions and if not accepting ECH. Changed config setting behavior to skip configs with unsupported mandatory extensions instead of failing. r=djackson

The following bogo tests test the changed behavior:
  - TLS-ECH-GREASE-Client-TLS12-RejectRetryConfigs
  - TLS-ECH-Client-TLS12-RejectRetryConfigs - This and above test, test correct rejection of TLS 1.2 server ECH extension with rettry configs (outside EncryptedExtensions)
  - TLS-ECH-Client-Accept-RejectRetryConfigs - Test correct rejection of retry configs received even though ECH was acceped by server.
  - TLS-ECH-Client-SelectECHConfig - Tests correct skipping of unsupported (mandatory extension) configs.

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

2 files changed, 37 insertions, 7 deletions

diff --git a/lib/ssl/tls13ech.c b/lib/ssl/tls13ech.c
index a245a885b..2c637e719 100644
--- a/lib/ssl/tls13ech.c
+++ b/lib/ssl/tls13ech.c
@@ -176,6 +176,7 @@ tls13_DecodeEchConfigContents(const sslReadBuffer *rawConfig,
     sslReader suiteReader;
     sslReader extensionReader;
     PRBool hasValidSuite = PR_FALSE;
+    PRBool unsupportedMandatoryXtn = PR_FALSE;
 
     /* HpkeKeyConfig key_config */
     /* uint8 config_id */
@@ -297,10 +298,12 @@ tls13_DecodeEchConfigContents(const sslReadBuffer *rawConfig,
         }
         extensionTypes[extensionIndex++] = (PRUint16)tmpn;
 
-        /* If it's mandatory, fail. */
+        /* Clients MUST parse the extension list and check for unsupported
+         * mandatory extensions.  If an unsupported mandatory extension is
+         * present, clients MUST ignore the ECHConfig
+         * [draft-ietf-tls-esni, Section 4.2]. */
         if (tmpn & (1 << 15)) {
-            PORT_SetError(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);
-            goto loser;
+            unsupportedMandatoryXtn = PR_TRUE;
         }
 
         /* Skip. */
@@ -316,10 +319,10 @@ tls13_DecodeEchConfigContents(const sslReadBuffer *rawConfig,
         goto loser;
     }
 
-    /* If the ciphersuites weren't compatible, don't
-     * set the outparam. Return success to indicate
-     * the config was well-formed. */
-    if (hasValidSuite) {
+    /* If the ciphersuites were compatible AND if NO unsupported mandatory
+     * extensions were found set the outparam. Return success either way if the
+     * config was well-formed. */
+    if (hasValidSuite && !unsupportedMandatoryXtn) {
         decodedConfig = PORT_ZNew(sslEchConfig);
         if (!decodedConfig) {
             goto loser;
diff --git a/lib/ssl/tls13exthandle.c b/lib/ssl/tls13exthandle.c
index c797feb26..3999025c8 100644
--- a/lib/ssl/tls13exthandle.c
+++ b/lib/ssl/tls13exthandle.c
@@ -1241,6 +1241,33 @@ tls13_ClientHandleEchXtn(const sslSocket *ss, TLSExtensionData *xtnData,
     PRCList parsedConfigs;
     PR_INIT_CLIST(&parsedConfigs);
 
+    /* The [retry config] response is valid only when the server used the
+     * ClientHelloOuter. If the server sent this extension in response to the
+     * inner variant [ECH was accepted], then the client MUST abort with an
+     * "unsupported_extension" alert [draft-ietf-tls-esni-14, Section 5]. */
+    if (ss->ssl3.hs.echAccepted) {
+        PORT_SetError(SSL_ERROR_RX_UNEXPECTED_EXTENSION);
+        ssl3_ExtSendAlert(ss, alert_fatal, unsupported_extension);
+        return SECFailure;
+    }
+
+    /* If the server is configured with any ECHConfigs, it MUST include the
+     * "encrypted_client_hello" extension in its EncryptedExtensions with the
+     * "retry_configs" field set to one or more ECHConfig structures with
+     * up-to-date keys [draft-ietf-tls-esni-14, Section 7.1]. */
+    if (ss->ssl3.hs.msg_type != ssl_hs_encrypted_extensions) {
+        PORT_SetError(SSL_ERROR_RX_UNEXPECTED_EXTENSION);
+        if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
+            /* For TLS < 1.3 the extension is unkown/unsupported. */
+            ssl3_ExtSendAlert(ss, alert_fatal, unsupported_extension);
+        } else {
+            /* For TLS 1.3 the extension is known but prohibited outside EE
+             * (see RFC8446, Section 4.2 for alert rationale). */
+            ssl3_ExtSendAlert(ss, alert_fatal, illegal_parameter);
+        }
+        return SECFailure;
+    }
+
     PORT_Assert(!xtnData->ech);
     xtnData->ech = PORT_ZNew(sslEchXtnState);
     if (!xtnData->ech) {