Bug 1481271, resend the same ticket in ClientHello after HRR, r=mt

Summary:
This is an another attempt to fix the issue: store the sent session ticket in `ssl3.hs` until the client receives ServerHello.
Test is not ready as I couldn't find any easy way to establish multiple connections in gtests to reproduce the scenario described in comment 7.

Reviewers: mt

Reviewed By: mt

Subscribers: franziskus, jcj, mt, ekr, ueno, rrelyea, Alex_Gaynor, mccr8, HubertKario

Tags: #secure-revision, PHID-PROJ-ffhf7tdvqze7zrdn6dh3

Bug #: 1481271

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

3 files changed, 114 insertions, 0 deletions

diff --git a/gtests/ssl_gtest/ssl_hrr_unittest.cc b/gtests/ssl_gtest/ssl_hrr_unittest.cc
index 9a019480c..6cb74f655 100644
--- a/gtests/ssl_gtest/ssl_hrr_unittest.cc
+++ b/gtests/ssl_gtest/ssl_hrr_unittest.cc
@@ -718,6 +718,86 @@ TEST_F(TlsConnectStreamTls13, RetryStatelessDamageSecondClientHello) {
   client_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
 }
 
+// Stream because SSL_SendSessionTicket only supports that.
+TEST_F(TlsConnectStreamTls13, SecondClientHelloSendSameTicket) {
+  // This simulates the scenario described at:
+  // https://bugzilla.mozilla.org/show_bug.cgi?id=1481271#c7
+  //
+  // Here two connections are interleaved.  Tickets are issued on one
+  // connection.  A HelloRetryRequest is triggered on the second connection,
+  // meaning that there are two ClientHellos.  We need to check that both
+  // ClientHellos have the same ticket, even if a new ticket is issued on the
+  // other connection in the meantime.
+  //
+  // Connection 1: <handshake>
+  // Connection 1: S->C: NST=X
+  // Connection 2: C->S: CH [PSK_ID=X]
+  // Connection 1: S->C: NST=Y
+  // Connection 2: S->C: HRR
+  // Connection 2: C->S: CH [PSK_ID=Y]
+
+  // Connection 1, send a ticket after handshake is complete.
+  ConfigureSessionCache(RESUME_TICKET, RESUME_TICKET);
+
+  Connect();
+
+  // Set this token so that RetryHelloWithToken() will check that this
+  // is the token that it receives in the HelloRetryRequest callback.
+  EXPECT_EQ(SECSuccess,
+            SSL_SendSessionTicket(server_->ssl_fd(), kApplicationToken,
+                                  sizeof(kApplicationToken)));
+  SendReceive(50);
+
+  // Connection 2, trigger HRR.
+  auto client2 =
+      std::make_shared<TlsAgent>(client_->name(), TlsAgent::CLIENT, variant_);
+  auto server2 =
+      std::make_shared<TlsAgent>(server_->name(), TlsAgent::SERVER, variant_);
+
+  client2->SetPeer(server2);
+  server2->SetPeer(client2);
+
+  client_.swap(client2);
+  server_.swap(server2);
+
+  ConfigureSessionCache(RESUME_TICKET, RESUME_TICKET);
+
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+
+  client_->StartConnect();
+  server_->StartConnect();
+
+  size_t cb_called = 0;
+  EXPECT_EQ(SECSuccess,
+            SSL_HelloRetryRequestCallback(server_->ssl_fd(),
+                                          RetryHelloWithToken, &cb_called));
+  client_->Handshake();  // Send ClientHello.
+  server_->Handshake();  // Process ClientHello, send HelloRetryRequest.
+
+  EXPECT_EQ(1U, cb_called) << "callback should be called once here";
+
+  // Connection 1, send another ticket.
+  client_.swap(client2);
+  server_.swap(server2);
+
+  // If the client uses this token, RetryHelloWithToken() will fail the test.
+  const uint8_t kAnotherApplicationToken[] = {0x92, 0x44, 0x01};
+  EXPECT_EQ(SECSuccess,
+            SSL_SendSessionTicket(server_->ssl_fd(), kAnotherApplicationToken,
+                                  sizeof(kAnotherApplicationToken)));
+  SendReceive(60);
+
+  // Connection 2, continue the handshake.
+  // The client should use kApplicationToken, not kAnotherApplicationToken.
+  client_.swap(client2);
+  server_.swap(server2);
+
+  client_->Handshake();
+  server_->Handshake();
+
+  EXPECT_EQ(2U, cb_called) << "callback should be called twice here";
+}
+
 // Read the cipher suite from the HRR and disable it on the identified agent.
 static void DisableSuiteFromHrr(
     std::shared_ptr<TlsAgent>& agent,
diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc
index 250ce8653..f9588bcf1 100644
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -1272,4 +1272,34 @@ TEST_P(TlsConnectGenericResumption, ConnectResumeClientAuth) {
   SendReceive();
 }
 
+TEST_F(TlsConnectStreamTls13, ExternalTokenAfterHrr) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  Connect();
+  SendReceive();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  ExpectResumption(RESUME_TICKET);
+
+  static const std::vector<SSLNamedGroup> groups = {ssl_grp_ec_secp384r1,
+                                                    ssl_grp_ec_secp521r1};
+  server_->ConfigNamedGroups(groups);
+
+  StartConnect();
+  ASSERT_TRUE(client_->MaybeSetResumptionToken());
+
+  client_->Handshake();  // Send ClientHello.
+  server_->Handshake();  // Process ClientHello, send HelloRetryRequest.
+
+  auto& token = client_->GetResumptionToken();
+  SECStatus rv =
+      SSL_SetResumptionToken(client_->ssl_fd(), token.data(), token.size());
+  ASSERT_EQ(SECFailure, rv);
+  ASSERT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError());
+
+  Handshake();
+  CheckConnected();
+  SendReceive();
+}
+
 }  // namespace nss_test
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index dc754b7ab..d7e845231 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -4774,6 +4774,10 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
         sid = ssl_ReferenceSID(ss->sec.ci.sid);
         SSL_TRC(3, ("%d: SSL3[%d]: using external resumption token in ClientHello",
                     SSL_GETPID(), ss->fd));
+    } else if (ss->sec.ci.sid && ss->statelessResume && type == client_hello_retry) {
+        /* If we are sending a second ClientHello, reuse the same SID
+         * as the original one. */
+        sid = ssl_ReferenceSID(ss->sec.ci.sid);
     } else if (!ss->opt.noCache) {
         /* We ignore ss->sec.ci.sid here, and use ssl_Lookup because Lookup
          * handles expired entries and other details.