import changes from mozilla nss repo:

- Bug 1641480, TLS 1.3: tighten CCS handling in compatibility mode, r=mt
- Bug 1672703, always tolerate the first CCS in TLS 1.3, r=mt
- Bug 1663661 - Guard against NULL token in nssSlot_IsTokenPresent. r=jcj
- Bug 1607449 - Lock cert->nssCertificate to prevent data race. r=jcj,keeler
- Bug 1679290 - Don't hold slot lock when taking session lock r=bbeurdouche

security/nss/gtests/ssl_gtest/ssl_tls13compat_unittest.cc

@@ -347,6 +347,85 @@ TEST_F(TlsConnectStreamTls13, ChangeCipherSpecBeforeClientHelloTwice) {
   client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT);
 }
 
+// The server accepts a ChangeCipherSpec even if the client advertises
+// an empty session ID.
+TEST_F(TlsConnectStreamTls13, ChangeCipherSpecAfterClientHelloEmptySid) {
+  EnsureTlsSetup();
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+
+  StartConnect();
+  client_->Handshake();  // Send ClientHello
+  client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs)));  // Send CCS
+
+  Handshake();
+  CheckConnected();
+}
+
+// The server rejects multiple ChangeCipherSpec even if the client
+// indicates compatibility mode with non-empty session ID.
+TEST_F(Tls13CompatTest, ChangeCipherSpecAfterClientHelloTwice) {
+  EnsureTlsSetup();
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  EnableCompatMode();
+
+  StartConnect();
+  client_->Handshake();  // Send ClientHello
+  // Send CCS twice in a row
+  client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs)));
+  client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs)));
+
+  server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+  server_->Handshake();  // Consume ClientHello and CCS.
+  server_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER);
+}
+
+// The client accepts a ChangeCipherSpec even if it advertises an empty
+// session ID.
+TEST_F(TlsConnectStreamTls13, ChangeCipherSpecAfterServerHelloEmptySid) {
+  EnsureTlsSetup();
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+
+  // To replace Finished with a CCS below
+  auto filter = MakeTlsFilter<TlsHandshakeDropper>(server_);
+  filter->SetHandshakeTypes({kTlsHandshakeFinished});
+  filter->EnableDecryption();
+
+  StartConnect();
+  client_->Handshake();  // Send ClientHello
+  server_->Handshake();  // Consume ClientHello, and
+                         // send ServerHello..CertificateVerify
+  // Send CCS
+  server_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs)));
+
+  // No alert is sent from the client. As Finished is dropped, we
+  // can't use Handshake() and CheckConnected().
+  client_->Handshake();
+}
+
+// The client rejects multiple ChangeCipherSpec in a row even if the
+// client indicates compatibility mode with non-empty session ID.
+TEST_F(Tls13CompatTest, ChangeCipherSpecAfterServerHelloTwice) {
+  EnsureTlsSetup();
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  EnableCompatMode();
+
+  // To replace Finished with a CCS below
+  auto filter = MakeTlsFilter<TlsHandshakeDropper>(server_);
+  filter->SetHandshakeTypes({kTlsHandshakeFinished});
+  filter->EnableDecryption();
+
+  StartConnect();
+  client_->Handshake();  // Send ClientHello
+  server_->Handshake();  // Consume ClientHello, and
+                         // send ServerHello..CertificateVerify
+                         // the ServerHello is followed by CCS
+  // Send another CCS
+  server_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs)));
+  client_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+  client_->Handshake();  // Consume ClientHello and CCS
+  client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER);
+}
+
 // If we negotiate 1.2, we abort.
 TEST_F(TlsConnectStreamTls13, ChangeCipherSpecBeforeClientHello12) {
   EnsureTlsSetup();

security/nss/lib/certdb/certdb.c

@@ -2908,16 +2908,27 @@ CERT_LockCertTrust(const CERTCertificate *cert)
     PZ_Lock(certTrustLock);
 }
 
-static PZLock *certTempPermLock = NULL;
+static PZLock *certTempPermCertLock = NULL;
 
 /*
- * Acquire the cert temp/perm lock
+ * Acquire the cert temp/perm/nssCert lock
  */
 void
 CERT_LockCertTempPerm(const CERTCertificate *cert)
 {
-    PORT_Assert(certTempPermLock != NULL);
+    PORT_Assert(certTempPermCertLock != NULL);
-    PZ_Lock(certTempPermLock);
+    PZ_Lock(certTempPermCertLock);
+}
+
+/* Maybe[Lock, Unlock] variants are only to be used by
+ * CERT_DestroyCertificate, since an application could
+ * call this after NSS_Shutdown destroys cert locks. */
+void
+CERT_MaybeLockCertTempPerm(const CERTCertificate *cert)
+{
+    if (certTempPermCertLock) {
+        PZ_Lock(certTempPermCertLock);
+    }
 }
 
 SECStatus
@@ -2941,10 +2952,10 @@ cert_InitLocks(void)
         }
     }
 
-    if (certTempPermLock == NULL) {
+    if (certTempPermCertLock == NULL) {
-        certTempPermLock = PZ_NewLock(nssILockCertDB);
+        certTempPermCertLock = PZ_NewLock(nssILockCertDB);
-        PORT_Assert(certTempPermLock != NULL);
+        PORT_Assert(certTempPermCertLock != NULL);
-        if (!certTempPermLock) {
+        if (!certTempPermCertLock) {
             PZ_DestroyLock(certTrustLock);
             PZ_DestroyLock(certRefCountLock);
             certRefCountLock = NULL;
@@ -2977,10 +2988,10 @@ cert_DestroyLocks(void)
         rv = SECFailure;
     }
 
-    PORT_Assert(certTempPermLock != NULL);
+    PORT_Assert(certTempPermCertLock != NULL);
-    if (certTempPermLock) {
+    if (certTempPermCertLock) {
-        PZ_DestroyLock(certTempPermLock);
+        PZ_DestroyLock(certTempPermCertLock);
-        certTempPermLock = NULL;
+        certTempPermCertLock = NULL;
     } else {
         rv = SECFailure;
     }
@@ -2999,16 +3010,24 @@ CERT_UnlockCertTrust(const CERTCertificate *cert)
 }
 
 /*
- * Free the temp/perm lock
+ * Free the temp/perm/nssCert lock
  */
 void
 CERT_UnlockCertTempPerm(const CERTCertificate *cert)
 {
-    PORT_Assert(certTempPermLock != NULL);
+    PORT_Assert(certTempPermCertLock != NULL);
-    PRStatus prstat = PZ_Unlock(certTempPermLock);
+    PRStatus prstat = PZ_Unlock(certTempPermCertLock);
     PORT_AssertArg(prstat == PR_SUCCESS);
 }
 
+void
+CERT_MaybeUnlockCertTempPerm(const CERTCertificate *cert)
+{
+    if (certTempPermCertLock) {
+        PZ_Unlock(certTempPermCertLock);
+    }
+}
+
 /*
  * Get the StatusConfig data for this handle
  */

security/nss/lib/certdb/stanpcertdb.c

@@ -32,6 +32,9 @@
 #include "dev.h"
 #include "secmodi.h"
 
+extern void CERT_MaybeLockCertTempPerm(const CERTCertificate *cert);
+extern void CERT_MaybeUnlockCertTempPerm(const CERTCertificate *cert);
+
 PRBool
 SEC_CertNicknameConflict(const char *nickname, const SECItem *derSubject,
                          CERTCertDBHandle *handle)
@@ -311,7 +314,9 @@ __CERT_AddTempCertToPerm(CERTCertificate *cert, char *nickname,
     nssPKIObject_AddInstance(&c->object, permInstance);
     nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1);
     /* reset the CERTCertificate fields */
+    CERT_LockCertTempPerm(cert);
     cert->nssCertificate = NULL;
+    CERT_UnlockCertTempPerm(cert);
     cert = STAN_GetCERTCertificateOrRelease(c); /* should return same pointer */
     if (!cert) {
         CERT_MapStanError();
@@ -808,9 +813,17 @@ CERT_DestroyCertificate(CERTCertificate *cert)
         /* don't use STAN_GetNSSCertificate because we don't want to
          * go to the trouble of translating the CERTCertificate into
          * an NSSCertificate just to destroy it.  If it hasn't been done
-         * yet, don't do it at all.
+         * yet, don't do it at all
-         */
+         *
+         * cert->nssCertificate contains its own locks and refcount, but as it
+         * may be NULL, the pointer itself must be guarded by some other lock.
+         * Rather than creating a new global lock for only this purpose, share
+         * an existing global lock that happens to be taken near the write in
+         * fill_CERTCertificateFields(). The longer-term goal is to refactor
+         * all these global locks to be certificate-scoped. */
+        CERT_MaybeLockCertTempPerm(cert);
         NSSCertificate *tmp = cert->nssCertificate;
+        CERT_MaybeUnlockCertTempPerm(cert);
         if (tmp) {
             /* delete the NSSCertificate */
             NSSCertificate_Destroy(tmp);

security/nss/lib/dev/devslot.c

@@ -171,11 +171,12 @@ nssSlot_IsTokenPresent(
 
     nssSlot_EnterMonitor(slot);
     ckrv = CKAPI(epv)->C_GetSlotInfo(slot->slotID, &slotInfo);
-    nssSlot_ExitMonitor(slot);
     if (ckrv != CKR_OK) {
+        if (slot->token) {
             slot->token->base.name[0] = 0; /* XXX */
+        }
         isPresent = PR_FALSE;
-        goto done;
+        goto done; /* slot lock held */
     }
     slot->ckFlags = slotInfo.flags;
     /* check for the presence of the token */
@@ -183,10 +184,11 @@ nssSlot_IsTokenPresent(
         if (!slot->token) {
             /* token was never present */
             isPresent = PR_FALSE;
-            goto done;
+            goto done; /* slot lock held */
         }
         session = nssToken_GetDefaultSession(slot->token);
         if (session) {
+            nssSlot_ExitMonitor(slot);
             nssSession_EnterMonitor(session);
             /* token is not present */
             if (session->handle != CK_INVALID_SESSION) {
@@ -196,6 +198,12 @@ nssSlot_IsTokenPresent(
                 session->handle = CK_INVALID_SESSION;
             }
             nssSession_ExitMonitor(session);
+            nssSlot_EnterMonitor(slot);
+            if (!slot->token) {
+                /* Check token presence after re-acquiring lock */
+                isPresent = PR_FALSE;
+                goto done; /* slot lock held */
+            }
         }
         if (slot->token->base.name[0] != 0) {
             /* notify the high-level cache that the token is removed */
@@ -206,14 +214,23 @@ nssSlot_IsTokenPresent(
         /* clear the token cache */
         nssToken_Remove(slot->token);
         isPresent = PR_FALSE;
-        goto done;
+        goto done; /* slot lock held */
     }
+    if (!slot->token) {
+        /* This should not occur, based on the fact that the
+         * below calls will dereference NULL otherwise. */
+        PORT_Assert(0);
+        isPresent = PR_FALSE;
+        goto done; /* slot lock held */
+    }
+
     /* token is present, use the session info to determine if the card
      * has been removed and reinserted.
      */
     session = nssToken_GetDefaultSession(slot->token);
     if (session) {
         PRBool tokenRemoved;
+        nssSlot_ExitMonitor(slot);
         nssSession_EnterMonitor(session);
         if (session->handle != CK_INVALID_SESSION) {
             CK_SESSION_INFO sessionInfo;
@@ -227,10 +244,16 @@ nssSlot_IsTokenPresent(
         }
         tokenRemoved = (session->handle == CK_INVALID_SESSION);
         nssSession_ExitMonitor(session);
+        nssSlot_EnterMonitor(slot);
         /* token not removed, finished */
         if (!tokenRemoved) {
             isPresent = PR_TRUE;
-            goto done;
+            goto done; /* slot lock held */
+        }
+        if (!slot->token) {
+            /* Check token presence after re-acquiring lock */
+            isPresent = PR_FALSE;
+            goto done; /* slot lock held */
         }
     }
     /* the token has been removed, and reinserted, or the slot contains
@@ -248,6 +271,7 @@ nssSlot_IsTokenPresent(
         isPresent = PR_FALSE;
     }
 done:
+    nssSlot_ExitMonitor(slot);
     /* Once we've set up the condition variable,
      * Before returning, it's necessary to:
      *  1) Set the lastTokenPingTime so that any other threads waiting on this

security/nss/lib/dev/devt.h

@@ -96,6 +96,9 @@ struct NSSSlotStr {
 };
 
 struct nssSessionStr {
+    /* Must not hold slot->lock when taking lock.
+     * See ordering in nssSlot_IsTokenPresent.
+     */
     PZLock *lock;
     CK_SESSION_HANDLE handle;
     NSSSlot *slot;

security/nss/lib/pk11wrap/pk11cert.c

@@ -1146,8 +1146,11 @@ PK11_ImportCert(PK11SlotInfo *slot, CERTCertificate *cert,
     }
 
     /* need to get the cert as a stan cert */
-    if (cert->nssCertificate) {
+    CERT_LockCertTempPerm(cert);
-        c = cert->nssCertificate;
+    NSSCertificate *nssCert = cert->nssCertificate;
+    CERT_UnlockCertTempPerm(cert);
+    if (nssCert) {
+        c = nssCert;
     } else {
         c = STAN_GetNSSCertificate(cert);
         if (c == NULL) {

security/nss/lib/pki/pki3hack.c

@@ -866,9 +866,9 @@ fill_CERTCertificateFields(NSSCertificate *c, CERTCertificate *cc, PRBool forced
     CERT_LockCertTempPerm(cc);
     cc->istemp = PR_FALSE; /* CERT_NewTemp will override this */
     cc->isperm = PR_TRUE;  /* by default */
-    CERT_UnlockCertTempPerm(cc);
     /* pointer back */
     cc->nssCertificate = c;
+    CERT_UnlockCertTempPerm(cc);
     if (trust) {
         /* force the cert type to be recomputed to include trust info */
         PRUint32 nsCertType = cert_ComputeCertType(cc);
@@ -919,7 +919,10 @@ stan_GetCERTCertificate(NSSCertificate *c, PRBool forceUpdate)
         nss_SetError(NSS_ERROR_INTERNAL_ERROR);
         goto loser;
     }
-    if (!cc->nssCertificate || forceUpdate) {
+    CERT_LockCertTempPerm(cc);
+    NSSCertificate *nssCert = cc->nssCertificate;
+    CERT_UnlockCertTempPerm(cc);
+    if (!nssCert || forceUpdate) {
         fill_CERTCertificateFields(c, cc, forceUpdate);
     } else if (CERT_GetCertTrust(cc, &certTrust) != SECSuccess) {
         CERTCertTrust *trust;
@@ -1018,7 +1021,9 @@ STAN_GetNSSCertificate(CERTCertificate *cc)
     nssCryptokiInstance *instance;
     nssPKIObject *pkiob;
     NSSArena *arena;
+    CERT_LockCertTempPerm(cc);
     c = cc->nssCertificate;
+    CERT_UnlockCertTempPerm(cc);
     if (c) {
         return c;
     }
@@ -1083,7 +1088,9 @@ STAN_GetNSSCertificate(CERTCertificate *cc)
         nssPKIObject_AddInstance(&c->object, instance);
     }
     c->decoding = create_decoded_pkix_cert_from_nss3cert(NULL, cc);
+    CERT_LockCertTempPerm(cc);
     cc->nssCertificate = c;
+    CERT_UnlockCertTempPerm(cc);
     return c;
 }
 

security/nss/lib/ssl/ssl3con.c

@@ -12993,8 +12993,14 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
             ss->ssl3.hs.ws != idle_handshake &&
             cText->buf->len == 1 &&
             cText->buf->buf[0] == change_cipher_spec_choice) {
-            /* Ignore the CCS. */
+            if (!ss->ssl3.hs.rejectCcs) {
+                /* Allow only the first CCS. */
+                ss->ssl3.hs.rejectCcs = PR_TRUE;
                 return SECSuccess;
+            } else {
+                alert = unexpected_message;
+                PORT_SetError(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER);
+            }
         }
 
         if (IS_DTLS(ss) ||

security/nss/lib/ssl/sslimpl.h

@@ -710,6 +710,7 @@ typedef struct SSL3HandshakeStateStr {
                                            * or received. */
     PRBool receivedCcs;                   /* A server received ChangeCipherSpec
                                            * before the handshake started. */
+    PRBool rejectCcs;                     /* Excessive ChangeCipherSpecs are rejected. */
     PRBool clientCertRequested;           /* True if CertificateRequest received. */
     ssl3KEADef kea_def_mutable;           /* Used to hold the writable kea_def
                                            * we use for TLS 1.3 */