Closed Bug 1010068 Opened 10 years ago Closed 9 years ago

Disable OCSP for DV certificates in Firefox for Android

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox39 wontfix, firefox40+ fixed, firefox41 fixed, fennec40+)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox39 --- wontfix
firefox40 + fixed
firefox41 --- fixed
fennec 40+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(4 files, 8 obsolete files)

658.56 KB, image/png
Details
1003.77 KB, image/png
Details
24.53 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
25.45 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Doug tells me that OCSP isn't providing any significant security to our users but is costing us in networking performance.
Attached patch disable_ocsp.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #8422209 - Flags: review?(mark.finkle)
Attachment #8422209 - Flags: review?(dougt)
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.
Attachment #8422209 - Flags: review?(mark.finkle) → review+
Was OCSP useless with cert revocations caused by the HeartBleed attack? At least Chrome had a log in updating their revocation list.
Attachment #8422209 - Flags: review?(dougt) → review+
> 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 :/ )
Flags: needinfo?(dkeeler)
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
Flags: needinfo?(dkeeler)
tracking-fennec: ? → 32+
OS: Mac OS X → Android
Hardware: x86 → All
Attached image With OCSP
Attached image without OCSP
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).
Attachment #8422209 - Flags: review?(ibarlow)
Comment on attachment 8422209 [details] [diff] [review]
disable_ocsp.patch

r+ based on our conversation about this in IRC just now
Attachment #8422209 - Flags: review?(ibarlow) → review+
Depends on: 1016555
tracking-fennec: 32+ → 33+
Blocks: powah
Summary: disable OCSP on Firefox for Android → Disable OCSP in Firefox for Android
tracking-fennec: 33+ → 36+
Depends on: 1128607
tracking-fennec: 36+ → 38+
tracking-fennec: 38+ → ?
Richard, what's the plan/schedule for this?
Flags: needinfo?(rlb)
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?
Flags: needinfo?(rlb)
NI for comment 12
Flags: needinfo?(blassey.bugs)
(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?
Flags: needinfo?(blassey.bugs) → needinfo?(rlb)
We can definitely get this done for 40, and can probably get it uplifted to 39.  Poking around for an assignee right now...
Flags: needinfo?(rlb)
Summary: Disable OCSP in Firefox for Android → Disable OCSP for DV certificates in Firefox for Android
Attached patch bug-1010068.patch (obsolete) — Splinter Review
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?
Attachment #8589974 - Flags: feedback?(dkeeler)
tracking-fennec: ? → 39+
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.
Attachment #8589974 - Flags: feedback?(dkeeler) → feedback+
Attached patch bug-1010068.1.patch (obsolete) — Splinter Review
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.
Attachment #8422209 - Attachment is obsolete: true
Attachment #8589974 - Attachment is obsolete: true
Attachment #8593976 - Flags: review?(dkeeler)
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).
Attachment #8593976 - Flags: review?(dkeeler) → review+
Status: NEW → ASSIGNED
Attached patch bug-1010068.2.patch r=keeler (obsolete) — Splinter Review
Rebase on top of current m-c.
Attachment #8593976 - Attachment is obsolete: true
Attachment #8606465 - Flags: review+
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?
Flags: needinfo?(dkeeler)
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.
Attached patch bug-1010068.3.patch (obsolete) — Splinter Review
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.
Attachment #8606465 - Attachment is obsolete: true
Flags: needinfo?(dkeeler)
Attachment #8607067 - Flags: feedback?(dkeeler)
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}.
Attachment #8607067 - Flags: feedback?(dkeeler) → feedback+
Attached patch bug-1010068.4.patch (obsolete) — Splinter Review
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.
Attachment #8607067 - Attachment is obsolete: true
Attachment #8607554 - Flags: review?(dkeeler)
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.
Attachment #8607554 - Flags: review?(dkeeler) → review-
Attached patch patch with test fixups (obsolete) — Splinter Review
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)
Attachment #8615653 - Flags: review?(rlb)
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.
Attachment #8615653 - Flags: review?(rlb) → review+
Attachment #8607554 - Attachment is obsolete: true
Keeler's patch, with nits fixed.
Attachment #8615653 - Attachment is obsolete: true
Attachment #8616671 - Flags: review+
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
Fixing the one remaining failing test.
Attachment #8616671 - Attachment is obsolete: true
Attachment #8616787 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4bc3d8e62192
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Blocks: 1173679
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
Attachment #8616787 - Flags: approval-mozilla-beta?
Attachment #8616787 - Flags: approval-mozilla-aurora?
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.
Attachment #8616787 - Flags: approval-mozilla-beta?
Attachment #8616787 - Flags: approval-mozilla-beta-
Attachment #8616787 - Flags: approval-mozilla-aurora?
Attachment #8616787 - Flags: approval-mozilla-aurora+
Does tracking-fennec: 39+ need changing? Also, this needs rebasing for Aurora uplift.
Flags: needinfo?(lhenry)
Flags: needinfo?(dkeeler)
Margaret is the tracking-fennec flag yours?
Flags: needinfo?(lhenry) → needinfo?(margaret.leibovic)
(In reply to Liz Henry (:lizzard) from comment #41)
> Margaret is the tracking-fennec flag yours?

Yes, the mobile team manages it.
tracking-fennec: 39+ → 40+
Flags: needinfo?(margaret.leibovic)
Blocks: 1479064
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.