bug 1428498 - don't require importing the server certificate for overrides to succeed r=jcj

Previously, adding a permanent certificate error override would depend on
successfully importing the server's certificate into the user's certificate
database. Consequently, if the user's database were in read-only mode (or if the
database couldn't be created due to code page issues on Windows), this would
prevent adding new certificate error overrides. It turns out this isn't even
necessary, because the implementation relies on the stored hash of the
certificate rather than the certificate itself. The stored certificate is only
for display purposes (and there's a fallback if the certificate can't be
stored).

There are remaining issues with non-ASCII characters in 8.3 paths on Windows
when the code page isn't western, but this is a larger issue that must be
addressed in other layers (i.e. NSS/NSPR).

MozReview-Commit-ID: KEzjxtAoeb4

--HG--
rename : security/manager/ssl/tests/unit/test_cert_overrides.js => security/manager/ssl/tests/unit/test_cert_overrides_read_only.js
extra : rebase_source : b41e863d8c85d80335dd56c8f5765b19b1de4e0c

security/manager/ssl/nsCertOverrideService.cpp

@@ -11,6 +11,7 @@
 #include "SharedSSLState.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/Unused.h"
 #include "nsAppDirectoryServiceDefs.h"
 #include "nsCRT.h"
 #include "nsILineInputStream.h"
@@ -378,11 +379,13 @@ nsCertOverrideService::RememberValidityOverride(const nsACString& aHostName,
       return NS_ERROR_FAILURE;
     }
 
-    SECStatus srv = PK11_ImportCert(slot.get(), nsscert.get(), CK_INVALID_HANDLE,
+    // This can fail (for example, if we're in read-only mode). Luckily, we
-                                    nickname.get(), false);
+    // don't even need it to succeed - we always match on the stored hash of the
-    if (srv != SECSuccess) {
+    // certificate rather than the full certificate. It makes the display a bit
-      return NS_ERROR_FAILURE;
+    // less informative (since we won't have a certificate to display), but it's
-    }
+    // better than failing the entire operation.
+    Unused << PK11_ImportCert(slot.get(), nsscert.get(), CK_INVALID_HANDLE,
+                              nickname.get(), false);
   }
 
   nsAutoCString fpStr;

security/manager/ssl/tests/unit/head_psm.js

@@ -339,9 +339,9 @@ function run_test() {
 }
 */
 
-function add_tls_server_setup(serverBinName, certsPath) {
+function add_tls_server_setup(serverBinName, certsPath, addDefaultRoot = true) {
   add_test(function() {
-    _setupTLSServerTest(serverBinName, certsPath);
+    _setupTLSServerTest(serverBinName, certsPath, addDefaultRoot);
   });
 }
 
@@ -491,11 +491,13 @@ function _getBinaryUtil(binaryUtilName) {
 }
 
 // Do not call this directly; use add_tls_server_setup
-function _setupTLSServerTest(serverBinName, certsPath) {
+function _setupTLSServerTest(serverBinName, certsPath, addDefaultRoot) {
   let certdb = Cc["@mozilla.org/security/x509certdb;1"]
                   .getService(Ci.nsIX509CertDB);
   // The trusted CA that is typically used for "good" certificates.
-  addCertFromFile(certdb, `${certsPath}/test-ca.pem`, "CTu,u,u");
+  if (addDefaultRoot) {
+    addCertFromFile(certdb, `${certsPath}/test-ca.pem`, "CTu,u,u");
+  }
 
   const CALLBACK_PORT = 8444;
 

security/manager/ssl/tests/unit/test_cert_overrides_read_only.js

@@ -0,0 +1,93 @@
+// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+"use strict";
+
+// Tests that permanent certificate error overrides can be added even if the
+// certificate/key databases are in read-only mode.
+
+// Helper function for add_read_only_cert_override_test. Probably doesn't need
+// to be called directly.
+function add_read_only_cert_override(aHost, aExpectedBits, aSecurityInfo) {
+  let sslstatus = aSecurityInfo.QueryInterface(Ci.nsISSLStatusProvider)
+                               .SSLStatus;
+  let bits =
+    (sslstatus.isUntrusted ? Ci.nsICertOverrideService.ERROR_UNTRUSTED : 0) |
+    (sslstatus.isDomainMismatch ? Ci.nsICertOverrideService.ERROR_MISMATCH : 0) |
+    (sslstatus.isNotValidAtThisTime ? Ci.nsICertOverrideService.ERROR_TIME : 0);
+
+  Assert.equal(bits, aExpectedBits,
+               "Actual and expected override bits should match");
+  let cert = sslstatus.serverCert;
+  let certOverrideService = Cc["@mozilla.org/security/certoverride;1"]
+                              .getService(Ci.nsICertOverrideService);
+  // Setting the last argument to false here ensures that we attempt to store a
+  // permanent override (which is what was failing in bug 1427273).
+  certOverrideService.rememberValidityOverride(aHost, 8443, cert, aExpectedBits,
+                                               false);
+}
+
+// Given a host, expected error bits (see nsICertOverrideService.idl), and an
+// expected error code, tests that an initial connection to the host fails with
+// the expected errors and that adding an override results in a subsequent
+// connection succeeding.
+function add_read_only_cert_override_test(aHost, aExpectedBits, aExpectedError) {
+  add_connection_test(aHost, aExpectedError, null,
+                      add_read_only_cert_override.bind(this, aHost, aExpectedBits));
+  add_connection_test(aHost, PRErrorCodeSuccess, null, aSecurityInfo => {
+    Assert.ok(aSecurityInfo.securityState &
+              Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN,
+              "Cert override flag should be set on the security state");
+  });
+}
+
+function run_test() {
+  let profile = do_get_profile();
+  const KEY_DB_NAME = "key4.db";
+  const CERT_DB_NAME = "cert9.db";
+  let srcKeyDBFile = do_get_file(`test_cert_overrides_read_only/${KEY_DB_NAME}`);
+  srcKeyDBFile.copyTo(profile, KEY_DB_NAME);
+  let srcCertDBFile = do_get_file(`test_cert_overrides_read_only/${CERT_DB_NAME}`);
+  srcCertDBFile.copyTo(profile, CERT_DB_NAME);
+
+  // set the databases to read-only
+  let keyDBFile = do_get_profile();
+  keyDBFile.append(KEY_DB_NAME);
+  keyDBFile.permissions = 0o400;
+  let certDBFile = do_get_profile();
+  certDBFile.append(CERT_DB_NAME);
+  certDBFile.permissions = 0o400;
+
+  Services.prefs.setIntPref("security.OCSP.enabled", 1);
+  // Specifying false as the last argument means we don't try to add the default
+  // test root CA (which would fail).
+  add_tls_server_setup("BadCertServer", "bad_certs", false);
+
+  let fakeOCSPResponder = new HttpServer();
+  fakeOCSPResponder.registerPrefixHandler("/", function (request, response) {
+    response.setStatusLine(request.httpVersion, 500, "Internal Server Error");
+  });
+  fakeOCSPResponder.start(8888);
+
+  // Since we can't add the root CA to the (read-only) trust db, all of these
+  // will result in an "unknown issuer error" and need the "untrusted" error bit
+  // set in addition to whatever other specific error bits are necessary.
+  add_read_only_cert_override_test("expired.example.com",
+                         Ci.nsICertOverrideService.ERROR_TIME |
+                         Ci.nsICertOverrideService.ERROR_UNTRUSTED,
+                         SEC_ERROR_UNKNOWN_ISSUER);
+  add_read_only_cert_override_test("selfsigned.example.com",
+                         Ci.nsICertOverrideService.ERROR_UNTRUSTED,
+                         SEC_ERROR_UNKNOWN_ISSUER);
+  add_read_only_cert_override_test("mismatch.example.com",
+                         Ci.nsICertOverrideService.ERROR_MISMATCH |
+                         Ci.nsICertOverrideService.ERROR_UNTRUSTED,
+                         SEC_ERROR_UNKNOWN_ISSUER);
+
+  add_test(function () {
+    fakeOCSPResponder.stop(run_next_test);
+  });
+
+  run_next_test();
+}

security/manager/ssl/tests/unit/xpcshell.ini

@@ -10,6 +10,7 @@ support-files =
   test_cert_embedded_null/**
   test_cert_isBuiltInRoot_reload/**
   test_cert_keyUsage/**
+  test_cert_overrides_read_only/**
   test_cert_sha1/**
   test_cert_signatures/**
   test_cert_trust/**
@@ -57,6 +58,8 @@ run-sequentially = hardcoded ports
 [test_cert_isBuiltInRoot_reload.js]
 [test_cert_overrides.js]
 run-sequentially = hardcoded ports
+[test_cert_overrides_read_only.js]
+run-sequentially = hardcoded ports
 [test_cert_override_bits_mismatches.js]
 run-sequentially = hardcoded ports
 [test_cert_sha1.js]