Bug 1496340 - Make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read. r=jcj, a=pascalc
authorDana Keeler <dkeeler@mozilla.com>
Thu, 04 Oct 2018 16:30:50 -0700
changeset 492862 8b900972f8fe092d0f1bf0d7cc626c23721998c4
parent 492861 5c16a996ed9b2a25dc572d7ba8ca8f0720e9dd48
child 492863 55ed1a3a910a6c46e79b2a69fdfae86767601d2c
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj, pascalc
bugs1496340
milestone63.0
Bug 1496340 - Make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read. r=jcj, a=pascalc Reviewers: jcj Tags: #secure-revision Bug #: 1496340 Differential Revision: https://phabricator.services.mozilla.com/D7803
security/manager/ssl/nsNSSCertificate.cpp
security/manager/ssl/tests/unit/test_cert_chains.js
--- a/security/manager/ssl/nsNSSCertificate.cpp
+++ b/security/manager/ssl/nsNSSCertificate.cpp
@@ -827,16 +827,20 @@ nsNSSCertList*
 nsNSSCertList::GetCertList()
 {
   return this;
 }
 
 NS_IMETHODIMP
 nsNSSCertList::AddCert(nsIX509Cert* aCert)
 {
+  if (!aCert) {
+    return NS_ERROR_INVALID_ARG;
+  }
+
   // We need an owning handle when calling nsIX509Cert::GetCert().
   UniqueCERTCertificate cert(aCert->GetCert());
   if (!cert) {
     NS_ERROR("Somehow got nullptr for mCertificate in nsNSSCertificate.");
     return NS_ERROR_FAILURE;
   }
 
   if (!mCertList) {
@@ -1015,27 +1019,29 @@ nsNSSCertList::Read(nsIObjectInputStream
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   for(uint32_t i = 0; i < certListLen; ++i) {
     nsCOMPtr<nsISupports> certSupports;
     rv = aStream->ReadObject(true, getter_AddRefs(certSupports));
     if (NS_FAILED(rv)) {
-      break;
+      return rv;
     }
-
     nsCOMPtr<nsIX509Cert> cert = do_QueryInterface(certSupports);
+    if (!cert) {
+      return NS_ERROR_UNEXPECTED;
+    }
     rv = AddCert(cert);
     if (NS_FAILED(rv)) {
-      break;
+      return rv;
     }
   }
 
-  return rv;
+  return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSCertList::GetEnumerator(nsISimpleEnumerator** _retval)
 {
   if (!mCertList) {
     return NS_ERROR_FAILURE;
   }
--- a/security/manager/ssl/tests/unit/test_cert_chains.js
+++ b/security/manager/ssl/tests/unit/test_cert_chains.js
@@ -16,19 +16,40 @@ function test_cert_equals() {
   ok(certA.equals(certB),
      "equals() on cert objects constructed from the same cert file should" +
      " return true");
   ok(!certA.equals(certC),
      "equals() on cert objects constructed from files for different certs" +
      " should return false");
 }
 
+function test_bad_cert_list_serialization() {
+  // Normally the serialization of an nsIX509CertList consists of some header
+  // junk (IIDs and whatnot), 4 bytes representing how many nsIX509Cert follow,
+  // and then the serialization of each nsIX509Cert. This serialization consists
+  // of the header junk for an nsIX509CertList with 1 "nsIX509Cert", but then
+  // instead of an nsIX509Cert, the subsequent bytes represent the serialization
+  // of another nsIX509CertList (with 0 nsIX509Cert). This test ensures that
+  // nsIX509CertList safely handles this unexpected input when deserializing.
+  const badCertListSerialization =
+    "lZ+xZWUXSH+rm9iRO+UxlwAAAAAAAAAAwAAAAAAAAEYAAAABlZ+xZWUXSH+rm9iRO+UxlwAAAAAA" +
+    "AAAAwAAAAAAAAEYAAAAA";
+  let serHelper = Cc["@mozilla.org/network/serialization-helper;1"]
+                    .getService(Ci.nsISerializationHelper);
+  throws(() => serHelper.deserializeObject(badCertListSerialization),
+         /NS_ERROR_UNEXPECTED/,
+         "deserializing a bogus nsIX509CertList should throw NS_ERROR_UNEXPECTED");
+}
+
 function test_cert_list_serialization() {
   let certList = build_cert_chain(["default-ee", "expired-ee"]);
 
+  throws(() => certList.addCert(null), /NS_ERROR_ILLEGAL_VALUE/,
+         "trying to add a null cert to an nsIX509CertList should throw");
+
   // Serialize the cert list to a string
   let serHelper = Cc["@mozilla.org/network/serialization-helper;1"]
                     .getService(Ci.nsISerializationHelper);
   certList.QueryInterface(Ci.nsISerializable);
   let serialized = serHelper.serializeToString(certList);
 
   // Deserialize from the string and compare to the original object
   let deserialized = serHelper.deserializeObject(serialized);
@@ -195,16 +216,21 @@ function run_test() {
   add_tls_server_setup("BadCertServer", "bad_certs");
 
   // Test nsIX509Cert.equals
   add_test(function() {
     test_cert_equals();
     run_next_test();
   });
 
+  add_test(function() {
+    test_bad_cert_list_serialization();
+    run_next_test();
+  });
+
   // Test serialization of nsIX509CertList
   add_test(function() {
     test_cert_list_serialization();
     run_next_test();
   });
 
   add_test(function() {
     test_cert_pkcs7_export();