Bug 1572791 - Fix ASAN cert errors when SSL gtests run on empty profile r=jcj
authorKevin Jacobs <kjacobs@mozilla.com>
Tue, 13 Aug 2019 17:32:31 +0000
changeset 15248 cef2aa7f3b8ce52571ac4b82242ba75cdeb756ef
parent 15247 360010725fdb499e0c63f8b37385f9c914808a0b
child 15249 ed50678575639fdf32c7d461b343aaf5f5b4ea77
push id3460
push userjjones@mozilla.com
push dateTue, 13 Aug 2019 17:33:17 +0000
reviewersjcj
bugs1572791
Bug 1572791 - Fix ASAN cert errors when SSL gtests run on empty profile r=jcj Differential Revision: https://phabricator.services.mozilla.com/D41787
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
--- a/gtests/ssl_gtest/ssl_auth_unittest.cc
+++ b/gtests/ssl_gtest/ssl_auth_unittest.cc
@@ -242,17 +242,19 @@ TEST_F(TlsConnectStreamTls13, PostHandsh
   // properly sent back.
   EXPECT_LT(0U, capture_cert_req->buffer().len());
   EXPECT_EQ(capture_cert_req->buffer().len(),
             capture_certificate->buffer().len());
   EXPECT_EQ(0, memcmp(capture_cert_req->buffer().data(),
                       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));
 }
 
 static SECStatus GetClientAuthDataHook(void* self, PRFileDesc* fd,
                                        CERTDistNames* caNames,
                                        CERTCertificate** clientCert,
                                        SECKEYPrivateKey** clientKey) {
   ScopedCERTCertificate cert;
@@ -284,30 +286,34 @@ TEST_F(TlsConnectStreamTls13, PostHandsh
   EXPECT_EQ(SECSuccess, SSL_SendCertificateRequest(server_->ssl_fd()))
       << "Unexpected error: " << PORT_ErrorToName(PORT_GetError());
   server_->SendData(50);
   client_->ReadBytes(50);
   client_->SendData(50);
   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(
                             client_->ssl_fd(), GetClientAuthDataHook, nullptr));
   EXPECT_EQ(SECSuccess, SSL_SendCertificateRequest(server_->ssl_fd()))
       << "Unexpected error: " << PORT_ErrorToName(PORT_GetError());
   server_->SendData(50);
   client_->ReadBytes(50);
   client_->SendData(50);
   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));
 }
 
 TEST_F(TlsConnectStreamTls13, PostHandshakeAuthConcurrent) {
   client_->SetupClientAuth();
   EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(),
                                       SSL_ENABLE_POST_HANDSHAKE_AUTH, PR_TRUE));
@@ -378,30 +384,34 @@ TEST_F(TlsConnectStreamTls13, PostHandsh
   server_->SetAuthCertificateCallback(
       [&called](TlsAgent*, PRBool, PRBool) -> SECStatus {
         called++;
         return SECSuccess;
       });
   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(
                             client_->ssl_fd(), GetClientAuthDataHook, nullptr));
   EXPECT_EQ(SECSuccess, SSL_SendCertificateRequest(server_->ssl_fd()))
       << "Unexpected error: " << PORT_ErrorToName(PORT_GetError());
   server_->SendData(50);
   client_->ReadBytes(50);
   client_->SendData(50);
   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));
 }
 
 // Damages the request context in a CertificateRequest message.
 // We don't modify a Certificate message instead, so that the client
 // can compute CertificateVerify correctly.
 class TlsDamageCertificateRequestContextFilter : public TlsHandshakeFilter {
@@ -562,17 +572,19 @@ TEST_F(TlsConnectStreamTls13, PostHandsh
   EXPECT_EQ(SECSuccess, SSL_SendCertificateRequest(server_->ssl_fd()))
       << "Unexpected error: " << PORT_ErrorToName(PORT_GetError());
   server_->SendData(50);
   client_->ReadBytes(50);
   client_->SendData(50);
   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));
 }
 
 // In TLS 1.3, the client sends its cert rejection on the
 // second flight, and since it has already received the
 // server's Finished, it transitions to complete and
 // then gets an alert from the server. The test harness
 // doesn't handle this right yet.
@@ -609,17 +621,19 @@ static void CheckSigScheme(std::shared_p
                            uint16_t expected_scheme, size_t expected_size) {
   EXPECT_LT(offset + 2U, capture->buffer().len());
 
   uint32_t scheme = 0;
   capture->buffer().Read(offset, 2, &scheme);
   EXPECT_EQ(expected_scheme, static_cast<uint16_t>(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()));
 }
 
 // The server should prefer SHA-256 by default, even for the small key size used
 // in the default certificate.
 TEST_P(TlsConnectTls12, ServerAuthCheckSigAlg) {
   EnsureTlsSetup();
   auto capture_ske = MakeTlsFilter<TlsHandshakeRecorder>(
--- a/gtests/ssl_gtest/ssl_cert_ext_unittest.cc
+++ b/gtests/ssl_gtest/ssl_cert_ext_unittest.cc
@@ -38,20 +38,20 @@ class SignedCertificateTimestampsExtract
     client->SetHandshakeCallback([this](TlsAgent* agent) {
       const SECItem* scts = SSL_PeerSignedCertTimestamps(agent->ssl_fd());
       ASSERT_TRUE(scts);
       handshake_timestamps_.reset(new DataBuffer(scts->data, scts->len));
     });
   }
 
   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 =
         SSL_PeerSignedCertTimestamps(client_.lock()->ssl_fd());
     EXPECT_EQ(timestamps, DataBuffer(current->data, current->len));
   }
 
  private:
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -418,42 +418,46 @@ static int32_t SwitchCertificates(TlsAge
   if (!ok) return SSL_SNI_SEND_ALERT;
 
   return 0;  // first config
 };
 
 TEST_P(TlsConnectGeneric, ServerSNICertSwitch) {
   Connect();
   ScopedCERTCertificate cert1(SSL_PeerCertificate(client_->ssl_fd()));
+  ASSERT_NE(nullptr, cert1.get());
 
   Reset();
   ConfigureSessionCache(RESUME_NONE, RESUME_NONE);
 
   server_->SetSniCallback(SwitchCertificates);
 
   Connect();
   ScopedCERTCertificate cert2(SSL_PeerCertificate(client_->ssl_fd()));
+  ASSERT_NE(nullptr, cert2.get());
   CheckKeys();
   EXPECT_FALSE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert));
 }
 
 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);
 
   // Because we configure an RSA certificate here, it only adds a second, unused
   // certificate, which has no effect on what the server uses.
   server_->SetSniCallback(SwitchCertificates);
 
   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));
 }
 
 // Prior to TLS 1.3, we were not fully ephemeral; though 1.3 fixes that
 TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceReuseKey) {
   auto filter = MakeTlsFilter<TlsHandshakeRecorder>(
       server_, kTlsHandshakeServerKeyExchange);
@@ -528,62 +532,66 @@ TEST_P(TlsConnectTls13, TestTls13ResumeD
 // handshake, after resumption.
 TEST_P(TlsConnectTls13, TestTls13ResumeNoCertificateRequest) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   client_->SetupClientAuth();
   server_->RequestClientAuth(true);
   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);
   ExpectResumption(RESUME_TICKET);
   server_->RequestClientAuth(false);
   auto cr_capture =
       MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_certificate_request);
   cr_capture->EnableDecryption();
   Connect();
   SendReceive();
   EXPECT_EQ(0U, cr_capture->buffer().len()) << "expect nothing captured yet";
 
   // 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));
 }
 
 // Here we test that 0.5 RTT is available at the server when resuming, even if
 // configured to request a client certificate.  The resumed handshake relies on
 // the authentication from the original handshake, so no certificate is
 // requested this time around.  The server can write before the handshake
 // completes because the PSK binder is sufficient authentication for the client.
 TEST_P(TlsConnectTls13, WriteBeforeHandshakeCompleteOnResumption) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   client_->SetupClientAuth();
   server_->RequestClientAuth(true);
   Connect();
   SendReceive();  // Absorb the session ticket.
   ScopedCERTCertificate cert1(SSL_LocalCertificate(client_->ssl_fd()));
+  ASSERT_NE(nullptr, cert1.get());
 
   Reset();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ExpectResumption(RESUME_TICKET);
   server_->RequestClientAuth(false);
   StartConnect();
   client_->Handshake();  // ClientHello
   server_->Handshake();  // ServerHello
 
   server_->SendData(10);
   client_->ReadBytes(10);  // Client should emit the Finished as a side-effect.
   server_->Handshake();    // Server consumes the Finished.
   CheckConnected();
 
   // 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));
 }
 
 // We need to enable different cipher suites at different times in the following
 // tests.  Those cipher suites need to be suited to the version.
 static uint16_t ChooseOneCipher(uint16_t version) {
   if (version >= SSL_LIBRARY_VERSION_TLS_1_3) {
     return TLS_AES_128_GCM_SHA256;
@@ -754,33 +762,33 @@ TEST_F(TlsConnectTest, TestTls13Resumpti
   SendReceive();
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_rsae_sha256);
   // The filter will go away when we reset, so save the captured extension.
   DataBuffer initialTicket(c1->extension());
   ASSERT_LT(0U, initialTicket.len());
 
   ScopedCERTCertificate cert1(SSL_PeerCertificate(client_->ssl_fd()));
-  ASSERT_TRUE(!!cert1.get());
+  ASSERT_NE(nullptr, cert1.get());
 
   Reset();
   ClearStats();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
   auto c2 =
       MakeTlsFilter<TlsExtensionCapture>(client_, ssl_tls13_pre_shared_key_xtn);
   ExpectResumption(RESUME_TICKET);
   Connect();
   SendReceive();
   CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
             ssl_sig_rsa_pss_rsae_sha256);
   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.
   uint16_t resumed_suite;
   EXPECT_TRUE(server_->cipher_suite(&resumed_suite));
   EXPECT_EQ(original_suite, resumed_suite);
   EXPECT_TRUE(client_->cipher_suite(&resumed_suite));
   EXPECT_EQ(original_suite, resumed_suite);
@@ -1171,16 +1179,17 @@ TEST_P(TlsConnectGenericResumptionToken,
   ASSERT_TRUE(client_->MaybeSetResumptionToken());
 
   // Get resumption token infos
   SSLResumptionTokenInfo tokenInfo = {0};
   ScopedSSLResumptionTokenInfo token(&tokenInfo);
   client_->GetTokenInfo(token);
   ScopedCERTCertificate cert(
       PK11_FindCertFromNickname(server_->name().c_str(), nullptr));
+  ASSERT_NE(nullptr, cert.get());
 
   CheckGetInfoResult(now(), 0, 0, cert, token);
 
   Handshake();
   CheckConnected();
 
   SendReceive();
 }
@@ -1251,16 +1260,17 @@ TEST_P(TlsConnectGenericResumptionToken,
   ASSERT_TRUE(client_->MaybeSetResumptionToken());
 
   // Get resumption token infos
   SSLResumptionTokenInfo tokenInfo = {0};
   ScopedSSLResumptionTokenInfo token(&tokenInfo);
   client_->GetTokenInfo(token);
   ScopedCERTCertificate cert(
       PK11_FindCertFromNickname(server_->name().c_str(), nullptr));
+  ASSERT_NE(nullptr, cert.get());
 
   CheckGetInfoResult(now(), 1, 0, cert, token);
 
   Handshake();
   CheckConnected();
   CheckAlpn("a");
 
   SendReceive();
@@ -1286,17 +1296,17 @@ TEST_P(TlsConnectTls13ResumptionToken, C
   ASSERT_TRUE(client_->MaybeSetResumptionToken());
 
   // Get resumption token infos
   SSLResumptionTokenInfo tokenInfo = {0};
   ScopedSSLResumptionTokenInfo token(&tokenInfo);
   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);
   Handshake();
   ExpectEarlyDataAccepted(true);
   CheckConnected();
   CheckAlpn("a");
 
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -123,20 +123,24 @@ void TlsAgent::SetState(State s) {
   LOG("Changing state from " << state_ << " to " << s);
   state_ = s;
 }
 
 /*static*/ bool TlsAgent::LoadCertificate(const std::string& name,
                                           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;
 
   return true;
 }
 
 // Loads a key pair from the certificate identified by |id|.
 /*static*/ bool TlsAgent::LoadKeyPairFromCert(const std::string& name,