Bug 1751157 - Throw illegal_parameter alert for illegal extensions in handshake message. r=djackson

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

3 files changed, 75 insertions, 19 deletions

diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc
index c95554d17..b0c42c6fa 100644
--- a/gtests/ssl_gtest/ssl_extension_unittest.cc
+++ b/gtests/ssl_gtest/ssl_extension_unittest.cc
@@ -758,7 +758,7 @@ TEST_F(TlsExtensionTest13Stream, AddServerSignatureAlgorithmsOnResumption) {
   DataBuffer empty;
   MakeTlsFilter<TlsExtensionInjector>(server_, ssl_signature_algorithms_xtn,
                                       empty);
-  client_->ExpectSendAlert(kTlsAlertUnsupportedExtension);
+  client_->ExpectSendAlert(kTlsAlertIllegalParameter);
   server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
   ConnectExpectFail();
   EXPECT_EQ(SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION, client_->error_code());
@@ -1247,19 +1247,6 @@ TEST_P(TlsBogusExtensionTest13, AddBogusExtensionHelloRetryRequest) {
   Run(kTlsHandshakeHelloRetryRequest);
 }
 
-TEST_P(TlsBogusExtensionTest13, AddVersionExtensionEncryptedExtensions) {
-  Run(kTlsHandshakeEncryptedExtensions, ssl_tls13_supported_versions_xtn);
-}
-
-TEST_P(TlsBogusExtensionTest13, AddVersionExtensionCertificate) {
-  Run(kTlsHandshakeCertificate, ssl_tls13_supported_versions_xtn);
-}
-
-TEST_P(TlsBogusExtensionTest13, AddVersionExtensionCertificateRequest) {
-  server_->RequestClientAuth(false);
-  Run(kTlsHandshakeCertificateRequest, ssl_tls13_supported_versions_xtn);
-}
-
 // NewSessionTicket allows unknown extensions AND it isn't protected by the
 // Finished.  So adding an unknown extension doesn't cause an error.
 TEST_P(TlsBogusExtensionTest13, AddBogusExtensionNewSessionTicket) {
@@ -1277,6 +1264,55 @@ TEST_P(TlsBogusExtensionTest13, AddBogusExtensionNewSessionTicket) {
   SendReceive();
 }
 
+class TlsDisallowedExtensionTest13 : public TlsBogusExtensionTest {
+ protected:
+  void ConnectAndFail(uint8_t message) override {
+    ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
+  }
+};
+
+TEST_P(TlsDisallowedExtensionTest13, AddVersionExtensionEncryptedExtensions) {
+  Run(kTlsHandshakeEncryptedExtensions, ssl_tls13_supported_versions_xtn);
+}
+
+TEST_P(TlsDisallowedExtensionTest13, AddVersionExtensionCertificate) {
+  Run(kTlsHandshakeCertificate, ssl_tls13_supported_versions_xtn);
+}
+
+TEST_P(TlsDisallowedExtensionTest13, AddVersionExtensionCertificateRequest) {
+  server_->RequestClientAuth(false);
+  Run(kTlsHandshakeCertificateRequest, ssl_tls13_supported_versions_xtn);
+}
+
+/* For unadvertised disallowed extensions an unsupported_extension alert is
+ * thrown since NSS checks for unadvertised extensions before its disallowed
+ * extension check. */
+class TlsDisallowedUnadvertisedExtensionTest13 : public TlsBogusExtensionTest {
+ protected:
+  void ConnectAndFail(uint8_t message) override {
+    uint8_t alert = kTlsAlertUnsupportedExtension;
+    if (message == kTlsHandshakeCertificateRequest) {
+      alert = kTlsAlertIllegalParameter;
+    }
+    ConnectExpectAlert(client_, alert);
+  }
+};
+
+TEST_P(TlsDisallowedUnadvertisedExtensionTest13,
+       AddPSKExtensionEncryptedExtensions) {
+  Run(kTlsHandshakeEncryptedExtensions, ssl_tls13_pre_shared_key_xtn);
+}
+
+TEST_P(TlsDisallowedUnadvertisedExtensionTest13, AddPSKExtensionCertificate) {
+  Run(kTlsHandshakeCertificate, ssl_tls13_pre_shared_key_xtn);
+}
+
+TEST_P(TlsDisallowedUnadvertisedExtensionTest13,
+       AddPSKExtensionCertificateRequest) {
+  server_->RequestClientAuth(false);
+  Run(kTlsHandshakeCertificateRequest, ssl_tls13_pre_shared_key_xtn);
+}
+
 TEST_P(TlsConnectStream, IncludePadding) {
   EnsureTlsSetup();
   SSL_EnableTls13GreaseEch(client_->ssl_fd(), PR_FALSE);  // Don't GREASE
@@ -1347,4 +1383,13 @@ INSTANTIATE_TEST_SUITE_P(BogusExtension13, TlsBogusExtensionTest13,
                          ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
                                             TlsConnectTestBase::kTlsV13));
 
+INSTANTIATE_TEST_SUITE_P(DisallowedExtension13, TlsDisallowedExtensionTest13,
+                         ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
+                                            TlsConnectTestBase::kTlsV13));
+
+INSTANTIATE_TEST_SUITE_P(DisallowedUnadvertisedExtension13,
+                         TlsDisallowedUnadvertisedExtensionTest13,
+                         ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
+                                            TlsConnectTestBase::kTlsV13));
+
 }  // namespace nss_test
diff --git a/gtests/ssl_gtest/tls_ech_unittest.cc b/gtests/ssl_gtest/tls_ech_unittest.cc
index 72e5442e7..d933c48b6 100644
--- a/gtests/ssl_gtest/tls_ech_unittest.cc
+++ b/gtests/ssl_gtest/tls_ech_unittest.cc
@@ -2411,8 +2411,8 @@ TEST_F(TlsConnectStreamTls13, EchOuterExtensionsInCHOuter) {
                                       ssl_tls13_outer_extensions_xtn,
                                       outer_buf);
 
-  ConnectExpectAlert(server_, kTlsAlertUnsupportedExtension);
-  client_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_EXTENSION_ALERT);
+  ConnectExpectAlert(server_, kTlsAlertIllegalParameter);
+  client_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
   server_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CLIENT_HELLO);
 }
 
diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c
index d634bece4..d103ab858 100644
--- a/lib/ssl/ssl3ext.c
+++ b/lib/ssl/ssl3ext.c
@@ -550,12 +550,23 @@ ssl3_HandleParsedExtensions(sslSocket *ss, SSLHandshakeType message)
                     if (allowNotOffered) {
                         continue; /* Skip over unknown extensions. */
                     }
-                /* Fall through. */
+                    /* RFC8446 Section 4.2 - Implementations MUST NOT send extension responses if
+                     * the remote endpoint did not send the corresponding extension request ...
+                     * Upon receiving such an extension, an endpoint MUST abort the handshake with
+                     * an "unsupported_extension" alert. */
+                    SSL_TRC(3, ("%d: TLS13: unknown extension %d in message %d",
+                                SSL_GETPID(), extension, message));
+                    tls13_FatalError(ss, SSL_ERROR_RX_UNEXPECTED_EXTENSION,
+                                     unsupported_extension);
+                    return SECFailure;
                 case tls13_extension_disallowed:
-                    SSL_TRC(3, ("%d: TLS13: unexpected extension %d in message %d",
+                    /* RFC8446 Section 4.2 - If an implementation receives an extension which it
+                     * recognizes and which is not specified for the message in which it appears,
+                     * it MUST abort the handshake with an "illegal_parameter" alert. */
+                    SSL_TRC(3, ("%d: TLS13: disallowed extension %d in message %d",
                                 SSL_GETPID(), extension, message));
                     tls13_FatalError(ss, SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION,
-                                     unsupported_extension);
+                                     illegal_parameter);
                     return SECFailure;
             }
         }