Bug 1294977 - Fail on missing supported_groups when doing DHE. r=mt

7 files changed, 26 insertions, 1 deletions

diff --git a/external_tests/nss_bogo_shim/config.json b/external_tests/nss_bogo_shim/config.json
index 078b4e737..1c8b56a43 100644
--- a/external_tests/nss_bogo_shim/config.json
+++ b/external_tests/nss_bogo_shim/config.json
@@ -9,7 +9,7 @@
         "*SignatureType-TLS13":"SignatureScheme patch",
         "ECDSACurveMismatch-Verify-TLS13":"SignatureScheme patch",
         "ServerAuth-NoFallback-TLS13":"PSS",
-        "NoSupportedCurves":"NSS needs to fail when no supported curves. Bug 1294977",
+        "NoSupportedCurves":"This tests a non-spec behavior for TLS 1.2 and expects the wrong alert for TLS 1.3",
         "SendEmptyRecords":"Tests a non-spec behavior in BoGo where it chokes on too many empty records",
         "LargePlaintext":"NSS needs to check for over-long records. Bug 1294978",
         "TLS13-RC4-MD5-server":"This fails properly but returns an unexpected error. Not a bug but needs cleanup",
diff --git a/external_tests/ssl_gtest/ssl_extension_unittest.cc b/external_tests/ssl_gtest/ssl_extension_unittest.cc
index f8fad9606..15e688cb1 100644
--- a/external_tests/ssl_gtest/ssl_extension_unittest.cc
+++ b/external_tests/ssl_gtest/ssl_extension_unittest.cc
@@ -405,6 +405,14 @@ TEST_P(TlsExtensionTest12Plus, SignatureAlgorithmsOddLength) {
                                                 extension));
 }
 
+
+TEST_P(TlsExtensionTestGeneric, NoSupportedGroups) {
+  ClientHelloErrorTest(new TlsExtensionDropper(ssl_supported_groups_xtn),
+                       version_ < SSL_LIBRARY_VERSION_TLS_1_3 ?
+                       kTlsAlertDecryptError:
+                       kTlsAlertMissingExtension);
+}
+
 TEST_P(TlsExtensionTestGeneric, SupportedCurvesShort) {
   const uint8_t val[] = { 0x00, 0x01, 0x00 };
   DataBuffer extension(val, sizeof(val));
diff --git a/external_tests/ssl_gtest/tls_parser.h b/external_tests/ssl_gtest/tls_parser.h
index e7de1ecac..5e2fce482 100644
--- a/external_tests/ssl_gtest/tls_parser.h
+++ b/external_tests/ssl_gtest/tls_parser.h
@@ -41,6 +41,8 @@ const uint8_t kTlsAlertBadRecordMac = 20;
 const uint8_t kTlsAlertHandshakeFailure = 40;
 const uint8_t kTlsAlertIllegalParameter = 47;
 const uint8_t kTlsAlertDecodeError = 50;
+const uint8_t kTlsAlertDecryptError = 51;
+const uint8_t kTlsAlertMissingExtension = 109;
 const uint8_t kTlsAlertUnsupportedExtension = 110;
 const uint8_t kTlsAlertNoApplicationProtocol = 120;
 
diff --git a/lib/ssl/SSLerrs.h b/lib/ssl/SSLerrs.h
index d0f082f37..07c0d04ab 100644
--- a/lib/ssl/SSLerrs.h
+++ b/lib/ssl/SSLerrs.h
@@ -481,3 +481,6 @@ ER3(SSL_ERROR_MISSING_ALPN_EXTENSION, (SSL_ERROR_BASE + 150),
 
 ER3(SSL_ERROR_RX_UNEXPECTED_EXTENSION, (SSL_ERROR_BASE + 151),
     "SSL received an unexpected extension.")
+
+ER3(SSL_ERROR_MISSING_SUPPORTED_GROUPS, (SSL_ERROR_BASE + 152),
+    "SSL expected a supported groups extension.")
diff --git a/lib/ssl/ssl3ecc.c b/lib/ssl/ssl3ecc.c
index 49c3c4492..c8b23cb71 100644
--- a/lib/ssl/ssl3ecc.c
+++ b/lib/ssl/ssl3ecc.c
@@ -1235,5 +1235,8 @@ ssl_HandleSupportedGroupsXtn(sslSocket *ss, PRUint16 ex_type, SECItem *data)
         }
     }
 
+    /* Remember that we negotiated this extension. */
+    ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type;
+
     return SECSuccess;
 }
diff --git a/lib/ssl/sslerr.h b/lib/ssl/sslerr.h
index 47721e187..188e44485 100644
--- a/lib/ssl/sslerr.h
+++ b/lib/ssl/sslerr.h
@@ -235,6 +235,7 @@ typedef enum {
     SSL_ERROR_END_OF_EARLY_DATA_ALERT = (SSL_ERROR_BASE + 149),
     SSL_ERROR_MISSING_ALPN_EXTENSION = (SSL_ERROR_BASE + 150),
     SSL_ERROR_RX_UNEXPECTED_EXTENSION = (SSL_ERROR_BASE + 151),
+    SSL_ERROR_MISSING_SUPPORTED_GROUPS_EXTENSION = (SSL_ERROR_BASE + 152),
     SSL_ERROR_END_OF_LIST   /* let the c compiler determine the value of this. */
 } SSLErrorCodes;
 #endif /* NO_SECURITY_ERROR_ENUM */
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index d31d72356..487d30e6f 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -1122,6 +1122,14 @@ tls13_HandleClientKeyShare(sslSocket *ss)
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
 
+    /* Verify that the other side sent supported groups as required
+     * by the specification. */
+    if (!ssl3_ExtensionNegotiated(ss, ssl_supported_groups_xtn)) {
+        FATAL_ERROR(ss, SSL_ERROR_MISSING_SUPPORTED_GROUPS_EXTENSION,
+                    missing_extension);
+        return SECFailure;
+    }
+
     /* Figure out what group we expect */
     switch (ss->ssl3.hs.kea_def->exchKeyType) {
         case ssl_kea_ecdh: