Bug 1489945 - Handle second ticket with external ticket caching, r=fr…

…anziskus

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

@@ -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);

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),
+    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,18 +1202,24 @@ 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 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);
 

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) {