bug 1496340 - make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read r=jcj
authorDana Keeler <dkeeler@mozilla.com>
Thu, 04 Oct 2018 16:30:50 -0700
changeset 495623 f59ae5c08ae1a700fb60d7af951028d5da96429a
parent 495622 9f1fd88190e39614a71b069403b92169f38afc8e
child 495624 82b90238ee320a3b44e424646f42f278d0516993
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1496340
milestone64.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 1496340 - make sure each nsISupports is an nsIX509Cert in nsNSSCertList::Read r=jcj 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
@@ -886,16 +886,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;
   }
   mCerts.push_back(std::move(cert));
   return NS_OK;
@@ -1037,27 +1041,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)
 {
   nsCOMPtr<nsISimpleEnumerator> enumerator(new nsNSSCertListEnumerator(mCerts));
   enumerator.forget(_retval);
   return NS_OK;
--- 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();