diff --git a/security/nss/cmd/modutil/install-ds.c b/security/nss/cmd/modutil/install-ds.c index 576839f8f5..a013d05a33 100644 --- a/security/nss/cmd/modutil/install-ds.c +++ b/security/nss/cmd/modutil/install-ds.c @@ -889,8 +889,6 @@ Pk11Install_Platform_Generate(Pk11Install_Platform* _this, errStr = Pk11Install_PlatformName_Generate(&_this->equivName, subval->string); if (errStr) { - tmp = PR_smprintf("%s: %s", - Pk11Install_PlatformName_GetString(&_this->name), errStr); tmp = PR_smprintf("%s: %s", Pk11Install_PlatformName_GetString(&_this->name), errStr); PR_smprintf_free(errStr); diff --git a/security/nss/gtests/pk11_gtest/pk11_chacha20poly1305_unittest.cc b/security/nss/gtests/pk11_gtest/pk11_chacha20poly1305_unittest.cc index 882f1f0d2e..1bcf175c89 100644 --- a/security/nss/gtests/pk11_gtest/pk11_chacha20poly1305_unittest.cc +++ b/security/nss/gtests/pk11_gtest/pk11_chacha20poly1305_unittest.cc @@ -261,13 +261,16 @@ TEST_F(Pkcs11ChaCha20Poly1305Test, GenerateXor) { ScopedPK11SymKey key(PK11_KeyGen(slot.get(), kMech, nullptr, 32, nullptr)); EXPECT_TRUE(!!key); - SECItem ctrNonceItem = {siBuffer, toUcharPtr(kCtrNonce), - static_cast(sizeof(kCtrNonce))}; + std::vector iv(16); + SECStatus rv = PK11_GenerateRandomOnSlot(slot.get(), iv.data(), iv.size()); + EXPECT_EQ(SECSuccess, rv); + + SECItem ctrNonceItem = {siBuffer, toUcharPtr(iv.data()), + static_cast(iv.size())}; uint8_t encrypted[sizeof(kData)]; unsigned int encrypted_len = 88; // This should be overwritten. - SECStatus rv = - PK11_Encrypt(key.get(), kMechXor, &ctrNonceItem, encrypted, - &encrypted_len, sizeof(encrypted), kData, sizeof(kData)); + rv = PK11_Encrypt(key.get(), kMechXor, &ctrNonceItem, encrypted, + &encrypted_len, sizeof(encrypted), kData, sizeof(kData)); ASSERT_EQ(SECSuccess, rv); ASSERT_EQ(sizeof(kData), static_cast(encrypted_len)); } diff --git a/security/nss/gtests/ssl_gtest/ssl_hrr_unittest.cc b/security/nss/gtests/ssl_gtest/ssl_hrr_unittest.cc index 27bc036547..56b7a2bf0c 100644 --- a/security/nss/gtests/ssl_gtest/ssl_hrr_unittest.cc +++ b/security/nss/gtests/ssl_gtest/ssl_hrr_unittest.cc @@ -1055,6 +1055,41 @@ TEST_F(TlsConnectTest, Select12AfterHelloRetryRequest) { EXPECT_EQ(SSL_ERROR_RX_MALFORMED_SERVER_HELLO, client_->error_code()); } +// This class increments the low byte of the first Handshake.message_seq +// field in every handshake record. +class MessageSeqIncrementer : public TlsRecordFilter { + public: + MessageSeqIncrementer(const std::shared_ptr& a) + : TlsRecordFilter(a) {} + + protected: + PacketFilter::Action FilterRecord(const TlsRecordHeader& header, + const DataBuffer& data, + DataBuffer* changed) override { + if (header.content_type() != ssl_ct_handshake) { + return KEEP; + } + + *changed = data; + // struct { uint8 msg_type; uint24 length; uint16 message_seq; ... } + // Handshake; + changed->data()[5]++; + EXPECT_NE(0, changed->data()[5]); // Check for overflow. + return CHANGE; + } +}; + +// A server that receives a ClientHello with message_seq == 1 +// assumes that this is after a stateless HelloRetryRequest. +// However, it should reject the ClientHello if it lacks a cookie. +TEST_F(TlsConnectDatagram13, MessageSeq1ClientHello) { + EnsureTlsSetup(); + MakeTlsFilter(client_); + ConnectExpectAlert(server_, kTlsAlertMissingExtension); + EXPECT_EQ(SSL_ERROR_MISSING_COOKIE_EXTENSION, server_->error_code()); + EXPECT_EQ(SSL_ERROR_MISSING_EXTENSION_ALERT, client_->error_code()); +} + class HelloRetryRequestAgentTest : public TlsAgentTestClient { protected: void SetUp() override { @@ -1178,6 +1213,114 @@ TEST_P(TlsConnectStreamPre13, HrrRandomOnTls10) { server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); } +TEST_F(TlsConnectStreamTls13, HrrThenTls12) { + StartConnect(); + size_t cb_called = 0; + EXPECT_EQ(SECSuccess, SSL_HelloRetryRequestCallback(server_->ssl_fd(), + RetryHello, &cb_called)); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2, + SSL_LIBRARY_VERSION_TLS_1_3); + + client_->Handshake(); // Send CH (1.3) + server_->Handshake(); // Send HRR. + EXPECT_EQ(1U, cb_called); + + // Replace the client with a new TLS 1.2 client. Don't call Init(), since + // it will artifically limit the server's vrange. + client_.reset( + new TlsAgent(client_->name(), TlsAgent::CLIENT, ssl_variant_stream)); + client_->SetPeer(server_); + server_->SetPeer(client_); + client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2, + SSL_LIBRARY_VERSION_TLS_1_2); + + client_->StartConnect(); + client_->Handshake(); // Send CH (1.2) + ExpectAlert(server_, kTlsAlertProtocolVersion); + server_->Handshake(); + server_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION); + client_->Handshake(); + client_->CheckErrorCode(SSL_ERROR_PROTOCOL_VERSION_ALERT); +} + +TEST_F(TlsConnectStreamTls13, ZeroRttHrrThenTls12) { + SetupForZeroRtt(); + + client_->Set0RttEnabled(true); + size_t cb_called = 0; + EXPECT_EQ(SECSuccess, SSL_HelloRetryRequestCallback(server_->ssl_fd(), + RetryHello, &cb_called)); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2, + SSL_LIBRARY_VERSION_TLS_1_3); + + client_->Handshake(); // Send CH (1.3) + ZeroRttSendReceive(true, false); + server_->Handshake(); // Send HRR. + EXPECT_EQ(1U, cb_called); + + // Replace the client with a new TLS 1.2 client. Don't call Init(), since + // it will artifically limit the server's vrange. + client_.reset( + new TlsAgent(client_->name(), TlsAgent::CLIENT, ssl_variant_stream)); + client_->SetPeer(server_); + server_->SetPeer(client_); + client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2, + SSL_LIBRARY_VERSION_TLS_1_2); + + client_->StartConnect(); + client_->Handshake(); // Send CH (1.2) + ExpectAlert(server_, kTlsAlertProtocolVersion); + server_->Handshake(); + server_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION); + client_->Handshake(); + client_->CheckErrorCode(SSL_ERROR_PROTOCOL_VERSION_ALERT); + + // Try to write something + server_->Handshake(); + client_->ExpectReadWriteError(); + client_->SendData(1); + uint8_t buf[1]; + EXPECT_EQ(-1, PR_Read(server_->ssl_fd(), buf, sizeof(buf))); + EXPECT_EQ(SSL_ERROR_HANDSHAKE_FAILED, PR_GetError()); +} + +TEST_F(TlsConnectStreamTls13, HrrThenTls12SupportedVersions) { + SetupForZeroRtt(); + client_->Set0RttEnabled(true); + size_t cb_called = 0; + EXPECT_EQ(SECSuccess, SSL_HelloRetryRequestCallback(server_->ssl_fd(), + RetryHello, &cb_called)); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2, + SSL_LIBRARY_VERSION_TLS_1_3); + + client_->Handshake(); // Send CH (1.3) + ZeroRttSendReceive(true, false); + server_->Handshake(); // Send HRR. + EXPECT_EQ(1U, cb_called); + + // Replace the client with a new TLS 1.2 client. Don't call Init(), since + // it will artifically limit the server's vrange. + client_.reset( + new TlsAgent(client_->name(), TlsAgent::CLIENT, ssl_variant_stream)); + client_->SetPeer(server_); + server_->SetPeer(client_); + client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1, + SSL_LIBRARY_VERSION_TLS_1_2); + // Negotiate via supported_versions + static const uint8_t tls12[] = {0x02, 0x03, 0x03}; + auto replacer = MakeTlsFilter( + client_, ssl_tls13_supported_versions_xtn, + DataBuffer(tls12, sizeof(tls12))); + + client_->StartConnect(); + client_->Handshake(); // Send CH (1.2) + ExpectAlert(server_, kTlsAlertProtocolVersion); + server_->Handshake(); + server_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION); + client_->Handshake(); + client_->CheckErrorCode(SSL_ERROR_PROTOCOL_VERSION_ALERT); +} + INSTANTIATE_TEST_CASE_P(HelloRetryRequestAgentTests, HelloRetryRequestAgentTest, ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll, TlsConnectTestBase::kTlsV13)); diff --git a/security/nss/gtests/ssl_gtest/ssl_version_unittest.cc b/security/nss/gtests/ssl_gtest/ssl_version_unittest.cc index 3255bd512c..699edc64f5 100644 --- a/security/nss/gtests/ssl_gtest/ssl_version_unittest.cc +++ b/security/nss/gtests/ssl_gtest/ssl_version_unittest.cc @@ -102,6 +102,61 @@ TEST_F(TlsConnectTest, TestDisableDowngradeDetection) { server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE); } +typedef std::tuple // server version + TlsDowngradeProfile; + +class TlsDowngradeTest + : public TlsConnectTestBase, + public ::testing::WithParamInterface { + public: + TlsDowngradeTest() + : TlsConnectTestBase(std::get<0>(GetParam()), std::get<1>(GetParam())), + c_ver(std::get<1>(GetParam())), + s_ver(std::get<2>(GetParam())) {} + + protected: + const uint16_t c_ver; + const uint16_t s_ver; +}; + +TEST_P(TlsDowngradeTest, TlsDowngradeSentinelTest) { + static const uint8_t tls12_downgrade_random[] = {0x44, 0x4F, 0x57, 0x4E, + 0x47, 0x52, 0x44, 0x01}; + static const uint8_t tls1_downgrade_random[] = {0x44, 0x4F, 0x57, 0x4E, + 0x47, 0x52, 0x44, 0x00}; + static const size_t kRandomLen = 32; + + if (c_ver > s_ver) { + return; + } + + client_->SetVersionRange(c_ver, c_ver); + server_->SetVersionRange(c_ver, s_ver); + + auto sh = MakeTlsFilter(server_, ssl_hs_server_hello); + Connect(); + ASSERT_TRUE(sh->buffer().len() > (kRandomLen + 2)); + + const uint8_t* downgrade_sentinel = + sh->buffer().data() + 2 + kRandomLen - sizeof(tls1_downgrade_random); + if (c_ver < s_ver) { + if (c_ver == SSL_LIBRARY_VERSION_TLS_1_2) { + EXPECT_EQ(0, memcmp(downgrade_sentinel, tls12_downgrade_random, + sizeof(tls12_downgrade_random))); + } else { + EXPECT_EQ(0, memcmp(downgrade_sentinel, tls1_downgrade_random, + sizeof(tls1_downgrade_random))); + } + } else { + EXPECT_NE(0, memcmp(downgrade_sentinel, tls12_downgrade_random, + sizeof(tls12_downgrade_random))); + EXPECT_NE(0, memcmp(downgrade_sentinel, tls1_downgrade_random, + sizeof(tls1_downgrade_random))); + } +} + // TLS 1.1 clients do not check the random values, so we should // instead get a handshake failure alert from the server. TEST_F(TlsConnectTest, TestDowngradeDetectionToTls10) { @@ -280,4 +335,10 @@ TEST_F(TlsConnectStreamTls13, Ssl30ClientHelloWithSupportedVersions) { ConnectExpectAlert(server_, kTlsAlertProtocolVersion); } +INSTANTIATE_TEST_CASE_P( + TlsDowngradeSentinelTest, TlsDowngradeTest, + ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream, + TlsConnectTestBase::kTlsVAll, + TlsConnectTestBase::kTlsV12Plus)); + } // namespace nss_test diff --git a/security/nss/gtests/ssl_gtest/tls_esni_unittest.cc b/security/nss/gtests/ssl_gtest/tls_esni_unittest.cc index 26275e0bc1..0a02d06836 100644 --- a/security/nss/gtests/ssl_gtest/tls_esni_unittest.cc +++ b/security/nss/gtests/ssl_gtest/tls_esni_unittest.cc @@ -314,12 +314,20 @@ TEST_P(TlsConnectTls13, ConnectEsniHrr) { MakeTlsFilter(client_, ssl_server_name_xtn); auto filter2 = MakeTlsFilter(client_, ssl_server_name_xtn, true); + auto efilter = + MakeTlsFilter(client_, ssl_tls13_encrypted_sni_xtn); + auto efilter2 = MakeTlsFilter( + client_, ssl_tls13_encrypted_sni_xtn, true); + client_->SetFilter(std::make_shared( - ChainedPacketFilterInit({filter, filter2}))); + ChainedPacketFilterInit({filter, filter2, efilter, efilter2}))); server_->SetSniCallback(SniCallback); Connect(); CheckSniExtension(filter->extension()); CheckSniExtension(filter2->extension()); + ASSERT_TRUE(efilter->captured()); + ASSERT_TRUE(efilter2->captured()); + ASSERT_NE(efilter->extension(), efilter2->extension()); EXPECT_NE(0UL, hrr_capture->buffer().len()); } diff --git a/security/nss/lib/pki/pki3hack.c b/security/nss/lib/pki/pki3hack.c index 29d2fb5a40..eac4a5705e 100644 --- a/security/nss/lib/pki/pki3hack.c +++ b/security/nss/lib/pki/pki3hack.c @@ -921,14 +921,28 @@ stan_GetCERTCertificate(NSSCertificate *c, PRBool forceUpdate) } if (!cc->nssCertificate || forceUpdate) { fill_CERTCertificateFields(c, cc, forceUpdate); - } else if (CERT_GetCertTrust(cc, &certTrust) != SECSuccess && - !c->object.cryptoContext) { - /* if it's a perm cert, it might have been stored before the - * trust, so look for the trust again. But a temp cert can be - * ignored. - */ - CERTCertTrust *trust = NULL; - trust = nssTrust_GetCERTCertTrustForCert(c, cc); + } else if (CERT_GetCertTrust(cc, &certTrust) != SECSuccess) { + CERTCertTrust *trust; + if (!c->object.cryptoContext) { + /* If it's a perm cert, it might have been stored before the + * trust, so look for the trust again. + */ + trust = nssTrust_GetCERTCertTrustForCert(c, cc); + } else { + /* If it's a temp cert, it might have been stored before the + * builtin trust module is loaded, so look for the trust + * again, but don't set the empty trust if it is not found. + */ + NSSTrust *t = nssTrustDomain_FindTrustForCertificate(c->object.cryptoContext->td, c); + if (!t) { + goto loser; + } + trust = cert_trust_from_stan_trust(t, cc->arena); + nssTrust_Destroy(t); + if (!trust) { + goto loser; + } + } CERT_LockCertTrust(cc); cc->trust = trust; diff --git a/security/nss/lib/softoken/pkcs11c.c b/security/nss/lib/softoken/pkcs11c.c index 6bc5cc9391..a9cf0cd9cf 100644 --- a/security/nss/lib/softoken/pkcs11c.c +++ b/security/nss/lib/softoken/pkcs11c.c @@ -1238,7 +1238,7 @@ sftk_CryptInit(CK_SESSION_HANDLE hSession, CK_MECHANISM_PTR pMechanism, PRUint8 *param = pMechanism->pParameter; int i = 0; for (; i < 4; ++i) { - ctx->counter |= param[i] << (i * 8); + ctx->counter |= (PRUint32)param[i] << (i * 8); } memcpy(ctx->nonce, param + 4, 12); context->cipherInfo = ctx; diff --git a/security/nss/lib/ssl/dtlscon.c b/security/nss/lib/ssl/dtlscon.c index bbd2f6d79b..9417063f12 100644 --- a/security/nss/lib/ssl/dtlscon.c +++ b/security/nss/lib/ssl/dtlscon.c @@ -343,6 +343,7 @@ dtls_HandleHandshake(sslSocket *ss, DTLSEpoch epoch, sslSequenceNumber seqNum, SSL_TRC(5, ("%d: DTLS[%d]: Received apparent 2nd ClientHello", SSL_GETPID(), ss->fd)); ss->ssl3.hs.recvMessageSeq = 1; + ss->ssl3.hs.helloRetry = PR_TRUE; } /* There are three ways we could not be ready for this packet. diff --git a/security/nss/lib/ssl/ssl3con.c b/security/nss/lib/ssl/ssl3con.c index f3c723bbc2..60b247fd7c 100644 --- a/security/nss/lib/ssl/ssl3con.c +++ b/security/nss/lib/ssl/ssl3con.c @@ -394,12 +394,12 @@ static const SSLCipher2Mech alg2Mech[] = { { ssl_calg_chacha20, CKM_NSS_CHACHA20_POLY1305 }, }; -const PRUint8 tls13_downgrade_random[] = { 0x44, 0x4F, 0x57, 0x4E, - 0x47, 0x52, 0x44, 0x01 }; const PRUint8 tls12_downgrade_random[] = { 0x44, 0x4F, 0x57, 0x4E, - 0x47, 0x52, 0x44, 0x00 }; -PR_STATIC_ASSERT(sizeof(tls13_downgrade_random) == - sizeof(tls13_downgrade_random)); + 0x47, 0x52, 0x44, 0x01 }; +const PRUint8 tls1_downgrade_random[] = { 0x44, 0x4F, 0x57, 0x4E, + 0x47, 0x52, 0x44, 0x00 }; +PR_STATIC_ASSERT(sizeof(tls12_downgrade_random) == + sizeof(tls1_downgrade_random)); /* The ECCWrappedKeyInfo structure defines how various pieces of * information are laid out within wrappedSymmetricWrappingkey @@ -6713,13 +6713,13 @@ ssl_CheckServerRandom(sslSocket *ss) /* Both sections use the same sentinel region. */ PRUint8 *downgrade_sentinel = ss->ssl3.hs.server_random + - SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random); + SSL3_RANDOM_LENGTH - sizeof(tls12_downgrade_random); if (!PORT_Memcmp(downgrade_sentinel, - tls13_downgrade_random, - sizeof(tls13_downgrade_random)) || - !PORT_Memcmp(downgrade_sentinel, tls12_downgrade_random, - sizeof(tls12_downgrade_random))) { + sizeof(tls12_downgrade_random)) || + !PORT_Memcmp(downgrade_sentinel, + tls1_downgrade_random, + sizeof(tls1_downgrade_random))) { return SECFailure; } } @@ -8491,20 +8491,24 @@ ssl_GenerateServerRandom(sslSocket *ss) */ PRUint8 *downgradeSentinel = ss->ssl3.hs.server_random + - SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random); + SSL3_RANDOM_LENGTH - sizeof(tls12_downgrade_random); - switch (ss->vrange.max) { - case SSL_LIBRARY_VERSION_TLS_1_3: - PORT_Memcpy(downgradeSentinel, - tls13_downgrade_random, sizeof(tls13_downgrade_random)); - break; - case SSL_LIBRARY_VERSION_TLS_1_2: - PORT_Memcpy(downgradeSentinel, - tls12_downgrade_random, sizeof(tls12_downgrade_random)); - break; - default: - /* Do not change random. */ - break; + if (ss->vrange.max >= SSL_LIBRARY_VERSION_TLS_1_2) { + switch (ss->version) { + case SSL_LIBRARY_VERSION_TLS_1_2: + /* vrange.max > 1.2, since we didn't early exit above. */ + PORT_Memcpy(downgradeSentinel, + tls12_downgrade_random, sizeof(tls12_downgrade_random)); + break; + case SSL_LIBRARY_VERSION_TLS_1_1: + case SSL_LIBRARY_VERSION_TLS_1_0: + PORT_Memcpy(downgradeSentinel, + tls1_downgrade_random, sizeof(tls1_downgrade_random)); + break; + default: + /* Do not change random. */ + break; + } } return SECSuccess; diff --git a/security/nss/lib/ssl/tls13subcerts.c b/security/nss/lib/ssl/tls13subcerts.c index 8ae5447f7a..0fca9a7c5e 100644 --- a/security/nss/lib/ssl/tls13subcerts.c +++ b/security/nss/lib/ssl/tls13subcerts.c @@ -7,6 +7,7 @@ #include "nss.h" #include "pk11func.h" #include "secder.h" +#include "sechash.h" #include "ssl.h" #include "sslproto.h" #include "sslimpl.h" @@ -538,6 +539,15 @@ tls13_MakePssSpki(const SECKEYPublicKey *pub, SECOidTag hashOid) goto loser; /* Code already set. */ } + /* Always include saltLength: all hashes are larger than 20. */ + unsigned int saltLength = HASH_ResultLenByOidTag(hashOid); + PORT_Assert(saltLength > 20); + if (!SEC_ASN1EncodeInteger(arena, ¶ms.saltLength, saltLength)) { + PORT_SetError(SEC_ERROR_LIBRARY_FAILURE); + goto loser; + } + /* Omit the trailerField always. */ + SECItem *algorithmItem = SEC_ASN1EncodeItem(arena, NULL, ¶ms, SEC_ASN1_GET(SECKEY_RSAPSSParamsTemplate)); @@ -752,6 +762,8 @@ SSLExp_DelegateCredential(const CERTCertificate *cert, goto loser; } + PRINT_BUF(20, (NULL, "delegated credential", dcBuf.buf, dcBuf.len)); + SECKEY_DestroySubjectPublicKeyInfo(spki); SECKEY_DestroyPrivateKey(tmpPriv); tls13_DestroyDelegatedCredential(dc);