Bug 1341905 - double-check that uses of CERT_LIST_* are safe in PSM r=jcj a=gchang
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 24 Feb 2017 09:40:10 -0800
changeset 376446 75e0388a1c2ba34871f4e9b6bc3d4f372cc7fd70
parent 376445 0934029f7f0c8b1c7fe66b14ce1c8ed15c6892ec
child 376447 9a9d2ebca3f615630341ca238bf92de86d496353
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj, gchang
bugs1341905
milestone53.0a2
Bug 1341905 - double-check that uses of CERT_LIST_* are safe in PSM r=jcj a=gchang
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
@@ -222,22 +222,23 @@ CertVerifier::VerifySignedCertificateTim
             ("Got TLS SCT data of length %zu\n",
               static_cast<size_t>(sctsFromTLS.GetLength())));
   }
   if (!gotScts) {
     return Success;
   }
 
   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
@@ -1173,22 +1173,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);
 }