Make sure nsNSSCertList handling checks for valid certs.

2 files changed, 36 insertions, 3 deletions

diff --git a/security/manager/ssl/nsNSSCertificate.cpp b/security/manager/ssl/nsNSSCertificate.cpp
index 12fca5065f..f6685e89af 100644
--- a/security/manager/ssl/nsNSSCertificate.cpp
+++ b/security/manager/ssl/nsNSSCertificate.cpp
@@ -1208,6 +1208,10 @@ void nsNSSCertList::destructorSafeDestroyNSSReference()
 NS_IMETHODIMP
 nsNSSCertList::AddCert(nsIX509Cert* aCert)
 {
+  if (!aCert) {
+    return NS_ERROR_INVALID_ARG;
+  }
+
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     return NS_ERROR_NOT_AVAILABLE;
@@ -1369,17 +1373,20 @@ nsNSSCertList::Read(nsIObjectInputStream* aStream)
     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
diff --git a/security/manager/ssl/tests/unit/test_cert_chains.js b/security/manager/ssl/tests/unit/test_cert_chains.js
index 8abcb4e65c..dd1fc93697 100644
--- a/security/manager/ssl/tests/unit/test_cert_chains.js
+++ b/security/manager/ssl/tests/unit/test_cert_chains.js
@@ -31,9 +31,30 @@ function test_cert_equals() {
      " 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);
@@ -78,6 +99,11 @@ function run_test() {
 
   // Test serialization of nsIX509CertList
   add_test(function() {
+    test_bad_cert_list_serialization();
+    run_next_test();
+  });
+
+  add_test(function() {
     test_cert_list_serialization();
     run_next_test();
   });