Doug tells me that OCSP isn't providing any significant security to our users but is costing us in networking performance.

Comment on attachment 8422209 [details] [diff] [review] disable_ocsp.patch I did not realize desktop has an advanced, but visible option to turn this off. We also have telemetry that shows some small number of people have it turned off: http://telemetry.mozilla.org/#release/29/CERT_OCSP_ENABLED I am will to try it out, but I wish we had some measurements we could use to see if it has any impact.

Was OCSP useless with cert revocations caused by the HeartBleed attack? At least Chrome had a log in updating their revocation list.

> Was OCSP useless with cert revocations caused by the HeartBleed attack? At least Chrome had a log in updating their revocation list. they do not have a full revocation list -- it is a set of specially chosen crls. See https://www.imperialviolet.org/2014/04/19/revchecking.html.

hmm.. looks like this will break the green lock ui (which no one really understand :/ )

To still fetch OCSP for EV, we'll have to modify code around here: https://mxr.mozilla.org/mozilla-central/source/security/certverifier/CertVerifier.cpp#410 410 NSSCertDBTrustDomain 411 trustDomain(trustSSL, 412 ocspFetching == NSSCertDBTrustDomain::NeverFetchOCSP 413 ? NSSCertDBTrustDomain::LocalOnlyOCSPForEV 414 : NSSCertDBTrustDomain::FetchOCSPForEV, Line 412 should probably be (flags & FLAG_LOCAL_ONLY) We might also add a value to security.OCSP.enabled so we'll have 3 states instead of two: 0: never, ever fetch OCSP 1: fetch OCSP for DV and EV 2: fetch OCSP for EV only

Comment on attachment 8422209 [details] [diff] [review] disable_ocsp.patch Review of attachment 8422209 [details] [diff] [review]: ----------------------------------------------------------------- Ian, to test this change the security.OCSP.enabled pref to 0. The effect is to stop turning lock icons green for websites that send OCSP information. The first thing you'll notice is not a lot of mobile websites do send that information (for example, Twitter sends OCSP info for their desktop, but not for their mobile site while Facebook & Gmail don't send OCSP info at all). I attached a screenshot of bugzilla.mozilla.org, which does send OCSP data to mobile devices (since it is just sending the desktop content).

Comment on attachment 8422209 [details] [diff] [review] disable_ocsp.patch r+ based on our conversation about this in IRC just now

Richard, what's the plan/schedule for this?

After some discussion with the PKI team, I would like to propose that we: 1. Disable OCSP for DV validation 2. Continue to require OCSP for EV (falling to DV on timeout) Brad, does that sound OK to you?

NI for comment 12

(In reply to Richard Barnes [:rbarnes] from comment #12) > After some discussion with the PKI team, I would like to propose that we: > 1. Disable OCSP for DV validation > 2. Continue to require OCSP for EV (falling to DV on timeout) > > Brad, does that sound OK to you? That sounds good to me. What time frame do you think we can do this in?

We can definitely get this done for 40, and can probably get it uplifted to 39. Poking around for an assignee right now...

This patch disables OCSP checking for DV on Fennec. Keeler: Does this seem like an acceptable approach to you? I can see the appeal of the pref dance you propose in Comment 12, but I'm not sure it's worth the extra plumbing that would be required. Does this need testing other than manual?

Comment on attachment 8589974 [details] [diff] [review] bug-1010068.patch Review of attachment 8589974 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +370,5 @@ > > // TODO: need to verify that IsRevoked isn't called for trust anchors AND > // that that fact is documented in mozillapkix. > > +#ifdef MOZ_FENNEC A couple of thoughts: * this should be after the OCSP stapling checking, since we should still check/enforce that * as we discussed over video, this will work for the vast majority of users, but I imagine that some will want to reenable OCSP fetching on Fennec. We should file a follow-up to re-work the pref that controls this behavior to be a bit more nuanced. At the moment it's essentially an on/off pref (confusingly represented by an integer), but we could make it represent one of a number of modes (e.g. completely disabled, EV-only, DV-soft-fail, DV-hard-fail, etc.). This would have the added benefit of that we could potentially get rid of the separate soft-fail/hard-fail pref.

This patch moves the DV check down until after stapled and cached responses are checked, as Keeler suggests. It also does the check in a slightly different way.

Comment on attachment 8593976 [details] [diff] [review] bug-1010068.1.patch Review of attachment 8593976 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with comments addressed. I think this will require some test changes, just fyi. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +477,5 @@ > + // (b) We are validating a CA certificate for DV > + bool willNotFetch = (mOCSPFetching == NeverFetchOCSP) || > + (endEntityOrCA == EndEntityOrCA::MustBeCA && > + (mOCSPFetching == FetchOCSPForDVHardFail || > + mOCSPFetching == FetchOCSPForDVSoftFail)); nit: this block already had this problem, but we're inconsistent about whether we put braces around clauses in a == b || c == d - let's just pick one and go with it. @@ +483,5 @@ > + // For Fennec, we will use stapled or cached OCSP, but we will not do > + // a live fetch for any non-EV validation. > + willNotFetch = (mOCSPFetching == NeverFetchOCSP) || > + ((mOCSPFetching != LocalOnlyOCSPForEV) && > + (mOCSPFetching != FetchOCSPForEV)); Looks like the first clause is subsumed by the second one here (i.e. (mOCSPFetching == NeverFetchOCSP) isn't necessary).

Rebase on top of current m-c.

Backed out due to xpcshell test failures: Back out: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e525037fc7a Failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fe10feec1ede Keeler: We need to either make these xpcshell tests aware of when we're not going to fetch OCSP, or enable them to force an OCSP request. Either way, it seems like we probably need a pref to control this, since xpcshell tests don't seem to have a ready way to detect platform. What would you think about following up on the idea we had discussed earlier of assigning new values for security.OCSP.enabled? 1. Define 2 == "EV only" 2. Plumb through from GetRevocationBehaviorFromPrefs to some setting in NSSCertDBTrustDomain. 3. Set the Fennec default to be security.OCSP.enabled = 2 Then we should be able to fix the tests just by forcing them to have security.OCSP.enabled = 1. This also seems a little nicer in terms of forward-compatibility, if we think we might do it on other platforms. Does that strategy seem reasonable to you?

FWIW checking |AppConstants.platform == "android"| might work. At the very least I've confirmed that |AppConstants.platform| works fine on a xpcshell test on Linux.

Posting this for feedback on the approach. This patch should implement the proposed plan of having security.OCSP.enable = 2. Have not tested yet; might require some tests to explicitly set security.OCSP.enable = 1.

Comment on attachment 8607067 [details] [diff] [review] bug-1010068.3.patch Review of attachment 8607067 [details] [diff] [review]: ----------------------------------------------------------------- I think this is a good approach, but there are a couple of details that need working out. ::: security/certverifier/CertVerifier.cpp @@ +177,3 @@ > NSSCertDBTrustDomain::OCSPFetching ocspFetching > + = (mOCSPDownloadConfig == ocspOff) || > + (mOCSPDownloadConfig == ocspEVOnly) || I don't think this is quite right: if mOCSPDownloadConfig is ocspEVOnly, then ocspFetching will be NeverFetchOCSP. Then, when we verify an EV certificate, that will be upgraded to LocalOnlyOCSPForEV, which is not what we want. I think we need to add more cases to the NSSCertDBTrustDomain::OCSPFetching enum. ::: security/certverifier/CertVerifier.h @@ +74,5 @@ > pinningStrict = 2, > pinningEnforceTestMode = 3 > }; > > + enum OcspDownloadConfig { ocspOff = 0, ocspOn, ocspEVOnly }; Let's explicitly set these enum values so it's documented what pref value corresponds to which enum. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +192,5 @@ > MOZ_ASSERT(osc); > MOZ_ASSERT(ogc); > MOZ_ASSERT(certShortLifetimeInDays); > > // 0 = disabled, otherwise enabled nit: update comment? @@ +198,5 @@ > + switch (ocspLevel) { > + case 0: *odc = CertVerifier::ocspOff; break; > + case 1: *odc = CertVerifier::ocspOn; break; > + case 2: *odc = CertVerifier::ocspEVOnly; break; > + default: MOZ_ASSERT(false); // XXX: Set a default value? ocspOn? Yeah, since 1 is already the default value (on line 197), I would factor that out and default to 1 if the stored value isn't one of {0,1,2}.

Thanks for the comments, David. I think this patch should be good to go, modulo any possible broken tests that turn up in try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e9965232c8 (In reply to David Keeler [:keeler] (use needinfo?) from comment #25) > Comment on attachment 8607067 [details] [diff] [review] > bug-1010068.3.patch > > ::: security/certverifier/CertVerifier.cpp > @@ +177,3 @@ > > NSSCertDBTrustDomain::OCSPFetching ocspFetching > > + = (mOCSPDownloadConfig == ocspOff) || > > + (mOCSPDownloadConfig == ocspEVOnly) || > > I don't think this is quite right: if mOCSPDownloadConfig is ocspEVOnly, > then ocspFetching will be NeverFetchOCSP. Then, when we verify an EV > certificate, that will be upgraded to LocalOnlyOCSPForEV, which is not what > we want. I think we need to add more cases to the > NSSCertDBTrustDomain::OCSPFetching enum. Rather than adding more options there, I just changed the logic for how the EV fetching mode is decided so that it's doesn't branch off of ocspFetching, but is computed directly from the inputs.

Comment on attachment 8607554 [details] [diff] [review] bug-1010068.4.patch Review of attachment 8607554 [details] [diff] [review]: ----------------------------------------------------------------- Looks like we'll need to update nsNSSComponent::setValidationOptions / Telemetry::CERT_OCSP_ENABLED as well - they make some assumptions about security.OCSP.enabled. The UI also would need to be updated to reflect these changes. See https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.js#177 and https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.xul#105 (as well as the non-in-content versions, I'm assuming?) It appears that some tests still failed. I'm guessing this is because the tests assume the default prefs are particular values rather than setting them to the necessary values. Come to think of it, in addition to fixing that we should add tests that explicitly test this new value of security.OCSP.enabled. r- for now since this isn't ready to ship. ::: security/certverifier/CertVerifier.cpp @@ +173,1 @@ > NSSCertDBTrustDomain::OCSPFetching ocspFetching Maybe this should be something like "nonEVOCSPFetching"? @@ +215,5 @@ > // restrict the acceptable key usage based on the key exchange method > // chosen by the server. > > #ifndef MOZ_NO_EV_CERTS > + NSSCertDBTrustDomain::OCSPFetching evFetching nit: maybe "evOCSPFetching" @@ +224,1 @@ > // Try to validate for EV first. nit: I would have this comment above the OCSPFetching definition.

This adjusts the tests so they set the appropriate pref values. Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6533257fc208 (the test_cert_overrides.js failure is unrelated)

Comment on attachment 8615653 [details] [diff] [review] patch with test fixups Review of attachment 8615653 [details] [diff] [review]: ----------------------------------------------------------------- The only things that I noticed are the nits that :keeler already called out in his review. I have filed two follow-on bugs, bug 1172495 for the telemetry and bug 1172499 for the UI. ::: security/certverifier/CertVerifier.cpp @@ +215,5 @@ > // restrict the acceptable key usage based on the key exchange method > // chosen by the server. > > #ifndef MOZ_NO_EV_CERTS > + NSSCertDBTrustDomain::OCSPFetching evFetching Carrying forward nits from dkeeler review: ocspFetching -> defaultOCSPFetching evFetching -> evOCSPFetching @@ +224,1 @@ > // Try to validate for EV first. Carrying forward nit from dkeeler review: Move comment above evFetching definition.

Keeler's patch, with nits fixed.

Backed out for the same Android test_cert_overrides.js failures that hit the Try push. https://treeherder.mozilla.org/logviewer.html#?job_id=10529460&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba889bb0741

Fixing the one remaining failing test.

Comment on attachment 8616787 [details] [diff] [review] 1010068-fennec-ocsp-testfix.diff r=keeler Approval Request Comment [Feature/regressing bug #]: n/a [User impact if declined]: HTTPS will continue to be very slow due to OCSP [Describe test coverage new/current, TreeHerder]: OCSP tests have been updated to accommodate the revised policies. [Risks and why]: This patch is a focused, straightforward policy change. Low risk of regression. [String/UUID change made/needed]: n/a

I'd prefer to keep this in 40 as we're very late in beta at this point.

Comment on attachment 8616787 [details] [diff] [review] 1010068-fennec-ocsp-testfix.diff r=keeler Approved for uplift to aurora.

Does tracking-fennec: 39+ need changing? Also, this needs rebasing for Aurora uplift.

Margaret is the tracking-fennec flag yours?

(In reply to Liz Henry (:lizzard) from comment #41) > Margaret is the tracking-fennec flag yours? Yes, the mobile team manages it.