From 854efb98510c8c3fe7c7b1fb3635cbd34fd12f1c Mon Sep 17 00:00:00 2001 From: Mark Goodwin Date: Wed, 18 Nov 2015 11:53:54 +0000 Subject: [PATCH] Bug 1224467 - Add a preference for controlling whether oneCRL blocklists are updated via AMO. Also add a test. r=keeler,mossop --- security/manager/ssl/CertBlocklist.cpp | 18 ++ security/manager/ssl/CertBlocklist.h | 1 + .../ssl/tests/unit/test_cert_blocklist.js | 202 +++++++++++------- .../manager/ssl/tests/unit/test_ev_certs.js | 19 ++ .../mozapps/extensions/nsBlocklistService.js | 13 +- 5 files changed, 171 insertions(+), 82 deletions(-) diff --git a/security/manager/ssl/CertBlocklist.cpp b/security/manager/ssl/CertBlocklist.cpp index 016e885dace7..a1fe5d38bf4f 100644 --- a/security/manager/ssl/CertBlocklist.cpp +++ b/security/manager/ssl/CertBlocklist.cpp @@ -31,11 +31,13 @@ using namespace mozilla::pkix; #define PREF_BACKGROUND_UPDATE_TIMER "app.update.lastUpdateTime.blocklist-background-update-timer" #define PREF_MAX_STALENESS_IN_SECONDS "security.onecrl.maximum_staleness_in_seconds" +#define PREF_ONECRL_VIA_AMO "security.onecrl.via.amo" static PRLogModuleInfo* gCertBlockPRLog; uint32_t CertBlocklist::sLastBlocklistUpdate = 0U; uint32_t CertBlocklist::sMaxStaleness = 0U; +bool CertBlocklist::sUseAMO = true; CertBlocklistItem::CertBlocklistItem(const uint8_t* DNData, size_t DNLength, @@ -139,6 +141,9 @@ CertBlocklist::~CertBlocklist() Preferences::UnregisterCallback(CertBlocklist::PreferenceChanged, PREF_MAX_STALENESS_IN_SECONDS, this); + Preferences::UnregisterCallback(CertBlocklist::PreferenceChanged, + PREF_ONECRL_VIA_AMO, + this); } nsresult @@ -167,6 +172,12 @@ CertBlocklist::Init() if (NS_FAILED(rv)) { return rv; } + rv = Preferences::RegisterCallbackAndCall(CertBlocklist::PreferenceChanged, + PREF_ONECRL_VIA_AMO, + this); + if (NS_FAILED(rv)) { + return rv; + } // Get the profile directory rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, @@ -606,6 +617,11 @@ CertBlocklist::IsBlocklistFresh(bool* _retval) { MutexAutoLock lock(mMutex); *_retval = false; + if (!sUseAMO) { + // for the time being, if we're not using AMO data, assume the blocklist is + // not fresh (in particular, prevent OneCRL OCSP bypass). + return NS_OK; + } uint32_t now = uint32_t(PR_Now() / PR_USEC_PER_SEC); @@ -638,5 +654,7 @@ CertBlocklist::PreferenceChanged(const char* aPref, void* aClosure) } else if (strcmp(aPref, PREF_MAX_STALENESS_IN_SECONDS) == 0) { sMaxStaleness = Preferences::GetUint(PREF_MAX_STALENESS_IN_SECONDS, uint32_t(0)); + } else if (strcmp(aPref, PREF_ONECRL_VIA_AMO) == 0) { + sUseAMO = Preferences::GetBool(PREF_ONECRL_VIA_AMO, true); } } diff --git a/security/manager/ssl/CertBlocklist.h b/security/manager/ssl/CertBlocklist.h index 2cad45eef4b0..0f811e105969 100644 --- a/security/manager/ssl/CertBlocklist.h +++ b/security/manager/ssl/CertBlocklist.h @@ -81,6 +81,7 @@ protected: static void PreferenceChanged(const char* aPref, void* aClosure); static uint32_t sLastBlocklistUpdate; static uint32_t sMaxStaleness; + static bool sUseAMO; virtual ~CertBlocklist(); }; diff --git a/security/manager/ssl/tests/unit/test_cert_blocklist.js b/security/manager/ssl/tests/unit/test_cert_blocklist.js index 06aa6e43d789..f0d625610b3a 100644 --- a/security/manager/ssl/tests/unit/test_cert_blocklist.js +++ b/security/manager/ssl/tests/unit/test_cert_blocklist.js @@ -82,8 +82,7 @@ var certDB = Cc["@mozilla.org/security/x509certdb;1"] // set up a test server to serve the blocklist.xml var testserver = new HttpServer(); -var blocklist_contents = - "" + +var initialBlocklist = "" + "" + // test with some bad data ... "" + @@ -117,10 +116,28 @@ var blocklist_contents = "" + ""; -testserver.registerPathHandler("/push_blocked_cert/", - function serveResponse(request, response) { - response.write(blocklist_contents); - }); + +var updatedBlocklist = "" + + "" + + "" + + "" + + "and the serial number" + + "" + + +var blocklists = { + "/initialBlocklist/" : initialBlocklist, + "/updatedBlocklist/" : updatedBlocklist +} + +function serveResponse(request, response) { + do_print("Serving for path " + request.path + "\n"); + response.write(blocklists[request.path]); +} + +for (path in blocklists) { + testserver.registerPathHandler(path, serveResponse); +} // start the test server testserver.start(-1); @@ -170,6 +187,43 @@ function test_is_revoked(certList, issuerString, serialString, subjectString, pubKeyString ? pubKeyString.length : 0); } +function fetch_blocklist(blocklistPath) { + do_print("path is " + blocklistPath + "\n"); + let certblockObserver = { + observe: function(aSubject, aTopic, aData) { + Services.obs.removeObserver(this, "blocklist-updated"); + run_next_test(); + } + } + + Services.obs.addObserver(certblockObserver, "blocklist-updated", false); + Services.prefs.setCharPref("extensions.blocklist.url", + `http://localhost:${port}/${blocklistPath}`); + let blocklist = Cc["@mozilla.org/extensions/blocklist;1"] + .getService(Ci.nsITimerCallback); + blocklist.notify(null); +} + +function check_revocations_txt_contents(expected) { + let profile = do_get_profile(); + let revocations = profile.clone(); + revocations.append("revocations.txt"); + ok(revocations.exists(), "the revocations file should exist"); + let inputStream = Cc["@mozilla.org/network/file-input-stream;1"] + .createInstance(Ci.nsIFileInputStream); + inputStream.init(revocations,-1, -1, 0); + inputStream.QueryInterface(Ci.nsILineInputStream); + let contents = ""; + let hasmore = false; + do { + var line = {}; + hasmore = inputStream.readLine(line); + contents = contents + (contents.length == 0 ? "" : "\n") + line.value; + } while (hasmore); + + equal(contents, expected, "revocations.txt should be as expected"); +} + function run_test() { // import the certificates we need load_cert("test-ca", "CTu,CTu,CTu"); @@ -177,61 +231,64 @@ function run_test() { load_cert("other-test-ca", "CTu,CTu,CTu"); let certList = Cc["@mozilla.org/security/certblocklist;1"] - .getService(Ci.nsICertBlocklist); + .getService(Ci.nsICertBlocklist); - // check some existing items in revocations.txt are blocked. Since the - // CertBlocklistItems don't know about the data they contain, we can use - // arbitrary data (not necessarily DER) to test if items are revoked or not. - // This test corresponds to: - // issuer: c29tZSBpbWFnaW5hcnkgaXNzdWVy - // serial: c2VyaWFsLg== - ok(test_is_revoked(certList, "some imaginary issuer", "serial."), - "issuer / serial pair should be blocked"); + let expected = "# Auto generated contents. Do not edit.\n" + + "MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5\n"+ + "\tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=\n"+ + "MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=\n" + + " BVio/iQ21GCi2iUven8oJ/gae74=\n" + + "MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E=\n" + + " exJUIJpq50jgqOwQluhVrAzTF74=\n" + + "YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy\n" + + " YW5vdGhlciBzZXJpYWwu\n" + + " c2VyaWFsMi4="; - // This test corresponds to: - // issuer: YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy - // serial: c2VyaWFsLg== - ok(test_is_revoked(certList, "another imaginary issuer", "serial."), - "issuer / serial pair should be blocked"); + add_test(function () { + // check some existing items in revocations.txt are blocked. Since the + // CertBlocklistItems don't know about the data they contain, we can use + // arbitrary data (not necessarily DER) to test if items are revoked or not. + // This test corresponds to: + // issuer: c29tZSBpbWFnaW5hcnkgaXNzdWVy + // serial: c2VyaWFsLg== + ok(test_is_revoked(certList, "some imaginary issuer", "serial."), + "issuer / serial pair should be blocked"); - // And this test corresponds to: - // issuer: YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy - // serial: c2VyaWFsMi4= - // (we test this issuer twice to ensure we can read multiple serials) - ok(test_is_revoked(certList, "another imaginary issuer", "serial2."), - "issuer / serial pair should be blocked"); + // This test corresponds to: + // issuer: YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy + // serial: c2VyaWFsLg== + ok(test_is_revoked(certList, "another imaginary issuer", "serial."), + "issuer / serial pair should be blocked"); - // Soon we'll load a blocklist which revokes test-int.pem, which issued - // test-int-ee.pem. - // Check the cert validates before we load the blocklist - let file = "test_onecrl/test-int-ee.pem"; - verify_cert(file, PRErrorCodeSuccess); + // And this test corresponds to: + // issuer: YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy + // serial: c2VyaWFsMi4= + // (we test this issuer twice to ensure we can read multiple serials) + ok(test_is_revoked(certList, "another imaginary issuer", "serial2."), + "issuer / serial pair should be blocked"); - // The blocklist also revokes other-test-ca.pem, which issued other-ca-ee.pem. - // Check the cert validates before we load the blocklist - file = "bad_certs/other-issuer-ee.pem"; - verify_cert(file, PRErrorCodeSuccess); + // Soon we'll load a blocklist which revokes test-int.pem, which issued + // test-int-ee.pem. + // Check the cert validates before we load the blocklist + let file = "test_onecrl/test-int-ee.pem"; + verify_cert(file, PRErrorCodeSuccess); - // The blocklist will revoke same-issuer-ee.pem via subject / pubKeyHash. - // Check the cert validates before we load the blocklist - file = "test_onecrl/same-issuer-ee.pem"; - verify_cert(file, PRErrorCodeSuccess); + // The blocklist also revokes other-test-ca.pem, which issued + // other-ca-ee.pem. Check the cert validates before we load the blocklist + file = "bad_certs/other-issuer-ee.pem"; + verify_cert(file, PRErrorCodeSuccess); + + // The blocklist will revoke same-issuer-ee.pem via subject / pubKeyHash. + // Check the cert validates before we load the blocklist + file = "test_onecrl/same-issuer-ee.pem"; + verify_cert(file, PRErrorCodeSuccess); + + run_next_test(); + }); // blocklist load is async so we must use add_test from here add_test(function() { - let certblockObserver = { - observe: function(aSubject, aTopic, aData) { - Services.obs.removeObserver(this, "blocklist-updated"); - run_next_test(); - } - } - - Services.obs.addObserver(certblockObserver, "blocklist-updated", false); - Services.prefs.setCharPref("extensions.blocklist.url", "http://localhost:" + - port + "/push_blocked_cert/"); - let blocklist = Cc["@mozilla.org/extensions/blocklist;1"] - .getService(Ci.nsITimerCallback); - blocklist.notify(null); + fetch_blocklist("initialBlocklist/"); }); add_test(function() { @@ -255,32 +312,7 @@ function run_test() { // Check the blocklist entry has been persisted properly to the backing // file - let profile = do_get_profile(); - let revocations = profile.clone(); - revocations.append("revocations.txt"); - ok(revocations.exists(), "the revocations file should exist"); - let inputStream = Cc["@mozilla.org/network/file-input-stream;1"] - .createInstance(Ci.nsIFileInputStream); - inputStream.init(revocations,-1, -1, 0); - inputStream.QueryInterface(Ci.nsILineInputStream); - let contents = ""; - let hasmore = false; - do { - var line = {}; - hasmore = inputStream.readLine(line); - contents = contents + (contents.length == 0 ? "" : "\n") + line.value; - } while (hasmore); - let expected = "# Auto generated contents. Do not edit.\n" + - "MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5\n"+ - "\tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=\n"+ - "MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=\n" + - " BVio/iQ21GCi2iUven8oJ/gae74=\n" + - "MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E=\n" + - " exJUIJpq50jgqOwQluhVrAzTF74=\n" + - "YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy\n" + - " YW5vdGhlciBzZXJpYWwu\n" + - " c2VyaWFsMi4="; - equal(contents, expected, "revocations.txt should be as expected"); + check_revocations_txt_contents(expected); // Check the blocklisted intermediate now causes a failure let file = "test_onecrl/test-int-ee.pem"; @@ -315,6 +347,18 @@ function run_test() { run_next_test(); }); - // we need to start the async portions of the test + // disable AMO cert blocklist - and check blocklist.xml changes do not + // affect the data stored. + add_test(function() { + Services.prefs.setBoolPref("security.onecrl.via.amo", false); + fetch_blocklist("updatedBlocklist/"); + }); + + add_test(function() { + // Check the blocklist entry has not changed + check_revocations_txt_contents(expected); + run_next_test(); + }); + run_next_test(); } diff --git a/security/manager/ssl/tests/unit/test_ev_certs.js b/security/manager/ssl/tests/unit/test_ev_certs.js index 73eb049a19af..8753145a7e42 100644 --- a/security/manager/ssl/tests/unit/test_ev_certs.js +++ b/security/manager/ssl/tests/unit/test_ev_certs.js @@ -187,6 +187,25 @@ function run_test() { ocspResponder.stop(run_next_test); }); + add_test(function () { + // test that setting "security.onecrl.via.amo" to false will prevent + // OCSP skipping + Services.prefs.setBoolPref("security.onecrl.via.amo", false); + // enable OneCRL OCSP skipping - allow staleness of up to 30 hours + Services.prefs.setIntPref("security.onecrl.maximum_staleness_in_seconds", 108000); + // set the blocklist-background-update-timer value to the recent past + Services.prefs.setIntPref("app.update.lastUpdateTime.blocklist-background-update-timer", + Math.floor(Date.now() / 1000) - 1); + clearOCSPCache(); + // the intermediate should have an associated OCSP request + let ocspResponder = start_ocsp_responder( + gEVExpected ? ["int-ev-valid", "ev-valid"] + : ["ev-valid"]); + check_ee_for_ev("ev-valid", gEVExpected); + Services.prefs.clearUserPref("security.onecrl.maximum_staleness_in_seconds"); + ocspResponder.stop(run_next_test); + }); + // Test the EV continues to work with flags after successful EV verification add_test(function () { clearOCSPCache(); diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js index b0eee1109822..04d225416f5e 100644 --- a/toolkit/mozapps/extensions/nsBlocklistService.js +++ b/toolkit/mozapps/extensions/nsBlocklistService.js @@ -46,6 +46,7 @@ const PREF_BLOCKLIST_LEVEL = "extensions.blocklist.level"; const PREF_BLOCKLIST_PINGCOUNTTOTAL = "extensions.blocklist.pingCountTotal"; const PREF_BLOCKLIST_PINGCOUNTVERSION = "extensions.blocklist.pingCountVersion"; const PREF_BLOCKLIST_SUPPRESSUI = "extensions.blocklist.suppressUI"; +const PREF_ONECRL_VIA_AMO = "security.onecrl.via.amo"; const PREF_PLUGINS_NOTIFYUSER = "plugins.update.notifyUser"; const PREF_GENERAL_USERAGENT_LOCALE = "general.useragent.locale"; const PREF_APP_DISTRIBUTION = "distribution.id"; @@ -890,6 +891,8 @@ Blocklist.prototype = { return; } + var populateCertBlocklist = getPref("getBoolPref", PREF_ONECRL_VIA_AMO, true); + var childNodes = doc.documentElement.childNodes; for (let element of childNodes) { if (!(element instanceof Ci.nsIDOMElement)) @@ -915,8 +918,10 @@ Blocklist.prototype = { this._handlePluginItemNode); break; case "certItems": - this._processItemNodes(element.childNodes, "cert", - this._handleCertItemNode.bind(this)); + if (populateCertBlocklist) { + this._processItemNodes(element.childNodes, "cert", + this._handleCertItemNode.bind(this)); + } break; default: Services.obs.notifyObservers(element, @@ -924,7 +929,9 @@ Blocklist.prototype = { null); } } - gCertBlocklistService.saveEntries(); + if (populateCertBlocklist) { + gCertBlocklistService.saveEntries(); + } } catch (e) { LOG("Blocklist::_loadBlocklistFromFile: Error constructing blocklist " + e);