diff --git a/security/nss/gtests/ssl_gtest/ssl_tls13compat_unittest.cc b/security/nss/gtests/ssl_gtest/ssl_tls13compat_unittest.cc index 769413cc99..8a805545c5 100644 --- a/security/nss/gtests/ssl_gtest/ssl_tls13compat_unittest.cc +++ b/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(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(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(); diff --git a/security/nss/lib/certdb/certdb.c b/security/nss/lib/certdb/certdb.c index 0796fe5d75..4a713b6d7f 100644 --- a/security/nss/lib/certdb/certdb.c +++ b/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); - PZ_Lock(certTempPermLock); + PORT_Assert(certTempPermCertLock != NULL); + 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) { - certTempPermLock = PZ_NewLock(nssILockCertDB); - PORT_Assert(certTempPermLock != NULL); - if (!certTempPermLock) { + if (certTempPermCertLock == NULL) { + certTempPermCertLock = PZ_NewLock(nssILockCertDB); + PORT_Assert(certTempPermCertLock != NULL); + if (!certTempPermCertLock) { PZ_DestroyLock(certTrustLock); PZ_DestroyLock(certRefCountLock); certRefCountLock = NULL; @@ -2977,10 +2988,10 @@ cert_DestroyLocks(void) rv = SECFailure; } - PORT_Assert(certTempPermLock != NULL); - if (certTempPermLock) { - PZ_DestroyLock(certTempPermLock); - certTempPermLock = NULL; + PORT_Assert(certTempPermCertLock != NULL); + if (certTempPermCertLock) { + PZ_DestroyLock(certTempPermCertLock); + 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); - PRStatus prstat = PZ_Unlock(certTempPermLock); + PORT_Assert(certTempPermCertLock != NULL); + 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 */ diff --git a/security/nss/lib/certdb/stanpcertdb.c b/security/nss/lib/certdb/stanpcertdb.c index e2a668bb19..8e1cf279ab 100644 --- a/security/nss/lib/certdb/stanpcertdb.c +++ b/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); diff --git a/security/nss/lib/dev/devslot.c b/security/nss/lib/dev/devslot.c index ebd6e6aa5e..13c09d32ce 100644 --- a/security/nss/lib/dev/devslot.c +++ b/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) { - slot->token->base.name[0] = 0; /* XXX */ + 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 diff --git a/security/nss/lib/dev/devt.h b/security/nss/lib/dev/devt.h index 0f6d9e49a1..06a57ad05b 100644 --- a/security/nss/lib/dev/devt.h +++ b/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; diff --git a/security/nss/lib/pk11wrap/pk11cert.c b/security/nss/lib/pk11wrap/pk11cert.c index 655c5f9703..8ff55fce7d 100644 --- a/security/nss/lib/pk11wrap/pk11cert.c +++ b/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) { - c = cert->nssCertificate; + CERT_LockCertTempPerm(cert); + NSSCertificate *nssCert = cert->nssCertificate; + CERT_UnlockCertTempPerm(cert); + if (nssCert) { + c = nssCert; } else { c = STAN_GetNSSCertificate(cert); if (c == NULL) { diff --git a/security/nss/lib/pki/pki3hack.c b/security/nss/lib/pki/pki3hack.c index eac4a5705e..7fe9113e4a 100644 --- a/security/nss/lib/pki/pki3hack.c +++ b/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; } diff --git a/security/nss/lib/ssl/ssl3con.c b/security/nss/lib/ssl/ssl3con.c index 94e1ed28f1..c6ab4f0516 100644 --- a/security/nss/lib/ssl/ssl3con.c +++ b/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. */ - return SECSuccess; + 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) || diff --git a/security/nss/lib/ssl/sslimpl.h b/security/nss/lib/ssl/sslimpl.h index 1b3eeb2938..bed5ca3dec 100644 --- a/security/nss/lib/ssl/sslimpl.h +++ b/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 */