Bug 1330612 - Assume P-256 is all that is supported if client doesn't include supported_groups, r=ekr

2 files changed, 66 insertions, 0 deletions

diff --git a/gtests/ssl_gtest/ssl_ecdh_unittest.cc b/gtests/ssl_gtest/ssl_ecdh_unittest.cc
index 43dfcbac8..2f50ec4ee 100644
--- a/gtests/ssl_gtest/ssl_ecdh_unittest.cc
+++ b/gtests/ssl_gtest/ssl_ecdh_unittest.cc
@@ -191,6 +191,58 @@ TEST_P(TlsConnectGenericPre13, P384PriorityFromModelSocket) {
             ssl_sig_rsa_pss_sha256);
 }
 
+class TlsKeyExchangeGroupCapture : public TlsHandshakeFilter {
+ public:
+  TlsKeyExchangeGroupCapture() : group_(ssl_grp_none) {}
+
+  SSLNamedGroup group() const { return group_; }
+
+ protected:
+  virtual PacketFilter::Action FilterHandshake(const HandshakeHeader &header,
+                                               const DataBuffer &input,
+                                               DataBuffer *output) {
+    if (header.handshake_type() != kTlsHandshakeServerKeyExchange) {
+      return KEEP;
+    }
+
+    uint32_t value = 0;
+    EXPECT_TRUE(input.Read(0, 1, &value));
+    EXPECT_EQ(3U, value) << "curve type has to be 3";
+
+    EXPECT_TRUE(input.Read(1, 2, &value));
+    group_ = static_cast<SSLNamedGroup>(value);
+
+    return KEEP;
+  }
+
+ private:
+  SSLNamedGroup group_;
+};
+
+// If we strip the client's supported groups extension, the server should assume
+// P-256 is supported by the client (<= 1.2 only).
+TEST_P(TlsConnectGenericPre13, DropSupportedGroupExtensionP256) {
+  EnsureTlsSetup();
+  client_->SetPacketFilter(new TlsExtensionDropper(ssl_supported_groups_xtn));
+  auto group_capture = new TlsKeyExchangeGroupCapture();
+  server_->SetPacketFilter(group_capture);
+
+  ConnectExpectFail();
+  client_->CheckErrorCode(SSL_ERROR_DECRYPT_ERROR_ALERT);
+  server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
+
+  EXPECT_EQ(ssl_grp_ec_secp256r1, group_capture->group());
+}
+
+// Supported groups is mandatory in TLS 1.3.
+TEST_P(TlsConnectTls13, DropSupportedGroupExtension) {
+  EnsureTlsSetup();
+  client_->SetPacketFilter(new TlsExtensionDropper(ssl_supported_groups_xtn));
+  ConnectExpectFail();
+  client_->CheckErrorCode(SSL_ERROR_MISSING_EXTENSION_ALERT);
+  server_->CheckErrorCode(SSL_ERROR_MISSING_SUPPORTED_GROUPS_EXTENSION);
+}
+
 // If we only have a lame group, we fall back to static RSA.
 TEST_P(TlsConnectGenericPre13, UseLameGroup) {
   const std::vector<SSLNamedGroup> groups = {ssl_grp_ec_secp192r1};
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 154d22a49..42fa55178 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -8203,6 +8203,20 @@ ssl3_SelectServerCert(sslSocket *ss)
     const ssl3KEADef *kea_def = ss->ssl3.hs.kea_def;
     PRCList *cursor;
 
+    /* If the client didn't include the supported groups extension, assume just
+     * P-256 support and disable all the other ECDHE groups.  This also affects
+     * ECDHE group selection, but this function is called first. */
+    if (!ssl3_ExtensionNegotiated(ss, ssl_supported_groups_xtn)) {
+        unsigned int i;
+        for (i = 0; i < SSL_NAMED_GROUP_COUNT; ++i) {
+            if (ss->namedGroupPreferences[i] &&
+                ss->namedGroupPreferences[i]->keaType == ssl_kea_ecdh &&
+                ss->namedGroupPreferences[i]->name != ssl_grp_ec_secp256r1) {
+                ss->namedGroupPreferences[i] = NULL;
+            }
+        }
+    }
+
     /* This picks the first certificate that has:
      * a) the right authentication method, and
      * b) the right named curve (EC only)