Closed Bug 1128607 Opened 5 years ago Closed 5 years ago

Add freshness check for OneCRL

Categories

(Core :: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

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
Attached patch bug1128607.patch (obsolete) — Splinter Review
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)
Attached patch Bug1128607.patch (obsolete) — Splinter Review
Attachment #8565069 - Attachment is obsolete: true
Attached patch Bug1128607.patch (obsolete) — Splinter Review
Fixed some indentation nits
Attachment #8598021 - Attachment is obsolete: true
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+
Attached patch Bug1128607_alternative.patch (obsolete) — Splinter Review
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+
Attached patch Bug1128607.patch (obsolete) — Splinter Review
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)
Attached patch Bug1128607_test.patch (obsolete) — Splinter Review
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+
Attached patch Bug1128607.patch (obsolete) — Splinter Review
Feedback addressed.

Carrying keeler's r+
Attachment #8599328 - Attachment is obsolete: true
Attachment #8602628 - Flags: review+
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+
Attached patch Bug1128607.patchSplinter Review
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)
You need to log in before you can comment on or make changes to this bug.