From 66170e371657c1a41bbe974e42a71db5df2f3929 Mon Sep 17 00:00:00 2001 From: "J.C. Jones" Date: Thu, 15 Aug 2019 16:06:15 +0000 Subject: [PATCH] Bug 1564499 - land NSS bbfc55939d75 UPGRADE_NSS_RELEASE, r=kjacobs Revset: reverse(89aa19677e37~-1::bbfc55939d75) 2019-08-14 Kevin Jacobs * gtests/ssl_gtest/tls_agent.cc: Bug 1572593 - Re-revert call to CheckCertReqAgainstDefaultCAs to avoid memory leak (filed as bug 1573945). r=jcj Revert back to the changes Franziskus had made. Updated the in- source bug number to point to the new memleak bug. Differential Revision: https://phabricator.services.mozilla.com/D42020 [bbfc55939d75] [tip] 2019-08-12 Kevin Jacobs * gtests/freebl_gtest/freebl_gtest.gyp, gtests/mozpkix_gtest/mozpkix_gtest.gyp: Bug 1415118 - Fix --enable-libpkix builds from build.sh r=mt,jcj Differential Revision: https://phabricator.services.mozilla.com/D41617 [f8926908be71] 2019-08-14 J.C. Jones * gtests/ssl_gtest/tls_agent.cc, lib/ssl/ssl3ext.c: Bug 1572593 - Reset advertised extensions in ssl_ConstructExtensions r=mt,kjacobs Reset the list of advertised extensions before sending a new set. This reverts the changes of https://hg.mozilla.org/projects/nss/rev/ 1ca362213631d6edc885b6b965b52ecffcf29afd Differential Revision: https://phabricator.services.mozilla.com/D41302 [b03ff661491e] 2019-08-14 Kevin Jacobs * lib/freebl/ctr.c: Bug 1539788 - UBSAN fixup for 128b counter. r=mt,jcj Differential Revision: https://phabricator.services.mozilla.com/D41884 [9d1f5e71773d] 2019-08-13 Kevin Jacobs * lib/freebl/chacha20poly1305.c, lib/freebl/ctr.c, lib/freebl/gcm.c, lib/freebl/intel-gcm-wrap.c, lib/freebl/rsapkcs.c: Bug 1539788 - Add length checks for cryptographic primitives r=mt,jcj This patch adds additional length checks around cryptographic primitives. Differential Revision: https://phabricator.services.mozilla.com/D36079 [dfd6996fe742] 2019-08-13 Marcus Burghardt * gtests/freebl_gtest/mpi_unittest.cc, lib/freebl/mpi/README, lib/freebl/mpi/mpi.c, lib/freebl/mpi/mpi.h: Bug 1542077 - Added extra controls and tests to mp_set_int and mp_set_ulong. r=jcj,kjacobs Differential Revision: https://phabricator.services.mozilla.com/D40649 [9bc47e69613e] 2019-08-13 J.C. Jones * gtests/ssl_gtest/ssl_resumption_unittest.cc, gtests/ssl_gtest/tls_agent.cc: Bug 1572791 - Fixup clang-format r=bustage [ec113de50cdd] * gtests/ssl_gtest/tls_agent.cc, gtests/ssl_gtest/tls_subcerts_unittest.cc, lib/ssl/tls13subcerts.c: Bug 1572791 - Check for nulls in SSLExp_DelegateCredential and its tests r=kjacobs This particularly catches test errors in tls_subcerts_unittest when the profile is stale. Differential Revision: https://phabricator.services.mozilla.com/D41429 [ed5067857563] 2019-08-13 Kevin Jacobs * gtests/ssl_gtest/ssl_auth_unittest.cc, gtests/ssl_gtest/ssl_cert_ext_unittest.cc, gtests/ssl_gtest/ssl_resumption_unittest.cc, gtests/ssl_gtest/tls_agent.cc: Bug 1572791 - Fix ASAN cert errors when SSL gtests run on empty profile r=jcj Differential Revision: https://phabricator.services.mozilla.com/D41787 [cef2aa7f3b8c] 2019-08-09 Kevin Jacobs * tests/common/cleanup.sh: Bug 1560593 - Cleanup.sh to treat core dumps as test failures on optimized builds. r=jcj Differential Revision: https://phabricator.services.mozilla.com/D41392 [360010725fdb] Differential Revision: https://phabricator.services.mozilla.com/D42139 --HG-- extra : moz-landing-system : lando --- security/nss/TAG-INFO | 2 +- security/nss/coreconf/coreconf.dep | 1 - .../nss/gtests/freebl_gtest/freebl_gtest.gyp | 1 + .../nss/gtests/freebl_gtest/mpi_unittest.cc | 2 +- .../gtests/mozpkix_gtest/mozpkix_gtest.gyp | 1 + .../nss/gtests/ssl_gtest/ssl_auth_unittest.cc | 14 +++++++ .../gtests/ssl_gtest/ssl_cert_ext_unittest.cc | 4 +- .../ssl_gtest/ssl_resumption_unittest.cc | 16 +++++-- security/nss/gtests/ssl_gtest/tls_agent.cc | 11 ++++- .../gtests/ssl_gtest/tls_subcerts_unittest.cc | 1 + security/nss/lib/freebl/chacha20poly1305.c | 5 +++ security/nss/lib/freebl/ctr.c | 12 ++++++ security/nss/lib/freebl/gcm.c | 6 +++ security/nss/lib/freebl/intel-gcm-wrap.c | 22 ++++++++++ security/nss/lib/freebl/mpi/README | 1 + security/nss/lib/freebl/mpi/mpi.c | 42 +++++++++++++------ security/nss/lib/freebl/mpi/mpi.h | 11 ++++- security/nss/lib/freebl/rsapkcs.c | 23 +++++----- security/nss/lib/ssl/ssl3ext.c | 3 ++ security/nss/lib/ssl/tls13subcerts.c | 5 +++ security/nss/tests/common/cleanup.sh | 11 ++++- 21 files changed, 159 insertions(+), 35 deletions(-) diff --git a/security/nss/TAG-INFO b/security/nss/TAG-INFO index 04547d76e176..f5f3e347b7ec 100644 --- a/security/nss/TAG-INFO +++ b/security/nss/TAG-INFO @@ -1 +1 @@ -89aa19677e37 +bbfc55939d75 diff --git a/security/nss/coreconf/coreconf.dep b/security/nss/coreconf/coreconf.dep index 590d1bfaeee3..5182f75552c8 100644 --- a/security/nss/coreconf/coreconf.dep +++ b/security/nss/coreconf/coreconf.dep @@ -10,4 +10,3 @@ */ #error "Do not include this header file." - diff --git a/security/nss/gtests/freebl_gtest/freebl_gtest.gyp b/security/nss/gtests/freebl_gtest/freebl_gtest.gyp index 238f05e07355..033f891c2bd4 100644 --- a/security/nss/gtests/freebl_gtest/freebl_gtest.gyp +++ b/security/nss/gtests/freebl_gtest/freebl_gtest.gyp @@ -23,6 +23,7 @@ '<(DEPTH)/lib/dev/dev.gyp:nssdev', '<(DEPTH)/lib/pki/pki.gyp:nsspki', '<(DEPTH)/lib/ssl/ssl.gyp:ssl', + '<(DEPTH)/lib/libpkix/libpkix.gyp:libpkix', ], }, { diff --git a/security/nss/gtests/freebl_gtest/mpi_unittest.cc b/security/nss/gtests/freebl_gtest/mpi_unittest.cc index fbc27d056dca..56b7454dc96e 100644 --- a/security/nss/gtests/freebl_gtest/mpi_unittest.cc +++ b/security/nss/gtests/freebl_gtest/mpi_unittest.cc @@ -290,4 +290,4 @@ TEST_F(DISABLED_MPITest, MpiCmpConstTest) { mp_clear(&c); } -} // nss_test +} // namespace nss_test diff --git a/security/nss/gtests/mozpkix_gtest/mozpkix_gtest.gyp b/security/nss/gtests/mozpkix_gtest/mozpkix_gtest.gyp index 899b849fcb57..1623d76bb5bc 100644 --- a/security/nss/gtests/mozpkix_gtest/mozpkix_gtest.gyp +++ b/security/nss/gtests/mozpkix_gtest/mozpkix_gtest.gyp @@ -43,6 +43,7 @@ '<(DEPTH)/lib/base/base.gyp:nssb', '<(DEPTH)/lib/dev/dev.gyp:nssdev', '<(DEPTH)/lib/pki/pki.gyp:nsspki', + '<(DEPTH)/lib/libpkix/libpkix.gyp:libpkix', '<(DEPTH)/lib/mozpkix/mozpkix.gyp:mozpkix', '<(DEPTH)/lib/mozpkix/mozpkix.gyp:mozpkix-testlib', ], diff --git a/security/nss/gtests/ssl_gtest/ssl_auth_unittest.cc b/security/nss/gtests/ssl_gtest/ssl_auth_unittest.cc index 340e98745f70..8c28bd159009 100644 --- a/security/nss/gtests/ssl_gtest/ssl_auth_unittest.cc +++ b/security/nss/gtests/ssl_gtest/ssl_auth_unittest.cc @@ -247,7 +247,9 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuth) { capture_certificate->buffer().data(), capture_cert_req->buffer().len())); ScopedCERTCertificate cert1(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); ScopedCERTCertificate cert2(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); } @@ -289,7 +291,9 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuthMultiple) { server_->ReadBytes(50); EXPECT_EQ(1U, called); ScopedCERTCertificate cert1(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); ScopedCERTCertificate cert2(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); // Send 2nd CertificateRequest. EXPECT_EQ(SECSuccess, SSL_GetClientAuthDataHook( @@ -302,7 +306,9 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuthMultiple) { server_->ReadBytes(50); EXPECT_EQ(2U, called); ScopedCERTCertificate cert3(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert3.get()); ScopedCERTCertificate cert4(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert4.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert3->derCert, &cert4->derCert)); EXPECT_FALSE(SECITEM_ItemsAreEqual(&cert3->derCert, &cert1->derCert)); } @@ -383,7 +389,9 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuthAfterClientAuth) { Connect(); EXPECT_EQ(1U, called); ScopedCERTCertificate cert1(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); ScopedCERTCertificate cert2(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); // Send CertificateRequest. EXPECT_EQ(SECSuccess, SSL_GetClientAuthDataHook( @@ -396,7 +404,9 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuthAfterClientAuth) { server_->ReadBytes(50); EXPECT_EQ(2U, called); ScopedCERTCertificate cert3(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert3.get()); ScopedCERTCertificate cert4(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert4.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert3->derCert, &cert4->derCert)); EXPECT_FALSE(SECITEM_ItemsAreEqual(&cert3->derCert, &cert1->derCert)); } @@ -567,7 +577,9 @@ TEST_F(TlsConnectStreamTls13, PostHandshakeAuthWithSessionTicketsEnabled) { server_->ReadBytes(50); EXPECT_EQ(1U, called); ScopedCERTCertificate cert1(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); ScopedCERTCertificate cert2(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); } @@ -614,7 +626,9 @@ static void CheckSigScheme(std::shared_ptr& capture, EXPECT_EQ(expected_scheme, static_cast(scheme)); ScopedCERTCertificate remote_cert(SSL_PeerCertificate(peer->ssl_fd())); + ASSERT_NE(nullptr, remote_cert.get()); ScopedSECKEYPublicKey remote_key(CERT_ExtractPublicKey(remote_cert.get())); + ASSERT_NE(nullptr, remote_key.get()); EXPECT_EQ(expected_size, SECKEY_PublicKeyStrengthInBits(remote_key.get())); } diff --git a/security/nss/gtests/ssl_gtest/ssl_cert_ext_unittest.cc b/security/nss/gtests/ssl_gtest/ssl_cert_ext_unittest.cc index f725faa4c11b..26e5fb5028d8 100644 --- a/security/nss/gtests/ssl_gtest/ssl_cert_ext_unittest.cc +++ b/security/nss/gtests/ssl_gtest/ssl_cert_ext_unittest.cc @@ -43,10 +43,10 @@ class SignedCertificateTimestampsExtractor { } void assertTimestamps(const DataBuffer& timestamps) { - EXPECT_TRUE(auth_timestamps_); + ASSERT_NE(nullptr, auth_timestamps_); EXPECT_EQ(timestamps, *auth_timestamps_); - EXPECT_TRUE(handshake_timestamps_); + ASSERT_NE(nullptr, handshake_timestamps_); EXPECT_EQ(timestamps, *handshake_timestamps_); const SECItem* current = diff --git a/security/nss/gtests/ssl_gtest/ssl_resumption_unittest.cc b/security/nss/gtests/ssl_gtest/ssl_resumption_unittest.cc index 22a90f3b2915..e48b9fa09373 100644 --- a/security/nss/gtests/ssl_gtest/ssl_resumption_unittest.cc +++ b/security/nss/gtests/ssl_gtest/ssl_resumption_unittest.cc @@ -423,6 +423,7 @@ static int32_t SwitchCertificates(TlsAgent* agent, const SECItem* srvNameArr, TEST_P(TlsConnectGeneric, ServerSNICertSwitch) { Connect(); ScopedCERTCertificate cert1(SSL_PeerCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); Reset(); ConfigureSessionCache(RESUME_NONE, RESUME_NONE); @@ -431,6 +432,7 @@ TEST_P(TlsConnectGeneric, ServerSNICertSwitch) { Connect(); ScopedCERTCertificate cert2(SSL_PeerCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); CheckKeys(); EXPECT_FALSE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); } @@ -439,6 +441,7 @@ TEST_P(TlsConnectGeneric, ServerSNICertTypeSwitch) { Reset(TlsAgent::kServerEcdsa256); Connect(); ScopedCERTCertificate cert1(SSL_PeerCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); Reset(); ConfigureSessionCache(RESUME_NONE, RESUME_NONE); @@ -449,6 +452,7 @@ TEST_P(TlsConnectGeneric, ServerSNICertTypeSwitch) { Connect(); ScopedCERTCertificate cert2(SSL_PeerCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); CheckKeys(ssl_kea_ecdh, ssl_auth_ecdsa); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); } @@ -533,6 +537,7 @@ TEST_P(TlsConnectTls13, TestTls13ResumeNoCertificateRequest) { Connect(); SendReceive(); // Need to read so that we absorb the session ticket. ScopedCERTCertificate cert1(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); Reset(); ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); @@ -548,6 +553,7 @@ TEST_P(TlsConnectTls13, TestTls13ResumeNoCertificateRequest) { // Sanity check whether the client certificate matches the one // decrypted from ticket. ScopedCERTCertificate cert2(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); } @@ -563,6 +569,7 @@ TEST_P(TlsConnectTls13, WriteBeforeHandshakeCompleteOnResumption) { Connect(); SendReceive(); // Absorb the session ticket. ScopedCERTCertificate cert1(SSL_LocalCertificate(client_->ssl_fd())); + ASSERT_NE(nullptr, cert1.get()); Reset(); ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); @@ -579,6 +586,7 @@ TEST_P(TlsConnectTls13, WriteBeforeHandshakeCompleteOnResumption) { // Check whether the client certificate matches the one from the ticket. ScopedCERTCertificate cert2(SSL_PeerCertificate(server_->ssl_fd())); + ASSERT_NE(nullptr, cert2.get()); EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); } @@ -759,7 +767,7 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) { ASSERT_LT(0U, initialTicket.len()); ScopedCERTCertificate cert1(SSL_PeerCertificate(client_->ssl_fd())); - ASSERT_TRUE(!!cert1.get()); + ASSERT_NE(nullptr, cert1.get()); Reset(); ClearStats(); @@ -775,7 +783,7 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) { ASSERT_LT(0U, c2->extension().len()); ScopedCERTCertificate cert2(SSL_PeerCertificate(client_->ssl_fd())); - ASSERT_TRUE(!!cert2.get()); + ASSERT_NE(nullptr, cert2.get()); // Check that the cipher suite is reported the same on both sides, though in // TLS 1.3 resumption actually negotiates a different cipher suite. @@ -1176,6 +1184,7 @@ TEST_P(TlsConnectGenericResumptionToken, ConnectResumeGetInfo) { client_->GetTokenInfo(token); ScopedCERTCertificate cert( PK11_FindCertFromNickname(server_->name().c_str(), nullptr)); + ASSERT_NE(nullptr, cert.get()); CheckGetInfoResult(now(), 0, 0, cert, token); @@ -1256,6 +1265,7 @@ TEST_P(TlsConnectGenericResumptionToken, ConnectResumeGetInfoAlpn) { client_->GetTokenInfo(token); ScopedCERTCertificate cert( PK11_FindCertFromNickname(server_->name().c_str(), nullptr)); + ASSERT_NE(nullptr, cert.get()); CheckGetInfoResult(now(), 1, 0, cert, token); @@ -1291,7 +1301,7 @@ TEST_P(TlsConnectTls13ResumptionToken, ConnectResumeGetInfoZeroRtt) { client_->GetTokenInfo(token); ScopedCERTCertificate cert( PK11_FindCertFromNickname(server_->name().c_str(), nullptr)); - + ASSERT_NE(nullptr, cert.get()); CheckGetInfoResult(now(), 1, 1024, cert, token); ZeroRttSendReceive(true, true); diff --git a/security/nss/gtests/ssl_gtest/tls_agent.cc b/security/nss/gtests/ssl_gtest/tls_agent.cc index d60140136825..8fe58f328f23 100644 --- a/security/nss/gtests/ssl_gtest/tls_agent.cc +++ b/security/nss/gtests/ssl_gtest/tls_agent.cc @@ -128,10 +128,14 @@ void TlsAgent::SetState(State s) { ScopedCERTCertificate* cert, ScopedSECKEYPrivateKey* priv) { cert->reset(PK11_FindCertFromNickname(name.c_str(), nullptr)); + EXPECT_NE(nullptr, cert); + if (!cert) return false; EXPECT_NE(nullptr, cert->get()); if (!cert->get()) return false; priv->reset(PK11_FindKeyByAnyCert(cert->get(), nullptr)); + EXPECT_NE(nullptr, priv); + if (!priv) return false; EXPECT_NE(nullptr, priv->get()); if (!priv->get()) return false; @@ -162,7 +166,10 @@ void TlsAgent::DelegateCredential(const std::string& name, SECItem* dc) { ScopedCERTCertificate cert; ScopedSECKEYPrivateKey cert_priv; - EXPECT_TRUE(TlsAgent::LoadCertificate(name, &cert, &cert_priv)); + EXPECT_TRUE(TlsAgent::LoadCertificate(name, &cert, &cert_priv)) + << "Could not load delegate certificate: " << name + << "; test db corrupt?"; + EXPECT_EQ(SECSuccess, SSL_DelegateCredential(cert.get(), cert_priv.get(), dc_pub.get(), dc_cert_verify_alg, dc_valid_for, now, dc)); @@ -337,7 +344,7 @@ SECStatus TlsAgent::GetClientAuthDataHook(void* self, PRFileDesc* fd, ScopedCERTCertificate peerCert(SSL_PeerCertificate(agent->ssl_fd())); EXPECT_TRUE(peerCert) << "Client should be able to see the server cert"; - // See bug 1457716 + // See bug 1573945 // CheckCertReqAgainstDefaultCAs(caNames); ScopedCERTCertificate cert; diff --git a/security/nss/gtests/ssl_gtest/tls_subcerts_unittest.cc b/security/nss/gtests/ssl_gtest/tls_subcerts_unittest.cc index 75e01018bc3d..96b5ad1a1106 100644 --- a/security/nss/gtests/ssl_gtest/tls_subcerts_unittest.cc +++ b/security/nss/gtests/ssl_gtest/tls_subcerts_unittest.cc @@ -240,6 +240,7 @@ TEST_P(TlsConnectTls13, DCAbortBadSignature) { StackSECItem dc; TlsAgent::DelegateCredential(kDelegatorId, pub, kDCScheme, kDCValidFor, now(), &dc); + ASSERT_TRUE(dc.data != nullptr); // Flip the first bit of the DC so that the signature is invalid. dc.data[0] ^= 0x01; diff --git a/security/nss/lib/freebl/chacha20poly1305.c b/security/nss/lib/freebl/chacha20poly1305.c index 9cf92f81d5db..63359a22a3f5 100644 --- a/security/nss/lib/freebl/chacha20poly1305.c +++ b/security/nss/lib/freebl/chacha20poly1305.c @@ -258,6 +258,11 @@ ChaCha20Poly1305_Open(const ChaCha20Poly1305Context *ctx, unsigned char *output, PORT_SetError(SEC_ERROR_OUTPUT_LEN); return SECFailure; } + // ChaCha has a 64 octet block, with a 32-bit block counter. + if (inputLen >= (1ULL << (6 + 32)) + ctx->tagLen) { + PORT_SetError(SEC_ERROR_INPUT_LEN); + return SECFailure; + } PORT_Memset(block, 0, sizeof(block)); // Generate a block of keystream. The first 32 bytes will be the poly1305 diff --git a/security/nss/lib/freebl/ctr.c b/security/nss/lib/freebl/ctr.c index d7652c0606db..4d26a5b06a2c 100644 --- a/security/nss/lib/freebl/ctr.c +++ b/security/nss/lib/freebl/ctr.c @@ -128,6 +128,12 @@ CTR_Update(CTRContext *ctr, unsigned char *outbuf, unsigned int tmp; SECStatus rv; + // Limit block count to 2^counterBits - 2 + if (ctr->counterBits < (sizeof(unsigned int) * 8) && + inlen > ((1 << ctr->counterBits) - 2) * AES_BLOCK_SIZE) { + PORT_SetError(SEC_ERROR_INPUT_LEN); + return SECFailure; + } if (maxout < inlen) { *outlen = inlen; PORT_SetError(SEC_ERROR_OUTPUT_LEN); @@ -199,6 +205,12 @@ CTR_Update_HW_AES(CTRContext *ctr, unsigned char *outbuf, unsigned int tmp; SECStatus rv; + // Limit block count to 2^counterBits - 2 + if (ctr->counterBits < (sizeof(unsigned int) * 8) && + inlen > ((1 << ctr->counterBits) - 2) * AES_BLOCK_SIZE) { + PORT_SetError(SEC_ERROR_INPUT_LEN); + return SECFailure; + } if (maxout < inlen) { *outlen = inlen; PORT_SetError(SEC_ERROR_OUTPUT_LEN); diff --git a/security/nss/lib/freebl/gcm.c b/security/nss/lib/freebl/gcm.c index 2d3d17ab6af3..9902d79bf66f 100644 --- a/security/nss/lib/freebl/gcm.c +++ b/security/nss/lib/freebl/gcm.c @@ -479,6 +479,12 @@ gcmHash_Reset(gcmHashContext *ghash, const unsigned char *AAD, { SECStatus rv; + // Limit AADLen in accordance with SP800-38D + if (sizeof(AADLen) >= 8 && AADLen > (1ULL << 61) - 1) { + PORT_SetError(SEC_ERROR_INPUT_LEN); + return SECFailure; + } + ghash->cLen = 0; PORT_Memset(ghash->counterBuf, 0, GCM_HASH_LEN_LEN * 2); ghash->bufLen = 0; diff --git a/security/nss/lib/freebl/intel-gcm-wrap.c b/security/nss/lib/freebl/intel-gcm-wrap.c index 7bc07a6e98af..34d4c1283283 100644 --- a/security/nss/lib/freebl/intel-gcm-wrap.c +++ b/security/nss/lib/freebl/intel-gcm-wrap.c @@ -62,6 +62,12 @@ intel_AES_GCM_CreateContext(void *context, PORT_SetError(SEC_ERROR_INVALID_ARGS); return NULL; } + // Limit AADLen in accordance with SP800-38D + if (sizeof(AAD_whole_len) >= 8 && AAD_whole_len > (1ULL << 61) - 1) { + PORT_SetError(SEC_ERROR_INPUT_LEN); + return NULL; + } + gcm = PORT_ZNew(intel_AES_GCMContext); if (gcm == NULL) { return NULL; @@ -160,6 +166,14 @@ intel_AES_GCM_EncryptUpdate(intel_AES_GCMContext *gcm, unsigned char T[AES_BLOCK_SIZE]; unsigned int j; + // GCM has a 16 octet block, with a 32-bit block counter + // Limit in accordance with SP800-38D + if (sizeof(inlen) > 4 && + inlen >= ((1ULL << 32) - 2) * AES_BLOCK_SIZE) { + PORT_SetError(SEC_ERROR_INPUT_LEN); + return SECFailure; + } + tagBytes = (gcm->tagBits + (PR_BITS_PER_BYTE - 1)) / PR_BITS_PER_BYTE; if (UINT_MAX - inlen < tagBytes) { PORT_SetError(SEC_ERROR_INPUT_LEN); @@ -217,6 +231,14 @@ intel_AES_GCM_DecryptUpdate(intel_AES_GCMContext *gcm, inlen -= tagBytes; intag = inbuf + inlen; + // GCM has a 16 octet block, with a 32-bit block counter + // Limit in accordance with SP800-38D + if (sizeof(inlen) > 4 && + inlen >= ((1ULL << 32) - 2) * AES_BLOCK_SIZE) { + PORT_SetError(SEC_ERROR_INPUT_LEN); + return SECFailure; + } + if (maxout < inlen) { *outlen = inlen; PORT_SetError(SEC_ERROR_OUTPUT_LEN); diff --git a/security/nss/lib/freebl/mpi/README b/security/nss/lib/freebl/mpi/README index cf43027580d0..a49aa9d8d73a 100644 --- a/security/nss/lib/freebl/mpi/README +++ b/security/nss/lib/freebl/mpi/README @@ -167,6 +167,7 @@ To set an mp_int to a given value, the following functions are given: mp_set(mp_int *mp, mp_digit d); mp_set_int(mp_int *mp, long z); + mp_set_ulong(mp_int *mp, unsigned long z); The mp_set() function sets the mp_int to a single digit value, while mp_set_int() sets the mp_int to a signed long integer value. diff --git a/security/nss/lib/freebl/mpi/mpi.c b/security/nss/lib/freebl/mpi/mpi.c index 2cf2a6e3995b..7e96e51ff8ba 100644 --- a/security/nss/lib/freebl/mpi/mpi.c +++ b/security/nss/lib/freebl/mpi/mpi.c @@ -344,6 +344,8 @@ mp_set_int(mp_int *mp, long z) unsigned long v = labs(z); mp_err res; + ARGCHK(mp != NULL, MP_BADARG); + /* https://bugzilla.mozilla.org/show_bug.cgi?id=1509432 */ if ((res = mp_set_ulong(mp, v)) != MP_OKAY) { /* avoids duplicated code */ return res; @@ -1427,7 +1429,7 @@ s_mp_exptmod(const mp_int *a, const mp_int *b, const mp_int *m, mp_int *c) mp_digit d; unsigned int dig, bit; - ARGCHK(a != NULL && b != NULL && c != NULL, MP_BADARG); + ARGCHK(a != NULL && b != NULL && c != NULL && m != NULL, MP_BADARG); if (mp_cmp_z(b) < 0 || mp_cmp_z(m) <= 0) return MP_RANGE; @@ -1514,7 +1516,7 @@ mp_exptmod_d(const mp_int *a, mp_digit d, const mp_int *m, mp_int *c) mp_int s, x; mp_err res; - ARGCHK(a != NULL && c != NULL, MP_BADARG); + ARGCHK(a != NULL && c != NULL && m != NULL, MP_BADARG); if ((res = mp_init(&s)) != MP_OKAY) return res; @@ -1567,6 +1569,8 @@ X: int mp_cmp_z(const mp_int *a) { + ARGMPCHK(a != NULL); + if (SIGN(a) == NEG) return MP_LT; else if (USED(a) == 1 && DIGIT(a, 0) == 0) @@ -1657,7 +1661,7 @@ mp_cmp_mag(const mp_int *a, const mp_int *b) int mp_isodd(const mp_int *a) { - ARGCHK(a != NULL, 0); + ARGMPCHK(a != NULL); return (int)(DIGIT(a, 0) & 1); @@ -2001,7 +2005,7 @@ s_mp_almost_inverse(const mp_int *a, const mp_int *p, mp_int *c) mp_err k = 0; mp_int d, f, g; - ARGCHK(a && p && c, MP_BADARG); + ARGCHK(a != NULL && p != NULL && c != NULL, MP_BADARG); MP_DIGITS(&d) = 0; MP_DIGITS(&f) = 0; @@ -2135,7 +2139,7 @@ s_mp_invmod_odd_m(const mp_int *a, const mp_int *m, mp_int *c) mp_err res; mp_int x; - ARGCHK(a && m && c, MP_BADARG); + ARGCHK(a != NULL && m != NULL && c != NULL, MP_BADARG); if (mp_cmp_z(a) == 0 || mp_cmp_z(m) == 0) return MP_RANGE; @@ -2173,7 +2177,7 @@ mp_invmod_xgcd(const mp_int *a, const mp_int *m, mp_int *c) mp_int g, x; mp_err res; - ARGCHK(a && m && c, MP_BADARG); + ARGCHK(a != NULL && m != NULL && c != NULL, MP_BADARG); if (mp_cmp_z(a) == 0 || mp_cmp_z(m) == 0) return MP_RANGE; @@ -2269,6 +2273,8 @@ s_mp_invmod_even_m(const mp_int *a, const mp_int *m, mp_int *c) mp_int oddPart, evenPart; /* parts to combine via CRT. */ mp_int C2, tmp1, tmp2; + ARGCHK(a != NULL && m != NULL && c != NULL, MP_BADARG); + /*static const mp_digit d1 = 1; */ /*static const mp_int one = { MP_ZPOS, 1, 1, (mp_digit *)&d1 }; */ @@ -2347,8 +2353,7 @@ CLEANUP: mp_err mp_invmod(const mp_int *a, const mp_int *m, mp_int *c) { - - ARGCHK(a && m && c, MP_BADARG); + ARGCHK(a != NULL && m != NULL && c != NULL, MP_BADARG); if (mp_cmp_z(a) == 0 || mp_cmp_z(m) == 0) return MP_RANGE; @@ -2715,6 +2720,8 @@ mp_strerror(mp_err ec) mp_err s_mp_grow(mp_int *mp, mp_size min) { + ARGCHK(mp != NULL, MP_BADARG); + if (min > ALLOC(mp)) { mp_digit *tmp; @@ -2744,6 +2751,8 @@ s_mp_grow(mp_int *mp, mp_size min) mp_err s_mp_pad(mp_int *mp, mp_size min) { + ARGCHK(mp != NULL, MP_BADARG); + if (min > USED(mp)) { mp_err res; @@ -2863,6 +2872,8 @@ s_mp_lshd(mp_int *mp, mp_size p) mp_err res; unsigned int ix; + ARGCHK(mp != NULL, MP_BADARG); + if (p == 0) return MP_OKAY; @@ -2995,6 +3006,8 @@ s_mp_mul_2(mp_int *mp) unsigned int ix, used; mp_digit kin = 0; + ARGCHK(mp != NULL, MP_BADARG); + /* Shift digits leftward by 1 bit */ used = MP_USED(mp); pd = MP_DIGITS(mp); @@ -3104,6 +3117,8 @@ s_mp_norm(mp_int *a, mp_int *b, mp_digit *pd) mp_digit b_msd; mp_err res = MP_OKAY; + ARGCHK(a != NULL && b != NULL && pd != NULL, MP_BADARG); + d = 0; mask = DIGIT_MAX & ~(DIGIT_MAX >> 1); /* mask is msb of digit */ b_msd = DIGIT(b, USED(b) - 1); @@ -4368,6 +4383,8 @@ CLEANUP: int s_mp_cmp(const mp_int *a, const mp_int *b) { + ARGMPCHK(a != NULL && b != NULL); + mp_size used_a = MP_USED(a); { mp_size used_b = MP_USED(b); @@ -4419,6 +4436,8 @@ IS_GT: int s_mp_cmp_d(const mp_int *a, mp_digit d) { + ARGMPCHK(a != NULL); + if (USED(a) > 1) return MP_GT; @@ -4445,6 +4464,8 @@ s_mp_ispow2(const mp_int *v) mp_digit d; int extra = 0, ix; + ARGMPCHK(v != NULL); + ix = MP_USED(v) - 1; d = MP_DIGIT(v, ix); /* most significant digit of v */ @@ -4772,10 +4793,7 @@ mp_to_fixlen_octets(const mp_int *mp, unsigned char *str, mp_size length) int ix, jx; unsigned int bytes; - ARGCHK(mp != NULL, MP_BADARG); - ARGCHK(str != NULL, MP_BADARG); - ARGCHK(!SIGN(mp), MP_BADARG); - ARGCHK(length > 0, MP_BADARG); + ARGCHK(mp != NULL && str != NULL && !SIGN(mp) && length > 0, MP_BADARG); /* Constant time on the value of mp. Don't use mp_unsigned_octet_size. */ bytes = USED(mp) * MP_DIGIT_SIZE; diff --git a/security/nss/lib/freebl/mpi/mpi.h b/security/nss/lib/freebl/mpi/mpi.h index d5aef46d7cec..af608b43dfd2 100644 --- a/security/nss/lib/freebl/mpi/mpi.h +++ b/security/nss/lib/freebl/mpi/mpi.h @@ -288,7 +288,14 @@ void freebl_cpuid(unsigned long op, unsigned long *eax, #define DIGITS(MP) MP_DIGITS(MP) #define DIGIT(MP, N) MP_DIGIT(MP, N) +/* Functions which return an mp_err value will NULL-check their arguments via + * ARGCHK(condition, return), where the caller is responsible for checking the + * mp_err return code. For functions that return an integer type, the caller + * has no way to tell if the value is an error code or a legitimate value. + * Therefore, ARGMPCHK(condition) will trigger an assertion failure on debug + * builds, but no-op in optimized builds. */ #if MP_ARGCHK == 1 +#define ARGMPCHK(X) /* */ #define ARGCHK(X, Y) \ { \ if (!(X)) { \ @@ -297,9 +304,11 @@ void freebl_cpuid(unsigned long op, unsigned long *eax, } #elif MP_ARGCHK == 2 #include +#define ARGMPCHK(X) assert(X) #define ARGCHK(X, Y) assert(X) #else -#define ARGCHK(X, Y) /* */ +#define ARGMPCHK(X) /* */ +#define ARGCHK(X, Y) /* */ #endif #ifdef CT_VERIF diff --git a/security/nss/lib/freebl/rsapkcs.c b/security/nss/lib/freebl/rsapkcs.c index 875e4e28d3db..f26cd29540c5 100644 --- a/security/nss/lib/freebl/rsapkcs.c +++ b/security/nss/lib/freebl/rsapkcs.c @@ -115,7 +115,7 @@ rsa_FormatOneBlock(unsigned modulusLen, { unsigned char *block; unsigned char *bp; - int padLen; + unsigned int padLen; int i, j; SECStatus rv; @@ -135,14 +135,15 @@ rsa_FormatOneBlock(unsigned modulusLen, switch (blockType) { /* - * Blocks intended for private-key operation. - */ + * Blocks intended for private-key operation. + */ case RSA_BlockPrivate: /* preferred method */ /* - * 0x00 || BT || Pad || 0x00 || ActualData - * 1 1 padLen 1 data->len - * Pad is either all 0x00 or all 0xff bytes, depending on blockType. - */ + * 0x00 || BT || Pad || 0x00 || ActualData + * 1 1 padLen 1 data->len + * padLen must be at least RSA_BLOCK_MIN_PAD_LEN (8) bytes. + * Pad is either all 0x00 or all 0xff bytes, depending on blockType. + */ padLen = modulusLen - data->len - 3; PORT_Assert(padLen >= RSA_BLOCK_MIN_PAD_LEN); if (padLen < RSA_BLOCK_MIN_PAD_LEN) { @@ -162,7 +163,7 @@ rsa_FormatOneBlock(unsigned modulusLen, /* * 0x00 || BT || Pad || 0x00 || ActualData * 1 1 padLen 1 data->len - * Pad is all non-zero random bytes. + * Pad is 8 or more non-zero random bytes. * * Build the block left to right. * Fill the entire block from Pad to the end with random bytes. @@ -171,6 +172,7 @@ rsa_FormatOneBlock(unsigned modulusLen, * If we need more than that, refill the bytes after Pad with * new random bytes as necessary. */ + padLen = modulusLen - (data->len + 3); PORT_Assert(padLen >= RSA_BLOCK_MIN_PAD_LEN); if (padLen < RSA_BLOCK_MIN_PAD_LEN) { @@ -236,8 +238,9 @@ rsa_FormatBlock(SECItem *result, * The "3" below is the first octet + the second octet + the 0x00 * octet that always comes just before the ActualData. */ - PORT_Assert(data->len <= (modulusLen - (3 + RSA_BLOCK_MIN_PAD_LEN))); - + if (data->len > (modulusLen - (3 + RSA_BLOCK_MIN_PAD_LEN))) { + return SECFailure; + } result->data = rsa_FormatOneBlock(modulusLen, blockType, data); if (result->data == NULL) { result->len = 0; diff --git a/security/nss/lib/ssl/ssl3ext.c b/security/nss/lib/ssl/ssl3ext.c index 032795bc4e79..7e674f0e0375 100644 --- a/security/nss/lib/ssl/ssl3ext.c +++ b/security/nss/lib/ssl/ssl3ext.c @@ -714,6 +714,9 @@ ssl_ConstructExtensions(sslSocket *ss, sslBuffer *buf, SSLHandshakeType message) PORT_Assert(buf->len == 0); + /* Clear out any extensions previously advertised */ + ss->xtnData.numAdvertised = 0; + switch (message) { case ssl_hs_client_hello: if (ss->vrange.max > SSL_LIBRARY_VERSION_3_0) { diff --git a/security/nss/lib/ssl/tls13subcerts.c b/security/nss/lib/ssl/tls13subcerts.c index 43d31b51657a..90e04d568368 100644 --- a/security/nss/lib/ssl/tls13subcerts.c +++ b/security/nss/lib/ssl/tls13subcerts.c @@ -665,6 +665,11 @@ SSLExp_DelegateCredential(const CERTCertificate *cert, sslDelegatedCredential *dc = NULL; sslBuffer dcBuf = SSL_BUFFER_EMPTY; + if (!cert || !certPriv || !dcPub || !out) { + PORT_SetError(SEC_ERROR_INVALID_ARGS); + return SECFailure; + } + dc = PORT_ZNew(sslDelegatedCredential); if (!dc) { PORT_SetError(SEC_ERROR_NO_MEMORY); diff --git a/security/nss/tests/common/cleanup.sh b/security/nss/tests/common/cleanup.sh index 97c139321731..dfa598d35483 100755 --- a/security/nss/tests/common/cleanup.sh +++ b/security/nss/tests/common/cleanup.sh @@ -6,6 +6,12 @@ if [ -z "${CLEANUP}" -o "${CLEANUP}" = "${SCRIPTNAME}" ]; then + if [ -z "${BUILD_OPT}" ] && [ "$OBJDIR" == "Debug" ]; then + BUILD_OPT=0; + elif [ -z "${BUILD_OPT}" ] && [ "$OBJDIR" == "Release" ]; then + BUILD_OPT=1; + fi + echo echo "SUMMARY:" echo "========" @@ -51,9 +57,10 @@ if [ -z "${CLEANUP}" -o "${CLEANUP}" = "${SCRIPTNAME}" ]; then echo html "END_OF_TEST
" - html "" + html "" rm -f ${TEMPFILES} 2>/dev/null - if [ ${FAILED_CNT} -gt 0 ] || [ ${ASAN_CNT} -gt 0 ]; then + if [ ${FAILED_CNT} -gt 0 ] || [ ${ASAN_CNT} -gt 0 ] || + ([ ${BUILD_OPT} -eq 1 ] && [ ${CORE_CNT} -gt 0 ]); then exit 1 fi