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

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 f092f8d12..6c52b1f57 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 f79c23fc7..415b6ad93 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 e1698a24d..c0cfa5e5a 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) {