From 74bce7bb3e4128a6e92964316d41ada278b12c1f Mon Sep 17 00:00:00 2001
From: Martin Thomson <martin.thomson@gmail.com>
Date: Fri, 12 Oct 2018 16:52:44 +1100
Subject: [PATCH] Bug 1489945 - Handle second ticket with external ticket
 caching, r=franziskus

Summary:
If we get a second session ticket in TLS 1.3 (as boringssl is wont to
do, and maybe others) while the external session cache is enabled, we assert.
The fix is to stop assuming that only in_client_cache sessions have a ticket
attached.  The bigger fix ensures that sessions are properly labelled so that we
correctly create a new session in the event that we get multiple tickets from a
server.

I *think* that this isn't that high a priority.  Michal is apparently working on
code related to this, but should still be able to make progress by disabling TLS
1.3 (or avoiding boringSSL servers).

Reviewers: franziskus, ekr

Reviewed By: franziskus

Bug #: 1489945

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

--HG--
extra : rebase_source : 5203e4275b86605cf71662c2abd4fe58ec8b560c
extra : amend_source : ad8290b441bee98fb5fe3615c0c96f4fe2e41d6c
---
 gtests/ssl_gtest/ssl_resumption_unittest.cc | 30 +++++++++++++++++++
 lib/ssl/sslnonce.c                          | 32 +++++++++++++--------
 lib/ssl/tls13con.c                          |  3 +-
 3 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc
index f092f8d126..6c52b1f57e 100644
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -946,6 +946,36 @@ TEST_F(TlsConnectDatagram13, SendSessionTicketDtls) {
   EXPECT_EQ(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION, PORT_GetError());
 }
 
+TEST_F(TlsConnectStreamTls13, ExternalResumptionUseSecondTicket) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+
+  struct ResumptionTicketState {
+    std::vector<uint8_t> ticket;
+    size_t invoked = 0;
+  } ticket_state;
+  auto cb = [](PRFileDesc* fd, const PRUint8* ticket, unsigned int ticket_len,
+               void* arg) -> SECStatus {
+    auto state = reinterpret_cast<ResumptionTicketState*>(arg);
+    state->ticket.assign(ticket, ticket + ticket_len);
+    state->invoked++;
+    return SECSuccess;
+  };
+  SSL_SetResumptionTokenCallback(client_->ssl_fd(), cb, &ticket_state);
+
+  Connect();
+  EXPECT_EQ(SECSuccess, SSL_SendSessionTicket(server_->ssl_fd(), nullptr, 0));
+  SendReceive();
+  EXPECT_EQ(2U, ticket_state.invoked);
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  client_->SetResumptionToken(ticket_state.ticket);
+  ExpectResumption(RESUME_TICKET);
+  Connect();
+  SendReceive();
+}
+
 TEST_F(TlsConnectTest, TestTls13ResumptionDowngrade) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
diff --git a/lib/ssl/sslnonce.c b/lib/ssl/sslnonce.c
index f79c23fc72..415b6ad933 100644
--- a/lib/ssl/sslnonce.c
+++ b/lib/ssl/sslnonce.c
@@ -1093,10 +1093,12 @@ ssl_CacheExternalToken(sslSocket *ss)
     PRINT_BUF(40, (ss, "SSL: encoded resumption token",
                    SSL_BUFFER_BASE(&encodedToken),
                    SSL_BUFFER_LEN(&encodedToken)));
-    ss->resumptionTokenCallback(ss->fd, SSL_BUFFER_BASE(&encodedToken),
-                                SSL_BUFFER_LEN(&encodedToken),
-                                ss->resumptionTokenContext);
-
+    SECStatus rv = ss->resumptionTokenCallback(
+        ss->fd, SSL_BUFFER_BASE(&encodedToken), SSL_BUFFER_LEN(&encodedToken),
+        ss->resumptionTokenContext);
+    if (rv == SECSuccess) {
+        sid->cached = in_external_cache;
+    }
     sslBuffer_Clear(&encodedToken);
 }
 
@@ -1200,17 +1202,23 @@ ssl3_SetSIDSessionTicket(sslSessionID *sid,
     PORT_Assert(newSessionTicket->ticket.data);
     PORT_Assert(newSessionTicket->ticket.len != 0);
 
-    /* if sid->u.ssl3.lock, we are updating an existing entry that is already
-     * cached or was once cached, so we need to acquire and release the write
-     * lock. Otherwise, this is a new session that isn't shared with anything
-     * yet, so no locking is needed.
+    /* If this is in the client cache, we are updating an existing entry that is
+     * already cached or was once cached, so we need to acquire and release the
+     * write lock. Otherwise, this is a new session that isn't shared with
+     * anything yet, so no locking is needed.
      */
     if (sid->u.ssl3.lock) {
+        PORT_Assert(sid->cached == in_client_cache);
         PR_RWLock_Wlock(sid->u.ssl3.lock);
-        if (sid->u.ssl3.locked.sessionTicket.ticket.data) {
-            SECITEM_FreeItem(&sid->u.ssl3.locked.sessionTicket.ticket,
-                             PR_FALSE);
-        }
+    }
+    /* If this was in the client cache, then we might have to free the old
+     * ticket.  In TLS 1.3, we might get a replacement ticket if the server
+     * sends more than one ticket. */
+    if (sid->u.ssl3.locked.sessionTicket.ticket.data) {
+        PORT_Assert(sid->cached == in_client_cache ||
+                    sid->version >= SSL_LIBRARY_VERSION_TLS_1_3);
+        SECITEM_FreeItem(&sid->u.ssl3.locked.sessionTicket.ticket,
+                         PR_FALSE);
     }
 
     PORT_Assert(!sid->u.ssl3.locked.sessionTicket.ticket.data);
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index e1698a24da..c0cfa5e5a2 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -4727,7 +4727,8 @@ tls13_HandleNewSessionTicket(sslSocket *ss, PRUint8 *b, PRUint32 length)
 
         /* Replace a previous session ticket when
          * we receive a second NewSessionTicket message. */
-        if (ss->sec.ci.sid->cached == in_client_cache) {
+        if (ss->sec.ci.sid->cached == in_client_cache ||
+            ss->sec.ci.sid->cached == in_external_cache) {
             /* Create a new session ID. */
             sslSessionID *sid = ssl3_NewSessionID(ss, PR_FALSE);
             if (!sid) {