Bug 1315735 - TLS 1.3 draft 17 - Replace key shares in response to HRR. r=mt
authorEKR <ekr@rtfm.com>
Thu, 03 Nov 2016 12:16:13 -0700
changeset 12815 a170993900e8ee0cf437cebf7664036f64d51577
parent 12814 cfc2bb0dbe625ef563320f267bca2124fe4419a0
child 12816 cfb19895717121f4c6d02eb963e9e7c51ee3cd60
push id1754
push userekr@mozilla.com
push dateMon, 07 Nov 2016 22:00:24 +0000
reviewersmt
bugs1315735
Bug 1315735 - TLS 1.3 draft 17 - Replace key shares in response to HRR. r=mt Subscribers: mt Differential Revision: https://nss-dev.phacility.com/D129
gtests/ssl_gtest/ssl_ecdh_unittest.cc
gtests/ssl_gtest/ssl_hrr_unittest.cc
gtests/ssl_gtest/tls_connect.cc
gtests/ssl_gtest/tls_connect.h
gtests/ssl_gtest/tls_filter.cc
gtests/ssl_gtest/tls_filter.h
lib/ssl/tls13exthandle.c
--- a/gtests/ssl_gtest/ssl_ecdh_unittest.cc
+++ b/gtests/ssl_gtest/ssl_ecdh_unittest.cc
@@ -282,17 +282,17 @@ TEST_P(TlsKeyExchangeTest13, Curve25519P
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
 
   Connect();
 
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_sha256);
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_secp256r1};
-  CheckKEXDetails(client_groups, shares, false);
+  CheckKEXDetails(client_groups, shares);
 }
 
 TEST_P(TlsKeyExchangeTest13, Curve25519P256EqualPriorityServer13) {
   EnsureKeyShareSetup();
 
   // The client sends a 25519 key share while the server prefers P256.
   // We have to accept 25519 without retry.
   const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_curve25519,
@@ -302,17 +302,17 @@ TEST_P(TlsKeyExchangeTest13, Curve25519P
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
 
   Connect();
 
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_sha256);
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
-  CheckKEXDetails(client_groups, shares, false);
+  CheckKEXDetails(client_groups, shares);
 }
 
 TEST_P(TlsKeyExchangeTest13, EqualPriorityTestRetryECServer13) {
   EnsureKeyShareSetup();
 
   // The client sends a 25519 key share while the server prefers P256.
   // The server prefers P-384 over x25519, so it must not consider P-256 and
   // x25519 to be equivalent. It will therefore request a P-256 share
@@ -324,17 +324,17 @@ TEST_P(TlsKeyExchangeTest13, EqualPriori
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
 
   Connect();
 
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_sha256);
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
-  CheckKEXDetails(client_groups, shares, true);
+  CheckKEXDetails(client_groups, shares, ssl_grp_ec_secp256r1);
 }
 
 TEST_P(TlsKeyExchangeTest13, NotEqualPriorityWithIntermediateGroup13) {
   EnsureKeyShareSetup();
 
   // The client sends a 25519 key share while the server prefers P256.
   // The server prefers ffdhe_2048 over x25519, so it must not consider the
   // P-256 and x25519 to be equivalent. It will therefore request a P-256 share
@@ -346,17 +346,17 @@ TEST_P(TlsKeyExchangeTest13, NotEqualPri
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
 
   Connect();
 
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_sha256);
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
-  CheckKEXDetails(client_groups, shares, true);
+  CheckKEXDetails(client_groups, shares, ssl_grp_ec_secp256r1);
 }
 
 TEST_P(TlsKeyExchangeTest13,
        NotEqualPriorityWithUnsupportedFFIntermediateGroup13) {
   EnsureKeyShareSetup();
 
   // As in the previous test, the server prefers ffdhe_2048. Thus, even though
   // the client doesn't support this group, the server must not regard x25519 as
@@ -368,17 +368,17 @@ TEST_P(TlsKeyExchangeTest13,
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
 
   Connect();
 
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_sha256);
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
-  CheckKEXDetails(client_groups, shares, true);
+  CheckKEXDetails(client_groups, shares, ssl_grp_ec_secp256r1);
 }
 
 TEST_P(TlsKeyExchangeTest13,
        NotEqualPriorityWithUnsupportedECIntermediateGroup13) {
   EnsureKeyShareSetup();
 
   // As in the previous test, the server prefers P-384. Thus, even though
   // the client doesn't support this group, the server must not regard x25519 as
@@ -390,17 +390,17 @@ TEST_P(TlsKeyExchangeTest13,
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
 
   Connect();
 
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_sha256);
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
-  CheckKEXDetails(client_groups, shares, true);
+  CheckKEXDetails(client_groups, shares, ssl_grp_ec_secp256r1);
 }
 
 TEST_P(TlsKeyExchangeTest13, EqualPriority13) {
   EnsureKeyShareSetup();
 
   // The client sends a 25519 key share while the server prefers P256.
   // We have to accept 25519 without retry because it's considered equivalent to
   // P256 by the server.
@@ -410,17 +410,17 @@ TEST_P(TlsKeyExchangeTest13, EqualPriori
                                                     ssl_grp_ec_curve25519};
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
 
   Connect();
 
   CheckKeys();
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
-  CheckKEXDetails(client_groups, shares, false);
+  CheckKEXDetails(client_groups, shares);
 }
 #endif
 
 TEST_P(TlsConnectGeneric, P256ClientAndCurve25519Server) {
   EnsureTlsSetup();
   client_->DisableAllCiphers();
   client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);
 
@@ -453,17 +453,17 @@ TEST_P(TlsKeyExchangeTest13, MultipleCli
 
   Connect();
 
   // The server would accept 25519 but its preferred group (P256) has to win.
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_sha256);
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519,
                                              ssl_grp_ec_secp256r1};
-  CheckKEXDetails(client_groups, shares, false);
+  CheckKEXDetails(client_groups, shares);
 }
 
 // Replace the point in the client key exchange message with an empty one
 class ECCClientKEXFilter : public TlsHandshakeFilter {
  public:
   ECCClientKEXFilter() {}
 
  protected:
--- a/gtests/ssl_gtest/ssl_hrr_unittest.cc
+++ b/gtests/ssl_gtest/ssl_hrr_unittest.cc
@@ -124,34 +124,34 @@ TEST_P(TlsKeyExchange13, ConnectEcdhePre
   static const std::vector<SSLNamedGroup> server_groups = {
       ssl_grp_ec_curve25519, ssl_grp_ec_secp384r1};
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
   Connect();
   CheckKeys();
   static const std::vector<SSLNamedGroup> expectedShares = {
       ssl_grp_ec_secp384r1};
-  CheckKEXDetails(client_groups, expectedShares, true /* expect_hrr */);
+  CheckKEXDetails(client_groups, expectedShares, ssl_grp_ec_curve25519);
 }
 
 // This should work, but not use HRR because the key share for x25519 was
 // pre-generated by the client.
 TEST_P(TlsKeyExchange13, ConnectEcdhePreferenceMismatchHrrExtraShares) {
   EnsureKeyShareSetup();
   static const std::vector<SSLNamedGroup> client_groups = {
       ssl_grp_ec_secp384r1, ssl_grp_ec_curve25519};
   static const std::vector<SSLNamedGroup> server_groups = {
       ssl_grp_ec_curve25519, ssl_grp_ec_secp384r1};
   client_->ConfigNamedGroups(client_groups);
   server_->ConfigNamedGroups(server_groups);
   EXPECT_EQ(SECSuccess, SSL_SendAdditionalKeyShares(client_->ssl_fd(), 1));
 
   Connect();
   CheckKeys();
-  CheckKEXDetails(client_groups, client_groups, false /* expect_hrr */);
+  CheckKEXDetails(client_groups, client_groups);
 }
 
 TEST_F(TlsConnectTest, Select12AfterHelloRetryRequest) {
   EnsureTlsSetup();
   client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
                            SSL_LIBRARY_VERSION_TLS_1_3);
   server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
                            SSL_LIBRARY_VERSION_TLS_1_3);
--- a/gtests/ssl_gtest/tls_connect.cc
+++ b/gtests/ssl_gtest/tls_connect.cc
@@ -613,19 +613,21 @@ TlsConnectTls12Plus::TlsConnectTls12Plus
 
 TlsConnectTls13::TlsConnectTls13()
     : TlsConnectTestBase(GetParam(), SSL_LIBRARY_VERSION_TLS_1_3) {}
 
 void TlsKeyExchangeTest::EnsureKeyShareSetup() {
   EnsureTlsSetup();
   groups_capture_ = new TlsExtensionCapture(ssl_supported_groups_xtn);
   shares_capture_ = new TlsExtensionCapture(ssl_tls13_key_share_xtn);
+  shares_capture2_ = new TlsExtensionCapture(ssl_tls13_key_share_xtn, true);
   std::vector<PacketFilter*> captures;
   captures.push_back(groups_capture_);
   captures.push_back(shares_capture_);
+  captures.push_back(shares_capture2_);
   client_->SetPacketFilter(new ChainedPacketFilter(captures));
   capture_hrr_ =
       new TlsInspectorRecordHandshakeMessage(kTlsHandshakeHelloRetryRequest);
   server_->SetPacketFilter(capture_hrr_);
 }
 
 void TlsKeyExchangeTest::ConfigNamedGroups(
     const std::vector<SSLNamedGroup>& groups) {
@@ -678,9 +680,31 @@ void TlsKeyExchangeTest::CheckKEXDetails
     EXPECT_EQ(expected_shares, shares);
   } else {
     EXPECT_EQ(0U, shares_capture_->extension().len());
   }
 
   EXPECT_EQ(expect_hrr, capture_hrr_->buffer().len() != 0);
 }
 
+void TlsKeyExchangeTest::CheckKEXDetails(
+    const std::vector<SSLNamedGroup>& expected_groups,
+    const std::vector<SSLNamedGroup>& expected_shares) {
+  CheckKEXDetails(expected_groups, expected_shares, false);
+}
+
+void TlsKeyExchangeTest::CheckKEXDetails(
+    const std::vector<SSLNamedGroup>& expected_groups,
+    const std::vector<SSLNamedGroup>& expected_shares,
+    SSLNamedGroup expected_share2) {
+  CheckKEXDetails(expected_groups, expected_shares, true);
+
+  for (auto it : expected_shares) {
+    EXPECT_NE(expected_share2, it);
+  }
+  std::vector<SSLNamedGroup> expected_shares2 = {
+    expected_share2
+  };
+  std::vector<SSLNamedGroup> shares =
+        GetShareDetails(shares_capture2_->extension());
+  EXPECT_EQ(expected_shares2, shares);
+}
 }  // namespace nss_test
--- a/gtests/ssl_gtest/tls_connect.h
+++ b/gtests/ssl_gtest/tls_connect.h
@@ -242,25 +242,32 @@ class TlsConnectDatagram13 : public TlsC
 
 // A variant that is used only with Pre13.
 class TlsConnectGenericPre13 : public TlsConnectGeneric {};
 
 class TlsKeyExchangeTest : public TlsConnectGeneric {
  protected:
   TlsExtensionCapture* groups_capture_;
   TlsExtensionCapture* shares_capture_;
+  TlsExtensionCapture* shares_capture2_;
   TlsInspectorRecordHandshakeMessage* capture_hrr_;
 
   void EnsureKeyShareSetup();
   void ConfigNamedGroups(const std::vector<SSLNamedGroup>& groups);
   std::vector<SSLNamedGroup> GetGroupDetails(const DataBuffer& ext);
   std::vector<SSLNamedGroup> GetShareDetails(const DataBuffer& ext);
   void CheckKEXDetails(const std::vector<SSLNamedGroup>& expectedGroups,
+                       const std::vector<SSLNamedGroup>& expectedShares);
+  void CheckKEXDetails(const std::vector<SSLNamedGroup>& expectedGroups,
                        const std::vector<SSLNamedGroup>& expectedShares,
-                       bool expect_hrr = false);
+                       SSLNamedGroup expectedShare2);
+ private:
+  void CheckKEXDetails(const std::vector<SSLNamedGroup>& expectedGroups,
+                       const std::vector<SSLNamedGroup>& expectedShares,
+                       bool expect_hrr);
 };
 
 class TlsKeyExchangeTest13 : public TlsKeyExchangeTest {};
 class TlsKeyExchangeTestPre13 : public TlsKeyExchangeTest {};
 
 }  // namespace nss_test
 
 #endif
--- a/gtests/ssl_gtest/tls_filter.cc
+++ b/gtests/ssl_gtest/tls_filter.cc
@@ -426,17 +426,17 @@ PacketFilter::Action TlsExtensionFilter:
     output->Write(length_offset, newlen, 2);
     return CHANGE;
   }
   return KEEP;
 }
 
 PacketFilter::Action TlsExtensionCapture::FilterExtension(
     uint16_t extension_type, const DataBuffer& input, DataBuffer* output) {
-  if (extension_type == extension_ && data_.len() == 0) {
+  if (extension_type == extension_ && (last_ || (data_.len() == 0))) {
     data_.Assign(input);
   }
   return KEEP;
 }
 
 PacketFilter::Action TlsExtensionReplacer::FilterExtension(
     uint16_t extension_type, const DataBuffer& input, DataBuffer* output) {
   if (extension_type != extension_) {
--- a/gtests/ssl_gtest/tls_filter.h
+++ b/gtests/ssl_gtest/tls_filter.h
@@ -229,27 +229,29 @@ class TlsExtensionFilter : public TlsHan
  private:
   PacketFilter::Action FilterExtensions(TlsParser* parser,
                                         const DataBuffer& input,
                                         DataBuffer* output);
 };
 
 class TlsExtensionCapture : public TlsExtensionFilter {
  public:
-  TlsExtensionCapture(uint16_t ext) : extension_(ext), data_() {}
+  TlsExtensionCapture(uint16_t ext, bool last = false) :
+      extension_(ext), last_(last), data_() {}
 
   const DataBuffer& extension() const { return data_; }
 
  protected:
   PacketFilter::Action FilterExtension(uint16_t extension_type,
                                        const DataBuffer& input,
                                        DataBuffer* output) override;
 
  private:
   const uint16_t extension_;
+  bool last_;
   DataBuffer data_;
 };
 
 class TlsExtensionReplacer : public TlsExtensionFilter {
  public:
   TlsExtensionReplacer(uint16_t extension, const DataBuffer& data)
       : extension_(extension), data_(data) {}
   PacketFilter::Action FilterExtension(uint16_t extension_type,
--- a/lib/ssl/tls13exthandle.c
+++ b/lib/ssl/tls13exthandle.c
@@ -309,16 +309,20 @@ tls13_ClientHandleKeyShareXtnHrr(const s
      * requested group, abort. */
     if (!ssl_NamedGroupEnabled(ss, group) ||
         ssl_HaveEphemeralKeyPair(ss, group)) {
         ssl3_ExtSendAlert(ss, alert_fatal, illegal_parameter);
         PORT_SetError(SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST);
         return SECFailure;
     }
 
+    /* Now delete all the key shares per [draft-ietf-tls-tls13 S 4.1.2] */
+    ssl_FreeEphemeralKeyPairs(CONST_CAST(sslSocket, ss));
+
+    /* And replace with our new share. */
     rv = tls13_CreateKeyShare(CONST_CAST(sslSocket, ss), group);
     if (rv != SECSuccess) {
         ssl3_ExtSendAlert(ss, alert_fatal, internal_error);
         PORT_SetError(SEC_ERROR_KEYGEN_FAIL);
         return SECFailure;
     }
 
     return SECSuccess;
@@ -355,16 +359,27 @@ tls13_ServerHandleKeyShareXtn(const sslS
         goto loser;
     }
 
     while (data->len) {
         rv = tls13_HandleKeyShareEntry(ss, xtnData, data);
         if (rv != SECSuccess)
             goto loser;
     }
+
+    /* Check that the client only offered one share if this is
+     * after HRR. */
+    if (ss->ssl3.hs.helloRetry) {
+        if (PR_PREV_LINK(&xtnData->remoteKeyShares) !=
+            PR_NEXT_LINK(&xtnData->remoteKeyShares)) {
+            PORT_SetError(SSL_ERROR_RX_MALFORMED_CLIENT_HELLO);
+            goto loser;
+        }
+    }
+
     return SECSuccess;
 
 loser:
     tls13_DestroyKeyShares(&xtnData->remoteKeyShares);
     return SECFailure;
 }
 
 PRInt32