Closed Bug 1024809 (OneCRL) Opened 10 years ago Closed 10 years ago

Add Revoked Intermediate Certs to revocation list push mechanism

Categories

(Core :: Security: PSM, enhancement)

Product:
Core
Component:
Security: PSM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
relnote-firefox --- 37+

People

(Reporter: kathleen.a.wilson, Assigned: mgoodwin)

References

Details

Attachments

(1 file, 27 obsolete files)

bug1024809-blocklist-certificate-revocation-mechanism.patch
181.08 KB, patch
mgoodwin
: review+
Details | Diff | Splinter Review
This bug tracks adding previously revoked intermediate certs to a revocation list push mechanism in Firefox. https://wiki.mozilla.org/CA:Communications#July_30.2C_2013 "5. ... As part of this effort, Mozilla will be implementing a revocation list push mechanism in Firefox, which will push revocation lists of intermediate certificates to Firefox browsers on a regular basis, asynchronously and independently of any SSL site visit. This will improve security by ensuring the browser has a comprehensive list of revocations in a manner that is not likely to be blocked by a network attacker. More information will follow, and policy will be added soon to require CAs to send Mozilla revocation information. We encourage CAs to start participating in this effort now by sending Mozilla previously revoked intermediate certificates..."
Alias: RevokedIntermediates
Depends on: 907347, 905557, 906611, 901941
Assignee: nobody → hpathak
Attachment #8446674 - Flags: review?(dkeeler)
Comment on attachment 8446674 [details] [diff] [review] Bug_1024809_push_revoked_intermediate_certs.diff Review of attachment 8446674 [details] [diff] [review]: ----------------------------------------------------------------- This is a good start. Don't forget to write and attach some tests. See comments for what needs to be addressed. ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +54,5 @@ > const VULNERABILITYSTATUS_NO_UPDATE = 2; > > const EXTENSION_BLOCK_FILTERS = ["id", "name", "creator", "homepageURL", "updateURL"]; > > +const certCache = Cc["@mozilla.org/security/nsscertcache;1"] nit: 'var gCertCache' would probably be better @@ +56,5 @@ > const EXTENSION_BLOCK_FILTERS = ["id", "name", "creator", "homepageURL", "updateURL"]; > > +const certCache = Cc["@mozilla.org/security/nsscertcache;1"] > + .getService(Ci.nsINSSCertCache); > +var gLoggingEnabled = true; Don't change gLoggingEnabled for the whole file - there's a way to conditionally enable it while developing in about:config: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#271 @@ +838,5 @@ > this._pluginEntries = this._processItemNodes(element.childNodes, "plugin", > this._handlePluginItemNode); > break; > + case "certItems": > + this._certEntries = this._processItemNodes(element.childNodes, "cert", nit: trailing space @@ +845,5 @@ > default: > Services.obs.notifyObservers(element, > "blocklist-data-" + element.localName, > null); > + LOG("\n No certificate revocation information found. "); nit: this line will run if no "em", plugin, or cert items are found (I would just take it out) @@ +869,5 @@ > } > return result; > }, > > + _handleCertItemNode: function Blocklist_handleCertItemNode(blocklistElement, result) { this function should start with an indent of 2 spaces, not 3 @@ +871,5 @@ > }, > > + _handleCertItemNode: function Blocklist_handleCertItemNode(blocklistElement, result) { > + > + var certDB2, certDB, certList, enumerator, cert_trust = "p,p,p", found = 0, certInfo; nit: declare these where they're used @@ +873,5 @@ > + _handleCertItemNode: function Blocklist_handleCertItemNode(blocklistElement, result) { > + > + var certDB2, certDB, certList, enumerator, cert_trust = "p,p,p", found = 0, certInfo; > + var cert = blocklistElement.textContent; > + cert = cert.replace('-----BEGIN CERTIFICATE-----',''); It would be nice if stripping out the BEGIN/END CERTIFICATE and whitespace parts of the PEM encoding were part of a utility library - CertUtils.jsm, for example. @@ +878,5 @@ > + cert = cert.replace('-----END CERTIFICATE-----',''); > + cert = cert.replace(/\s+/g, ''); > + > + /* > + Try getting a connection to the certDB It's actually not a database in that sense - it's just a collection of certificates. @@ +882,5 @@ > + Try getting a connection to the certDB > + */ > + try { > + certDB2 = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB2); > + certDB = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB); certDB is actually a nsIX509CertDB2 as well as a nsIX509CertDB, so you only need one of these (and, after your patch to merge them, you really only need one of them) @@ +885,5 @@ > + certDB2 = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB2); > + certDB = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB); > + } > + catch(e) { > + LOG("\n Unable to get connection to certificate database \n" + e); This shouldn't fail - if it does, it's catastropic and we shouldn't continue. Also, the logging style seems to be: LOG("Blocklist::_handleCertItemNode: <error message>"); (with no need for a carriage-return at the end) @@ +895,5 @@ > + certList = certCache.getX509CachedCerts(); > + enumerator = certList.getEnumerator(); > + while (enumerator.hasMoreElements()) { > + certInfo = enumerator.getNext().QueryInterface(Ci.nsIX509Cert); > + if ((certInfo.sha1Fingerprint == temp_x509.sha1Fingerprint) || watch out for trailing whitespace @@ +899,5 @@ > + if ((certInfo.sha1Fingerprint == temp_x509.sha1Fingerprint) || > + ((certInfo.issuerName == temp_x509.issuerName) && > + (certInfo.serialNumber == temp_x509.serialNumber))) { > + > + certDB.setCertTrustFromString(temp_x509,cert_trust); nit: have a space after each comma in argument lists Also, indent two spaces from where the conditional block opens Also, shouldn't this be "certInfo", not "temp_x509"? @@ +900,5 @@ > + ((certInfo.issuerName == temp_x509.issuerName) && > + (certInfo.serialNumber == temp_x509.serialNumber))) { > + > + certDB.setCertTrustFromString(temp_x509,cert_trust); > + found = 1; use true/flse @@ +903,5 @@ > + certDB.setCertTrustFromString(temp_x509,cert_trust); > + found = 1; > + break; > + } > + }//end while space after } @@ +906,5 @@ > + } > + }//end while > + > + if(!found) { > + certDB2.addCertFromBase64(cert,cert_trust,"revoked"); I still don't understand why we can't call this directly. Would you mind writing an xpcshell test that illustrates what the issue is if we just call addCertFromBase64 without searching for a preexisting cert first?
Attachment #8446674 - Flags: review?(dkeeler) → review-
Added xpcshell tests.
Attachment #8446674 - Attachment is obsolete: true
Attachment #8449754 - Flags: review?(dkeeler)
Comment on attachment 8449754 [details] [diff] [review] Bug_1024809_push_revoked_intermediate_certs.diff Review of attachment 8449754 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, but still some issues to address. I think the issue with the tests was the need to call clearSessionCache. Also, as we discussed on IRC, we should really figure out what's going wrong with the interaction between constructX509FromBase64 / setCertTrustFromString / built-in roots. ::: browser/app/blocklist.xml @@ +2259,5 @@ > </devices> > <feature>DIRECT3D_9_LAYERS</feature> <featureStatus>BLOCKED_DEVICE</featureStatus> </gfxBlacklistEntry> > <gfxBlacklistEntry blockID="g511"> <os>WINNT 5.1</os> <vendor>0x8086</vendor> <feature>DIRECT3D_9_LAYERS, WEBGL_ANGLE</feature> <featureStatus>BLOCKED_DRIVER_VERSION</featureStatus> <driverVersion>6.14.10.5218</driverVersion> <driverVersionComparator>LESS_THAN</driverVersionComparator> </gfxBlacklistEntry> > </gfxItems> > + <certItems> Does the blocklist service fail if these stub entries aren't present? ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +1,1 @@ > +/** Checks that the blocklist entries for certificates are added nit: modeline and license boilerplate: http://www.mozilla.org/MPL/headers/ https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line @@ +44,5 @@ > + check_cert_trust("Test CA"); > + }); > + > + /*This should fail as test-ca certificate is marked as distrusted.*/ > + add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK); Looks like I was mistaken about add_connection_test - it doesn't actually clear the session cache. Do something like this: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_ocsp_stapling.js#18 @@ +58,5 @@ > +/** > + * Auxiliary function to check if certificates are added. > + * Add as a test when required. > + */ > +function dump_certs() { Don't include functions that aren't going to be used when submitting patches. @@ +70,5 @@ > +} > + > +/** > + * Since the second add_connection_test passes after the first one, > + * probably due to asynchronous execution, this function will check Well, the tests are asynchronous in that control flow returns to the event handler, but the connections and each test run between an add_test and the next run_next_test are serial. In any case, I bet the problem is with not clearing the session cache (see above). @@ +78,5 @@ > + var cert = certDB.findCertByNickname(null, name); > + let hasEVPolicy = {}; > + let verifiedChain = {}; > + let error_code = certDB.verifyCertNow(cert, certificateUsageSSLServer, > + NO_FLAGS, verifiedChain, hasEVPolicy); nit: line NO_FLAGS up with cert ::: security/manager/ssl/tests/unit/test_cert_trust/test_cert_blocklist.xml @@ +1,4 @@ > +<?xml version="1.0" encoding="UTF-8"?> > +<blocklist xmlns="http://www.mozilla.org/2006/addons-blocklist"> > + <certItems> > + <certItem>MIIFazCCBFOgAwIBAgIQZlj6quBJzLpimKDL+e/FUTANBgkqhkiG9w0BAQUFADCBtTELMAkGA1UEBhMCVVMxFzAVBgNVBAoTDlZlcmlTaWduLCBJbmMuMR8wHQYDVQQLExZWZXJpU2lnbiBUcnVzdCBOZXR3b3JrMTswOQYDVQQLEzJUZXJtcyBvZiB1c2UgYXQgaHR0cHM6Ly93d3cudmVyaXNpZ24uY29tL3JwYSAoYykxMDEvMC0GA1UEAxMmVmVyaVNpZ24gQ2xhc3MgMyBTZWN1cmUgU2VydmVyIENBIC0gRzMwHhcNMTMwNjI1MDAwMDAwWhcNMTYwNjI0MjM1OTU5WjCBqDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFTATBgNVBAcUDFJlZHdvb2QgQ2l0eTEdMBsGA1UEChQURXZlcm5vdGUgQ29ycG9yYXRpb24xMzAxBgNVBAsUKlRlcm1zIG9mIHVzZSBhdCB3d3cudmVyaXNpZ24uY29tL3JwYSAoYykwNTEZMBcGA1UEAxQQd3d3LmV2ZXJub3RlLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAJoX7DliDqe8b+OQLs77V9Oe5H/uC7CAuUulUDRe6IWpkl2oIf3zfaPqpyaxgy9HOXUIT7c9BMgQAkjZWwY2CquT6bpo4KlgPlptQxHQ6UuE35KGitLxHGmobPEN49G9Cl5IoxXOLoV59eFU47sqLozx2TJxqaKO9jwG3/rUF8knfJN+bDNVXO99NBt5Y4MaNcPF9vrY0RNYeDME+tyK4z8lR/klc4JKS913A+8XNHMNDTdE9+f3nYtHGpXit7VKRHSCS0hU4JhxkIVMyREVZiRF5NmKVcZLGMgDs5EiUz07KThXz6/FzGzgz9EUmpjjIhRGH0eZuK6LlHY08fhFRosCAwEAAaOCAYAwggF8MBsGA1UdEQQUMBKCEHd3dy5ldmVybm90ZS5jb20wCQYDVR0TBAIwADAOBgNVHQ8BAf8EBAMCBaAwRQYDVR0fBD4wPDA6oDigNoY0aHR0cDovL1NWUlNlY3VyZS1HMy1jcmwudmVyaXNpZ24uY29tL1NWUlNlY3VyZUczLmNybDBDBgNVHSAEPDA6MDgGCmCGSAGG+EUBBzYwKjAoBggrBgEFBQcCARYcaHR0cHM6Ly93d3cudmVyaXNpZ24uY29tL2NwczAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwHwYDVR0jBBgwFoAUDURcFlNEwYJ+HSCrJfQBY9i+eaUwdgYIKwYBBQUHAQEEajBoMCQGCCsGAQUFBzABhhhodHRwOi8vb2NzcC52ZXJpc2lnbi5jb20wQAYIKwYBBQUHMAKGNGh0dHA6Ly9TVlJTZWN1cmUtRzMtYWlhLnZlcmlzaWduLmNvbS9TVlJTZWN1cmVHMy5jZXIwDQYJKoZIhvcNAQEFBQADggEBACvt6FDgh2S+qRrtr0gfeDqkoloQWKhLiOJ2ZMQSSv+S3netrTQ4+AHJ4urksj2WSCmsqJ/eiJgf71+8j4rJMTbwb6XW/ktACFZHbGV4rIvdpR6HvoPSZ0NT6s3n3lQpYiXNPOo+5bOlpE7eHyoXsma0RpnGEQtCagFY3nhFovOQKH38YO8IijoGUPzK227FI/NYSWNGOfyxT4k1bllV1vtcra2bNqzPnA3Aayi3RRBqR5Pbtcz7hB0Os5UO4JqV+rX0j3UUn3baG3q01yVuSUJox6aBE0aF2szXPxTQFCaTDDhtq4IBX6K130UxaDu6f2i4aE6e+1IUV1S26dyp9iE=</certItem> What are these certificates? How did you generate them? (see files like security/manager/ssl/tests/unit/psm_common_py/CertUtils.py and security/manager/ssl/tests/unit/test_cert_trust/generate.py for a way to guarantee we'll always have a way of generating certificates appropriate to a specific test) ::: security/manager/ssl/tests/unit/xpcshell.ini @@ +89,5 @@ > [test_ocsp_no_hsts_upgrade.js] > run-sequentially = hardcoded ports > # Bug 1009158: this test times out on Android > skip-if = os == "android" > +[test_cert_blocklist.js] \ No newline at end of file The following lines are necessary for any test that uses add_tls_server_setup: run-sequentially = hardcoded ports # Bug 1009158: this test times out on Android skip-if = os == "android" ::: testing/xpcshell/head.js @@ +13,1 @@ > var _quit = false; More than necessary was copied to this file. See what you can remove. For example, mapFile is basically a wrapper around server.registerPathHandler, and that path handler can be much simpler than what's here (you could just have the contents of test_cert_blocklist.xml be a string in test_cert_blocklist.js and serve that up, which would get rid of a lot of the added code here). createAppInfo might be trickier, but since only test_cert_blocklist.js needs it, I would just copy it there instead of here. (Have you confirmed that it is necessary?) ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +874,5 @@ > + var certDB = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); > + > + try { > + var cert_trust = "p,p,p", found = false; nit: declare variables each on their own line: var certTrust = ... var found = false; @@ +879,5 @@ > + var temp_x509 = certDB.constructX509FromBase64(blocklistElement.textContent); > + gCertCache.cacheAllCerts(); > + var certList = gCertCache.getX509CachedCerts(); > + var enumerator = certList.getEnumerator(); > + while(enumerator.hasMoreElements()) { nit: "while (..." @@ +881,5 @@ > + var certList = gCertCache.getX509CachedCerts(); > + var enumerator = certList.getEnumerator(); > + while(enumerator.hasMoreElements()) { > + let certInfo = enumerator.getNext().QueryInterface(Ci.nsIX509Cert); > + if((certInfo.sha1Fingerprint == temp_x509.sha1Fingerprint)|| nit: "if (...) ||" @@ +882,5 @@ > + var enumerator = certList.getEnumerator(); > + while(enumerator.hasMoreElements()) { > + let certInfo = enumerator.getNext().QueryInterface(Ci.nsIX509Cert); > + if((certInfo.sha1Fingerprint == temp_x509.sha1Fingerprint)|| > + ((certInfo.issuerName == temp_x509.issuerName)&& nit: ") &&" @@ +888,5 @@ > + certDB.setCertTrustFromString(certInfo, cert_trust); > + found = true; > + break; > + } > + }//end while nit: this comment isn't necessary
Attachment #8449754 - Flags: review?(dkeeler) → review-
I think we would still need startupManager and createAppInfo. Have removed extra code from these functions and moved them to test_cert_blocklist.js. The xml had additional certificates which i had downloaded. Have removed those and just kept the one, as a string, required for the test. Its the test-ca.der certificate used for TLS tests.
Attachment #8449754 - Attachment is obsolete: true
Attachment #8451823 - Flags: review?(dkeeler)
One thing to consider about this approach is that some users block access to AMO completely. I was told that enterprise installations are particularly likely to block access to AMO's servers via firewall configuration. Note that enterprise firewall configurations affect anybody using that network, not just the enterprise-IT-configured/operated systems. That is why I don't consider blocklist.xml to be a reliable (enough) delivery mechanism for this and why I think that the updates should be delivered from AUS or a separate server instead. Note also that some enterprises disable access to AUS too. Consequently, at a minimum we need to document how this works in the release notes and in our documentation for IT departments, so they can can (re)configure their firewalls and Firefox installations appropriately.
Comment on attachment 8451823 [details] [diff] [review] Bug_1024809_push_revoked_intermediate_certs.diff Review of attachment 8451823 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. As soon as the addCertFromBase64 bug is fixed and the nsBlocklistService changes are re-done, this should be just about ready to go. I've noted a few more nits to address. ::: browser/app/blocklist.xml @@ -2259,5 @@ > </devices> > <feature>DIRECT3D_9_LAYERS</feature> <featureStatus>BLOCKED_DEVICE</featureStatus> </gfxBlacklistEntry> > <gfxBlacklistEntry blockID="g511"> <os>WINNT 5.1</os> <vendor>0x8086</vendor> <feature>DIRECT3D_9_LAYERS, WEBGL_ANGLE</feature> <featureStatus>BLOCKED_DRIVER_VERSION</featureStatus> <driverVersion>6.14.10.5218</driverVersion> <driverVersionComparator>LESS_THAN</driverVersionComparator> </gfxBlacklistEntry> > </gfxItems> > - This file shouldn't need to be touched at all in this patch. ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +8,5 @@ > + * should fail after a successful update. The xml which contains the > + * certificate is a string and is returned as a reponse to blocklist.notify(); > + */ > +do_get_profile(); > +Cu.import("resource://testing-common/httpd.js"); This is already available to the tests in this directory: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/head_psm.js#13 @@ +56,5 @@ > + * TLS connections. > + */ > +var blocked_cert = "<?xml version=\"1.0\" encoding=\"UTF-8\"?><blocklist xmlns=\"http://www.mozilla.org/2006/addons-blocklist\"><certItems><certItem>MIIBtDCCAR2gAwIBAgIFAJ8fvcUwDQYJKoZIhvcNAQEFBQAwEjEQMA4GA1UEAxMHVGVzdCBDQTAeFw0xNDA0MDkyMTAzMzNaFw00NDA0MDkyMTAzMzNaMBIxEDAOBgNVBAMTB1Rlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAPMUVCW5/K2H0VddCdmMz+Q8eWdfzYru9Uq/rDo69q4jOdmMG5yFzqxMhrzHUCYaTgCdjafH8Q1SfnyQzXKpY50YLxK0r5TgldvIBzhquJ65DqKEUMoaHTQqifv/Ehwe9bzbkBW/gX6Ay9HKZltWvWwZjp4UuJWJ/3yOsNiZPRO1AgMBAAGjFjAUMBIGA1UdEwEB/wQIMAYBAf8CAQEwDQYJKoZIhvcNAQEFBQADgYEASgWk6gu0+fnA9bYcp+Z/eSj19bUuZ+C4PtdI5HWjo+2h9W1FFamDW6bHd2C/hwpsJq8PfJ9yrq10Q6Wv5F7Isq3mjdTS72sbRkfIfAMhiSJB2AyfwUsnKDb66pduHU5QO/cgUpJCaPUelGPvqsTOxOKTqul/ycG1/WEqgKQa3Xg=</certItem></certItems></blocklist>"; > +testserver.registerPathHandler("/push_blocked_cert/", > + function serveResponse(request, response) { just indent this function block 2 spaces from the top level @@ +64,5 @@ > +function load_blocklist() { > + Services.obs.addObserver(function() { > + Services.obs.removeObserver(arguments.callee, "blocklist-updated"); > + run_next_test(); > + clearSessionCache(); clearSessionCache shouldn't go here (in general nothing should come after a call to run_next_test). @@ +82,5 @@ > + load_blocklist(); > + }); > + > + add_test(function () { > + check_cert_trust("Test CA"); This shouldn't be necessary. @@ +86,5 @@ > + check_cert_trust("Test CA"); > + }); > + > + /*This will fail as test-ca certificate is marked as distrusted.*/ > + add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK); This shouldn't be Cr.NS_OK - the clearSessionCache call should be right before this, like so: add_test(function () { clearSessionCache(); run_next_test(); }); ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +880,5 @@ > + var temp_x509 = certDB.constructX509FromBase64(blocklistElement.textContent); > + gCertCache.cacheAllCerts(); > + var certList = gCertCache.getX509CachedCerts(); > + var enumerator = certList.getEnumerator(); > + while (enumerator.hasMoreElements()) { Once the addCertFromBase64 bug has been fixed, this won't be necessary. @@ +893,5 @@ > + } > + > + if(!found) { > + certDB.addCertFromBase64(blocklistElement.textContent, cert_trust, > + "revoked"); It turns out that the certdB.addCert... functions ignore the nickname argument. There is a bug on fixing it (bug 857627), but it's not clear what the right thing to do is. Let's just call this "ignored argument" for now.
Attachment #8451823 - Flags: review?(dkeeler) → review-
Attachment #8451823 - Attachment is obsolete: true
Attachment #8452901 - Flags: review?(dkeeler)
Comment on attachment 8452901 [details] [diff] [review] Bug_1024809_push_revoked_intermediate_certs.diff Review of attachment 8452901 [details] [diff] [review]: ----------------------------------------------------------------- This is just about ready to go. Just a few nits. I'd like to have another look at the final version, so r- for now. ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +10,5 @@ > + */ > +do_get_profile(); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +var certDB = Cc["@mozilla.org/security/x509certdb;1"] let, not var @@ +51,5 @@ > +/** > + * Serve the blocked certificate as a response. > + * These is the pem encoding of the test-ca.der used to test TLS connections. > + */ > +var blocked_cert = "<?xml version=\"1.0\" encoding=\"UTF-8\"?><blocklist xmlns=\"http://www.mozilla.org/2006/addons-blocklist\"><certItems><certItem>MIIBtDCCAR2gAwIBAgIFAJ8fvcUwDQYJKoZIhvcNAQEFBQAwEjEQMA4GA1UEAxMHVGVzdCBDQTAeFw0xNDA0MDkyMTAzMzNaFw00NDA0MDkyMTAzMzNaMBIxEDAOBgNVBAMTB1Rlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAPMUVCW5/K2H0VddCdmMz+Q8eWdfzYru9Uq/rDo69q4jOdmMG5yFzqxMhrzHUCYaTgCdjafH8Q1SfnyQzXKpY50YLxK0r5TgldvIBzhquJ65DqKEUMoaHTQqifv/Ehwe9bzbkBW/gX6Ay9HKZltWvWwZjp4UuJWJ/3yOsNiZPRO1AgMBAAGjFjAUMBIGA1UdEwEB/wQIMAYBAf8CAQEwDQYJKoZIhvcNAQEFBQADgYEASgWk6gu0+fnA9bYcp+Z/eSj19bUuZ+C4PtdI5HWjo+2h9W1FFamDW6bHd2C/hwpsJq8PfJ9yrq10Q6Wv5F7Isq3mjdTS72sbRkfIfAMhiSJB2AyfwUsnKDb66pduHU5QO/cgUpJCaPUelGPvqsTOxOKTqul/ycG1/WEqgKQa3Xg=</certItem></certItems></blocklist>"; It would be best if this were constructed from data read from tlsserver/default-ee.der ::: security/manager/ssl/tests/unit/xpcshell.ini @@ +92,5 @@ > # Bug 1009158: this test times out on Android > skip-if = os == "android" > +[test_cert_blocklist.js] > +run-sequentially = hardcoded ports > +# Bug 1024809: this test times out on Android Actually, this should read "Bug 1009158"
Attachment #8452901 - Flags: review?(dkeeler) → review-
Attachment #8452901 - Attachment is obsolete: true
Attachment #8453952 - Flags: review?(dkeeler)
Comment on attachment 8453952 [details] [diff] [review] Bug_1024809_push_revoked_intermediate_certs.diff Review of attachment 8453952 [details] [diff] [review]: ----------------------------------------------------------------- If my review comments are ever unclear, feel free to ask for clarification. ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +48,5 @@ > +} > + > +// Serve the blocked certificate as a response. > +// These is the pem encoding of the test-ca.der used to test TLS connections. > +var blocked_cert = "<?xml version=\"1.0\" encoding=\"UTF-8\"?><blocklist xmlns=\"http://www.mozilla.org/2006/addons-blocklist\"><certItems><certItem>MIIBtDCCAR2gAwIBAgIFAJ8fvcUwDQYJKoZIhvcNAQEFBQAwEjEQMA4GA1UEAxMHVGVzdCBDQTAeFw0xNDA0MDkyMTAzMzNaFw00NDA0MDkyMTAzMzNaMBIxEDAOBgNVBAMTB1Rlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAPMUVCW5/K2H0VddCdmMz+Q8eWdfzYru9Uq/rDo69q4jOdmMG5yFzqxMhrzHUCYaTgCdjafH8Q1SfnyQzXKpY50YLxK0r5TgldvIBzhquJ65DqKEUMoaHTQqifv/Ehwe9bzbkBW/gX6Ay9HKZltWvWwZjp4UuJWJ/3yOsNiZPRO1AgMBAAGjFjAUMBIGA1UdEwEB/wQIMAYBAf8CAQEwDQYJKoZIhvcNAQEFBQADgYEASgWk6gu0+fnA9bYcp+Z/eSj19bUuZ+C4PtdI5HWjo+2h9W1FFamDW6bHd2C/hwpsJq8PfJ9yrq10Q6Wv5F7Isq3mjdTS72sbRkfIfAMhiSJB2AyfwUsnKDb66pduHU5QO/cgUpJCaPUelGPvqsTOxOKTqul/ycG1/WEqgKQa3Xg=</certItem></certItems></blocklist>"; > @@ +51,5 @@ > > +/** > > + * Serve the blocked certificate as a response. > > + * These is the pem encoding of the test-ca.der used to test TLS connections. > > + */ > > +var blocked_cert = "<?xml version=\"1.0\" encoding=\"UTF-8\"?><blocklist xmlns=\"http://www.mozilla.org/2006/addons-blocklist\"><certItems><certItem>MIIBtDCCAR2gAwIBAgIFAJ8fvcUwDQYJKoZIhvcNAQEFBQAwEjEQMA4GA1UEAxMHVGVzdCBDQTAeFw0xNDA0MDkyMTAzMzNaFw00NDA0MDkyMTAzMzNaMBIxEDAOBgNVBAMTB1Rlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAPMUVCW5/K2H0VddCdmMz+Q8eWdfzYru9Uq/rDo69q4jOdmMG5yFzqxMhrzHUCYaTgCdjafH8Q1SfnyQzXKpY50YLxK0r5TgldvIBzhquJ65DqKEUMoaHTQqifv/Ehwe9bzbkBW/gX6Ay9HKZltWvWwZjp4UuJWJ/3yOsNiZPRO1AgMBAAGjFjAUMBIGA1UdEwEB/wQIMAYBAf8CAQEwDQYJKoZIhvcNAQEFBQADgYEASgWk6gu0+fnA9bYcp+Z/eSj19bUuZ+C4PtdI5HWjo+2h9W1FFamDW6bHd2C/hwpsJq8PfJ9yrq10Q6Wv5F7Isq3mjdTS72sbRkfIfAMhiSJB2AyfwUsnKDb66pduHU5QO/cgUpJCaPUelGPvqsTOxOKTqul/ycG1/WEqgKQa3Xg=</certItem></certItems></blocklist>"; > > It would be best if this were constructed from data read from > tlsserver/default-ee.der I guess that was unclear. I meant that the test should read_file("tlsserver/default-ee.der"); (or wherever it is), base-64 encode it, and use that as the contents of the blocklist. ::: security/manager/ssl/tests/unit/xpcshell.ini @@ +92,5 @@ > # Bug 1009158: this test times out on Android > skip-if = os == "android" > +[test_cert_blocklist.js] > +run-sequentially = hardcoded ports > +# Bug 1024809: this test times out on Android Again, this should be "Bug 1009158: this test times out on Android"
Attachment #8453952 - Flags: review?(dkeeler) → review-
Attachment #8453952 - Attachment is obsolete: true
Attachment #8454111 - Flags: review?(dkeeler)
Comment on attachment 8454111 [details] [diff] [review] Bug_1024809_push_revoked_intermediate_certs.diff Review of attachment 8454111 [details] [diff] [review]: ----------------------------------------------------------------- Great - r=me with nits addressed. Be sure to get additional review from an addons/blocklist peer. ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +5,5 @@ > + > +// Checks that the blocklist entries for certificates are added > +// as expected. Connection tests which use these certificates > +// should fail after a successful update. The xml which contains the > +// certificate is a string and is returned as a reponse to blocklist.notify(); nit: add a blank line after this comment @@ +12,5 @@ > + > +let certDB = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); > + > +var testserver = new HttpServer(); Use let instead of var everywhere in this file. @@ +51,5 @@ > +// certificate. > +var blocked_cert = btoa(readFile(do_get_file("tlsserver/test-ca.der"))); > +var blocklist_header = "<?xml version=\"1.0\" encoding=\"UTF-8\"?><blocklist xmlns=\"http://www.mozilla.org/2006/addons-blocklist\"><certItems><certItem>"; > +var blocklist_footer = "</certItem></certItems></blocklist>"; > +blocked_cert = blocklist_header + blocked_cert + blocklist_footer; declare a new variable "blocklist_contents" instead of reusing blocked_cert @@ +72,5 @@ > + blocklist.notify(null); > +} > + > +function add_all_tests() { > + add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK); This really isn't the best domain to use - I would actually use ocsp-stapling-none.example.com with OCSPStaplingServer instead of BadCertServer. I would add a comment above add_tls_server_setup that says something like "Even though we're using OCSPStaplingServer, the only host we connect to is ocsp-stapling-none.example.com, so stapling shouldn't affect this test at all."
Attachment #8454111 - Flags: review?(dkeeler) → review+
Hi Blair, as discussed via e-mails, could you please review this change to the blocklist file ?
Attachment #8454111 - Attachment is obsolete: true
Attachment #8454631 - Flags: review?(bmcbride)
Attachment #8454631 - Flags: review?(bmcbride) → review+
I think we are gonna need that notification. The blocklist does send the "blocklist-updated" notification at if (addonList.length == 0) { Services.obs.notifyObservers(self, "blocklist-updated", ""); but its probably async and doesn't add the timing delay we need.
Attachment #8454631 - Attachment is obsolete: true
Attachment #8462865 - Flags: review?(dkeeler)
Well, the "blocklist-update" notification does get sent after the addCert call completes. But its again the same NSS issue which is causing a problem. By the time it reaches the above condition, a reference gets gc'ed and hence the check for REVOKED_CERTIFICATE fails. Checked this by adding newCert->GetCert, which causes the test to pass.
Reverted to the original logic. Changed the test case to use an intermediate cert and vertifyCertNow(). Related changes to addcertfrombase64() and for the error code are in that bugs patch.
Attachment #8462865 - Attachment is obsolete: true
Attachment #8462865 - Flags: review?(dkeeler)
Attachment #8463594 - Flags: review?(dkeeler)
Comment on attachment 8463594 [details] [diff] [review] Bug1024809_push_revoked_intermediate_certs.diff Review of attachment 8463594 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling review based on the discussion we had on IRC. I'm leaving in what comments I made in case they're helpful in the future. ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +18,5 @@ > +// this test, the condition will always be true as no addons are part of the blocklist > +// file used for this test. Once the notification is sent, this will execute the > +// verifyCertNow again with a revoked intermediate. > +let certblockObserver = { > + observe : function(aSubject, aTopic, aData) { nit: "observe: function(...)" @@ +53,5 @@ > + registrar.registerFactory(XULAPPINFO_CID, "XULAppInfo", > + "@mozilla.org/xre/app-info;1", XULAppInfoFactory); > +} > + > +function verify_certs(usage) { maybe "verifyCert" @@ +57,5 @@ > +function verify_certs(usage) { > + let file = "test_intermediate_basic_usage_constraints/ee-int-limited-depth.der"; > + let cert_der = readFile(do_get_file(file)); > + let ee = certDB.constructX509(cert_der, cert_der.length); > + equal(usage, certDB.verifyCertNow(ee, certificateUsageSSLServer, nit: 'usage' isn't a good name for this parameter - maybe expectedError? @@ +61,5 @@ > + equal(usage, certDB.verifyCertNow(ee, certificateUsageSSLServer, > + NO_FLAGS, {}, {})); > +} > + > + nit: unnecessary blank line @@ +100,5 @@ > + > +function run_test() { > + createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1"); > + load_cert("ca", "CTu,CTu,CTu"); > + load_cert("int-limited-depth", "CTu,CTu,CTu"); this trust string should be ",," @@ +103,5 @@ > + load_cert("ca", "CTu,CTu,CTu"); > + load_cert("int-limited-depth", "CTu,CTu,CTu"); > + verify_certs(Cr.NS_OK); > + add_test(function() { > + generate_blocklist_contents(); generate_blocklist_contents is synchronous, so it shouldn't need to be wrapped in add_test or call run_next_test @@ +105,5 @@ > + verify_certs(Cr.NS_OK); > + add_test(function() { > + generate_blocklist_contents(); > + }); > + add_test( function () { nit: no space before/after "function" @@ +107,5 @@ > + generate_blocklist_contents(); > + }); > + add_test( function () { > + startupManager(); > + load_blocklist(); startupManager and load_blocklist are also synchronous, so same deal
Attachment #8463594 - Flags: review?(dkeeler)
Alias: RevokedIntermediates → OneCRL
Blocks: 1016555
Depends on: 1076940
Assignee: hpathak → mgoodwin
Attached patch Broken patch (obsolete) — — Splinter Review
Any idea what's up with appinfo? Try: ./mach xpcshell-test security/manager/ssl/tests/unit/test_cert_blocklist.js
Flags: needinfo?(mmc)
Attached file mark (obsolete) —
This one fails but later. The problem was that nsBlocklistService was being instantiated and the lazy getter being called before createAppInfo was done, so I moved that call to head_psm. I did not do any cleanup.
Flags: needinfo?(mmc)
This internet draft: http://datatracker.ietf.org/doc/draft-hallambaker-compressedcrlset/?include_text=1 seems very interesting; perhaps we could use the ideas here to significantly reduce the amount of data needed for OneCRL. Gerv
Attached patch Working work-in-progress (obsolete) — — Splinter Review
I've not nit-combed this yet, but could you take a quick look at a) the general approach and b) nsCertBlocklist(.h|.cpp) in case there are things I need to look at tomorrow?
Attachment #8463594 - Attachment is obsolete: true
Attachment #8506448 - Attachment is obsolete: true
Attachment #8506494 - Attachment is obsolete: true
Attachment #8517748 - Flags: feedback?(dkeeler)
Comment on attachment 8517748 [details] [diff] [review] Working work-in-progress Review of attachment 8517748 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - I don't see any structural issues. I made a few comments. Be sure to look at the style guidelines and similar code (although I realize there's code of just about every style in the tree). ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +340,5 @@ > if (endEntityOrCA == EndEntityOrCA::MustBeCA) { > maxOCSPLifetimeInDays = 365; > } > > + nsCOMPtr<nsICertBlocklist> certList(do_GetService(NS_CERTBLOCKLIST_CONTRACTID)); This could probably be a member of NSSCertDBTrustDomain itself, so we don't have to call do_GetService each time CheckRevocation is called. ::: security/manager/boot/src/nsCertBlocklist.cpp @@ +48,5 @@ > +} > + > +uint32_t nsCertBlocklistItem::Hash() const > +{ > + return *serialData; I'm not sure this is a great hash function - I think the last 4 bytes of the serial number are likely to be more well-distributed. ::: security/manager/boot/src/nsCertBlocklist.h @@ +34,5 @@ > + > +class nsCertBlocklist : public nsICertBlocklist > +{ > +public: > + NS_DECL_THREADSAFE_ISUPPORTS To actually be threadsafe, we need nsCertBlocklist to have a Mutex. See Mutex.h for Mutex and MutexAutoLock.
Attachment #8517748 - Flags: feedback?(dkeeler) → feedback+
Changes: 1) Moved the local certList to be a member of NSSCertDBTrustDomain (and renamed accordingly) 2) Fixed the hash function on nsCertBlocklistItem to use the last four bytes of the serialNumber when four or more bytes are available. 3) Used MutexAutoLocks for the AddBlockedCert and IsCertBlocked methods of nsCertBlocklist 4) Added tests to ensure that other chains still validate, other invalid certs are still rejected 5) Removed various unnecessary 'LOG's and 'dump's 6) Various style and formatting fixes. Any suggestions for suitable reviewers?
Attachment #8517748 - Attachment is obsolete: true
Flags: needinfo?(dkeeler)
(In reply to Mark Goodwin [:mgoodwin] from comment #24) > Any suggestions for suitable reviewers? Well, me :) :jcj should also have a look to get more familiar with this code. Someone like Blair (:Unfocused) should review the blocklist-related changes.
Flags: needinfo?(dkeeler)
Comment on attachment 8518240 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Please can you take a look?
Flags: needinfo?(jjones)
Attachment #8518240 - Flags: review?(dkeeler)
Attachment #8518240 - Flags: feedback?(bmcbride)
Comment on attachment 8518240 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8518240 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for tagging me so I could take a look at how this part CertVerifier works! There are a couple of nits below, but one structure question: Do we need to worry about downloading the blocklist more than once per session? E.g., should the AddBlockedCert method in nsCertBlocklist check for duplicates? ::: security/manager/boot/src/nsCertBlocklist.cpp @@ +1,1 @@ > +#include "nsCertBlocklist.h" Missing the MPL header. ::: security/manager/boot/src/nsCertBlocklist.h @@ +44,5 @@ > +protected: > + Mutex mMutex; > + virtual ~nsCertBlocklist(); > +}; > +#endif Nit: Typically headers put a end-line comment on the guard #endif, like #endif // __NSBLOCKEDCERTS_H__ ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1075,5 @@ > +// call the blocklistService to ensure that all revocation information > +// is updated in the nsCertBlocklist XPCOM component. > + nsCOMPtr<nsIBlocklistService> blocklist = > + do_GetService("@mozilla.org/extensions/blocklist;1"); > + if (blocklist) { nit: indentation weirdness
(In reply to J.C. Jones [:jcj] from comment #27) > Thanks for tagging me so I could take a look at how this part CertVerifier > works! Thanks for looking; always appreciated! > There are a couple of nits below I'll address the nits when others have commented. > but one structure question: Do we need to > worry about downloading the blocklist more than once per session? E.g., > should the AddBlockedCert method in nsCertBlocklist check for duplicates? We will be downloading the blocklist more than once per session (I think it's 24 hour intervals?) but this shouldn't be a problem; nsTHashtable used in this way is essentially a set (see no entry type) so I don't think we can have duplicates.
I'm not going to be able to get to this this week - I'll review it as soon as I can next week.
Comment on attachment 8518240 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8518240 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1077,5 @@ > + nsCOMPtr<nsIBlocklistService> blocklist = > + do_GetService("@mozilla.org/extensions/blocklist;1"); > + if (blocklist) { > + uint32_t unusedState = 0; > + blocklist->GetPluginBlocklistState(NULL, EmptyString(), This is happening on startup, right? Which would mean the blocklist is now synchronously force-loaded on every startup, where previously we avoided that wherever possible. We're adding more and more to the file, and it's not a compact format (XML) - this will undoubtedly cause startup time regressions. I think we have two options: * Gather data on how long it takes to synchronously load the blocklist, to determine whether we think it's a hit we can take; * Or save this data separately in a compact format for persistence between restarts.
Attachment #8518240 - Flags: feedback?(bmcbride) → feedback-
Flags: needinfo?(jjones)
My apologies, Blair, I had assumed conversations had already taken place about the basic approach for this (I inherited this bug mid-implementation). (In reply to Blair McBride [:Unfocused] from comment #30) > This is happening on startup, right? Not necessarily; I'll check the details, but keeler suggests this is lazy-loaded before the first HTTPS connection. > Which would mean the blocklist is now synchronously force-loaded on every > startup, where previously we avoided that wherever possible. We're adding > more and more to the file, and it's not a compact format (XML) - this will > undoubtedly cause startup time regressions. The amount we're adding could be an issue; my current count is something like 2.2k entries. This will likely grow. > I think we have two options: > * Gather data on how long it takes to synchronously load the blocklist, to > determine whether we think it's a hit we can take; I think we need this data in any case. I'll measure: 1) Startup impact, without revocation data 2) Startup impact with revocation data > * Or save this data separately in a compact format for persistence between > restarts. There are 3 possible outcomes, I think: 1) It's decided that it's a hit we can take and continue with the current approach 2) We try to use the blocklist mechanism for distribution but manage our own persistence 3) We separate this completely I'll get the data; that will inform any decision we make.
(In reply to Mark Goodwin [:mgoodwin] from comment #31) > The amount we're adding could be an issue; my current count is something > like 2.2k entries. This will likely grow. Uh, ouch. I was originally put under the impression that it would be orders of magnitude less than this :\ I highly doubt the current implementation of the blocklist will scale favourably to that degree.
mgoodwin: just to be clear, you are saying that Mozilla is aware of 2.2k unexpired revoked intermediate certificates? If this is a performance issue, we may need to bypass this initial implementation and move directly to what I understand is the final design - revocation based on issuer and serial number only, rather than on whole certificates. Gerv
(In reply to Gervase Markham [:gerv] from comment #33) > mgoodwin: just to be clear, you are saying that Mozilla is aware of 2.2k > unexpired revoked intermediate certificates? Based on the CRLs at the CRL locations retrieved from the intermediates used by the test sites in our builtin CA spreadsheet, yes. > If this is a performance issue, we may need to bypass this initial > implementation and move directly to what I understand is the final design - > revocation based on issuer and serial number only, rather than on whole > certificates. This implementation already relies only on issuer and serial number.
(In reply to Mark Goodwin [:mgoodwin] from comment #34) > This implementation already relies only on issuer and serial number. Oh, OK. I was under the impression that we were doing an interim implementation which required whole certificates first. Was that ever the plan, or am I hallucinating? Anyway, 2.2k issuers and serial numbers must be < 200kb of data, uncompressed. Hopefully we can find a way to manage that which isn't a performance issue... Gerv
(In reply to Mark Goodwin [:mgoodwin] from comment #34) > (In reply to Gervase Markham [:gerv] from comment #33) > > mgoodwin: just to be clear, you are saying that Mozilla is aware of 2.2k > > unexpired revoked intermediate certificates? > > Based on the CRLs at the CRL locations retrieved from the intermediates used > by the test sites in our builtin CA spreadsheet, yes. I would not be surprised if the vast majority of those 2.2k certs are long-lived end-entity certs that were issued before direct root issuance was banned by the Mozilla CA Certificate Policy.
(In reply to Gervase Markham [:gerv] from comment #21) > This internet draft: > http://datatracker.ietf.org/doc/draft-hallambaker-compressedcrlset/ > ?include_text=1 > seems very interesting; perhaps we could use the ideas here to significantly > reduce the amount of data needed for OneCRL. +1 (but then I am a co-author of that I-D ;-) ) (In reply to Gervase Markham [:gerv] from comment #35) <snip> > Anyway, 2.2k issuers and serial numbers must be < 200kb of data, > uncompressed. Hopefully we can find a way to manage that which isn't a > performance issue... Just to whet your appetite: With Compressed CRLSets we can cover revocation status for _all_ unexpired publicly-trusted end-entity certs in ~200kb of data. :-)
(In reply to Gervase Markham [:gerv] from comment #35) > Oh, OK. I was under the impression that we were doing an interim > implementation which required whole certificates first. Was that ever the > plan, or am I hallucinating? No, I had also heard this was originally the plan. It could be that the change here was due to data availability issues. (In reply to Rob Stradling from comment #37) > Just to whet your appetite: With Compressed CRLSets we can cover revocation > status for _all_ unexpired publicly-trusted end-entity certs in ~200kb of > data. :-) My reading of this was that we'd need the hashes of the full set of issued certificates for this to be viable; did I misread?
(In reply to Mark Goodwin [:mgoodwin] from comment #38) <snip> > (In reply to Rob Stradling from comment #37) > > Just to whet your appetite: With Compressed CRLSets we can cover revocation > > status for _all_ unexpired publicly-trusted end-entity certs in ~200kb of > > data. :-) > > My reading of this was that we'd need the hashes of the full set of issued > certificates for this to be viable; did I misread? That's correct. (BTW, should we move this discussion of Compressed CRLSets somewhere else?)
(In reply to Rob Stradling from comment #36) > I would not be surprised if the vast majority of those 2.2k certs are > long-lived end-entity certs that were issued before direct root issuance was > banned by the Mozilla CA Certificate Policy. From speaking with Kathleen, I think this is most likely the case; we're expecting 200-300 initially.
TL;DR - 2.5k additional entries has an impact (blocklist takes twice as long to load). (In reply to Blair McBride [:Unfocused] from comment #30) > ::: security/manager/ssl/src/nsNSSComponent.cpp > @@ +1077,5 @@ > > + nsCOMPtr<nsIBlocklistService> blocklist = > > + do_GetService("@mozilla.org/extensions/blocklist;1"); > > + if (blocklist) { > > + uint32_t unusedState = 0; > > + blocklist->GetPluginBlocklistState(NULL, EmptyString(), <snip/> > * Gather data on how long it takes to synchronously load the blocklist, to > determine whether we think it's a hit we can take I've done some work on this. For the 2.5(ish)k entries I have so far, the blocklist is increased from 140k to 288k. I measured the impact (on this part of nsNSSComponent - I don't think this is synchronous, on startup): An explanation: All timings are millis, "without" is the original blocklist, "with" includes the couple of thousand extra entries. I can get more detailed specs of the machines used. I went for 'slow', 'medium' and 'fast'. I did 10 runs for each, with and without. 'Mean' is the mean interval. 'SD' is standard deviation: Workstation (Big xeon, lots of RAM, conventional HDD) Without: Mean: 48.4 SD: 3.37 With: 88.9 SD: 1.91 Laptop (i7, lots of RAM, SSD) Without: Mean: 124.8 SD: 3.08 With: Mean: 232.45 SD: 7.2 Netbook (1GHz atom, 2Gb RAM, SSD) Without: Mean: 484.7 SD: 3.71 With: Mean: 964.3 SD: 19.68
(In reply to Mark Goodwin [:mgoodwin] from comment #40) > (In reply to Rob Stradling from comment #36) > > I would not be surprised if the vast majority of those 2.2k certs are > > long-lived end-entity certs that were issued before direct root issuance was > > banned by the Mozilla CA Certificate Policy. > > From speaking with Kathleen, I think this is most likely the case; we're > expecting 200-300 initially. (In reply to Mark Goodwin [:mgoodwin] from comment #41) > TL;DR - 2.5k additional entries has an impact (blocklist takes twice as long > to load). Then I suggest trying to remove as many of the long-lived end-entity certs from that list as possible. The public CT logs should help. Failing that, ask the CAs.
Comment on attachment 8518240 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8518240 [details] [diff] [review]: ----------------------------------------------------------------- This is great progress. There are a few issues to address, though. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +25,5 @@ > #include "ScopedNSSTypes.h" > #include "secerr.h" > #include "secmod.h" > +#include "nsCOMPtr.h" > +#include "nsServiceManagerUtils.h" nit: these should be inserted where they sort, not appended @@ +340,5 @@ > if (endEntityOrCA == EndEntityOrCA::MustBeCA) { > maxOCSPLifetimeInDays = 365; > } > > + if ( !mCertList ) { nit: "if (!mCertList) {" @@ +344,5 @@ > + if ( !mCertList ) { > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + > + bool rval; nit: I would call this 'isCertBlocked' @@ +345,5 @@ > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + > + bool rval; > + mCertList->IsCertBlocked((uint8_t *)certID.issuer.UnsafeGetData(), nit: in general, use c++-style casts: const_cast<uint8_t*>(certID.issuer.UnsafeGetData()) (however, in this case, see the comment in the idl file) @@ +349,5 @@ > + mCertList->IsCertBlocked((uint8_t *)certID.issuer.UnsafeGetData(), > + certID.issuer.GetLength(), > + (uint8_t *)certID.serialNumber.UnsafeGetData(), > + certID.serialNumber.GetLength(), > + &rval); nit: these should be lined up to appear after IsCertBlocked( ::: security/certverifier/NSSCertDBTrustDomain.h @@ +9,5 @@ > > #include "pkix/pkixtypes.h" > #include "secmodt.h" > #include "CertVerifier.h" > +#include "nsICertBlocklist.h" nit: sort these #includes alphabetically, please ::: security/manager/boot/src/nsCertBlocklist.cpp @@ +3,5 @@ > +#include "nsCRTGlue.h" > +#include "nsTHashtable.h" > +#include "nsIX509Cert.h" > +#include "prlog.h" > +#include "mozilla/Base64.h" nit: sort these (but keep nsCertBlocklist.h first, with a blank line after it) @@ +7,5 @@ > +#include "mozilla/Base64.h" > + > +NS_IMPL_ISUPPORTS(nsCertBlocklist, nsICertBlocklist) > + > +static PRLogModuleInfo* gCertBlockPRLog; I'm not sure we need an entirely new logging module - maybe just use the certverifier one? @@ +10,5 @@ > + > +static PRLogModuleInfo* gCertBlockPRLog; > + > +nsCertBlocklistItem::nsCertBlocklistItem(uint8_t * aIssuer, uint32_t aIssuer_length, > + uint8_t * aSerial, uint32_t aSerial_length) nit: uint8_t* Also, let's avoid underscores: aIssuerLength, etc. @@ +13,5 @@ > +nsCertBlocklistItem::nsCertBlocklistItem(uint8_t * aIssuer, uint32_t aIssuer_length, > + uint8_t * aSerial, uint32_t aSerial_length) > +{ > + issuerData = new uint8_t[aIssuer_length]; > + memcpy(aIssuer, issuerData, aIssuer_length); memcpy is destination-first, right? So shouldn't issuerData be first? @@ +14,5 @@ > + uint8_t * aSerial, uint32_t aSerial_length) > +{ > + issuerData = new uint8_t[aIssuer_length]; > + memcpy(aIssuer, issuerData, aIssuer_length); > + issuer.Init(issuerData, aIssuer_length); Input.Init can fail if the length of the data is too large. Can this constructor take two Inputs instead of two pointers/lengths? (that way, we know initializing new ones shouldn't fail) @@ +17,5 @@ > + memcpy(aIssuer, issuerData, aIssuer_length); > + issuer.Init(issuerData, aIssuer_length); > + > + serialData = new uint8_t[aSerial_length]; > + memcpy(aSerial, serialData, aSerial_length); same here with memcpy @@ +25,5 @@ > +nsCertBlocklistItem::nsCertBlocklistItem(const nsCertBlocklistItem& item) > +{ > + uint32_t issuer_length = item.issuer.GetLength(); > + issuerData = new uint8_t[issuer_length]; > + memcpy(item.issuerData, issuerData, issuer_length); same @@ +30,5 @@ > + issuer.Init(issuerData, issuer_length); > + > + uint32_t serial_length = item.serial.GetLength(); > + serialData = new uint8_t[serial_length]; > + memcpy(item.serialData, serialData, serial_length); same @@ +40,5 @@ > + delete issuerData; > + delete serialData; > +} > + > +bool nsCertBlocklistItem::operator==(const nsCertBlocklistItem& item) const nit: return value on previous line @@ +49,5 @@ > + > +uint32_t nsCertBlocklistItem::Hash() const > +{ > + uint32_t hash; > + uint32_t serial_length = serial.GetLength(); nit: avoid underscores in names: "serialLength" @@ +60,5 @@ > + } > + return hash; > +} > + > +nsCertBlocklist::nsCertBlocklist(): mMutex("nsCertBlocklist::mMutex") nit: initializations on their own lines: nsCertBlocklist::nsCertBlocklist() : mMutex("nsCertBlocklist::mMutex" { ... } @@ +62,5 @@ > +} > + > +nsCertBlocklist::nsCertBlocklist(): mMutex("nsCertBlocklist::mMutex") > +{ > + if (!gCertBlockPRLog) nit: braces around conditional bodies @@ +70,5 @@ > +nsCertBlocklist::~nsCertBlocklist() > +{ > +} > + > +/* void addBlockedCert (in string issuer, in string serialNumber); */ nit: use //-style comments @@ +84,5 @@ > + > + mozilla::Base64Decode(nsDependentCString(issuer), decodedIssuer); > + mozilla::Base64Decode(nsDependentCString(serialNumber), decodedSerial); > + > + nsCertBlocklistItem item((uint8_t*) decodedIssuer.get(), I think we should just pass in the Inputs directly here. @@ +93,5 @@ > + blocklist.PutEntry(item); > + return NS_OK; > +} > + > +/* boolean isCertBlocked([array, size_is(issuer_length)] in octet issuer, nit: //-style comments @@ +98,5 @@ > + * in unsigned long issuer_length, > + * [array, size_is(serial_length)] in octet serial, > + * in unsigned long serial_length); */ > +NS_IMETHODIMP nsCertBlocklist::IsCertBlocked(uint8_t * issuer, uint32_t issuer_length, > + uint8_t * serial, uint32_t serial_length, nit: uint8_t*, etc. @@ +102,5 @@ > + uint8_t * serial, uint32_t serial_length, > + bool *_retval) > +{ > + MutexAutoLock lock(mMutex); > + nsCertBlocklistItem item = nsCertBlocklistItem(issuer, issuer_length, nit: use the initialization style as in ::AddBlockedCert ::: security/manager/boot/src/nsCertBlocklist.h @@ +1,5 @@ > +/* 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/. */ > + > +#ifndef __NSBLOCKEDCERTS_H__ nit: I think this would be nsCertBlocklist_h @@ +8,5 @@ > +#include "nsICertBlocklist.h" > +#include "nsCOMPtr.h" > +#include "nsIX509CertDB.h" > +#include "mozilla/Mutex.h" > +#include "nsDataHashtable.h" nit: sort these @@ +41,5 @@ > + > +private: > + nsTHashtable<nsGenericHashKey<nsCertBlocklistItem> > blocklist; > +protected: > + Mutex mMutex; We should probably be consistent about naming member variables. Either have everything prefixed with 'm' or don't at all. ::: security/manager/ssl/public/nsICertBlocklist.idl @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- > + * nit: not sure this blank line is necessary @@ +18,5 @@ > +[scriptable, uuid(44b0ee42-1af3-45e7-b601-7f17bd67c5cc)] > +interface nsICertBlocklist : nsISupports { > + > + /** > + * Add details of a blocked certificate : issuer name and serial number. Add documentation that they're base-64 encoded. @@ +27,5 @@ > + * Check if a certificate is blocked. > + */ > + boolean isCertBlocked([array, size_is(issuer_length)] in octet issuer, > + in unsigned long issuer_length, > + [array, size_is(serial_length)] in octet serial, Can these be const? That way, we don't have to cast away the constness when calling this from NSSCertDBTrustDomain. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1077,5 @@ > + nsCOMPtr<nsIBlocklistService> blocklist = > + do_GetService("@mozilla.org/extensions/blocklist;1"); > + if (blocklist) { > + uint32_t unusedState = 0; > + blocklist->GetPluginBlocklistState(NULL, EmptyString(), So, we might be able to have nsICertBlocklist dispatch to the main thread and wait for the blocklist to initialize if it hasn't yet, the first time nsICertBlocklist is used (of course, this would be a straight initialization call if it's already on the main thread). @@ +1080,5 @@ > + uint32_t unusedState = 0; > + blocklist->GetPluginBlocklistState(NULL, EmptyString(), > + EmptyString(), &unusedState); > + } > + else { nit: "} else {" ::: security/manager/ssl/tests/unit/head_psm.js @@ +11,5 @@ > let { Services } = Cu.import("resource://gre/modules/Services.jsm", {}); > let { Promise } = Cu.import("resource://gre/modules/Promise.jsm", {}); > let { HttpServer } = Cu.import("resource://testing-common/httpd.js", {}); > let { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm"); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); nit: " let { XPCOMUtils } = ..." @@ +546,5 @@ > }, > } > + > +function createAppInfo(id, name, version, platformVersion) { > + var gAppInfo = { nit: let, not var @@ +569,5 @@ > + // nsICrashReporter > + annotations: {}, > + > + annotateCrashReport: function(key, data) { > + this.annotations[key] = data; we can probably do nothing here @@ +594,5 @@ > + registrar.registerFactory(XULAPPINFO_CID, "XULAppInfo", > + XULAPPINFO_CONTRACTID, XULAppInfoFactory); > +} > + > +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2"); Maybe this should be in a function that only tests that need it will call, rather than having this run for every xpcshell test in security/manager/ssl/... ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +32,5 @@ > + gInternalManager.observe(null, "addons-startup", null); > +} > + > +const XULAPPINFO_CONTRACTID = "@mozilla.org/xre/app-info;1"; > +const XULAPPINFO_CID = Components.ID("{c763b610-9d49-455a-bbd2-ede71682a1ac}"); These don't appear to be used in this file. @@ +66,5 @@ > + let derString = ''; > + for (let i = 0; i < derArray.length; i++) { > + derString += String.fromCharCode(derArray[i]); > + } > + let blocked_cert = btoa(derString); This doesn't appear to be used anymore.
Attachment #8518240 - Flags: review?(dkeeler) → review-
Talos runs confirm that initializing the blocklist at this point does affect startup time; even with no CRL data, relying on the blocklist service will slow startup by around half a second on my older machine. I'll implement a certificate blocklist cache.
Changes: Nits addressed (though more likely introduced) Modified nsNSSComponent.cpp to initialize the nsCertBlocklist, rather than nsBlocklistService. Added support for loading and persisting revoked certificate data independently of nsBlocklistService: * nsICertBlocklist now has an init method for loading persisted state * nsICertBlocklist now has a saveEntries method for saving updated state * nsCertBlocklistItem now stores state on whether it is old or current data * nsCertBlocklist now tracks if it has been modified since load The tests are not yet providing sufficient coverage; still trying to work out a few things with profile data and xpcshell (see questions) Questions: 1) Currently, saveEntries directly overwrites any existing revocations.txt file in the profile. Should it write to a temp file and move it over if the write was successful instead? 2) I'm having problems with profile data in xpcshell; I can create a file in the profile, see it exists and has a correct path before and after invoking nsICertBlocklist.init - and yet the file isn't there when nsCertBlocklist::Init attempts to open it. Am I missing something here?
Attachment #8518240 - Attachment is obsolete: true
Flags: needinfo?(dkeeler)
Attachment #8526224 - Flags: feedback?(dkeeler)
The current approach has rather less impact on nsBlocklistService. Could you take a look at the blocklist parts, please?
Flags: needinfo?(bmcbride)
(In reply to Mark Goodwin [:mgoodwin] from comment #45) I wasn't able to review this in detail today - I'll be sure to take a look tomorrow. > Questions: > 1) Currently, saveEntries directly overwrites any existing revocations.txt > file in the profile. Should it write to a temp file and move it over if the > write was successful instead? That's probably a good idea, yes. Do we have an atomic file move operation we can use? > 2) I'm having problems with profile data in xpcshell; I can create a file in > the profile, see it exists and has a correct path before and after invoking > nsICertBlocklist.init - and yet the file isn't there when > nsCertBlocklist::Init attempts to open it. Am I missing something here? Hmmm - I'll see if I can figure out what's going on here.
Flags: needinfo?(dkeeler)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #47) > That's probably a good idea, yes. Do we have an atomic file move operation > we can use? There's an nsAtomicFileOutputStream that QIs to a nsISafeOutputStream; It doesn't overwrite old data until you call Finish(). I used that. > Hmmm - I'll see if I can figure out what's going on here. Thanks. I updated the patch to have the tests I think we need; one of them is failing (thanks to this issue) though I managed to work around the issue for loading revocation data by moving the revocations.txt copy into head_psm.js - maybe that's a clue to what's breaking. I also made some other (minor) changes: * renamed some things to make the code clearer * added checks for staleness in IsCertBlocked for cases where a blocklist.xml update should remove a revoked item (for Kathleen's mistaken revocation case)
Attachment #8526224 - Attachment is obsolete: true
Attachment #8526224 - Flags: feedback?(dkeeler)
Attachment #8526821 - Flags: feedback?(dkeeler)
Test now passes. I also spotted a bug in incorrect use of nsDependentCString in nsCertBlocklistItem::toBase64.
Attachment #8526821 - Attachment is obsolete: true
Attachment #8526821 - Flags: feedback?(dkeeler)
Attachment #8527029 - Flags: feedback?(dkeeler)
Comment on attachment 8527029 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8527029 [details] [diff] [review]: ----------------------------------------------------------------- Looks good in general. There are probably a lot of optimizations we can make (e.g. why bother saving as ASCII base64 if what we're going to be dealing with is essentially binary data? Also, I think we're doing more memcpy'ing than we need when we insert/search/update the hash table.) There are some style nits I noted. The most important one is probably to handle the exceptional case first, return early, and otherwise continue with the rest of whatever operation (but with no else after the if/return). This keeps indentation levels down and makes the code flow more understandable. We should probably have another look at the way the tables are operated on, and how the file i/o goes. It's easy to get that wrong, and we need to be very sure of it. Also, more tests (e.g. are improperly-formatted revocations.txt files handled?) and more documentation would be good. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +8,5 @@ > > #include <stdint.h> > > #include "ExtendedValidation.h" > +#include "nsCOMPtr.h" Is nsCOMPtr.h necessary here? @@ +16,3 @@ > #include "OCSPRequestor.h" > #include "certdb.h" > #include "mozilla/Telemetry.h" Something has gone wrong with the sorting here - it might be nice to fix it. @@ +345,5 @@ > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + > + bool isCertBlocked; > + mCertList->IsCertBlocked(const_cast<uint8_t*>(certID.issuer.UnsafeGetData()), nit: check return value of IsCertBlocked (we should probably just return Result::ERROR_FATAL_LIBRARY_FAILURE if it fails) @@ +347,5 @@ > + > + bool isCertBlocked; > + mCertList->IsCertBlocked(const_cast<uint8_t*>(certID.issuer.UnsafeGetData()), > + certID.issuer.GetLength(), > + const_cast<uint8_t*>(certID.serialNumber.UnsafeGetData()), We shouldn't need these casts anymore, right? ::: security/certverifier/NSSCertDBTrustDomain.h @@ +9,5 @@ > > #include "pkix/pkixtypes.h" > #include "secmodt.h" > #include "CertVerifier.h" > +#include "nsICertBlocklist.h" nit: please sort these @@ +110,5 @@ > CertVerifier::PinningMode mPinningMode; > const unsigned int mMinimumNonECCBits; > const char* mHostname; // non-owning - only used for pinning checks > ScopedCERTCertList* mBuiltChain; // non-owning > + nsCOMPtr<nsICertBlocklist> mCertList; Maybe mCertBlocklist? ::: security/manager/boot/src/nsBOOTModule.cpp @@ +8,5 @@ > #include "nsEntropyCollector.h" > #include "nsSecureBrowserUIImpl.h" > #include "nsSecurityWarningDialogs.h" > #include "nsSiteSecurityService.h" > +#include "nsCertBlocklist.h" nit: sorted, please Also, I think we're supposed to move away from the "ns" prefix - we can just call these CertBlocklist.{h,cpp}, etc. ::: security/manager/boot/src/nsCertBlocklist.cpp @@ +1,1 @@ > +#include "nsCertBlocklist.h" nit: modeline and license boilerplate @@ +5,5 @@ > +#include "nsIFileStreams.h" > +#include "nsILineInputStream.h" > +#include "nsIX509Cert.h" > +#include "prlog.h" > +#include "mozilla/Base64.h" nit: sort these, please @@ +39,5 @@ > + > +nsCertBlocklistItem::~nsCertBlocklistItem() > +{ > + delete mIssuerData; > + delete mSerialData; These should be delete[], right? @@ +58,5 @@ > + > +bool > +nsCertBlocklistItem::operator==(const nsCertBlocklistItem& aItem) const > +{ > + bool retval = InputsAreEqual(aItem.mIssuer, mIssuer) && InputsAreEqual(aItem.mSerial, mSerial); Looks like this line is >80 chars - you can break it after the && @@ +78,5 @@ > + return hash; > +} > + > +nsCertBlocklist::nsCertBlocklist() > + : mMutex("nsCertBlocklist::mMutex"), mInitialized(false) nit: one initialization per line @@ +97,5 @@ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, ("nsCertBlockList::Init")); > + if (!NS_IsMainThread()) { > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, ("nsCertBlockList::Init - called off main thread")); > + return NS_ERROR_NOT_SAME_THREAD; > + } else { nit: no else after return @@ +98,5 @@ > + if (!NS_IsMainThread()) { > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, ("nsCertBlockList::Init - called off main thread")); > + return NS_ERROR_NOT_SAME_THREAD; > + } else { > + if (!mInitialized) { Let's flip this: if (mInitialized) { return NS_OK; } Although, wouldn't it be an error if Init were called twice? (Particularly if the changes I suggest in nsICertBlocklist.idl are made) @@ +104,5 @@ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, ("nsCertBlockList::Init - not initialized; initializing")); > + nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > + getter_AddRefs(mBackingFile)); > + if (NS_FAILED(rv) || !mBackingFile) { > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, ("nsCertBlockList::Init - couldn't get profile dir")); We might not want to return failure in this case - there are some xpcshell tests where we won't have a profile. See e.g. what nsSiteSecurityService does: https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSiteSecurityService.cpp#256 @@ +111,5 @@ > + } > + return NS_ERROR_FAILURE; > + } > + > + mBackingFile->Append(NS_LITERAL_STRING("revocations.txt")); If any of these operations can fail, we should be checking the return value. @@ +120,5 @@ > + rv = mBackingFile->Exists(&exists); > + if (NS_FAILED(rv)) { > + return rv; > + } > + if (exists) { handle if (!exists) instead and return early if so @@ +123,5 @@ > + } > + if (exists) { > + nsAutoCString issuer; > + nsAutoCString serial; > + nsCOMPtr<nsIFileInputStream> fileStream(do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID, &rv)); nit: watch out for lines >80 chars @@ +124,5 @@ > + if (exists) { > + nsAutoCString issuer; > + nsAutoCString serial; > + nsCOMPtr<nsIFileInputStream> fileStream(do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID, &rv)); > + if (NS_FAILED(rv)) nit: if (NS_FAILED(rv)) { return rv; } @@ +133,5 @@ > + nsCOMPtr<nsILineInputStream> lineStream(do_QueryInterface(fileStream, &rv)); > + nsAutoCString line; > + bool more = true; > + do { > + if (!line.IsEmpty() && line.First() != '#') { Document the expected format of the file, please. @@ +166,5 @@ > +NS_IMETHODIMP nsCertBlocklist::AddBlockedCert(const char* aIssuer, > + const char* aSerialNumber) > +{ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("nsCertBlockList::addBlockedCert - issuer is: %s and serial: %s", nit: align the '(' with the first 'g' from the previous line @@ +175,5 @@ > + > +nsresult > +nsCertBlocklist::AddBlockedCertInternal(const char* aIssuer, > + const char* aSerialNumber, > + bool aIsNew) Maybe consider turning this bool into an enum. true/false is less descriptive than something like CertNewFromBlocklist/CertOldFromLocalCache. Also, a common thing we do with internal functions where the caller must have already acquired a lock is to pass in a reference to the MutexAutoLock that acquired the lock, to essentially prove that calling code is doing the right thing. @@ +187,5 @@ > + Input issuer; > + Input serial; > + > + issuer.Init((uint8_t*) decodedIssuer.get(), decodedIssuer.Length()); > + serial.Init((uint8_t*) decodedSerial.get(), decodedSerial.Length()); nit: C++-style casts @@ +203,5 @@ > + > + return NS_OK; > +} > + > +// Write a line for a given string in the output stream Isn't there already a utility function for this? I thought there was, but I can't seem to find one... @@ +225,5 @@ > + { > + return NS_ERROR_FAILURE; > + } > + } > + else nit: } else {, etc. @@ +245,5 @@ > + nsAutoCString encSerial; > + > + nsresult rv = item.toBase64(encIssuer, encSerial); > + > + if (NS_SUCCEEDED(rv)) { In general, handle these like so: if (NS_FAILED(rv)) { return PL_DHASH_STOP; // commonly this would be rv instead } // continue with non-error-case operations @@ +269,5 @@ > +{ > + BlocklistSaveInfo* saveInfo = (BlocklistSaveInfo *) aUserArg; > + > + nsresult rv = WriteLine(saveInfo->outputStream, > + NS_LITERAL_CSTRING(" ")+aHashKey -> GetKey()); nit: spaces around '+', no spaces around '->' @@ +270,5 @@ > + BlocklistSaveInfo* saveInfo = (BlocklistSaveInfo *) aUserArg; > + > + nsresult rv = WriteLine(saveInfo->outputStream, > + NS_LITERAL_CSTRING(" ")+aHashKey -> GetKey()); > + if (!NS_SUCCEEDED(rv)) { if (NS_FAILED(rv)) @@ +307,5 @@ > +// Each item is stored on a separate line; each issuer is followed by its > +// revoked serial numbers, indented by one space. > +// > +// lines starting with a # character are ignored > +NS_IMETHODIMP nsCertBlocklist::SaveEntries() nit: return value ("NS_IMETHODIMP") on its own line @@ +315,5 @@ > + BlocklistSaveInfo saveInfo; > + nsresult rv; > + rv = NS_NewAtomicFileOutputStream(getter_AddRefs(saveInfo.outputStream), > + mBackingFile, -1, -1, 0); > + NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE); if (NS_FAILED(rv)) { return rv; } @@ +356,5 @@ > + > + Input issuer; > + Input serial; > + issuer.Init(aIssuer, aIssuerLength); > + serial.Init(aSerial, aSerialLength); We should check these return values. @@ +362,5 @@ > + nsCertBlocklistItem item(issuer, serial); > + > + *_retval = mBlocklist.Contains(item); > + > + // If the blocklist is modified, ensure the entry is current I think we can assume that if a certificate is ever on the list, it's unlikely to be removed, and it's ok to have false positives for a bit if it ever happens. As it is, it's possible for a certificate that should be revoked to appear not to be revoked, if NSSCertDBTrustDomain queries nsICertBlocklist while the blocklist is being updated. That would be a worse thing to have happen. @@ +365,5 @@ > + > + // If the blocklist is modified, ensure the entry is current > + if (*_retval && mModified) { > + BlocklistItemKey* itemKey = mBlocklist.GetEntry(item); > + *_retval = itemKey->GetKey().mIsCurrent == mModified ? true : false; Isn't mModified guaranteed to be true here? ::: security/manager/boot/src/nsCertBlocklist.h @@ +56,5 @@ > + nsresult AddBlockedCertInternal(const char* aIssuer, const char* aSerial, bool aIsNew); > + > + Mutex mMutex; > + bool mInitialized; > + bool mModified = false; This should probably be initialized in the constructor instead of here. ::: security/manager/ssl/public/moz.build @@ +9,5 @@ > 'nsIASN1PrintableItem.idl', > 'nsIASN1Sequence.idl', > 'nsIAssociatedContentSecurity.idl', > 'nsIBadCertListener2.idl', > + 'nsICertBlocklist.idl', Can this go in security/manager/boot, since that's where the implementation is? ::: security/manager/ssl/public/nsICertBlocklist.idl @@ +19,5 @@ > +interface nsICertBlocklist : nsISupports { > + /** > + * Ensure the blocklist is initialized > + */ > + void init(); Rather than having an explicit init function, we can do what nsSiteSecurityService does in nsBOOTModule.cpp: 16 NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsSiteSecurityService, Init) It should get called the first time do_GetService is called for it, which is in NSS startup at the moment. @@ +24,5 @@ > + > + /** > + * Add details of a blocked certificate : issuer name and serial number. > + */ > + void addBlockedCert(in string issuer, in string serialNumber); Maybe "addRevokedCert"? @@ +29,5 @@ > + > + /** > + * Persist (fresh) blocklist entries to the profile. > + */ > + void saveEntries(); This involves synchronous i/o - needs to be documented. @@ +34,5 @@ > + > + /** > + * Check if a certificate is blocked. > + */ > + boolean isCertBlocked([const, array, size_is(issuer_length)] in octet issuer, Maybe "isCertRevoked"? That's the error we return, anyway... ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1072,5 @@ > return NS_ERROR_FAILURE; > } > > + nsCOMPtr<nsICertBlocklist> certList = do_GetService(NS_CERTBLOCKLIST_CONTRACTID); > + if (certList) { If certList is null, we should probably also fail. @@ +1073,5 @@ > } > > + nsCOMPtr<nsICertBlocklist> certList = do_GetService(NS_CERTBLOCKLIST_CONTRACTID); > + if (certList) { > + if (!NS_SUCCEEDED(certList->Init())) { nit: if (NS_FAILED(...)) It's also nice to preserve the original return value: nsresult rv = certList->Init(); if (NS_FAILED(rv)) { return rv; } ::: security/manager/ssl/tests/unit/head_psm.js @@ +11,5 @@ > let { Services } = Cu.import("resource://gre/modules/Services.jsm", {}); > let { Promise } = Cu.import("resource://gre/modules/Promise.jsm", {}); > let { HttpServer } = Cu.import("resource://testing-common/httpd.js", {}); > let { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm"); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); nit: let { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {}); @@ +594,5 @@ > + registrar.registerFactory(XULAPPINFO_CID, "XULAppInfo", > + XULAPPINFO_CONTRACTID, XULAppInfoFactory); > +} > + > +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2"); This should be done per test that needs it, not unconditionally for every test. @@ +608,5 @@ > + existing.copyTo(profile,"revocations.txt"); > + } > +} > + > +setup_revocations(); This should happen per test that needs it. I think the trick will be to ensure that it happens before any code calls for a certDB or makes a TLS connection. ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +let profile = do_get_profile(); > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); If this is in head_psm.js, we don't need this here (although, if the other tests don't need it, we should just have it here and not there. Also, use the 'let { XPCOMUtils } = Cu.import("...", {});' form. @@ +12,5 @@ > +let testserver = new HttpServer(); > +testserver.start(-1); > +let gPort = testserver.identity.primaryPort; > + > +let converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] nit: Cc["@mozilla.org..." @@ +13,5 @@ > +testserver.start(-1); > +let gPort = testserver.identity.primaryPort; > + > +let converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] > + .createInstance(Components.interfaces.nsIScriptableUnicodeConverter); nit: Ci.nsIScriptable... (also, line up the '.createInstance' with the '[' on the previous line)
Attachment #8527029 - Flags: feedback?(dkeeler) → feedback+
Changes: * Fixed the sorting on various imports * Changed things around so that, where possible, we return early on error conditions to keep indentation low * Added missing checks of return values, where appropriate * replaced C style casts with C++ style * Renamed nsCertBlocklist* to CertBlocklist* - and fixed imports, etc. to reflect changed build order in moz.build * Renamed mCertList to mCertBlocklist in NSSCertDBTrustDomain * Added missing modeline / licence boilerplate from new files * Fixed (most) line length issues * modified CertBlocklist::Init to work in cases where there's no profile * Documented the format of revocations.txt in CertBlocklist::Init * Added an enum to represent certificate blocklist item state (new from blocklist, or existing from profile dir) * Removed checks for staleness on isCertRevoked since we don't mind waiting a while in the case of a OneCRL mis-revocation * Moved nsICertBlocklist.idl to security/manager/boot/public * Changed nsBOOTModule.cpp so that Init is implicitly called on CertBlocklist when do_GetService is called for nsICertBlocklist * Renamed addBlockedCert and isCertBlocked to addRevokedCert and isCertRevoked * Documented use of sync. I/O * Tidied up tests, added data to the test file (and test XML) to ensure that errors are handled gracefully on startup or blocklist load * Various nits addressed (In reply to David Keeler (:keeler) [use needinfo?] from comment #50) > Looks good in general. There are probably a lot of optimizations we can make > (e.g. why bother saving as ASCII base64 if what we're going to be dealing > with is essentially binary data? Generally, I'd agree; in this case, the only really perf. sensitive bit is the lookup (isCertRevoked) and here we're dealing solely with binary data. For the others, we're limited by I/O (disk read / write) or other much slower things (XML parsing) so I'm not sure we get much benefit for changing. On the other hand, in favour of keeping base64; we're better able to recover if the revocations file is damaged in some way. I can obviously change this; I'm just not sure we win much by doing so. > Also, I think we're doing more memcpy'ing > than we need when we insert/search/update the hash table.) I'll ping you about the best way to fix this later. > > +// Write a line for a given string in the output stream > > Isn't there already a utility function for this? I thought there was, but I can't seem to find one... You'd have thought so but I can't find one either. There's one for reading lines though..?
Attachment #8527029 - Attachment is obsolete: true
Attachment #8528450 - Flags: review?(dkeeler)
Comment on attachment 8528450 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8528450 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. I should probably have another look, though, so r- for now. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +345,5 @@ > + } > + > + bool isCertRevoked; > + nsresult > + res = mCertBlocklist->IsCertRevoked(certID.issuer.UnsafeGetData(), nit: maybe "nsrv"? (unfortunately I see the more common "rv" is already taken by another type) Also, breaking the overly-long line more like so is more common, I think: nsresult nsrv = mCertBlocklist->IsCertRevoked( certID.issuer.UnsafeGetData(), certID.issuer.GetLength(), ... @@ +350,5 @@ > + certID.issuer.GetLength(), > + certID.serialNumber.UnsafeGetData(), > + certID.serialNumber.GetLength(), > + &isCertRevoked); > + if (!NS_SUCCEEDED(res)) { nit: "if (NS_FAILED(nsrv)) {" ::: security/manager/boot/public/nsICertBlocklist.idl @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- > + * We should be consistent about the boilerplate - the other new files don't have a blank comment line here (I don't think this one should either). @@ +19,5 @@ > +interface nsICertBlocklist : nsISupports { > + /** > + * Ensure the blocklist is initialized > + */ > + void init(); This isn't necessary anymore. @@ +22,5 @@ > + */ > + void init(); > + > + /** > + * Add details of a revoked certificate : issuer name and serial number. More documentation, please: we're expecting these as base-64 encoded DER data, right? @@ +35,5 @@ > + > + /** > + * Check if a certificate is blocked. > + */ > + boolean isCertRevoked([const, array, size_is(issuer_length)] in octet issuer, Documentation again: these are DER encodings, right? ::: security/manager/boot/src/CertBlocklist.cpp @@ +54,5 @@ > + delete[] mSerialData; > +} > + > +nsresult > +CertBlocklistItem::toBase64(nsACString& b64IssuerOut, nsACString& b64SerialOut) nit: ToBase64 (C++ functions start with upper-case letters in our codebase) @@ +59,5 @@ > +{ > + nsDependentCSubstring issuerString((char*)mIssuerData, mIssuer.GetLength()); > + nsDependentCSubstring serialString((char*)mSerialData, mSerial.GetLength()); > + nsresult rv = mozilla::Base64Encode(issuerString, b64IssuerOut); > + if (NS_SUCCEEDED(rv)) { nit: return early: if (NS_FAILED(rv)) { return rv; } rv = mozilla::Base64Encode... @@ +70,5 @@ > +bool > +CertBlocklistItem::operator==(const CertBlocklistItem& aItem) const > +{ > + bool retval = InputsAreEqual(aItem.mIssuer, mIssuer) && > + InputsAreEqual(aItem.mSerial, mSerial); nit: line up the two InputsAreEqual calls vertically @@ +92,5 @@ > + > +CertBlocklist::CertBlocklist() > + : mMutex("CertBlocklist::mMutex"), > + mInitialized(false), > + mModified(false) nit: : mMutex("CertBlocklist::mMutex") , mInitialized(false) , mModified(false) @@ +104,5 @@ > +{ > +} > + > +// void init(); > +NS_IMETHODIMP This should just be 'nsresult' now (and no need for the comment - this isn't implementing an interface method) @@ +114,5 @@ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init - called off main thread")); > + return NS_ERROR_NOT_SAME_THREAD; > + } > + if (mInitialized) { I'm fairly sure it's not possible for Init to be called twice using NS_GENERIC_FACTORY_CONSTRUCTOR_INIT, so mInitialized isn't necessary. @@ +121,5 @@ > + // Load the revocations file into the cert blocklist > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init - not initialized; initializing")); > + nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > + getter_AddRefs(mBackingFile)); nit: align getter_AddRefs... with NS_APP_USER... @@ +124,5 @@ > + nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > + getter_AddRefs(mBackingFile)); > + if (NS_FAILED(rv) || !mBackingFile) { > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init - couldn't get profile dir")); Return early here with NS_OK. @@ +125,5 @@ > + getter_AddRefs(mBackingFile)); > + if (NS_FAILED(rv) || !mBackingFile) { > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init - couldn't get profile dir")); > + } else { (so this else isn't necessary) @@ +127,5 @@ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init - couldn't get profile dir")); > + } else { > + rv = mBackingFile->Append(NS_LITERAL_STRING("revocations.txt")); > + NS_ENSURE_SUCCESS(rv, rv); NS_ENSURE_SUCCESS isn't really something we want to be using anymore (it hides a return, for one thing). This should be fine: if (NS_FAILED(rv)) { return rv; } @@ +130,5 @@ > + rv = mBackingFile->Append(NS_LITERAL_STRING("revocations.txt")); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsAutoCString path; > + mBackingFile->GetNativePath(path); Can this fail? (be sure to check the return value if so) @@ +161,5 @@ > + // contains a comment, a base64 encoded issuer or a base64 encoded serial > + // number. Comment lines start with '#', serial number lines, ' ' (a > + // space) and anything else is assumed to be an issuer. > + do { > + if (!line.IsEmpty() && line.First() != '#') { I think we should apply the early-return paradigm here as well. e.g.: if (line.IsEmpty() || line.First() == '#') { continue; } if (line.First() != ' ') { issuer = line; continue; } serial = line; ... if (issuer.IsEmpty() || serial.IsEmpty()) { continue; } rv = AddRevoked... @@ +171,5 @@ > + // serial numbers 'belong' to the last issuer line seen; if no > + // issuer has been seen, the serial number is ignored > + if (!issuer.IsEmpty() && !serial.IsEmpty()) { > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init adding: %s %s", issuer.get(), nit: align the '(' with the 'g' from the previous line @@ +181,5 @@ > + if (NS_FAILED(rv)) { > + // we warn here, rather than abandoning, since we need to > + // ensure that as many items as possible are read > + PR_LOG(gCertBlockPRLog, PR_LOG_WARN, > + ("CertBlocklist::Init adding revoked cert failed")); nit: same alignment as above @@ +215,5 @@ > +nsresult > +CertBlocklist::AddRevokedCertInternal(const char* aIssuer, > + const char* aSerialNumber, > + CertBlocklistItemState aItemState, > + mozilla::MutexAutoLock& lock) Sorry - forgot to mention we usually call this 'mozilla::MutexAutoLock& /*proofOfLock*/' unless it needs to be passed to another internal function. That way, compilers won't warn about unused named parameters. @@ +222,5 @@ > + nsCString decodedSerial; > + > + nsresult rv; > + rv = mozilla::Base64Decode(nsDependentCString(aIssuer), decodedIssuer); > + NS_ENSURE_SUCCESS(rv, rv); Again, do: if (NS_FAILED(rv) { return rv; } @@ +224,5 @@ > + nsresult rv; > + rv = mozilla::Base64Decode(nsDependentCString(aIssuer), decodedIssuer); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mozilla::Base64Decode(nsDependentCString(aSerialNumber), decodedSerial); > + NS_ENSURE_SUCCESS(rv, rv); same @@ +231,5 @@ > + mozilla::pkix::Input serial; > + > + issuer.Init(reinterpret_cast<const uint8_t*>(decodedIssuer.get()), > + decodedIssuer.Length()); > + serial.Init(reinterpret_cast<const uint8_t*>(decodedSerial.get()), Do check the return values on the Init calls, because we could potentially get passed input that is too long. @@ +236,5 @@ > + decodedSerial.Length()); > + > + CertBlocklistItem item(issuer, serial); > + > + if (CertNewFromBlocklist == aItemState) { nit: switch these around: 'if (aItemState == CertNewFromBlocklist) {' @@ +252,5 @@ > +// Write a line for a given string in the output stream > +nsresult > +WriteLine(nsIOutputStream* outputStream, const nsACString& string) > +{ > + nsAutoCString line(string + NS_LITERAL_CSTRING("\n")); I might just do this here: nsAutoCString line(string); line.Append('\n'); @@ +260,5 @@ > + nsresult rv = NS_OK; > + while (NS_SUCCEEDED(rv) && length) { > + uint32_t bytesWritten = 0; > + rv = outputStream->Write(data, length, &bytesWritten); > + if (NS_SUCCEEDED(rv)) return early: if (NS_FAILED(rv)) { return rv; } @@ +281,5 @@ > +PLDHashOperator > +CertBlocklist::ProcessEntry(BlocklistItemKey* aHashKey, > + void* aUserArg) > +{ > + BlocklistSaveInfo* saveInfo = (BlocklistSaveInfo *) aUserArg; nit: reinterpret_cast<BlocklistSaveInfo*>(aUserArg); @@ +289,5 @@ > + > + nsresult rv = item.toBase64(encIssuer, encSerial); > + > + if (NS_FAILED(rv)) { > + return PL_DHASH_STOP; Shouldn't this also have 'saveInfo->success = false;'? @@ +292,5 @@ > + if (NS_FAILED(rv)) { > + return PL_DHASH_STOP; > + } > + > + if (item.mIsCurrent) { We probably want to return early if !item.mIsCurrent to avoid the base-64 conversion, right? @@ +296,5 @@ > + if (item.mIsCurrent) { > + saveInfo->issuers.PutEntry(encIssuer); > + BlocklistStringSet* issuerSet = saveInfo->issuerTable.Get(encIssuer); > + if (!issuerSet) { > + issuerSet = new BlocklistStringSet; nit: new BlocklistStringSet(); @@ +310,5 @@ > +PLDHashOperator > +CertBlocklist::WriteSerial(nsCStringHashKey* aHashKey, > + void* aUserArg) > +{ > + BlocklistSaveInfo* saveInfo = (BlocklistSaveInfo *) aUserArg; nit: reinterpret_cast<BlocklistSaveInfo*>(aUserArg); @@ +326,5 @@ > +PLDHashOperator > +CertBlocklist::WriteIssuer(nsCStringHashKey* aHashKey, > + void* aUserArg) > +{ > + BlocklistSaveInfo* saveInfo = (BlocklistSaveInfo *) aUserArg; nit: reinterpret_cast<BlocklistSaveInfo*>(aUserArg); @@ +332,5 @@ > + > + saveInfo->issuerTable.RemoveAndForget(aHashKey->GetKey(), issuerSet); > + > + nsresult rv = WriteLine(saveInfo->outputStream, aHashKey->GetKey()); > + if (!NS_SUCCEEDED(rv)) { Again, we can use saveInfo->success to signal success/failure to the caller, right? @@ +355,5 @@ > +NS_IMETHODIMP > +CertBlocklist::SaveEntries() > +{ > + mozilla::MutexAutoLock lock(mMutex); > + if (mModified && mBackingFile) { return early: if (!mModified || !mBackingFile) { return NS_OK; } @@ +360,5 @@ > + BlocklistSaveInfo saveInfo; > + nsresult rv; > + rv = NS_NewAtomicFileOutputStream(getter_AddRefs(saveInfo.outputStream), > + mBackingFile, -1, -1, 0); > + NS_ENSURE_SUCCESS(rv, rv); if (NS_FAILED(rv)) { return rv; } @@ +361,5 @@ > + nsresult rv; > + rv = NS_NewAtomicFileOutputStream(getter_AddRefs(saveInfo.outputStream), > + mBackingFile, -1, -1, 0); > + NS_ENSURE_SUCCESS(rv, rv); > + mBlocklist.EnumerateEntries(ProcessEntry, &saveInfo); Seems like we should check saveInfo.success here. @@ +367,5 @@ > + rv = WriteLine(saveInfo.outputStream, > + NS_LITERAL_CSTRING("# Auto generated contents. Do not edit.")); > + NS_ENSURE_SUCCESS(rv, rv); > + > + saveInfo.issuers.EnumerateEntries(WriteIssuer, &saveInfo); Let's check saveInfo.success after this. @@ +372,5 @@ > + > + nsCOMPtr<nsISafeOutputStream> safeStream = > + do_QueryInterface(saveInfo.outputStream); > + NS_ASSERTION(safeStream, "expected a safe output stream!"); > + if (safeStream) { return early: if (!safeStream) { return NS_ERROR_FAILURE; } @@ +408,5 @@ > + mozilla::pkix::Input serial; > + if (IsFatalError(issuer.Init(aIssuer, aIssuerLength))) { > + return NS_ERROR_FAILURE; > + } > + if (IsFatalError(serial.Init(aSerial, aSerialLength))) { We should do 'if (issuer/serial.Init(...) != Success) {' for these two cases ::: security/manager/boot/src/CertBlocklist.h @@ +81,5 @@ > + IssuerTable issuerTable; > + BlocklistStringSet issuers; > + nsCOMPtr<nsIOutputStream> outputStream; > + bool success = true; > +}; nit: add a blank line after this one @@ +82,5 @@ > + BlocklistStringSet issuers; > + nsCOMPtr<nsIOutputStream> outputStream; > + bool success = true; > +}; > +#endif nit: add // CertBlocklist_h ::: security/manager/ssl/tests/unit/head_psm.js @@ +545,5 @@ > throw Components.results.NS_ERROR_NO_INTERFACE; > }, > } > + > +function createAppInfo(id, name, version, platformVersion) { We should really only add things to this file that are common to multiple tests in this directory. This isn't the case yet, so we don't need to add these two functions or the XPCOMUtils import here - only in test_cert_blocklist.js. @@ +594,5 @@ > + registrar.registerFactory(XULAPPINFO_CID, "XULAppInfo", > + XULAPPINFO_CONTRACTID, XULAppInfoFactory); > +} > + > +function setup_revocation_data(){ nit: space before { ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +1,1 @@ > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ This file needs some cleanup attention. In particular, unused things need to be removed and we should be consistent about using underscores or intercaps for function/variable names. Also, the contents of run_test is hard to understand. I think most of the problem is that there are too many add_test/run_next_test pairs. If there are a bunch of serial operations to do, they don't need to be in separate add_test/run_next_test pairs. Maybe start with some documentation on what this test is intended to do, and then structure the code so that it's easier to understand that that's what it's doing. @@ +2,5 @@ > +/* 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/. */ > + > +let profile = do_get_profile(); We can probably put this closer to where it's used. @@ +18,5 @@ > +testserver.start(-1); > +let gPort = testserver.identity.primaryPort; > + > +let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"] > + .createInstance(Components.interfaces.nsIScriptableUnicodeConverter); nit: Ci.nsIScriptableUnicodeConverter @@ +25,5 @@ > +// This observer is notified from the nsBlocklistService.js when the condition > +// if ( addonList.length == 0 ) is true. Since it is just for the purposes of > +// this test, the condition will always be true as no addons are part of the blocklist > +// file used for this test. Once the notification is sent, this will execute the > +// verifyCertNow again with a revoked intermediate. This comment seems out of date at least, and misleading at worst. We can probably just remove it. @@ +33,5 @@ > + } > +} > + > +function startupManager() { > + let gInternalManager = null; This isn't actually a global, so prefixing the name with 'g' doesn't make much sense. @@ +41,5 @@ > + gInternalManager.observe(null, "addons-startup", null); > +} > + > +const XULAPPINFO_CONTRACTID = "@mozilla.org/xre/app-info;1"; > +const XULAPPINFO_CID = Components.ID("{c763b610-9d49-455a-bbd2-ede71682a1ac}"); These don't appear to be used. @@ +43,5 @@ > + > +const XULAPPINFO_CONTRACTID = "@mozilla.org/xre/app-info;1"; > +const XULAPPINFO_CID = Components.ID("{c763b610-9d49-455a-bbd2-ede71682a1ac}"); > + > +function verifyCert(file, expectedError) { verifyCert is synchronous - it shouldn't call run_next_test itself @@ +61,5 @@ > +function load_blocklist() { > + Services.obs.addObserver(certblockObserver, "blocklist-updated", false); > + Services.prefs.setCharPref("extensions.blocklist.url", "http://localhost:" + > + gPort + "/push_blocked_cert/"); > + Services.prefs.setBoolPref("extensions.logging.enabled", true); I don't think setting this is necessary. @@ +72,5 @@ > + 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); nit: alignment @@ +82,5 @@ > + 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" + nit: alignment @@ +93,5 @@ > +} > + > +function generate_blocklist_contents() > +{ > + let cert = certDB.findCertByNickname(null, "int-limited-depth"); cert, derArray, derString, and blocked_cert don't appear to be used anymore. @@ +121,5 @@ > + "<serialNumber>AkHVNA==</serialNumber></issuer>" + > + "</certItem><certItem>" + > + // ... and some good > + "<issuer name='YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy'>" + > + "<serialNumber>c2VyaWFsMi4=</serialNumber></issuer>" + Where are these from? We need to know how to regenerate them if things change. @@ +143,5 @@ > + startupManager(); > + // check something in revocations.txt is blocked > + add_test(function() { > + let certList = Cc["@mozilla.org/security/certblocklist;1"] > + .getService(Ci.nsICertBlocklist); nit: align the '.' with the '[' @@ +144,5 @@ > + // check something in revocations.txt is blocked > + add_test(function() { > + let certList = Cc["@mozilla.org/security/certblocklist;1"] > + .getService(Ci.nsICertBlocklist); > + certList.init(); this isn't necessary anymore @@ +146,5 @@ > + let certList = Cc["@mozilla.org/security/certblocklist;1"] > + .getService(Ci.nsICertBlocklist); > + certList.init(); > + > + ok(testIsRevoked(certList, "some imaginary issuer","serial."), why "serial."? @@ +154,5 @@ > + "issuer / serial pair should be blocked"); > + run_next_test(); > + }); > + // Check the cert validates before we load the blocklist > + add_test(function() { Only use add_test when there's actually asynchronous behavior. @@ +166,5 @@ > + // check the item not in the blocklist.xml has gone... but the > + // other still remains > + add_test(function() { > + let certList = Cc["@mozilla.org/security/certblocklist;1"] > + .getService(Ci.nsICertBlocklist); nit: alignment @@ +179,5 @@ > + check_revocations_file(); > + }); > + // Check the blocklisted intermediate now causes a failure > + add_test(function() { > + let file = "test_intermediate_basic_usage_constraints/ee-int-limited-depth.der"; Using files from an unrelated test is confusing. Can you modify generate_certs.sh to have testINT issue an otherwise ok certificate, block testINT, and verify that the end-entity is blocked? ::: security/manager/ssl/tests/unit/test_onecrl/sample_revocations.txt @@ +1,1 @@ > +# a sample revocations.txt for tests maybe add a comment that lines starting with '#' are ignored
Attachment #8528450 - Flags: review?(dkeeler) → review-
Changes: * renamed various things as suggested * replaced various instances of !NS_SUCCEEDED with NS_FAILED * references to Init in nsICertBlocklist.idl removed and replaced with corresponding entry in CertBlocklist.h * improved some comments and documentation (e.g. more explicit about encodings) * removed mInitialized (and references to it) from CertBlocklist.h/.cpp * removed all use of NS_ENSURE_SUCCESS * added return value checks to use of nsIFile::GetNativePath and Input::Init * various early-return improvements (in particular, CertBlocklist::Init is rather less horrible now) * fixed the mentioned indentation nits * used the /*proofOfLock*/ technique to avoid compiler warnings on unused mutex names * switched from const / enum lvalues to rvalues (though I still think this is a safer coding style, if less readable) * replaced the remaining C style casts with C++ style * improved setting (and checking) of saveInfo.success * switched from using IsFatalError to checking for mozilla::pkix::Success * moved all changes in head_psm.js to test_cert_blocklist * addressed various nits * cleaned up test_cert_blocklist: * improved documentation on intent and detail of the tests * extracted various setup functions so order is more explicit * removed use of other tests' certs, added to and made use of tlsserver/ certs instead * moved all possible sync parts of the test together * improved documentation on what the test data means (and how it's generated) * removed dead code * improved formatting
Attachment #8528450 - Attachment is obsolete: true
Attachment #8529215 - Flags: review?(dkeeler)
Attachment #8529215 - Flags: feedback?(bmcbride)
Comment on attachment 8529215 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8529215 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +729,5 @@ > +# <certItem> > +# <!-- issuer name is the DER issuer name data base64 encoded... --> > +# <issuer name="MA0xCzAJBgNVBAMMAmNh"> > +# <!-- ... as is the serial number DER data --> > +# <serialNumber>AkHVNA==</serialNumber> This feels verbose - I'd like to reduce the verboseness of anything we add here, to help keep the filesize small. How about: <certItems> <issuer name="MA0xCzAJBgNVBAMMAmNh"> <serialNumber>AkHVNA==</serialNumber> </issuer> </certItems> @@ +847,5 @@ > }), > > _loadBlocklistFromString : function Blocklist_loadBlocklistFromString(text) { > + if (!this._certBlocklist) { > + this._certBlocklist = Cc["@mozilla.org/security/certblocklist;1"] Nit: Prefer XPCOMUtils.defineLazyServiceGetter() @@ +910,5 @@ > + _handleCertItemNode: function Blocklist_handleCertItemNode(blocklistElement, > + result) { > + let childNodes = blocklistElement.childNodes; > + for (let x = 0; x < childNodes.length; x++) { > + let issuerElement = childNodes.item(x); Nit: for-of loops for any case like this now days - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of ie: for (let issuerElement of blocklistElement.childNodes) { @@ +911,5 @@ > + result) { > + let childNodes = blocklistElement.childNodes; > + for (let x = 0; x < childNodes.length; x++) { > + let issuerElement = childNodes.item(x); > + if (!(issuerElement instanceof Ci.nsIDOMElement)) { If you iterate over blocklistElement.children, you don't need this check (childNodes is any node type, children is only elements).
Attachment #8529215 - Flags: feedback?(bmcbride) → feedback+
Comment on attachment 8529215 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8529215 [details] [diff] [review]: ----------------------------------------------------------------- Looks great - just a few nits and some suggestions for additional test-cases. ::: security/manager/boot/public/nsICertBlocklist.idl @@ +23,5 @@ > + void addRevokedCert(in string issuer, in string serialNumber); > + > + /** > + * Persist (fresh) blocklist entries to the profile (if a profile directory is > + * available. Note: calling this will result in synchronous I/O. nit: add closing parenthesis ::: security/manager/boot/src/CertBlocklist.cpp @@ +130,5 @@ > + return rv; > + } > + > + nsAutoCString path; > + nit: remove this blank line @@ +139,5 @@ > + > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init certList path: %s", path.get())); > + bool exists = false; > + nit: move this blank line before the bool @@ +152,5 @@ > + return NS_OK; > + } > + > + nsAutoCString issuer; > + nsAutoCString serial; nit: move these closer to the for loop @@ +170,5 @@ > + // read in the revocations file. The file format is as follows: each line > + // contains a comment, base64 encoded DER for an issuer or base64 encoded DER > + // for a serial number. Comment lines start with '#', serial number lines, ' ' > + // (a space) and anything else is assumed to be an issuer. > + for (bool more = true; more; rv = lineStream->ReadLine(line, &more)) { As far as I can tell from source/xpcom/io/nsILineInputStream.idl, more can be false even if line is a valid line (i.e. it's the last one). So, I think it's possible for this to not process the final line in the file. I thought the do/while from the previous patch worked well. (Be sure to add a test that handles specifically this issue when you fix it, or if you determine that I'm full of it and I don't know what I'm talking about.) @@ +299,5 @@ > +PLDHashOperator > +CertBlocklist::ProcessEntry(BlocklistItemKey* aHashKey, > + void* aUserArg) > +{ > + BlocklistSaveInfo* saveInfo = reinterpret_cast<BlocklistSaveInfo *>(aUserArg); nit: <BlocklistSaveInfo*> @@ +330,5 @@ > +PLDHashOperator > +CertBlocklist::WriteSerial(nsCStringHashKey* aHashKey, > + void* aUserArg) > +{ > + BlocklistSaveInfo* saveInfo = reinterpret_cast<BlocklistSaveInfo *>(aUserArg); nit: <BlocklistSaveInfo*> @@ +346,5 @@ > +PLDHashOperator > +CertBlocklist::WriteIssuer(nsCStringHashKey* aHashKey, > + void* aUserArg) > +{ > + BlocklistSaveInfo* saveInfo = reinterpret_cast<BlocklistSaveInfo *>(aUserArg); nit: <BlocklistSaveInfo*> @@ +422,5 @@ > + } > + rv = safeStream->Finish(); > + if (NS_FAILED(rv)) { > + PR_LOG(gCertBlockPRLog, PR_LOG_WARN, > + ("CertBlocklist::SaveEntries saving revocation data failed")); nit: indent one less space ::: security/manager/boot/src/CertBlocklist.h @@ +56,5 @@ > + static PLDHashOperator ProcessEntry(BlocklistItemKey* aHashKey, > + void* aUserArg); > + static PLDHashOperator WriteSerial(nsCStringHashKey* aHashKey, > + void* aUserArg); > + static PLDHashOperator WriteIssuer(nsCStringHashKey* aHashKey, These three methods don't actually need to be part of CertBlocklist, right? (that way, they can live just in CertBlocklist.cpp and not be visible at all to outside code) @@ +75,5 @@ > + virtual ~CertBlocklist(); > +}; > + > +// Data needed for writing blocklist items out to the revocations file > +struct BlocklistSaveInfo This probably isn't needed in the header file, right? (Nothing outside of CertBlocklist.cpp uses it.) ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +46,5 @@ > + > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIXULAppInfo, > + Ci.nsIXULRuntime, > + Ci.nsICrashReporter, > + Ci.nsISupports]) nit: alignment @@ +53,5 @@ > +let XULAppInfoFactory = { > + createInstance: function (outer, iid) { > + appInfo.QueryInterface(iid); > + if (outer != null) { > + throw Components.results.NS_ERROR_NO_AGGREGATION; nit: Cr.NS_ERROR_NO_AGGREGATION @@ +97,5 @@ > + "<serialNumber>and serial</serialNumber></issuer>" + > + "</certItem><certItem>" + > + // some mixed > + // In this case, the issuer name and the valid serialNumber correspond > + // to test-int.der in tlsserver/ Something we could do in a follow-up is dynamically read and insert the issuer/serial number here (because as it is, if generate_certs.sh is ever re-ran and those certs get regenerated, those values could change and break this test). @@ +109,5 @@ > + // serialNumber is "serial." base-64 encoded > + // We need this to ensure that existing items are retained if they're > + // also in the blocklist > + "<issuer name='YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy'>" + > + "<serialNumber>c2VyaWFsMi4=</serialNumber></issuer>" + Let's test with multiple serial numbers for a single issuer, too. @@ +184,5 @@ > + // blocklist load is async so we must use add_test from here > + add_test(function() { > + let certblockObserver = { > + observe: function(aSubject, aTopic, aData) { > + run_next_test(); nit: have the observer remove itself from observing any more events (before run_next_test) @@ +190,5 @@ > + } > + > + Services.obs.addObserver(certblockObserver, "blocklist-updated", false); > + Services.prefs.setCharPref("extensions.blocklist.url", "http://localhost:" + > + port + "/push_blocked_cert/"); nit: indentation ::: security/manager/ssl/tests/unit/test_onecrl/sample_revocations.txt @@ +1,2 @@ > +# a sample revocations.txt for tests > +# Lines starting with '#' are ignore - as are empty lines like this: nit: 'ignored' @@ +22,5 @@ > + c2VyaWFsLg== > +# issuer is "another imaginary issuer" base-64 encoded > +# serial is "serial2." base-64 encoded > +YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy > + c2VyaWFsMi4= Let's test with multiple serial numbers for a single issuer, too. @@ +23,5 @@ > +# issuer is "another imaginary issuer" base-64 encoded > +# serial is "serial2." base-64 encoded > +YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy > + c2VyaWFsMi4= > +#let's end with a comment (see comment regarding testing whether or not we ignore the last line in CertBlocklist.cpp) ::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh @@ +322,5 @@ > export_cert v1Cert v1Cert.der > make_EE eeIssuedByV1Cert 'CN=EE Issued by V1 Cert' v1Cert "localhost,*.example.com" > > +# Make a valid EE using testINT to test OneCRL revocation of testINT > +make_EE localhostAndExampleComUsingTestINT 'CN=Test End-entity with intermediate' testINT "localhost,*.example.com,*.pinning.example.com,*.include-subdomains.pinning.example.com,*.exclude-subdomains.pinning.example.com" Maybe the nickname here should be 'eeIssuedByIntermediate' instead of 'localhostAndExampleComUsingTestINT'. Also, it only needs to be valid for "localhost", really (it doesn't actually need to be valid for any hosts, but let's just keep it simple)
Attachment #8529215 - Flags: review?(dkeeler) → review+
Changes: * Removed the <issuer> element, moved issuerName to certItem * tidied up the parsing code * addressed Unfocused's nits * added a test to ensure save is a no-op if no items are added
Attachment #8529215 - Attachment is obsolete: true
Attachment #8529598 - Flags: review?(bmcbride)
Comment on attachment 8529598 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8529598 [details] [diff] [review]: ----------------------------------------------------------------- That cut down on code quite nicely :)
Attachment #8529598 - Flags: review?(bmcbride) → review+
Comments from keeler and Unfocused addressed: carrying both r+
Attachment #8529598 - Attachment is obsolete: true
Attachment #8529723 - Flags: review+
As a drive-by, your implementation doesn't seem to match Mozilla's public statements as to how OneCRL will be implemented. Is there more recent documentation than https://wiki.mozilla.org/CA:RevocationPlan#OneCRL ? I would encourage you to re-think the Issuer+SerialNumber being the only blocking mechanism. Both CRLSets and Microsoft's Certificate Distrust Lists have found that in the real world of revocations (most notably, DigiNotar), issuer+serial number is NOT sufficient. There are times where you want Subject+Public Key, as a given Subject+PublicKey may have many issuers, some of which the affected CA does not disclose. Considering that the threat model for OneCRL MUST include a CA failing to abide by Mozilla's policies of disclosing all intermediates, or, in the event of an exceptional compromise, may be unable to, it's important to realize you may only have the public key and subject of the certificate.
Thanks for the drive-by Ryan, (In reply to Ryan Sleevi from comment #59) > As a drive-by, your implementation doesn't seem to match Mozilla's public > statements as to how OneCRL will be implemented. Is there more recent > documentation than https://wiki.mozilla.org/CA:RevocationPlan#OneCRL ? Not that I'm aware of. > I would encourage you to re-think the Issuer+SerialNumber being the only > blocking mechanism. This isn't "done" yet; there's much work still to be done (e.g. improved distribution, see comment #21) and we're aware there are threats (the one you've identified, and others) not addressed by the current implementation. I think discussions on both of these belongs somewhere other than this bug (so they can continue after these parts land).
(In reply to Mark Goodwin [:mgoodwin] from comment #60) > This isn't "done" yet; there's much work still to be done (e.g. improved > distribution, see comment #21) and we're aware there are threats (the one > you've identified, and others) not addressed by the current implementation. Cool. As this was the only bug I saw regarding OneCRL at a high level, my fear was this was "it" for implementation. If there's another place to follow along on the efforts, happy to do so. > I think discussions on both of these belongs somewhere other than this bug > (so they can continue after these parts land). Absolutely agreed. So that we don't drop the ball here, do you have any idea where that discussion should continue? Some new meta-bug in between this and the "disable OCSP"? Some Mozilla forum? I just want to make sure momentum - and concerns - aren't lost in between :)
Blocks: 1105731
(In reply to Ryan Sleevi from comment #62) > Absolutely agreed. So that we don't drop the ball here, do you have any idea > where that discussion should continue? Some new meta-bug in between this and > the "disable OCSP"? Some Mozilla forum? I just want to make sure momentum - > and concerns - aren't lost in between :) I made a new meta bug: bug 1105731 (In reply to Rob Stradling from comment #39) > (BTW, should we move this discussion of Compressed CRLSets > somewhere else?) ^^ we can make a new blocker for the above to discuss this.
Flags: needinfo?(bmcbride)
https://tbpl.mozilla.org/?tree=Try&rev=0423d3d5b5ba Android build problem resolved
Attachment #8529723 - Attachment is obsolete: true
Attachment #8529826 - Flags: review+
Keywords: checkin-needed
sorry had to back this out for Android 4.0 test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4279205&repo=mozilla-inbound
Depends on: 1106021
Added saveInfo.success = true; after the declaration of saveInfo in CertBlocklist::SaveEntries to stop intermittent test failures.
Attachment #8530113 - Attachment is obsolete: true
Attachment #8530254 - Flags: review+
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=afd157989149 Still looks green (but then the failure from comment 69 was intermittent).
Changed to move CertBlocklist initialization from main thread: * Removed checks in CertBlocklist.cpp that ensured init happened on main thread. * nsNSSComponent.cpp no longer forces CertBlocklist init at startup. As such, init is called the first time a cert is verified.
Attachment #8530254 - Attachment is obsolete: true
Attachment #8534633 - Flags: review?(dkeeler)
Comment on attachment 8534633 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8534633 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Ship it! (with comments addressed) ::: security/manager/boot/src/CertBlocklist.cpp @@ +27,5 @@ > + : mIsCurrent(false) > +{ > + mIssuerData = new uint8_t[aIssuer.GetLength()]; > + memcpy(mIssuerData, aIssuer.UnsafeGetData(), aIssuer.GetLength()); > + mIssuer.Init(mIssuerData, aIssuer.GetLength()); nit: we might do 'unused << mIssuer.Init...' here, to indicate that we do know there is a return value and that we're deliberately ignoring it @@ +31,5 @@ > + mIssuer.Init(mIssuerData, aIssuer.GetLength()); > + > + mSerialData = new uint8_t[aSerial.GetLength()]; > + memcpy(mSerialData, aSerial.UnsafeGetData(), aSerial.GetLength()); > + mSerial.Init(mSerialData, aSerial.GetLength()); Same here @@ +39,5 @@ > +{ > + uint32_t issuerLength = aItem.mIssuer.GetLength(); > + mIssuerData = new uint8_t[issuerLength]; > + memcpy(mIssuerData, aItem.mIssuerData, issuerLength); > + mIssuer.Init(mIssuerData, issuerLength); Same @@ +44,5 @@ > + > + uint32_t serialLength = aItem.mSerial.GetLength(); > + mSerialData = new uint8_t[serialLength]; > + memcpy(mSerialData, aItem.mSerialData, serialLength); > + mSerial.Init(mSerialData, serialLength); Same @@ +57,5 @@ > + > +nsresult > +CertBlocklistItem::ToBase64(nsACString& b64IssuerOut, nsACString& b64SerialOut) > +{ > + nsDependentCSubstring issuerString(reinterpret_cast<char *>(mIssuerData), nit: <char*> @@ +59,5 @@ > +CertBlocklistItem::ToBase64(nsACString& b64IssuerOut, nsACString& b64SerialOut) > +{ > + nsDependentCSubstring issuerString(reinterpret_cast<char *>(mIssuerData), > + mIssuer.GetLength()); > + nsDependentCSubstring serialString(reinterpret_cast<char *>(mSerialData), Same @@ +355,5 @@ > +// Write issuer data to the output stream > +PLDHashOperator > +WriteIssuer(nsCStringHashKey* aHashKey, void* aUserArg) > +{ > + BlocklistSaveInfo* saveInfo = reinterpret_cast<BlocklistSaveInfo *>(aUserArg); nit: reinterpret_cast<BlocklistSaveInfo*> @@ +361,5 @@ > + > + saveInfo->issuerTable.RemoveAndForget(aHashKey->GetKey(), issuerSet); > + > + nsresult rv = WriteLine(saveInfo->outputStream, aHashKey->GetKey()); > + if (!NS_SUCCEEDED(rv)) { if (NS_FAILED(rv)) { ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +13,5 @@ > #include "nsComponentManagerUtils.h" > #include "nsDirectoryServiceDefs.h" > #include "nsICertOverrideService.h" > +#include "NSSCertDBTrustDomain.h" > +#include "nsThreadUtils.h" Since we're not otherwise modifying this file, we should probably just leave it alone entirely (as much as I do like seeing #includes sorted...) ::: security/manager/ssl/tests/unit/test_cert_blocklist.js @@ +140,5 @@ > + let file = "tlsserver/" + cert + ".der"; > + addCertFromFile(certDB, file, trust); > +} > + > +function testIsRevoked(certList, issuerString, serialString) { Maybe test_is_revoked, to be consistent with other function names in this file @@ +163,5 @@ > + // arbitrary data (not necessarily DER) to test if items are revoked or not. > + // This test corresponds to: > + // issuer: c29tZSBpbWFnaW5hcnkgaXNzdWVy > + // serial: c2VyaWFsLg== > + ok(testIsRevoked(certList, "some imaginary issuer","serial."), nit: space after the second ',' @@ +169,5 @@ > + > + // And this test corresponds to: > + // issuer: YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy > + // serial: c2VyaWFsMi4= > + ok(testIsRevoked(certList, "another imaginary issuer","serial2."), Same @@ +183,5 @@ > + add_test(function() { > + let certblockObserver = { > + observe: function(aSubject, aTopic, aData) { > + run_next_test(); > + Services.obs.removeObserver(this, "blocklist-updated"); Have the run_next_test be after removing the observer @@ +223,5 @@ > + let contents = ""; > + let hasmore = false; > + do { > + var line = {}; > + hasmore = inputStream.readLine(line); I think there's a utility like readFileToString or readAllToString or readStreamToString that should do this for you. ::: security/manager/ssl/tests/unit/test_onecrl/sample_revocations.txt @@ +22,5 @@ > + c2VyaWFsLg== > +# issuer is "another imaginary issuer" base-64 encoded > +# serial is "serial2." base-64 encoded > +YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy > + c2VyaWFsMi4= How about having another serial here so we can test reading multiple serials per issuer? ::: security/manager/ssl/tests/unit/xpcshell.ini @@ +45,5 @@ > [test_certificate_usages.js] > [test_ocsp_stapling.js] > run-sequentially = hardcoded ports > +[test_cert_blocklist.js] > +skip-if = buildapp == "b2g" Cykesiopka recently made some changes that enabled many of these tests to work on more platforms, so you might see if it's still the case that we need to skip this on b2g. ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +909,5 @@ > + result) { > + let issuer = blocklistElement.getAttribute("issuerName"); > + for (let snElement of blocklistElement.children) { > + try { > + if (issuer) { Why would issuer not be defined? Also, this is in a try block, so it doesn't seem like we need to guard against this, even if it did happen.
Attachment #8534633 - Flags: review?(dkeeler) → review+
(In reply to Mark Goodwin [:mgoodwin] from comment #63) <snip> > I made a new meta bug: bug 1105731 > > (In reply to Rob Stradling from comment #39) > > (BTW, should we move this discussion of Compressed CRLSets > > somewhere else?) > > ^^ we can make a new blocker for the above to discuss this. I filed bug 1111613.
Feedback addressed.
Attachment #8534633 - Attachment is obsolete: true
Attachment #8537482 - Flags: review+
... this time with all of the files
Attachment #8537482 - Attachment is obsolete: true
Attachment #8537484 - Flags: review+
Keywords: checkin-needed
No longer depends on: 1045907
See comment 80.
Flags: needinfo?(mgoodwin)
So it turns out NS_GetSpecialDirectory must be called on the main thread. The problem here is that I've been instructed to keep the filesystem access out of startup... So what I've done is to change CertBlocklist::Init to be called from nsNSSComponent::InitializeNSS *but* only get the profile directory (and do nothing else). This satisfies the NS_GetSpecialDirectory from main thread requirement. I've then added CertBlocklist::EnsureBackingFileInitialized containing the rest of what used to happen in Init (reading in data, if files exist, etc) which gets called from SaveEntries / IsCertRevoked when blocklist or cert verifier stuff happens - this gets the file I/O out of the way of startup. You can review the patch as a whole, or, you can see just these changes to the code you've previously reviewed here: https://hg.mozilla.org/try/rev/b250fee37b9a I've a try run showing Android xpcshell and mochitests pass and talos xperf tests are still ok here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=91cbd2cc8131 and another showing things look sane elsewhere here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ef70d0d355ba
Attachment #8537484 - Attachment is obsolete: true
Flags: needinfo?(mgoodwin) → needinfo?(dkeeler)
Attachment #8544053 - Flags: review?(dkeeler)
Comment on attachment 8544053 [details] [diff] [review] bug1024809-blocklist-certificate-revocation-mechanism.patch Review of attachment 8544053 [details] [diff] [review]: ----------------------------------------------------------------- This is unfortunate, but not a terrible way to do things (in fact, it's quite reasonable given the constraints). I would like to explore why NS_GetSpecialDirectory doesn't work off the main thread, but I think we can punt on that for now. ::: security/manager/boot/src/CertBlocklist.cpp @@ +100,5 @@ > +{ > + if (!gCertBlockPRLog) { > + gCertBlockPRLog = PR_NewLogModule("CertBlock"); > + } > + mBackingIsInitialized = false; nit: this can go with mMutex and mModified (i.e. in the initialization list) Also, we can probably call it 'mBackingFileIsInitialized' @@ +135,5 @@ > +nsresult > +CertBlocklist::EnsureBackingFileInitialized(mozilla::MutexAutoLock& lock) > +{ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::EnsureBackingFileInitialized")); nit: indentation @@ +136,5 @@ > +CertBlocklist::EnsureBackingFileInitialized(mozilla::MutexAutoLock& lock) > +{ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::EnsureBackingFileInitialized")); > + if (!mBackingIsInitialized) { Let's flip this: if (mBackingIsInitialized || !mBackingFile) { return NS_OK; } (I added in the test for mBackingFile as well, so we can get rid of that level of indentation too) @@ +138,5 @@ > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::EnsureBackingFileInitialized")); > + if (!mBackingIsInitialized) { > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::EnsureBackingFileInitialized - not initialized")); nit: indentation @@ +153,5 @@ > + return rv; > + } > + > + PR_LOG(gCertBlockPRLog, PR_LOG_DEBUG, > + ("CertBlocklist::Init certList path: %s", path.get())); nit: indentation @@ +163,5 @@ > + } > + > + if (!exists) { > + PR_LOG(gCertBlockPRLog, PR_LOG_WARN, > + ("CertBlocklist::Init no revocations file")); nit: indentation @@ +415,5 @@ > + return NS_OK; > + } > + > + nsresult rv = EnsureBackingFileInitialized(lock); > + nit: no blank line here @@ +481,5 @@ > +{ > + mozilla::MutexAutoLock lock(mMutex); > + > + nsresult rv = EnsureBackingFileInitialized(lock); > + nit: no blank line here @@ +496,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + CertBlocklistItem item(issuer, serial); > + Probably don't need this blank line either ::: security/manager/boot/src/CertBlocklist.h @@ +61,5 @@ > + CertBlocklistItemState aItemState, > + mozilla::MutexAutoLock& /*proofOfLock*/); > + mozilla::Mutex mMutex; > + bool mModified; > + // Use Get What's this comment about?
Attachment #8544053 - Flags: review?(dkeeler) → review+
Flags: needinfo?(dkeeler)
Changes: 1) renamed mBackingIsInitialized to mBackingFileIsInitialized and moved initialization to the initializer list. 2) fixed some indentation / whitespace nits 3) Flipped the mBackingFileIsInitialized and mBackingFile checks to return early (to reduce indentation and improve readibility). Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0ddff8933e6
Attachment #8544053 - Attachment is obsolete: true
Attachment #8545236 - Flags: review+
Keywords: checkin-needed
Moving to PSM in hopes of the sheriffs checking this in.
Component: CA Certificates → Security: PSM
Product: NSS → Core
Version: trunk → Trunk
https://hg.mozilla.org/mozilla-central/rev/5f8dbb495675
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
relnoted as "Added support for OneCRL for centralized certificate revocation"
relnote-firefox: --- → 37+
Depends on: 1139809
No longer depends on: 1139809
No longer depends on: 1076940
No longer depends on: 907347
No longer depends on: 906611
No longer depends on: 905557
No longer depends on: 901941
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: