Bug 1315735 - TLS 1.3 draft 17 - Replace key shares in response to HR…

…R. r=mt Subscribers: mt Differential Revision: https://nss-dev.phacility.com/D129
sailfishos-mirror · Nov 7, 2016 · e1b4c05 · e1b4c05
1 parent a77fdfb
commit e1b4c05
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 13 deletions.
diff --git a/gtests/ssl_gtest/ssl_ecdh_unittest.cc b/gtests/ssl_gtest/ssl_ecdh_unittest.cc
@@ -287,7 +287,7 @@ TEST_P(TlsKeyExchangeTest13, Curve25519P256EqualPriorityClient13) {
   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) {
@@ -307,7 +307,7 @@ TEST_P(TlsKeyExchangeTest13, Curve25519P256EqualPriorityServer13) {
   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) {
@@ -329,7 +329,7 @@ TEST_P(TlsKeyExchangeTest13, EqualPriorityTestRetryECServer13) {
   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) {
@@ -351,7 +351,7 @@ TEST_P(TlsKeyExchangeTest13, NotEqualPriorityWithIntermediateGroup13) {
   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,
@@ -373,7 +373,7 @@ TEST_P(TlsKeyExchangeTest13,
   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,
@@ -395,7 +395,7 @@ TEST_P(TlsKeyExchangeTest13,
   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) {
@@ -415,7 +415,7 @@ TEST_P(TlsKeyExchangeTest13, EqualPriority13) {
 
   CheckKeys();
   const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
-  CheckKEXDetails(client_groups, shares, false);
+  CheckKEXDetails(client_groups, shares);
 }
 #endif
 
@@ -458,7 +458,7 @@ TEST_P(TlsKeyExchangeTest13, MultipleClientShares) {
             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

diff --git a/gtests/ssl_gtest/ssl_hrr_unittest.cc b/gtests/ssl_gtest/ssl_hrr_unittest.cc
@@ -129,7 +129,7 @@ TEST_P(TlsKeyExchange13, ConnectEcdhePreferenceMismatchHrr) {
   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
@@ -146,7 +146,7 @@ TEST_P(TlsKeyExchange13, ConnectEcdhePreferenceMismatchHrrExtraShares) {
 
   Connect();
   CheckKeys();
-  CheckKEXDetails(client_groups, client_groups, false /* expect_hrr */);
+  CheckKEXDetails(client_groups, client_groups);
 }
 
 TEST_F(TlsConnectTest, Select12AfterHelloRetryRequest) {

diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc
@@ -618,9 +618,11 @@ 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);
@@ -683,4 +685,26 @@ void TlsKeyExchangeTest::CheckKEXDetails(
   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
diff --git a/gtests/ssl_gtest/tls_connect.h b/gtests/ssl_gtest/tls_connect.h
@@ -247,15 +247,22 @@ 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,
+                       SSLNamedGroup expectedShare2);
+ private:
   void CheckKEXDetails(const std::vector<SSLNamedGroup>& expectedGroups,
                        const std::vector<SSLNamedGroup>& expectedShares,
-                       bool expect_hrr = false);
+                       bool expect_hrr);
 };
 
 class TlsKeyExchangeTest13 : public TlsKeyExchangeTest {};

diff --git a/gtests/ssl_gtest/tls_filter.cc b/gtests/ssl_gtest/tls_filter.cc
@@ -431,7 +431,7 @@ PacketFilter::Action TlsExtensionFilter::FilterExtensions(
 
 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;

diff --git a/gtests/ssl_gtest/tls_filter.h b/gtests/ssl_gtest/tls_filter.h
@@ -234,7 +234,8 @@ class TlsExtensionFilter : public TlsHandshakeFilter {
 
 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_; }
 
@@ -245,6 +246,7 @@ class TlsExtensionCapture : public TlsExtensionFilter {
 
  private:
   const uint16_t extension_;
+  bool last_;
   DataBuffer data_;
 };
 

diff --git a/lib/ssl/tls13exthandle.c b/lib/ssl/tls13exthandle.c
@@ -314,6 +314,10 @@ tls13_ClientHandleKeyShareXtnHrr(const sslSocket *ss, TLSExtensionData *xtnData,
         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);
@@ -360,6 +364,17 @@ tls13_ServerHandleKeyShareXtn(const sslSocket *ss, TLSExtensionData *xtnData, PR
         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: