Bug 1432144 - clean-up sid handling, r=mt

Summary:
SIDs usage is pretty messy. In this patch I move all *sid to point to ss->sec.ci.sid (unless the SID is purely local to the function).
This allows us to free sids when uncaching them.

Reviewers: mt

Reviewed By: mt

Bug #: 1432144

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

16 files changed, 211 insertions, 209 deletions

diff --git a/gtests/ssl_gtest/ssl_0rtt_unittest.cc b/gtests/ssl_gtest/ssl_0rtt_unittest.cc
index 8847312b8..7d6120dc1 100644
--- a/gtests/ssl_gtest/ssl_0rtt_unittest.cc
+++ b/gtests/ssl_gtest/ssl_0rtt_unittest.cc
@@ -519,7 +519,7 @@ TEST_P(TlsConnectTls13, SendTooMuchEarlyData) {
 TEST_P(TlsConnectTls13, ReceiveTooMuchEarlyData) {
   EnsureTlsSetup();
 
-  const size_t limit = 5;
+  size_t limit = 5;
   EXPECT_EQ(SECSuccess, SSL_SetMaxEarlyDataSize(server_->ssl_fd(), limit));
   SetupForZeroRtt();
 
@@ -549,6 +549,9 @@ TEST_P(TlsConnectTls13, ReceiveTooMuchEarlyData) {
   server_->Handshake();  // This reads the early data and maybe throws an error.
   if (variant_ == ssl_variant_stream) {
     server_->CheckErrorCode(SSL_ERROR_TOO_MUCH_EARLY_DATA);
+    // We drop the SID when sending the alert such that max_early_data_size is 0
+    // here.
+    limit = 0;
   } else {
     EXPECT_EQ(TlsAgent::STATE_CONNECTING, server_->state());
   }
diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc
index 127e53955..f1a789367 100644
--- a/gtests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc
@@ -530,6 +530,11 @@ INSTANTIATE_TEST_CASE_P(
                        TlsConnectTestBase::kTlsV11V12));
 INSTANTIATE_TEST_CASE_P(Pre13StreamOnly, TlsConnectStreamPre13,
                         TlsConnectTestBase::kTlsV10ToV12);
+INSTANTIATE_TEST_CASE_P(
+    Pre13Stream, TlsConnectStreamResumptionPre13,
+    ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,
+                       TlsConnectTestBase::kTlsV10ToV12,
+                       ::testing::Values(true, false)));
 
 INSTANTIATE_TEST_CASE_P(Version12Plus, TlsConnectTls12Plus,
                         ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc
index d93a190e0..4f3a98ad4 100644
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -1035,4 +1035,27 @@ TEST_P(TlsConnectGenericResumption, ConnectResumeClientAuth) {
   SendReceive();
 }
 
+// Renegotiate a resumed session.
+TEST_P(TlsConnectStreamResumptionPre13, ConnectResumeRenegotiateClient) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  Connect();
+  SendReceive();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  ExpectResumption(RESUME_TICKET);
+  Connect();
+
+  // Disable resumption and prepare for renegotiation.
+  server_->ExpectResumption(false);
+  server_->PrepareForRenegotiate();
+  client_->ExpectResumption(false);
+  client_->StartRenegotiate();
+  Handshake();
+  // Don't CheckConnected its logic doesn't work in this case.
+  // It assumes a certain number of SIDs, resumed sessions, and cache
+  // hits/misses.
+  SendReceive();
+}
+
 }  // namespace nss_test
diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc
index 172fc9195..66453498e 100644
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -588,7 +588,9 @@ void TlsAgent::EnableFalseStart() {
   SetOption(SSL_ENABLE_FALSE_START, PR_TRUE);
 }
 
-void TlsAgent::ExpectResumption() { expect_resumption_ = true; }
+void TlsAgent::ExpectResumption(bool expected) {
+  expect_resumption_ = expected;
+}
 
 void TlsAgent::EnableAlpn(const uint8_t* val, size_t len) {
   EXPECT_TRUE(EnsureTlsSetup());
diff --git a/gtests/ssl_gtest/tls_agent.h b/gtests/ssl_gtest/tls_agent.h
index 731296a20..9bde5dfda 100644
--- a/gtests/ssl_gtest/tls_agent.h
+++ b/gtests/ssl_gtest/tls_agent.h
@@ -135,7 +135,7 @@ class TlsAgent : public PollTarget {
   void SetServerKeyBits(uint16_t bits);
   void ExpectReadWriteError();
   void EnableFalseStart();
-  void ExpectResumption();
+  void ExpectResumption(bool expected = true);
   void SkipVersionChecks();
   void SetSignatureSchemes(const SSLSignatureScheme* schemes, size_t count);
   void EnableAlpn(const uint8_t* val, size_t len);
diff --git a/gtests/ssl_gtest/tls_connect.h b/gtests/ssl_gtest/tls_connect.h
index b13f15842..9746f9865 100644
--- a/gtests/ssl_gtest/tls_connect.h
+++ b/gtests/ssl_gtest/tls_connect.h
@@ -313,6 +313,7 @@ class TlsConnectDatagramPre13 : public TlsConnectDatagram {
 
 // A variant that is used only with Pre13.
 class TlsConnectGenericPre13 : public TlsConnectGeneric {};
+class TlsConnectStreamResumptionPre13 : public TlsConnectGenericResumption {};
 
 class TlsKeyExchangeTest : public TlsConnectGeneric {
  protected:
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 89fd06dfc..b7715378a 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -50,7 +50,6 @@ static SECStatus ssl3_SendServerHelloDone(sslSocket *ss);
 static SECStatus ssl3_SendServerKeyExchange(sslSocket *ss);
 static SECStatus ssl3_HandleClientHelloPart2(sslSocket *ss,
                                              SECItem *suites,
-                                             sslSessionID *sid,
                                              const PRUint8 *msg,
                                              unsigned int len);
 static SECStatus ssl3_HandleServerHelloPart2(sslSocket *ss,
@@ -2714,9 +2713,7 @@ SSL3_SendAlert(sslSocket *ss, SSL3AlertLevel level, SSL3AlertDescription desc)
         ssl_GetSSL3HandshakeLock(ss);
     }
     if (level == alert_fatal) {
-        if (ss->sec.ci.sid) {
-            ssl_UncacheSessionID(ss);
-        }
+        ssl_UncacheSessionID(ss);
     }
 
     rv = tls13_SetAlertCipherSpec(ss);
@@ -4528,7 +4525,6 @@ ssl_SetClientHelloSpecVersion(sslSocket *ss, ssl3CipherSpec *spec)
 SECStatus
 ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
 {
-    sslSessionID *sid;
     SECStatus rv;
     unsigned int i;
     unsigned int length;
@@ -4605,19 +4601,26 @@ 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;
         SSL_TRC(3, ("%d: SSL3[%d]: using external resumption token in ClientHello",
                     SSL_GETPID(), ss->fd));
-    } else if (!ss->opt.noCache) {
-        /* We ignore ss->sec.ci.sid here, and use ssl_Lookup because Lookup
-         * handles expired entries and other details.
-         * XXX If we've been called from ssl_BeginClientHandshake, then
-         * this lookup is duplicative and wasteful.
-         */
-        sid = ssl_LookupSID(&ss->sec.ci.peer, ss->sec.ci.port, ss->peerID, ss->url);
     } else {
-        sid = NULL;
+        /* We allocated some sid but don't want to use it. */
+        if (ss->sec.ci.sid) {
+            ssl_FreeSID(ss->sec.ci.sid);
+            ss->sec.ci.sid = NULL;
+        }
+        if (!ss->opt.noCache) {
+            /* We ignore ss->sec.ci.sid here, and use ssl_Lookup because Lookup
+             * handles expired entries and other details.
+             * XXX If we've been called from ssl_BeginClientHandshake, then
+             * this lookup is duplicative and wasteful.
+             */
+            ss->sec.ci.sid = ssl_LookupSID(&ss->sec.ci.peer,
+                                           ss->sec.ci.port,
+                                           ss->peerID, ss->url);
+        }
     }
+    sslSessionID *sid = ss->sec.ci.sid;
 
     /* We can't resume based on a different token. If the sid exists,
      * make sure the token that holds the master secret still exists ...
@@ -4709,7 +4712,6 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
         if (!sidOK) {
             SSL_AtomicIncrementLong(&ssl3stats.sch_sid_cache_not_ok);
             ssl_UncacheSessionID(ss);
-            ssl_FreeSID(sid);
             sid = NULL;
         }
     }
@@ -4737,10 +4739,11 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type)
             version = ss->clientHelloVersion;
         }
 
-        sid = ssl3_NewSessionID(ss, PR_FALSE);
-        if (!sid) {
+        rv = ssl3_NewSessionID(ss, PR_FALSE);
+        if (rv != SECSuccess) {
             return SECFailure; /* memory error is set */
         }
+        sid = ss->sec.ci.sid;
         /* ss->version isn't set yet, but the sid needs a sane value. */
         sid->version = version;
     }
@@ -4753,11 +4756,6 @@ 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 */
-    }
-    ss->sec.ci.sid = sid;
-
     /* HACK for SCSV in SSL 3.0.  On initial handshake, prepend SCSV,
      * only if TLS is disabled.
      */
@@ -5015,7 +5013,6 @@ loser:
 static SECStatus
 ssl3_HandleHelloRequest(sslSocket *ss)
 {
-    sslSessionID *sid = ss->sec.ci.sid;
     SECStatus rv;
 
     SSL_TRC(3, ("%d: SSL3[%d]: handle hello_request handshake",
@@ -5038,12 +5035,7 @@ ssl3_HandleHelloRequest(sslSocket *ss)
         return SECFailure;
     }
 
-    if (sid) {
-        ssl_UncacheSessionID(ss);
-        ssl_FreeSID(sid);
-        ss->sec.ci.sid = NULL;
-    }
-
+    ssl_UncacheSessionID(ss);
     if (IS_DTLS(ss)) {
         dtls_RehandshakeCleanup(ss);
     }
@@ -6615,13 +6607,13 @@ ssl3_HandleServerHelloPart2(sslSocket *ss, const SECItem *sidBytes,
     /* throw the old one away */
     sid->u.ssl3.keys.resumable = PR_FALSE;
     ssl_UncacheSessionID(ss);
-    ssl_FreeSID(sid);
 
     /* get a new sid */
-    ss->sec.ci.sid = sid = ssl3_NewSessionID(ss, PR_FALSE);
-    if (sid == NULL) {
+    rv = ssl3_NewSessionID(ss, PR_FALSE);
+    if (rv != SECSuccess) {
         goto alert_loser; /* memory error is set. */
     }
+    sid = ss->sec.ci.sid;
 
     sid->version = ss->version;
     sid->u.ssl3.sessionIDLength = sidBytes->len;
@@ -7489,14 +7481,15 @@ ssl3_ServerNameCompare(const SECItem *name1, const SECItem *name2)
  *  ssl3_HandleClientHello()
  *  ssl3_HandleV2ClientHello()
  */
-sslSessionID *
+SECStatus
 ssl3_NewSessionID(sslSocket *ss, PRBool is_server)
 {
     sslSessionID *sid;
 
     sid = PORT_ZNew(sslSessionID);
-    if (sid == NULL)
-        return sid;
+    if (sid == NULL) {
+        return SECFailure;
+    }
 
     if (is_server) {
         const SECItem *srvName;
@@ -7510,7 +7503,7 @@ ssl3_NewSessionID(sslSocket *ss, PRBool is_server)
         ssl_ReleaseSpecReadLock(ss); /************************************/
         if (rv != SECSuccess) {
             PORT_Free(sid);
-            return NULL;
+            return SECFailure;
         }
     }
     sid->peerID = (ss->peerID == NULL) ? NULL : PORT_Strdup(ss->peerID);
@@ -7538,10 +7531,16 @@ ssl3_NewSessionID(sslSocket *ss, PRBool is_server)
         if (rv != SECSuccess) {
             ssl_FreeSID(sid);
             ssl_MapLowLevelError(SSL_ERROR_GENERATE_RANDOM_FAILURE);
-            return NULL;
+            return SECFailure;
         }
     }
-    return sid;
+
+    /* Destroy any sid we might have had. */
+    ssl_UncacheSessionID(ss);
+    PORT_Assert(!ss->sec.ci.sid);
+
+    ss->sec.ci.sid = sid;
+    return SECSuccess;
 }
 
 /* Called from:  ssl3_HandleClientHello, ssl3_HandleV2ClientHello */
@@ -7865,7 +7864,6 @@ ssl3_SelectServerCert(sslSocket *ss)
 static SECStatus
 ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
 {
-    sslSessionID *sid = NULL;
     PRUint32 tmp;
     unsigned int i;
     SECStatus rv;
@@ -8196,6 +8194,7 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
      * (2) the client support the session ticket extension, but sent an
      * empty ticket.
      */
+    sslSessionID *sid = ss->sec.ci.sid;
     if ((ss->version < SSL_LIBRARY_VERSION_TLS_1_3) &&
         (!ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn) ||
          ss->xtnData.emptySessionTicket)) {
@@ -8206,20 +8205,31 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
                         ss->sec.ci.peer.pr_s6_addr32[2],
                         ss->sec.ci.peer.pr_s6_addr32[3]));
             if (ssl_sid_lookup) {
-                sid = (*ssl_sid_lookup)(&ss->sec.ci.peer, sidBytes.data,
-                                        sidBytes.len, ss->dbHandle);
+                ssl_UncacheSessionID(ss);
+                ss->sec.ci.sid = sid = (*ssl_sid_lookup)(&ss->sec.ci.peer,
+                                                         sidBytes.data,
+                                                         sidBytes.len,
+                                                         ss->dbHandle);
             } else {
                 errCode = SSL_ERROR_SERVER_CACHE_NOT_CONFIGURED;
                 goto loser;
             }
+        } else {
+            /* Free a potentially leftover session ID from a previous handshake. */
+            ssl_UncacheSessionID(ss);
+            sid = NULL;
         }
     } else if (ss->statelessResume) {
         /* Fill in the client's session ID if doing a stateless resume.
          * (When doing stateless resumes, server echos client's SessionID.)
          * This branch also handles TLS 1.3 resumption-PSK.
          */
-        sid = ss->sec.ci.sid;
         PORT_Assert(sid != NULL); /* Should have already been filled in.*/
+        if (!sid) {
+            desc = handshake_failure;
+            errCode = SEC_ERROR_LIBRARY_FAILURE;
+            goto alert_loser;
+        }
 
         if (sidBytes.len > 0 && sidBytes.len <= SSL3_SESSIONID_BYTES) {
             sid->u.ssl3.sessionIDLength = sidBytes.len;
@@ -8229,13 +8239,10 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
         } else {
             sid->u.ssl3.sessionIDLength = 0;
         }
-        ss->sec.ci.sid = NULL;
-    }
-
-    /* Free a potentially leftover session ID from a previous handshake. */
-    if (ss->sec.ci.sid) {
-        ssl_FreeSID(ss->sec.ci.sid);
-        ss->sec.ci.sid = NULL;
+    } else {
+        /* Free a potentially leftover session ID from a previous handshake. */
+        ssl_UncacheSessionID(ss);
+        sid = NULL;
     }
 
     if (sid != NULL) {
@@ -8253,7 +8260,6 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
 
             SSL_AtomicIncrementLong(&ssl3stats.hch_sid_cache_not_ok);
             ssl_UncacheSessionID(ss);
-            ssl_FreeSID(sid);
             sid = NULL;
         }
     }
@@ -8264,10 +8270,9 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
     }
 
     if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
-        rv = tls13_HandleClientHelloPart2(ss, &suites, sid, savedMsg, savedLen);
+        rv = tls13_HandleClientHelloPart2(ss, &suites, savedMsg, savedLen);
     } else {
-        rv = ssl3_HandleClientHelloPart2(ss, &suites, sid,
-                                         savedMsg, savedLen);
+        rv = ssl3_HandleClientHelloPart2(ss, &suites, savedMsg, savedLen);
     }
     if (rv != SECSuccess) {
         errCode = PORT_GetError();
@@ -8284,10 +8289,11 @@ loser:
 }
 
 static SECStatus
-ssl3_UnwrapMasterSecretServer(sslSocket *ss, sslSessionID *sid, PK11SymKey **ms)
+ssl3_UnwrapMasterSecretServer(sslSocket *ss, PK11SymKey **ms)
 {
     PK11SymKey *wrapKey;
     CK_FLAGS keyFlags = 0;
+    sslSessionID *sid = ss->sec.ci.sid;
     SECItem wrappedMS = {
         siBuffer,
         sid->u.ssl3.keys.wrapped_master_secret,
@@ -8318,7 +8324,6 @@ ssl3_UnwrapMasterSecretServer(sslSocket *ss, sslSessionID *sid, PK11SymKey **ms)
 static SECStatus
 ssl3_HandleClientHelloPart2(sslSocket *ss,
                             SECItem *suites,
-                            sslSessionID *sid,
                             const PRUint8 *msg,
                             unsigned int len)
 {
@@ -8328,6 +8333,7 @@ ssl3_HandleClientHelloPart2(sslSocket *ss,
     SECStatus rv;
     unsigned int i;
     unsigned int j;
+    sslSessionID *sid = ss->sec.ci.sid;
 
     rv = ssl_HashHandshakeMessage(ss, ssl_hs_client_hello, msg, len);
     if (rv != SECSuccess) {
@@ -8457,22 +8463,12 @@ cipher_found:
                 }
             }
 
-            if (ss->sec.ci.sid) {
-                ssl_UncacheSessionID(ss);
-                PORT_Assert(ss->sec.ci.sid != sid); /* should be impossible, but ... */
-                if (ss->sec.ci.sid != sid) {
-                    ssl_FreeSID(ss->sec.ci.sid);
-                }
-                ss->sec.ci.sid = NULL;
-            }
-
             /* we need to resurrect the master secret.... */
-            rv = ssl3_UnwrapMasterSecretServer(ss, sid, &masterSecret);
+            rv = ssl3_UnwrapMasterSecretServer(ss, &masterSecret);
             if (rv != SECSuccess) {
                 break; /* not an error */
             }
 
-            ss->sec.ci.sid = sid;
             if (sid->peerCert != NULL) {
                 ss->sec.peerCert = CERT_DupCertificate(sid->peerCert);
             }
@@ -8555,7 +8551,6 @@ cipher_found:
         ss->statelessResume = PR_FALSE;
         SSL_AtomicIncrementLong(&ssl3stats.hch_sid_cache_not_ok);
         ssl_UncacheSessionID(ss);
-        ssl_FreeSID(sid);
         sid = NULL;
     }
     SSL_AtomicIncrementLong(&ssl3stats.hch_sid_cache_misses);
@@ -8589,12 +8584,12 @@ cipher_found:
         goto alert_loser;
     }
 
-    sid = ssl3_NewSessionID(ss, PR_TRUE);
-    if (sid == NULL) {
+    rv = ssl3_NewSessionID(ss, PR_TRUE);
+    if (rv != SECSuccess) {
         errCode = PORT_GetError();
         goto loser; /* memory error is set. */
     }
-    ss->sec.ci.sid = sid;
+    sid = ss->sec.ci.sid;
 
     sid->u.ssl3.keys.extendedMasterSecretUsed =
         ssl3_ExtensionNegotiated(ss, ssl_extended_master_secret_xtn);
@@ -8619,11 +8614,7 @@ alert_loser:
     (void)SSL3_SendAlert(ss, alert_fatal, desc);
 /* FALLTHRU */
 loser:
-    if (sid && sid != ss->sec.ci.sid) {
-        ssl_UncacheSessionID(ss);
-        ssl_FreeSID(sid);
-    }
-
+    ssl_UncacheSessionID(ss);
     if (haveXmitBufLock) {
         ssl_ReleaseXmitBufLock(ss);
     }
@@ -8640,7 +8631,6 @@ SECStatus
 ssl3_HandleV2ClientHello(sslSocket *ss, unsigned char *buffer, unsigned int length,
                          PRUint8 padding)
 {
-    sslSessionID *sid = NULL;
     unsigned char *suites;
     unsigned char *random;
     SSL3ProtocolVersion version;
@@ -8807,13 +8797,12 @@ suite_found:
 
     /* we don't even search for a cache hit here.  It's just a miss. */
     SSL_AtomicIncrementLong(&ssl3stats.hch_sid_cache_misses);
-    sid = ssl3_NewSessionID(ss, PR_TRUE);
-    if (sid == NULL) {
+    PORT_Assert(!ss->sec.ci.sid);
+    rv = ssl3_NewSessionID(ss, PR_TRUE);
+    if (rv != SECSuccess) {
         errCode = PORT_GetError();
         goto loser; /* memory error is set. */
     }
-    ss->sec.ci.sid = sid;
-    /* do not worry about memory leak of sid since it now belongs to ci */
 
     /* We have to update the handshake hashes before we can send stuff */
     rv = ssl3_UpdateHandshakeHashes(ss, buffer, length);
@@ -10876,6 +10865,7 @@ fail:
 
 /* wrap the master secret, and put it into the SID.
  * Caller holds the Spec read lock.
+ * sid here can be a local SID that's not stored in ss.
  */
 SECStatus
 ssl3_CacheWrappedSecret(sslSocket *ss, sslSessionID *sid,
@@ -10964,7 +10954,6 @@ ssl3_CacheWrappedSecret(sslSocket *ss, sslSessionID *sid,
 static SECStatus
 ssl3_HandleFinished(sslSocket *ss, PRUint8 *b, PRUint32 length)
 {
-    sslSessionID *sid = ss->sec.ci.sid;
     SECStatus rv = SECSuccess;
     PRBool isServer = ss->sec.isServer;
     PRBool isTLS;
@@ -11108,8 +11097,9 @@ xmit_loser:
         return rv;
     }
 
-    if (sid->cached == never_cached && !ss->opt.noCache) {
-        rv = ssl3_FillInCachedSID(ss, sid, ss->ssl3.crSpec->masterSecret);
+    PORT_Assert(ss->sec.ci.sid);
+    if (ss->sec.ci.sid->cached == never_cached && !ss->opt.noCache) {
+        rv = ssl3_FillInCachedSID(ss, ss->ssl3.crSpec->masterSecret);
 
         /* If the wrap failed, we don't cache the sid.
          * The connection continues normally however.
@@ -11133,9 +11123,10 @@ xmit_loser:
 }
 
 SECStatus
-ssl3_FillInCachedSID(sslSocket *ss, sslSessionID *sid, PK11SymKey *secret)
+ssl3_FillInCachedSID(sslSocket *ss, PK11SymKey *secret)
 {
     PORT_Assert(secret);
+    sslSessionID *sid = ss->sec.ci.sid;
 
     /* fill in the sid */
     sid->u.ssl3.cipherSuite = ss->ssl3.hs.cipher_suite;
@@ -12644,7 +12635,6 @@ ssl3_InitSocketPolicy(sslSocket *ss)
 SECStatus
 ssl3_RedoHandshake(sslSocket *ss, PRBool flushCache)
 {
-    sslSessionID *sid = ss->sec.ci.sid;
     SECStatus rv;
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
@@ -12668,10 +12658,8 @@ ssl3_RedoHandshake(sslSocket *ss, PRBool flushCache)
         return SECFailure;
     }
 
-    if (sid && flushCache) {
+    if (flushCache) {
         ssl_UncacheSessionID(ss); /* remove it from whichever cache it's in. */
-        ssl_FreeSID(sid);         /* dec ref count and free if zero. */
-        ss->sec.ci.sid = NULL;
     }
 
     ssl_GetXmitBufLock(ss); /**************************************/
diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c
index e6388945e..11568def9 100644
--- a/lib/ssl/ssl3exthandle.c
+++ b/lib/ssl/ssl3exthandle.c
@@ -667,6 +667,7 @@ ssl3_EncodeSessionTicket(sslSocket *ss, const NewSessionTicket *ticket,
     SECStatus rv;
     sslBuffer plaintext = SSL_BUFFER_EMPTY;
     SECItem ticket_buf = { 0, NULL, 0 };
+    /* This SID is NOT the one in ss and only used in this function. */
     sslSessionID sid;
     unsigned char wrapped_ms[SSL3_MASTER_SECRET_LENGTH];
     SECItem ms_item = { 0, NULL, 0 };
@@ -1155,15 +1156,13 @@ ssl_ParseSessionTicket(sslSocket *ss, const SECItem *decryptedTicket,
 
 static SECStatus
 ssl_CreateSIDFromTicket(sslSocket *ss, const SECItem *rawTicket,
-                        SessionTicket *parsedTicket, sslSessionID **out)
+                        SessionTicket *parsedTicket)
 {
-    sslSessionID *sid;
-    SECStatus rv;
-
-    sid = ssl3_NewSessionID(ss, PR_TRUE);
-    if (sid == NULL) {
-        return SECFailure;
+    SECStatus rv = ssl3_NewSessionID(ss, PR_TRUE);
+    if (rv != SECSuccess) {
+        return rv;
     }
+    sslSessionID *sid = ss->sec.ci.sid;
 
     /* Copy over parameters. */
     sid->version = parsedTicket->ssl_version;
@@ -1227,11 +1226,10 @@ ssl_CreateSIDFromTicket(sslSocket *ss, const SECItem *rawTicket,
         }
     }
 
-    *out = sid;
     return SECSuccess;
 
 loser:
-    ssl_FreeSID(sid);
+    ssl_UncacheSessionID(ss);
     return SECFailure;
 }
 
@@ -1242,15 +1240,9 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, const SECItem *ticket,
 {
     SECItem decryptedTicket = { siBuffer, NULL, 0 };
     SessionTicket parsedTicket;
-    sslSessionID *sid = NULL;
     SECStatus rv;
 
-    if (ss->sec.ci.sid != NULL) {
-        ssl_UncacheSessionID(ss);
-        ssl_FreeSID(ss->sec.ci.sid);
-        ss->sec.ci.sid = NULL;
-    }
-
+    ssl_UncacheSessionID(ss);
     if (!SECITEM_AllocItem(NULL, &decryptedTicket, ticket->len)) {
         return SECFailure;
     }
@@ -1289,7 +1281,7 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, const SECItem *ticket,
     if (parsedTicket.timestamp + ssl_ticket_lifetime * PR_USEC_PER_SEC >
         ssl_TimeUsec()) {
 
-        rv = ssl_CreateSIDFromTicket(ss, ticket, &parsedTicket, &sid);
+        rv = ssl_CreateSIDFromTicket(ss, ticket, &parsedTicket);
         if (rv != SECSuccess) {
             goto loser; /* code already set */
         }
@@ -1302,7 +1294,6 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, const SECItem *ticket,
         }
 
         ss->statelessResume = PR_TRUE;
-        ss->sec.ci.sid = sid;
 
         /* We have the baseline value for the obfuscated ticket age here.  Save
          * that in xtnData temporarily.  This value is updated in
@@ -1315,9 +1306,7 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, const SECItem *ticket,
     return SECSuccess;
 
 loser:
-    if (sid) {
-        ssl_FreeSID(sid);
-    }
+    ssl_UncacheSessionID(ss);
     SECITEM_ZfreeItem(&decryptedTicket, PR_FALSE);
     PORT_Memset(&parsedTicket, 0, sizeof(parsedTicket));
     return SECFailure;
diff --git a/lib/ssl/sslcon.c b/lib/ssl/sslcon.c
index bc63e1537..03288df66 100644
--- a/lib/ssl/sslcon.c
+++ b/lib/ssl/sslcon.c
@@ -119,7 +119,6 @@ ssl_CheckConfigSanity(sslSocket *ss)
 SECStatus
 ssl_BeginClientHandshake(sslSocket *ss)
 {
-    sslSessionID *sid = NULL;
     SECStatus rv;
 
     PORT_Assert(ss->opt.noLocks || ssl_Have1stHandshakeLock(ss));
@@ -155,14 +154,16 @@ ssl_BeginClientHandshake(sslSocket *ss)
 
     SSL_TRC(3, ("%d: SSL[%d]: sending client-hello", SSL_GETPID(), ss->fd));
 
+    sslSessionID *sid = NULL;
     /* If there's an sid set from an external cache, use it. */
     if (ss->sec.ci.sid && ss->sec.ci.sid->cached == in_external_cache) {
         sid = ss->sec.ci.sid;
         SSL_TRC(3, ("%d: SSL[%d]: using external token", SSL_GETPID(), ss->fd));
     } else if (!ss->opt.noCache) {
         /* Try to find server in our session-id cache */
-        sid = ssl_LookupSID(&ss->sec.ci.peer, ss->sec.ci.port, ss->peerID,
-                            ss->url);
+        ssl_UncacheSessionID(ss);
+        ss->sec.ci.sid = sid = ssl_LookupSID(&ss->sec.ci.peer, ss->sec.ci.port,
+                                             ss->peerID, ss->url);
     }
 
     if (sid) {
@@ -171,12 +172,11 @@ ssl_BeginClientHandshake(sslSocket *ss)
             ss->sec.localCert = CERT_DupCertificate(sid->localCert);
         } else {
             ssl_UncacheSessionID(ss);
-            ssl_FreeSID(sid);
             sid = NULL;
         }
     }
     if (!sid) {
-        sid = PORT_ZNew(sslSessionID);
+        ss->sec.ci.sid = sid = PORT_ZNew(sslSessionID);
         if (!sid) {
             goto loser;
         }
@@ -191,7 +191,6 @@ ssl_BeginClientHandshake(sslSocket *ss)
             sid->urlSvrName = PORT_Strdup(ss->url);
         }
     }
-    ss->sec.ci.sid = sid;
 
     PORT_Assert(sid != NULL);
 
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 10d0333d9..0e0403c16 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1157,7 +1157,7 @@ extern int ssl_Do1stHandshake(sslSocket *ss);
 
 extern SECStatus ssl3_InitPendingCipherSpecs(sslSocket *ss, PK11SymKey *secret,
                                              PRBool derive);
-extern sslSessionID *ssl3_NewSessionID(sslSocket *ss, PRBool is_server);
+extern SECStatus ssl3_NewSessionID(sslSocket *ss, PRBool is_server);
 extern sslSessionID *ssl_LookupSID(const PRIPv6Addr *addr, PRUint16 port,
                                    const char *peerID, const char *urlSvrName);
 extern void ssl_FreeSID(sslSessionID *sid);
@@ -1621,8 +1621,7 @@ PK11SymKey *ssl3_GetWrappingKey(sslSocket *ss,
                                 PK11SlotInfo *masterSecretSlot,
                                 CK_MECHANISM_TYPE masterWrapMech,
                                 void *pwArg);
-SECStatus ssl3_FillInCachedSID(sslSocket *ss, sslSessionID *sid,
-                               PK11SymKey *secret);
+SECStatus ssl3_FillInCachedSID(sslSocket *ss, PK11SymKey *secret);
 const ssl3CipherSuiteDef *ssl_LookupCipherSuiteDef(ssl3CipherSuite suite);
 SECStatus ssl3_SelectServerCert(sslSocket *ss);
 SECStatus ssl_PickSignatureScheme(sslSocket *ss,
diff --git a/lib/ssl/sslnonce.c b/lib/ssl/sslnonce.c
index f79c23fc7..18cd9b9d3 100644
--- a/lib/ssl/sslnonce.c
+++ b/lib/ssl/sslnonce.c
@@ -1127,19 +1127,21 @@ ssl_CacheSessionID(sslSocket *ss)
 void
 ssl_UncacheSessionID(sslSocket *ss)
 {
-    if (ss->opt.noCache) {
-        return;
-    }
 
     sslSecurityInfo *sec = &ss->sec;
     PORT_Assert(sec);
 
     if (sec->ci.sid) {
-        if (sec->isServer) {
-            ssl_ServerUncacheSessionID(sec->ci.sid);
-        } else if (!ss->resumptionTokenCallback) {
-            LockAndUncacheSID(sec->ci.sid);
+        if (!ss->opt.noCache) {
+            if (sec->isServer) {
+                ssl_ServerUncacheSessionID(sec->ci.sid);
+            } else if (!ss->resumptionTokenCallback) {
+                LockAndUncacheSID(sec->ci.sid);
+            }
         }
+        PORT_Assert(sec->ci.sid->references == 1);
+        ssl_FreeSID(sec->ci.sid);
+        sec->ci.sid = NULL;
     }
 }
 
diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c
index e08d5e232..fc39e2ad3 100644
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -4040,13 +4040,13 @@ 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) {
+    SECStatus rv = ssl3_NewSessionID(ss, PR_FALSE);
+    if (rv != SECSuccess) {
         goto done;
     }
 
     /* Populate NewSessionTicket values */
-    SECStatus rv = ssl_DecodeResumptionToken(ss->sec.ci.sid, token, len);
+    rv = ssl_DecodeResumptionToken(ss->sec.ci.sid, token, len);
     if (rv != SECSuccess) {
         // If decoding fails, we assume the token is bad.
         PORT_SetError(SSL_ERROR_BAD_RESUMPTION_TOKEN_ERROR);
@@ -4066,8 +4066,7 @@ SSLExp_SetResumptionToken(PRFileDesc *fd, const PRUint8 *token,
     /* 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->references = 1;
     ss->sec.ci.sid->lastAccessTime = ssl_TimeSec();
 
     ssl_ReleaseSSL3HandshakeLock(ss);
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index c06acc83a..654616125 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -61,8 +61,7 @@ static SECStatus
 tls13_SendCertificateVerify(sslSocket *ss, SECKEYPrivateKey *privKey);
 static SECStatus tls13_HandleCertificateVerify(
     sslSocket *ss, PRUint8 *b, PRUint32 length);
-static SECStatus tls13_RecoverWrappedSharedSecret(sslSocket *ss,
-                                                  sslSessionID *sid);
+static SECStatus tls13_RecoverWrappedSharedSecret(sslSocket *ss);
 static SECStatus
 tls13_DeriveSecretWrap(sslSocket *ss, PK11SymKey *key,
                        const char *prefix,
@@ -457,14 +456,12 @@ tls13_SetupClientHello(sslSocket *ss)
     if (ss->statelessResume) {
         SECStatus rv;
 
-        PORT_Assert(ss->sec.ci.sid);
-        rv = tls13_RecoverWrappedSharedSecret(ss, ss->sec.ci.sid);
+        PORT_Assert(sid);
+        rv = tls13_RecoverWrappedSharedSecret(ss);
         if (rv != SECSuccess) {
             FATAL_ERROR(ss, SEC_ERROR_LIBRARY_FAILURE, internal_error);
             SSL_AtomicIncrementLong(&ssl3stats->sch_sid_cache_not_ok);
             ssl_UncacheSessionID(ss);
-            ssl_FreeSID(ss->sec.ci.sid);
-            ss->sec.ci.sid = NULL;
             return SECFailure;
         }
 
@@ -860,11 +857,12 @@ tls13_HandlePostHelloHandshakeMessage(sslSocket *ss, PRUint8 *b, PRUint32 length
 }
 
 static SECStatus
-tls13_RecoverWrappedSharedSecret(sslSocket *ss, sslSessionID *sid)
+tls13_RecoverWrappedSharedSecret(sslSocket *ss)
 {
     PK11SymKey *wrapKey; /* wrapping key */
     SECItem wrappedMS = { siBuffer, NULL, 0 };
     SSLHashType hashType;
+    sslSessionID *sid = ss->sec.ci.sid;
 
     SSL_TRC(3, ("%d: TLS13[%d]: recovering static secret (%s)",
                 SSL_GETPID(), ss->fd, SSL_ROLE(ss)));
@@ -1171,12 +1169,13 @@ tls13_ComputeFinalSecrets(sslSocket *ss)
 }
 
 static void
-tls13_RestoreCipherInfo(sslSocket *ss, sslSessionID *sid)
+tls13_RestoreCipherInfo(sslSocket *ss)
 {
     /* Set these to match the cached value.
      * TODO(ekr@rtfm.com): Make a version with the "true" values.
      * Bug 1256137.
      */
+    sslSessionID *sid = ss->sec.ci.sid;
     ss->sec.authType = sid->authType;
     ss->sec.authKeyBits = sid->authKeyBits;
     ss->sec.originalKeaGroup = ssl_LookupNamedGroup(sid->keaGroup);
@@ -1185,9 +1184,10 @@ tls13_RestoreCipherInfo(sslSocket *ss, sslSessionID *sid)
 
 /* Check whether resumption-PSK is allowed. */
 static PRBool
-tls13_CanResume(sslSocket *ss, const sslSessionID *sid)
+tls13_CanResume(sslSocket *ss)
 {
     const sslServerCert *sc;
+    sslSessionID *sid = ss->sec.ci.sid;
 
     if (!sid) {
         return PR_FALSE;
@@ -1214,8 +1214,9 @@ tls13_CanResume(sslSocket *ss, const sslSessionID *sid)
 }
 
 static PRBool
-tls13_CanNegotiateZeroRtt(sslSocket *ss, const sslSessionID *sid)
+tls13_CanNegotiateZeroRtt(sslSocket *ss)
 {
+    sslSessionID *sid = ss->sec.ci.sid;
     PORT_Assert(ss->ssl3.hs.zeroRttState == ssl_0rtt_sent);
 
     if (!sid)
@@ -1233,7 +1234,7 @@ tls13_CanNegotiateZeroRtt(sslSocket *ss, const sslSessionID *sid)
                             &sid->u.ssl3.alpnSelection) != 0)
         return PR_FALSE;
 
-    if (tls13_IsReplay(ss, sid)) {
+    if (tls13_IsReplay(ss)) {
         return PR_FALSE;
     }
 
@@ -1250,10 +1251,10 @@ tls13_CanNegotiateZeroRtt(sslSocket *ss, const sslSessionID *sid)
  * 5. We negotiated the same ALPN value as in the ticket.
  */
 static void
-tls13_NegotiateZeroRtt(sslSocket *ss, const sslSessionID *sid)
+tls13_NegotiateZeroRtt(sslSocket *ss)
 {
     SSL_TRC(3, ("%d: TLS13[%d]: negotiate 0-RTT %p",
-                SSL_GETPID(), ss->fd, sid));
+                SSL_GETPID(), ss->fd, ss->sec.ci.sid));
 
     /* tls13_ServerHandleEarlyDataXtn sets this to ssl_0rtt_sent, so this will
      * be ssl_0rtt_none unless early_data is present. */
@@ -1271,7 +1272,7 @@ tls13_NegotiateZeroRtt(sslSocket *ss, const sslSessionID *sid)
         return;
     }
 
-    if (!tls13_CanNegotiateZeroRtt(ss, sid)) {
+    if (!tls13_CanNegotiateZeroRtt(ss)) {
         SSL_TRC(3, ("%d: TLS13[%d]: ignore 0-RTT",
                     SSL_GETPID(), ss->fd));
         ss->ssl3.hs.zeroRttState = ssl_0rtt_ignored;
@@ -1574,7 +1575,6 @@ tls13_NegotiateAuthentication(sslSocket *ss)
 SECStatus
 tls13_HandleClientHelloPart2(sslSocket *ss,
                              const SECItem *suites,
-                             sslSessionID *sid,
                              const PRUint8 *msg,
                              unsigned int len)
 {
@@ -1663,12 +1663,12 @@ tls13_HandleClientHelloPart2(sslSocket *ss,
 
     /* Check if we could in principle resume. */
     if (ss->statelessResume) {
-        PORT_Assert(sid);
-        if (!sid) {
+        PORT_Assert(ss->sec.ci.sid);
+        if (!ss->sec.ci.sid) {
             FATAL_ERROR(ss, SEC_ERROR_LIBRARY_FAILURE, internal_error);
-            return SECFailure;
+            goto loser;
         }
-        if (!tls13_CanResume(ss, sid)) {
+        if (!tls13_CanResume(ss)) {
             ss->statelessResume = PR_FALSE;
         }
     }
@@ -1718,10 +1718,7 @@ tls13_HandleClientHelloPart2(sslSocket *ss,
         goto loser;
     }
     if (hrr) {
-        if (sid) { /* Free the sid. */
-            ssl_UncacheSessionID(ss);
-            ssl_FreeSID(sid);
-        }
+        ssl_UncacheSessionID(ss);
         PORT_Assert(ss->ssl3.hs.helloRetry);
         return SECSuccess;
     }
@@ -1733,6 +1730,7 @@ tls13_HandleClientHelloPart2(sslSocket *ss,
     }
 
     if (ss->statelessResume) {
+        sslSessionID *sid = ss->sec.ci.sid;
         /* We are now committed to trying to resume. */
         PORT_Assert(sid);
 
@@ -1748,13 +1746,13 @@ tls13_HandleClientHelloPart2(sslSocket *ss,
                                                 sid->namedCurve);
         PORT_Assert(ss->sec.serverCert);
 
-        rv = tls13_RecoverWrappedSharedSecret(ss, sid);
+        rv = tls13_RecoverWrappedSharedSecret(ss);
         if (rv != SECSuccess) {
             SSL_AtomicIncrementLong(&ssl3stats->hch_sid_cache_not_ok);
             FATAL_ERROR(ss, SEC_ERROR_LIBRARY_FAILURE, internal_error);
             goto loser;
         }
-        tls13_RestoreCipherInfo(ss, sid);
+        tls13_RestoreCipherInfo(ss);
 
         ss->sec.localCert = CERT_DupCertificate(ss->sec.serverCert->serverCert);
         if (sid->peerCert != NULL) {
@@ -1765,22 +1763,20 @@ tls13_HandleClientHelloPart2(sslSocket *ss,
             ss, &ss->xtnData,
             ssl_tls13_pre_shared_key_xtn, tls13_ServerSendPreSharedKeyXtn);
 
-        tls13_NegotiateZeroRtt(ss, sid);
+        tls13_NegotiateZeroRtt(ss);
     } else {
-        if (sid) { /* we had a sid, but it's no longer valid, free it */
+        if (ss->sec.ci.sid) { /* we had a sid, but it's no longer valid, free it */
             SSL_AtomicIncrementLong(&ssl3stats->hch_sid_cache_not_ok);
             ssl_UncacheSessionID(ss);
-            ssl_FreeSID(sid);
-            sid = NULL;
         }
-        tls13_NegotiateZeroRtt(ss, NULL);
+        tls13_NegotiateZeroRtt(ss);
     }
 
     /* Need to compute early secrets. */
     rv = tls13_ComputeEarlySecrets(ss);
     if (rv != SECSuccess) {
         FATAL_ERROR(ss, SEC_ERROR_LIBRARY_FAILURE, internal_error);
-        return SECFailure;
+        goto loser;
     }
 
     /* Now that we have the binder key check the binder. */
@@ -1826,24 +1822,20 @@ tls13_HandleClientHelloPart2(sslSocket *ss,
         SSL_AtomicIncrementLong(&ssl3stats->hch_sid_cache_hits);
         SSL_AtomicIncrementLong(&ssl3stats->hch_sid_stateless_resumes);
     } else {
-        if (sid) {
+        if (ss->sec.ci.sid) {
             /* We had a sid, but it's no longer valid, free it. */
             SSL_AtomicIncrementLong(&ssl3stats->hch_sid_cache_not_ok);
             ssl_UncacheSessionID(ss);
-            ssl_FreeSID(sid);
         } else {
             SSL_AtomicIncrementLong(&ssl3stats->hch_sid_cache_misses);
         }
 
-        sid = ssl3_NewSessionID(ss, PR_TRUE);
-        if (!sid) {
+        rv = ssl3_NewSessionID(ss, PR_TRUE);
+        if (rv != SECSuccess) {
             FATAL_ERROR(ss, PORT_GetError(), internal_error);
-            return SECFailure;
+            goto loser;
         }
     }
-    /* Take ownership of the session. */
-    ss->sec.ci.sid = sid;
-    sid = NULL;
 
     if (ss->ssl3.hs.zeroRttState == ssl_0rtt_accepted) {
         rv = tls13_DeriveEarlySecrets(ss);
@@ -1864,10 +1856,7 @@ tls13_HandleClientHelloPart2(sslSocket *ss,
     return SECSuccess;
 
 loser:
-    if (sid) {
-        ssl_UncacheSessionID(ss);
-        ssl_FreeSID(sid);
-    }
+    ssl_UncacheSessionID(ss);
     return SECFailure;
 }
 
@@ -2494,7 +2483,6 @@ SECStatus
 tls13_HandleServerHelloPart2(sslSocket *ss)
 {
     SECStatus rv;
-    sslSessionID *sid = ss->sec.ci.sid;
     SSL3Statistics *ssl3stats = SSL_GetStatistics();
 
     if (ssl3_ExtensionNegotiated(ss, ssl_tls13_pre_shared_key_xtn)) {
@@ -2510,7 +2498,7 @@ tls13_HandleServerHelloPart2(sslSocket *ss)
 
     if (ss->statelessResume) {
         if (tls13_GetHash(ss) !=
-            tls13_GetHashForCipherSuite(sid->u.ssl3.cipherSuite)) {
+            tls13_GetHashForCipherSuite(ss->sec.ci.sid->u.ssl3.cipherSuite)) {
             FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_SERVER_HELLO,
                         illegal_parameter);
             return SECFailure;
@@ -2524,9 +2512,9 @@ tls13_HandleServerHelloPart2(sslSocket *ss)
     if (ss->statelessResume) {
         /* PSK */
         ss->ssl3.hs.kea_def_mutable.authKeyType = ssl_auth_psk;
-        tls13_RestoreCipherInfo(ss, sid);
-        if (sid->peerCert) {
-            ss->sec.peerCert = CERT_DupCertificate(sid->peerCert);
+        tls13_RestoreCipherInfo(ss);
+        if (ss->sec.ci.sid->peerCert) {
+            ss->sec.peerCert = CERT_DupCertificate(ss->sec.ci.sid->peerCert);
         }
 
         SSL_AtomicIncrementLong(&ssl3stats->hsh_sid_cache_hits);
@@ -2536,7 +2524,7 @@ tls13_HandleServerHelloPart2(sslSocket *ss)
         if (ssl3_ExtensionAdvertised(ss, ssl_tls13_pre_shared_key_xtn)) {
             SSL_AtomicIncrementLong(&ssl3stats->hsh_sid_cache_misses);
         }
-        if (sid->cached == in_client_cache) {
+        if (ss->sec.ci.sid->cached == in_client_cache) {
             /* If we tried to resume and failed, let's not try again. */
             ssl_UncacheSessionID(ss);
         }
@@ -2556,18 +2544,23 @@ tls13_HandleServerHelloPart2(sslSocket *ss)
 
     /* Discard current SID and make a new one, though it may eventually
      * end up looking a lot like the old one.
+     * Note that we don't uncache here. By freeing the sid here the uncache
+     * in ssl3_NewSessionID has no effect.
      */
-    ssl_FreeSID(sid);
-    ss->sec.ci.sid = sid = ssl3_NewSessionID(ss, PR_FALSE);
-    if (sid == NULL) {
+    if (ss->sec.ci.sid) {
+        ssl_FreeSID(ss->sec.ci.sid);
+        ss->sec.ci.sid = NULL;
+    }
+    rv = ssl3_NewSessionID(ss, PR_FALSE);
+    if (rv != SECSuccess) {
         FATAL_ERROR(ss, PORT_GetError(), internal_error);
         return SECFailure;
     }
     if (ss->statelessResume) {
         PORT_Assert(ss->sec.peerCert);
-        sid->peerCert = CERT_DupCertificate(ss->sec.peerCert);
+        ss->sec.ci.sid->peerCert = CERT_DupCertificate(ss->sec.peerCert);
     }
-    sid->version = ss->version;
+    ss->sec.ci.sid->version = ss->version;
 
     rv = tls13_HandleServerKeyShare(ss);
     if (rv != SECSuccess) {
@@ -4663,24 +4656,22 @@ 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) {
-            /* Create a new session ID. */
-            sslSessionID *sid = ssl3_NewSessionID(ss, PR_FALSE);
-            if (!sid) {
+            /* We need the cert in the new sid. */
+            PORT_Assert(ss->sec.ci.sid->peerCert);
+            CERTCertificate *tmp = CERT_DupCertificate(ss->sec.ci.sid->peerCert);
+            if (!tmp) {
                 return SECFailure;
             }
 
-            /* Copy over the peerCert. */
-            PORT_Assert(ss->sec.ci.sid->peerCert);
-            sid->peerCert = CERT_DupCertificate(ss->sec.ci.sid->peerCert);
-            if (!sid->peerCert) {
-                ssl_FreeSID(sid);
+            /* Create a new session ID. The old one is being destroyed. */
+            rv = ssl3_NewSessionID(ss, PR_FALSE);
+            if (rv != SECSuccess) {
+                CERT_DestroyCertificate(tmp);
                 return SECFailure;
             }
 
-            /* Destroy the old SID. */
-            ssl_UncacheSessionID(ss);
-            ssl_FreeSID(ss->sec.ci.sid);
-            ss->sec.ci.sid = sid;
+            /* "Copy" over the peerCert. */
+            ss->sec.ci.sid->peerCert = tmp;
         }
 
         ssl3_SetSIDSessionTicket(ss->sec.ci.sid, &ticket);
@@ -4697,7 +4688,7 @@ tls13_HandleNewSessionTicket(sslSocket *ss, PRUint8 *b, PRUint32 length)
             return SECFailure;
         }
 
-        rv = ssl3_FillInCachedSID(ss, ss->sec.ci.sid, secret);
+        rv = ssl3_FillInCachedSID(ss, secret);
         PK11_FreeSymKey(secret);
         if (rv != SECSuccess) {
             return SECFailure;
@@ -5022,10 +5013,11 @@ tls13_UnprotectRecord(sslSocket *ss,
  * Called from tls13_ClientSendEarlyDataXtn().
  */
 PRBool
-tls13_ClientAllow0Rtt(const sslSocket *ss, const sslSessionID *sid)
+tls13_ClientAllow0Rtt(const sslSocket *ss)
 {
     /* We checked that the cipher suite was still allowed back in
      * ssl3_SendClientHello. */
+    const sslSessionID *sid = ss->sec.ci.sid;
     if (sid->version < SSL_LIBRARY_VERSION_TLS_1_3)
         return PR_FALSE;
     if (ss->ssl3.hs.helloRetry)
diff --git a/lib/ssl/tls13con.h b/lib/ssl/tls13con.h
index 1aaffb651..dc936bbb7 100644
--- a/lib/ssl/tls13con.h
+++ b/lib/ssl/tls13con.h
@@ -70,7 +70,6 @@ PRBool tls13_PskSuiteEnabled(sslSocket *ss);
 SECStatus tls13_WriteExtensionsWithBinder(sslSocket *ss, sslBuffer *extensions);
 SECStatus tls13_HandleClientHelloPart2(sslSocket *ss,
                                        const SECItem *suites,
-                                       sslSessionID *sid,
                                        const PRUint8 *msg,
                                        unsigned int len);
 SECStatus tls13_HandleServerHelloPart2(sslSocket *ss);
@@ -99,12 +98,12 @@ SECStatus tls13_ProtectRecord(sslSocket *ss,
                               sslBuffer *wrBuf);
 PRInt32 tls13_Read0RttData(sslSocket *ss, void *buf, PRInt32 len);
 SECStatus tls13_HandleEarlyApplicationData(sslSocket *ss, sslBuffer *origBuf);
-PRBool tls13_ClientAllow0Rtt(const sslSocket *ss, const sslSessionID *sid);
+PRBool tls13_ClientAllow0Rtt(const sslSocket *ss);
 PRUint16 tls13_EncodeDraftVersion(SSL3ProtocolVersion version);
 SECStatus tls13_NegotiateVersion(sslSocket *ss,
                                  const TLSExtension *supported_versions);
 
-PRBool tls13_IsReplay(const sslSocket *ss, const sslSessionID *sid);
+PRBool tls13_IsReplay(const sslSocket *ss);
 void tls13_AntiReplayRollover(PRTime now);
 
 SECStatus SSLExp_SetupAntiReplay(PRTime window, unsigned int k,
diff --git a/lib/ssl/tls13exthandle.c b/lib/ssl/tls13exthandle.c
index 899f23827..95792a1ce 100644
--- a/lib/ssl/tls13exthandle.c
+++ b/lib/ssl/tls13exthandle.c
@@ -627,7 +627,7 @@ SECStatus
 tls13_ClientSendEarlyDataXtn(const sslSocket *ss, TLSExtensionData *xtnData,
                              sslBuffer *buf, PRBool *added)
 {
-    if (!tls13_ClientAllow0Rtt(ss, ss->sec.ci.sid)) {
+    if (!tls13_ClientAllow0Rtt(ss)) {
         return SECSuccess;
     }
 
diff --git a/lib/ssl/tls13replay.c b/lib/ssl/tls13replay.c
index b090f9bca..78cf4477b 100644
--- a/lib/ssl/tls13replay.c
+++ b/lib/ssl/tls13replay.c
@@ -188,16 +188,17 @@ tls13_AntiReplayUpdate()
 }
 
 PRBool
-tls13_InWindow(const sslSocket *ss, const sslSessionID *sid)
+tls13_InWindow(const sslSocket *ss)
 {
     PRInt32 timeDelta;
+    PORT_Assert(ss->sec.ci.sid);
 
     /* Calculate the difference between the client's view of the age of the
      * ticket (in |ss->xtnData.ticketAge|) and the server's view, which we now
      * calculate.  The result should be close to zero.  timeDelta is signed to
      * make the comparisons below easier. */
     timeDelta = ss->xtnData.ticketAge -
-                ((ssl_TimeUsec() - sid->creationTime) / PR_USEC_PER_MSEC);
+                ((ssl_TimeUsec() - ss->sec.ci.sid->creationTime) / PR_USEC_PER_MSEC);
 
     /* Only allow the time delta to be at most half of our window.  This is
      * symmetrical, though it doesn't need to be; this assumes that clock errors
@@ -230,7 +231,7 @@ tls13_InWindow(const sslSocket *ss, const sslSessionID *sid)
  * replay.  In that case, we reject 0-RTT unnecessarily, but that's OK because
  * no client expects 0-RTT to work every time. */
 PRBool
-tls13_IsReplay(const sslSocket *ss, const sslSessionID *sid)
+tls13_IsReplay(const sslSocket *ss)
 {
     PRBool replay;
     unsigned int size;
@@ -245,7 +246,7 @@ tls13_IsReplay(const sslSocket *ss, const sslSessionID *sid)
         return PR_TRUE;
     }
 
-    if (!tls13_InWindow(ss, sid)) {
+    if (!tls13_InWindow(ss)) {
         return PR_TRUE;
     }