Bug 1493769 - Set session_id for external resumption tokens, r=franziskus

This also includes some cleanup that I performed when looking into this.

It turns out that the hacks that we were using for managing the reference count
on sids was unnecessary.  Daiki added a much neater solution in D7493 that I
stole.

The error handling in SSLExp_SetResumptionToken looks nicer after a
spring-clean too.

7 files changed, 73 insertions, 28 deletions

diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc
index 6c52b1f57..30d74acf7 100644
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -1126,6 +1126,34 @@ void CheckGetInfoResult(uint32_t alpnSize, uint32_t earlyDataSize,
   ASSERT_EQ(earlyDataSize, token->maxEarlyDataSize);
 }
 
+// The client should generate a new, randomized session_id
+// when resuming using an external token.
+TEST_P(TlsConnectGenericResumptionToken, CheckSessionId) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  auto original_sid = MakeTlsFilter<CaptureSessionId>(client_);
+  Connect();
+  SendReceive();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  ExpectResumption(RESUME_TICKET);
+
+  StartConnect();
+  ASSERT_TRUE(client_->MaybeSetResumptionToken());
+  auto resumed_sid = MakeTlsFilter<CaptureSessionId>(client_);
+
+  Handshake();
+  CheckConnected();
+  SendReceive();
+
+  if (version_ < SSL_LIBRARY_VERSION_TLS_1_3) {
+    EXPECT_NE(resumed_sid->sid(), original_sid->sid());
+    EXPECT_EQ(32U, resumed_sid->sid().len());
+  } else {
+    EXPECT_EQ(0U, resumed_sid->sid().len());
+  }
+}
+
 TEST_P(TlsConnectGenericResumptionToken, ConnectResumeGetInfo) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
   Connect();
diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc
index 488842a88..792d6dd89 100644
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -229,6 +229,7 @@ bool TlsAgent::EnsureTlsSetup(PRFileDesc* modelSocket) {
 
 bool TlsAgent::MaybeSetResumptionToken() {
   if (!resumption_token_.empty()) {
+    LOG("setting external resumption token");
     SECStatus rv = SSL_SetResumptionToken(ssl_fd(), resumption_token_.data(),
                                           resumption_token_.size());
 
diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc
index e65fea782..c48ae38ec 100644
--- a/gtests/ssl_gtest/tls_connect.cc
+++ b/gtests/ssl_gtest/tls_connect.cc
@@ -559,13 +559,15 @@ void TlsConnectTestBase::CheckResumption(SessionResumptionMode expected) {
   EXPECT_EQ(stateless_count, stats->hsh_sid_stateless_resumes);
 
   if (expected != RESUME_NONE) {
-    if (client_->version() < SSL_LIBRARY_VERSION_TLS_1_3) {
+    if (client_->version() < SSL_LIBRARY_VERSION_TLS_1_3 &&
+        client_->GetResumptionToken().size() == 0) {
       // Check that the last two session ids match.
       ASSERT_EQ(1U + expected_resumptions_, session_ids_.size());
       EXPECT_EQ(session_ids_[session_ids_.size() - 1],
                 session_ids_[session_ids_.size() - 2]);
     } else {
-      // TLS 1.3 only uses tickets.
+      // We've either chosen TLS 1.3 or are using an external resumption token,
+      // both of which only use tickets.
       EXPECT_TRUE(expected & RESUME_TICKET);
     }
   }
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index a44c846af..6c8c88b36 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -4771,7 +4771,7 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
      * If we have an sid and it comes from an external cache, we use it. */
     if (ss->sec.ci.sid && ss->sec.ci.sid->cached == in_external_cache) {
         PORT_Assert(!ss->sec.isServer);
-        sid = ss->sec.ci.sid;
+        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->opt.noCache) {
@@ -4919,9 +4919,7 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
     }
     ssl_ReleaseSpecWriteLock(ss);
 
-    if (ss->sec.ci.sid != NULL) {
-        ssl_FreeSID(ss->sec.ci.sid); /* decrement ref count, free if zero */
-    }
+    ssl_FreeSID(ss->sec.ci.sid); /* release the old sid */
     ss->sec.ci.sid = sid;
 
     /* HACK for SCSV in SSL 3.0.  On initial handshake, prepend SCSV,
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 59d2c3cb1..35240d2fb 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1184,6 +1184,7 @@ extern sslSessionID *ssl_LookupSID(const PRIPv6Addr *addr, PRUint16 port,
                                    const char *peerID, const char *urlSvrName);
 extern void ssl_FreeSID(sslSessionID *sid);
 extern void ssl_DestroySID(sslSessionID *sid, PRBool freeIt);
+extern sslSessionID *ssl_ReferenceSID(sslSessionID *sid);
 
 extern int ssl3_SendApplicationData(sslSocket *ss, const PRUint8 *in,
                                     int len, int flags);
@@ -1726,7 +1727,7 @@ void ssl_Trace(const char *format, ...);
 void ssl_CacheExternalToken(sslSocket *ss);
 SECStatus ssl_DecodeResumptionToken(sslSessionID *sid, const PRUint8 *encodedTicket,
                                     PRUint32 encodedTicketLen);
-PRBool ssl_IsResumptionTokenValid(sslSocket *ss);
+PRBool ssl_IsResumptionTokenUsable(sslSocket *ss, sslSessionID *sid);
 
 /* Remove when stable. */
 
diff --git a/lib/ssl/sslnonce.c b/lib/ssl/sslnonce.c
index 415b6ad93..f8fb5d50f 100644
--- a/lib/ssl/sslnonce.c
+++ b/lib/ssl/sslnonce.c
@@ -234,9 +234,20 @@ ssl_FreeLockedSID(sslSessionID *sid)
 void
 ssl_FreeSID(sslSessionID *sid)
 {
+    if (sid) {
+        LOCK_CACHE;
+        ssl_FreeLockedSID(sid);
+        UNLOCK_CACHE;
+    }
+}
+
+sslSessionID *
+ssl_ReferenceSID(sslSessionID *sid)
+{
     LOCK_CACHE;
-    ssl_FreeLockedSID(sid);
+    sid->references++;
     UNLOCK_CACHE;
+    return sid;
 }
 
 /************************************************************************/
@@ -704,10 +715,9 @@ ssl_DecodeResumptionToken(sslSessionID *sid, const PRUint8 *encodedToken,
 }
 
 PRBool
-ssl_IsResumptionTokenValid(sslSocket *ss)
+ssl_IsResumptionTokenUsable(sslSocket *ss, sslSessionID *sid)
 {
     PORT_Assert(ss);
-    sslSessionID *sid = ss->sec.ci.sid;
     PORT_Assert(sid);
 
     // Check that the ticket didn't expire.
diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c
index 9caae0ec2..e51da197f 100644
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -18,6 +18,7 @@
 #include "private/pprio.h"
 #include "nss.h"
 #include "pk11pqg.h"
+#include "pk11pub.h"
 #include "tls13esni.h"
 
 static const sslSocketOps ssl_default_ops = { /* No SSL. */
@@ -4102,6 +4103,7 @@ SSLExp_SetResumptionToken(PRFileDesc *fd, const PRUint8 *token,
                           unsigned int len)
 {
     sslSocket *ss = ssl_FindSocket(fd);
+    sslSessionID *sid = NULL;
 
     if (!ss) {
         SSL_DBG(("%d: SSL[%d]: bad socket in SSL_SetResumptionToken",
@@ -4115,7 +4117,7 @@ SSLExp_SetResumptionToken(PRFileDesc *fd, const PRUint8 *token,
     if (ss->firstHsDone || ss->ssl3.hs.ws != idle_handshake ||
         ss->sec.isServer || len == 0 || !token) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
-        goto done;
+        goto loser;
     }
 
     // We override any previously set session.
@@ -4126,41 +4128,44 @@ SSLExp_SetResumptionToken(PRFileDesc *fd, const PRUint8 *token,
 
     PRINT_BUF(50, (ss, "incoming resumption token", token, len));
 
-    ss->sec.ci.sid = ssl3_NewSessionID(ss, PR_FALSE);
-    if (!ss->sec.ci.sid) {
-        goto done;
+    sid = ssl3_NewSessionID(ss, PR_FALSE);
+    if (!sid) {
+        goto loser;
     }
 
     /* Populate NewSessionTicket values */
-    SECStatus rv = ssl_DecodeResumptionToken(ss->sec.ci.sid, token, len);
+    SECStatus rv = ssl_DecodeResumptionToken(sid, token, len);
     if (rv != SECSuccess) {
         // If decoding fails, we assume the token is bad.
         PORT_SetError(SSL_ERROR_BAD_RESUMPTION_TOKEN_ERROR);
-        ssl_FreeSID(ss->sec.ci.sid);
-        ss->sec.ci.sid = NULL;
-        goto done;
+        goto loser;
     }
 
-    // Make sure that the token is valid.
-    if (!ssl_IsResumptionTokenValid(ss)) {
-        ssl_FreeSID(ss->sec.ci.sid);
-        ss->sec.ci.sid = NULL;
+    // Make sure that the token is currently usable.
+    if (!ssl_IsResumptionTokenUsable(ss, sid)) {
         PORT_SetError(SSL_ERROR_BAD_RESUMPTION_TOKEN_ERROR);
-        goto done;
+        goto loser;
     }
 
+    // Generate a new random session ID for this ticket.
+    rv = PK11_GenerateRandom(sid->u.ssl3.sessionID, SSL3_SESSIONID_BYTES);
+    if (rv != SECSuccess) {
+        goto loser; // Code set by PK11_GenerateRandom.
+    }
+    sid->u.ssl3.sessionIDLength = SSL3_SESSIONID_BYTES;
     /* Use the sid->cached as marker that this is from an external cache and
      * we don't have to look up anything in the NSS internal cache. */
-    ss->sec.ci.sid->cached = in_external_cache;
-    // This has to be 2 to not free this in sendClientHello.
-    ss->sec.ci.sid->references = 2;
-    ss->sec.ci.sid->lastAccessTime = ssl_TimeSec();
+    sid->cached = in_external_cache;
+    sid->lastAccessTime = ssl_TimeSec();
+
+    ss->sec.ci.sid = sid;
 
     ssl_ReleaseSSL3HandshakeLock(ss);
     ssl_Release1stHandshakeLock(ss);
     return SECSuccess;
 
-done:
+loser:
+    ssl_FreeSID(sid);
     ssl_ReleaseSSL3HandshakeLock(ss);
     ssl_Release1stHandshakeLock(ss);