Closed
Bug 1128607
Opened 9 years ago
Closed 9 years ago
Add freshness check for OneCRL
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mgoodwin, Assigned: mgoodwin)
References
Details
Attachments
(2 files, 7 obsolete files)
3.11 KB,
patch
|
mgoodwin
:
review+
|
Details | Diff | Splinter Review |
16.68 KB,
patch
|
mgoodwin
:
review+
|
Details | Diff | Splinter Review |
It's important to ensure that the OneCRL Cert Blocklist is fresh to ensure that the new process for EV OCSP is semantically equivalent to the old. Short overview: * Check the blocklist was updated recently. * If it was, disable OCSP for intermediates in-program * If not, treat as before
Assignee | ||
Comment 1•9 years ago
|
||
We'll need something like this. We'll need some tests too. thoughts so far: 1) We should have a pref so that we can turn off disable OSCP bypass (off by default until we're happy we have good coverage) 2) I'm not comfortable with sharing the blocklist update time; I want a failure to persist date in CertBlocklist to prevent updates (so we always have a stale onecrl and OCSP checks aren't bypassed) 3) In the future, it'd be nice to have this more fine-grained (e.g. if we start populating OneCRL with harvested OSCP, that has expiry associated with it)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8565069 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
Fixed some indentation nits
Attachment #8598021 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8598027 [details] [diff] [review] Bug1128607.patch The included test fails at the moment. The reason for this is that NSSCertDBTrustDomain::CheckRevocation returns early here: https://dxr.mozilla.org/mozilla-central/source/security/certverifier/NSSCertDBTrustDomain.cpp#468 This happens because FetchOCSPForDVHardFail is true - which makes no sense as I'm explicitly setting security.OCSP.enabled to 1 in the test. Any ideas?
Flags: needinfo?(dkeeler)
Attachment #8598027 -
Flags: feedback?(dkeeler)
Comment on attachment 8598027 [details] [diff] [review] Bug1128607.patch Review of attachment 8598027 [details] [diff] [review]: ----------------------------------------------------------------- I think this is the right idea, but unfortunately some things will have to be restructured. See comments. ::: browser/app/profile/firefox.js @@ +1733,5 @@ > // 1 = allow MITM for certificate pinning checks. > pref("security.cert_pinning.enforcement_level", 1); > > +// Bypass OCSP for intermediates (if blocklist is fresh) > +pref("security.onecrl.bypass_ocsp", false); Instead of having an additional pref, we could set things up so a value of 0 of the freshness pref disables the feature. @@ +1737,5 @@ > +pref("security.onecrl.bypass_ocsp", false); > + > +// Required blocklist freshness for OneCRL OCSP bypass > +// (default should be at least as large as extensions.blocklist.interval) > +pref("security.onecrl.best_before", 86400); Maybe "maximum_staleness_in_seconds" instead of "best_before"? ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +510,5 @@ > return Success; > } > > + // Bypass the check if OneCRL data is fresh > + if (Preferences::GetBool("security.onecrl.bypass_ocsp")) { The static Preferences API can only be accessed on the main thread, so what we need to do is pass this preference value through nsNSSComponent -> SharedCertVerifier -> CertVerifier -> NSSCertDBTrustDomain. See GetOCSPBehaviorFromPrefs. Also, we should specify a default value (since currently this isn't defined for all products, just firefox for desktop). @@ +517,5 @@ > + // if the blocklist is recently updated, skip OCSP for CA Certs > + if (endEntityOrCA == EndEntityOrCA::MustBeCA) { > + uint32_t lastUpdate = Preferences::GetUint("app.update.lastUpdateTime.blocklist-background-update-timer"); > + uint32_t maxInterval = Preferences::GetUint("security.onecrl.best_before"); > + uint32_t now = uint32_t(PR_Now() / PR_USEC_PER_SEC); We should use the time passed in to CheckRevocation. ::: security/manager/ssl/tests/unit/test_ocsp_url.js @@ +120,5 @@ > + add_test(function() { > + clearOCSPCache(); > + let ocspResponder = start_ocsp_responder(["int"], ["int"]); > + let cert_name = "int"; > + let expected_error = PRErrorCodeSuccess; I would call this something more like expectedResult (as an aside, let's be more consistent about underscores vs. intercaps). @@ +121,5 @@ > + clearOCSPCache(); > + let ocspResponder = start_ocsp_responder(["int"], ["int"]); > + let cert_name = "int"; > + let expected_error = PRErrorCodeSuccess; > + let cert = constructCertFromFile("test_ocsp_url/" + cert_name + ".der"); I don't think these certificates are EV, so we wouldn't be fetching OCSP for their intermediates anyway.
Attachment #8598027 -
Flags: feedback?(dkeeler) → feedback+
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 6•9 years ago
|
||
I'm not happy with making changes in all of those places (nsNSSComponent, SharedCertVerifier, CertVerifier) for simple state that probably belongs to the cert blocklist in any case. Here's an alternative approach that performs the check in CertBlocklist. I think this is cleaner, don't you? While I was at it, I also cleaned up use of things in mozilla mozilla::pkix namespaces in CertBlocklist. Please let me know what you think.
Flags: needinfo?(dkeeler)
Attachment #8598690 -
Flags: feedback?(dkeeler)
Comment on attachment 8598690 [details] [diff] [review] Bug1128607_alternative.patch Review of attachment 8598690 [details] [diff] [review]: ----------------------------------------------------------------- This is a good solution. Perhaps we should be doing something more like this for other configurable revocation settings. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +482,5 @@ > > Result rv; > const char* url = nullptr; // owned by the arena > > + if (endEntityOrCA == EndEntityOrCA::MustBeCA) { I think this block should be moved above this line: https://hg.mozilla.org/mozilla-central/annotate/e0299ad29b85/security/certverifier/NSSCertDBTrustDomain.cpp#l452 The output would essentially be the skipOCSPRequest bool that gets incorporated into the if statement on line 452 (the reason being that if we have a cached response that says something more than "we don't know" about the certificate, we should use it). @@ +490,5 @@ > + if (NS_FAILED(nsrv)) { > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + > + if (skipOCSPRequest ) { nit: unnecessary space before ')' @@ +496,5 @@ > + ("NSSCertDBTrustDomain::CheckRevocation blocklist is fresh")); > + return Success; > + } else { > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("NSSCertDBTrustDomain::CheckRevocation blocklist is stale")); nit: indentation ::: security/manager/boot/public/nsICertBlocklist.idl @@ +51,5 @@ > [const, array, size_is(pubkey_length)] in octet pubkey, > in unsigned long pubkey_length); > + > + /** > + * Check that the blocklist data is current. nit: document what this means (i.e. it depends on a preference value and when the blocklist was last updated) ::: security/manager/boot/src/CertBlocklist.cpp @@ +148,5 @@ > return NS_ERROR_NOT_SAME_THREAD; > } > > + // Register preference callbacks > + nsresult rv = Preferences::RegisterCallbackAndCall(CertBlocklist::PreferenceChanged, nit: long line Also, we need another NS_FAILED(rv) check after this call. @@ +621,5 @@ > + > + // otherwise, ensure the interval from the last update is within the > + // maximum allowed staleness > + uint32_t now = uint32_t(PR_Now() / PR_USEC_PER_SEC); > + int64_t interval = now - sLastBlocklistUpdate; What about if sLastBlocklistUpdate > now? @@ +622,5 @@ > + // otherwise, ensure the interval from the last update is within the > + // maximum allowed staleness > + uint32_t now = uint32_t(PR_Now() / PR_USEC_PER_SEC); > + int64_t interval = now - sLastBlocklistUpdate; > + *_retval = sMaxStaleness >= interval; If this were sMaxStaleness > interval, we wouldn't need to specifically check for sMaxStaleness == 0. @@ +628,5 @@ > +} > + > + > +/* static */ > + nit: unnecessary blank line @@ +630,5 @@ > + > +/* static */ > + > +void > +CertBlocklist::PreferenceChanged(const char* aPref, void* aClosure) This and IsBlocklistFresh need to acquire the lock, right? (Which is problematic, given that this is a static function... I guess make the lock static? Or add a static lock? Or pass "this" as the closure and cast it to a CertBlocklist and acquire that lock, perhaps. If you don't end up using the closure variable, let's not name it so we know it's unused.) @@ +634,5 @@ > +CertBlocklist::PreferenceChanged(const char* aPref, void* aClosure) > + > +{ > + PR_LOG(gCertBlockPRLog, PR_LOG_WARN, > + ("CertBlocklist::PreferenceChanged %s changed", aPref)); nit: indentation
Attachment #8598690 -
Flags: feedback?(dkeeler) → feedback+
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 8•9 years ago
|
||
Feedback addressed; in particular: * Indentation and whitespace nits addressed * The call to IsBlocklistFresh is moved up so the bool in incorporated into the if statement previously on line 452. * The comment in the idl is now more explanatory * Missing NS_FAILED check added * bits of cleanup (In reply to David Keeler [:keeler] (use needinfo?) from comment #7) > This and IsBlocklistFresh need to acquire the lock, right? (Which is > problematic, given that this is a static function... I guess make the lock > static? Or add a static lock? Or pass "this" as the closure and cast it to a > CertBlocklist and acquire that lock, perhaps. I can see why we might want to lock on IsBlocklistFresh but I see no benefit in doing this on pref change (where we're changing a couple of unsigned integers) - have I missed something? I see no harm in this either, though, so I've made the change.
Attachment #8598027 -
Attachment is obsolete: true
Attachment #8598690 -
Attachment is obsolete: true
Attachment #8599328 -
Flags: review?(dkeeler)
Assignee | ||
Comment 9•9 years ago
|
||
Some tests. This explicitly tests for successful EV where: * an OCSP request should not be made because OneCRL data is fresh * an OCSP request should be made because OneCRL data is stale There are already various implicit tests for failure modes where OneCRL data is stale since a pref value of 0 prevents the OneCRL OCSP bypass.
Attachment #8599337 -
Flags: review?(dkeeler)
Comment on attachment 8599337 [details] [diff] [review] Bug1128607_test.patch Review of attachment 8599337 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I think we should add a couple more testcases. ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +159,5 @@ > + // enable OneCRL OCSP skipping - allow staleness of up to 1 day > + Services.prefs.setIntPref("security.onecrl.maximum_staleness_in_seconds", 86400); > + // set the blocklist-background-update-timer value to the recent past > + Services.prefs.setIntPref("app.update.lastUpdateTime.blocklist-background-update-timer", > + Math.floor(Date.now() / 1000) - 1); We should also have a test where app.update.lastUpdateTime.blocklist-background-update-timer is more than 86400 seconds ago (while also not being 0), right? Another good test might be to "revoke" the intermediate via nsICertBlocklist and ensure that we get SEC_ERROR_REVOKED_CERTIFICATE. @@ +165,5 @@ > + // the intermediate should not have an associated OCSP request > + let ocspResponder = start_ocsp_responder(["ev-valid"]); > + check_ee_for_ev("ev-valid", gEVExpected); > + ocspResponder.stop(run_next_test); > + Services.prefs.setIntPref("security.onecrl.maximum_staleness_in_seconds", 0); I think we should use clearUserPref to reset this to the default. Also, this should happen before ocspResponder.stop, I think. @@ +177,5 @@ > + gEVExpected ? ["int-ev-valid", "ev-valid"] > + : ["ev-valid"]); > + check_ee_for_ev("ev-valid", gEVExpected); > + ocspResponder.stop(run_next_test); > + Services.prefs.setIntPref("security.onecrl.maximum_staleness_in_seconds", 0); clearUserPref, before ocspResponder.stop
Attachment #8599337 -
Flags: review?(dkeeler) → review+
Comment on attachment 8599328 [details] [diff] [review] Bug1128607.patch Review of attachment 8599328 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me with comments addressed. Is the current blocklist up-to-date with all of the revoked intermediates we know of? ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +444,5 @@ > PR_ASSERT((!cachedResponsePresent && cachedResponseResult == Success) || > (cachedResponsePresent && cachedResponseResult != Success)); > > + // If we have a fresh OneCRL Blocklist we can skip OCSP for CA certs > + bool skipOCSPRequest; Maybe a better name for this would be "blocklistIsFresh" or something, since there are other reasons why we would skip making an OCSP request. @@ +490,5 @@ > > Result rv; > const char* url = nullptr; // owned by the arena > > + nit: unnecessary blank line ::: security/manager/boot/public/nsICertBlocklist.idl @@ +56,5 @@ > + * time is no more than security.onecrl.maximum_staleness_in_seconds seconds > + * after the last blocklist update (as stored in the > + * app.update.lastUpdateTime.blocklist-background-update-timer pref) > + */ > + boolean isBlocklistFresh(); Don't forget to rev the nsICertBlocklist interface uuid. ::: security/manager/boot/src/CertBlocklist.cpp @@ +626,5 @@ > + > + if (now > sLastBlocklistUpdate) { > + int64_t interval = now - sLastBlocklistUpdate; > + PR_LOG(gCertBlockPRLog, PR_LOG_WARN, > + ("CertBlocklist::IsBlocklistFresh we're after the last BlocklistUpdate " nit: indentation @@ +631,5 @@ > + "interval is %i, staleness %u", interval, sMaxStaleness)); > + *_retval = sMaxStaleness > interval; > + } > + PR_LOG(gCertBlockPRLog, PR_LOG_WARN, > + ("CertBlocklist::IsBlocklistFresh ? %s", *_retval ? "true" : "false")); nit: indentation @@ +632,5 @@ > + *_retval = sMaxStaleness > interval; > + } > + PR_LOG(gCertBlockPRLog, PR_LOG_WARN, > + ("CertBlocklist::IsBlocklistFresh ? %s", *_retval ? "true" : "false")); > + return NS_OK; If now <= sLastBlocklistUpdate, _retval won't be set, so let's initialize it to false at the top of the function. @@ +649,5 @@ > + ("CertBlocklist::PreferenceChanged %s changed", aPref)); > + if (strcmp(aPref, PREF_BACKGROUND_UPDATE_TIMER) == 0) { > + sLastBlocklistUpdate = Preferences::GetUint(PREF_BACKGROUND_UPDATE_TIMER, uint32_t(0)); > + } else if (strcmp(aPref, PREF_MAX_STALENESS_IN_SECONDS) == 0) { > + sMaxStaleness = Preferences::GetUint(PREF_MAX_STALENESS_IN_SECONDS, uint32_t(0)); nit: some long lines here ::: security/manager/boot/src/CertBlocklist.h @@ +79,5 @@ > > protected: > + static void PreferenceChanged(const char* aPref, void* aClosure); > + static uint32_t sLastBlocklistUpdate; > + static uint32_t sMaxStaleness; Since we're passing the CertBlocklist as an argument to PreferenceChanged, these two don't need to be static, but it's not a big deal.
Attachment #8599328 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Feedback addressed. Carrying keeler's r+
Attachment #8599328 -
Attachment is obsolete: true
Attachment #8602628 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Feedback addressed; in particular, a test has been added to ensure that the blocklist is not bypassed when the pref is set to allow some staleness, but the last update is older. I've not added a new test for ensuring that we get SEC_ERROR_REVOKED_CERTIFICATE on cert blocklist revocation since we already have tests for this in security/manager/ssl/tests/unit/test_cert_blocklist.js - if I've misunderstood, let me know and I'll address in an immediate followup. Carrying keeler's r+
Attachment #8599337 -
Attachment is obsolete: true
Flags: needinfo?(dkeeler)
Attachment #8602636 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c238f651f277
Assignee | ||
Comment 15•9 years ago
|
||
modified browser/app/profile/firefox.js so this lands disabled by default.
Attachment #8602628 -
Attachment is obsolete: true
Attachment #8602649 -
Flags: review+
(In reply to Mark Goodwin [:mgoodwin] from comment #13) > Created attachment 8602636 [details] [diff] [review] > Bug1128607_test.patch > > I've not added a new test for ensuring that we get > SEC_ERROR_REVOKED_CERTIFICATE on cert blocklist revocation since we already > have tests for this in > security/manager/ssl/tests/unit/test_cert_blocklist.js - if I've > misunderstood, let me know and I'll address in an immediate followup. I was thinking that we should ensure that the interaction between this feature and revocation via OneCRL works as expected, but I guess when we enable this by default, the existing tests will cover the necessary cases.
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c5305f3151c
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e5010cb3d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/b40e0753d6d3
https://hg.mozilla.org/mozilla-central/rev/a4e5010cb3d1 https://hg.mozilla.org/mozilla-central/rev/b40e0753d6d3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•