bug 1341905 - double-check that uses of CERT_LIST_* are safe in PSM r=jcj
authorDavid Keeler <dkeeler@mozilla.com>
Wed, 22 Feb 2017 15:07:05 -0800
changeset 344638 37a4221a05122c908f37c24f40bc7bc4946a151f
parent 344637 5b49024a794b6e3148fa6aa1d92101f9e0f37015
child 344639 8999082d8a89381727c2eee5e26ff4ab653cde4e
push id31414
push usercbook@mozilla.com
push dateFri, 24 Feb 2017 10:47:41 +0000
treeherdermozilla-central@be661bae6cb9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1341905
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1341905 - double-check that uses of CERT_LIST_* are safe in PSM r=jcj MozReview-Commit-ID: BhGHd9xUUbP
security/apps/AppSignatureVerification.cpp
security/certverifier/CertVerifier.cpp
security/manager/ssl/ContentSignatureVerifier.cpp
security/manager/ssl/SSLServerCertVerification.cpp
security/manager/ssl/tests/unit/test_cert_trust.js
--- a/security/apps/AppSignatureVerification.cpp
+++ b/security/apps/AppSignatureVerification.cpp
@@ -859,19 +859,23 @@ OpenSignedAppFile(AppTrustedRoot aTruste
   // Return the reader to the caller if they want it
   if (aZipReader) {
     zip.forget(aZipReader);
   }
 
   // Return the signer's certificate to the reader if they want it.
   // XXX: We should return an nsIX509CertList with the whole validated chain.
   if (aSignerCert) {
-    MOZ_ASSERT(CERT_LIST_HEAD(builtChain));
+    CERTCertListNode* signerCertNode = CERT_LIST_HEAD(builtChain);
+    if (!signerCertNode || CERT_LIST_END(signerCertNode, builtChain) ||
+        !signerCertNode->cert) {
+      return NS_ERROR_FAILURE;
+    }
     nsCOMPtr<nsIX509Cert> signerCert =
-      nsNSSCertificate::Create(CERT_LIST_HEAD(builtChain)->cert);
+      nsNSSCertificate::Create(signerCertNode->cert);
     NS_ENSURE_TRUE(signerCert, NS_ERROR_OUT_OF_MEMORY);
     signerCert.forget(aSignerCert);
   }
 
   return NS_OK;
 }
 
 nsresult
@@ -933,19 +937,23 @@ VerifySignedManifest(AppTrustedRoot aTru
   rv = VerifySignature(aTrustedRoot, signatureBuffer,
                        doubleDigest.get(), builtChain);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   // Return the signer's certificate to the reader if they want it.
   if (aSignerCert) {
-    MOZ_ASSERT(CERT_LIST_HEAD(builtChain));
+    CERTCertListNode* signerCertNode = CERT_LIST_HEAD(builtChain);
+    if (!signerCertNode || CERT_LIST_END(signerCertNode, builtChain) ||
+        !signerCertNode->cert) {
+      return NS_ERROR_FAILURE;
+    }
     nsCOMPtr<nsIX509Cert> signerCert =
-      nsNSSCertificate::Create(CERT_LIST_HEAD(builtChain)->cert);
+      nsNSSCertificate::Create(signerCertNode->cert);
     if (NS_WARN_IF(!signerCert)) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
 
     signerCert.forget(aSignerCert);
   }
 
   return NS_OK;
@@ -1486,19 +1494,23 @@ VerifySignedDirectory(AppTrustedRoot aTr
   // ParseMFUnpacked() checking them all, but it's a cheap sanity check.)
   if (items.Count() != 0) {
     return NS_ERROR_SIGNED_JAR_ENTRY_MISSING;
   }
 
   // Return the signer's certificate to the reader if they want it.
   // XXX: We should return an nsIX509CertList with the whole validated chain.
   if (aSignerCert) {
-    MOZ_ASSERT(CERT_LIST_HEAD(builtChain));
+    CERTCertListNode* signerCertNode = CERT_LIST_HEAD(builtChain);
+    if (!signerCertNode || CERT_LIST_END(signerCertNode, builtChain) ||
+        !signerCertNode->cert) {
+      return NS_ERROR_FAILURE;
+    }
     nsCOMPtr<nsIX509Cert> signerCert =
-      nsNSSCertificate::Create(CERT_LIST_HEAD(builtChain)->cert);
+      nsNSSCertificate::Create(signerCertNode->cert);
     NS_ENSURE_TRUE(signerCert, NS_ERROR_OUT_OF_MEMORY);
     signerCert.forget(aSignerCert);
   }
 
   return NS_OK;
 }
 
 class VerifySignedDirectoryTask final : public CryptoTask
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -281,22 +281,23 @@ CertVerifier::VerifyCertificateTranspare
   }
   if (sctsFromTLS.GetLength() > 0) {
     MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
             ("Got TLS SCT data of length %zu\n",
               static_cast<size_t>(sctsFromTLS.GetLength())));
   }
 
   CERTCertListNode* endEntityNode = CERT_LIST_HEAD(builtChain);
-  if (!endEntityNode) {
+  if (!endEntityNode || CERT_LIST_END(endEntityNode, builtChain)) {
     return Result::FATAL_ERROR_INVALID_ARGS;
   }
   CERTCertListNode* issuerNode = CERT_LIST_NEXT(endEntityNode);
-  if (!issuerNode) {
+  if (!issuerNode || CERT_LIST_END(issuerNode, builtChain)) {
     // Issuer certificate is required for SCT verification.
+    // TODO(bug 1294580): change this to Result::FATAL_ERROR_INVALID_ARGS
     return Success;
   }
 
   CERTCertificate* endEntity = endEntityNode->cert;
   CERTCertificate* issuer = issuerNode->cert;
   if (!endEntity || !issuer) {
     return Result::FATAL_ERROR_INVALID_ARGS;
   }
--- a/security/manager/ssl/ContentSignatureVerifier.cpp
+++ b/security/manager/ssl/ContentSignatureVerifier.cpp
@@ -153,17 +153,17 @@ ContentSignatureVerifier::CreateContextI
   }
 
   nsresult rv = ReadChainIntoCertList(aCertChain, certCertList.get(), locker);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   CERTCertListNode* node = CERT_LIST_HEAD(certCertList.get());
-  if (!node || !node->cert) {
+  if (!node || CERT_LIST_END(node, certCertList.get()) || !node->cert) {
     return NS_ERROR_FAILURE;
   }
 
   SECItem* certSecItem = &node->cert->derCert;
 
   Input certDER;
   mozilla::pkix::Result result =
     certDER.Init(BitwiseCast<uint8_t*, unsigned char*>(certSecItem->data),
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -1185,22 +1185,22 @@ GatherRootCATelemetry(const UniqueCERTCe
 const uint64_t ONE_WEEK_IN_SECONDS = (7 * (24 * 60 *60));
 const uint64_t ONE_YEAR_IN_WEEKS   = 52;
 
 // Gathers telemetry on the certificate lifetimes we observe in the wild
 void
 GatherEndEntityTelemetry(const UniqueCERTCertList& certList)
 {
   CERTCertListNode* endEntityNode = CERT_LIST_HEAD(certList);
-  MOZ_ASSERT(endEntityNode);
-  if (!endEntityNode) {
+  MOZ_ASSERT(endEntityNode && !CERT_LIST_END(endEntityNode, certList));
+  if (!endEntityNode || CERT_LIST_END(endEntityNode, certList)) {
     return;
   }
 
-  CERTCertificate * endEntityCert = endEntityNode->cert;
+  CERTCertificate* endEntityCert = endEntityNode->cert;
   MOZ_ASSERT(endEntityCert);
   if (!endEntityCert) {
     return;
   }
 
   PRTime notBefore;
   PRTime notAfter;
 
--- a/security/manager/ssl/tests/unit/test_cert_trust.js
+++ b/security/manager/ssl/tests/unit/test_cert_trust.js
@@ -199,9 +199,19 @@ function run_test() {
   let ee_cert = loadedCerts[2];
   notEqual(ee_cert, null, "EE cert should have successfully loaded");
 
   setup_basic_trusts(ca_cert, int_cert);
   test_ca_distrust(ee_cert, ca_cert, true);
 
   setup_basic_trusts(ca_cert, int_cert);
   test_ca_distrust(ee_cert, int_cert, false);
+
+  // Reset trust to default ("inherit trust")
+  setCertTrust(ca_cert, ",,");
+  setCertTrust(int_cert, ",,");
+
+  // It turns out that if an end-entity certificate is manually trusted, it can
+  // be the root of its own verified chain. This will be removed in bug 1294580.
+  setCertTrust(ee_cert, "C,,");
+  checkCertErrorGeneric(certdb, ee_cert, PRErrorCodeSuccess,
+                        certificateUsageSSLServer);
 }