1368868 - re-think strictness of OCSP stapling given that other browsers aren't as strict

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #1368433 +++

bug 1368433 (and many, many duplicates) report being unable to reach a vast swath of Microsoft cloud-hosted servers due to incorrect stapled OCSP responses. The quick fix in that bug is to disable stapling. To avoid cross-conversations in that largely operational "stop the pain" bug I'm opening this to consider what we should do about stapling in Firefox 54 (shipping in two weeks!) and beyond.

In bug 1368433 comment 26 ekr suggests making stapling "soft-fail" as Chrome does, so let's start with that. The advantage is that stapling can succeed and then we get the performance and privacy benefits we sought. If it fails because the server set their automation up incorrectly it fails over to the CA's server who presumably are better at that kind of thing.

The disadvantage is that if stapling is broken the server likely won't ever know because the site will just work. We should at the very least spit an error into the web console to give sites a fighting chance.

Do we need to distinguish between error reasons? If the stapled response is an outright "REVOKED" status and otherwise completely validly signed can we hard-fail on that, at least? AIUI that's not the kind of error MS had.

Comment 1

[Daniel Veditz [:dveditz]]

Ultimate decision-making and implementation probably falls on Keeler.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 54 and 55. 
We may also want to ship a fix for this in ESR52.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

We already more or less disregard expired stapled OCSP responses, so we may be able to also disregard responses from expired OCSP responders.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Since the big problem was fixed  by Microsoft I don't think we need to consider this a blocker for 54 anymore. Keeler, anything happening here for 55/56?  Are we simply wontfixing this for 54 and sticking with the status quo for now?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

The crypto engineering team has been discussing this, and I think we're leaning towards keeping this behavior in that Firefox displays an error when it encounters unacceptable stapled OCSP information (the benefit being that otherwise Microsoft might have never discovered there was a problem). We will probably need to change the error classification such that it's overridable, though, because otherwise users just have to switch browsers to complete their tasks.

Comment 6

[Eric Rescorla (:ekr)]

I'm not enthusiastic about making it overridable because there are *other* errors where overriding is very dangerous and users have trouble distinguishing them. Why not instead allow the connection to proceed but provide an infobar that shows the problem or some other non-modal warning.

I'm fairly concerned about being the only browser that has this behavior where other browsers succeed...

NI-ing Panos.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

We have been trying to avoid infobars in general as a notification mechanism, but it may make sense in this case and there is at least some precedent. A somewhat similar situation occurs with a captive portal, where we display an error page when failing to connect to a site, but use an infobar in other tabs that are still functioning.

Needinfo-ing jsavory who works on security and bram who designed our error pages for the UX perspective.

Comment 8

[Bram Pitoyo [:bram]]

I think using the infobar makes sense in this case. If we maintain our own or use Chrome’s CRLSets, would it help make these infobars appear less?

Photon has a design for the infobar, as seen here: https://mozilla.invisionapp.com/share/7DC3VVUJZ#/229252107_Notification_-_Default – see the next two pages, as well.

If we decide to go with it, there are three broad things to consider.

#1
What’s the nature of this problem? This will determine the infobar style that we’ll use in Photon. If informational or not dangerous (like captive portal) then use the grey/white bar. If potentially dangerous, use yellow. If definitely dangerous (e.g. you’ve just ignored a security warning), use red.

#2
Is there a way to phrase this problem that’s accurate, friendly and readable? Keep in mind that most users won’t read the infobar, and those who read it won’t care to finish if it’s jargony. There’s a possibility to explain more by using a “Learn more…” link that leads to SUMO, for example.

#3
What’s the action that we want to encourage our users to do? There should be one. This is why we choose to use the infobar, after all. Is it to report the problem? Is it to go back to safety? If it’s just a notification that’s like saying “Just so you know”, and the only action is a close “x” button, we should consider not interrupting the user at all.

Comment 9

[Gerry Chang [:gchang]]

Mark 54 fix-optional as 54 was released.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like this is still under discussion, not something that going to ship in 55 and likely not 56. 
I'll drop tracking for now but please re-nom if we start planning to ship this as a new feature.

Comment 11

[Julien Cristau [:jcristau]]

I guess whatever ends up happening here won't be 52esr material.

Comment 15

[sharapanoff]

David Keeler has marked bug 1408727 as duplicate to this, but in this thread there is no description of the impact of the FF "strictness"

let me summarize - On Sunday (15.Oct.2017) Firefox has blocked for about 24 hours random internet sites. These sites are running on different web servers(IIS, AkamaiGHost, cloudflare-nginx ...) These sites have HTTPS certificates from different CA (GeoTrust, Symantec, thawte). The certificates were valid (in common sense of valid - trusted, not in CRL, can be validated by OCSP). There was no way regular user to access the blocked sites using Firefox (except if he is smart enough to set security.ssl.enable_ocsp_stapling=false). The site owners couldn't do anything to help the affected users (now there is a recommendation from symantec to restart web service on the impacted server to force caching to renew). 

This bug is 5 months old, are there any plans to be fixed?

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

We're working on addressing this.

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1368868 - give up on ocsp stapling strictness because we can't have nice things

Review commit: https://reviewboard.mozilla.org/r/198290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198290/

Comment 19

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8927072 [details]
bug 1368868 - give up on ocsp stapling strictness because we can't have nice things

https://reviewboard.mozilla.org/r/198290/#review203760

Time for another flag, I think... but this patch looks good, and let's not hold it up. We should nominate it for uplift to 58 beta, since I think we're missing the merge.

::: security/certverifier/NSSCertDBTrustDomain.cpp:424
(Diff revision 1)
> +      // Stapled OCSP response present but invalid for a small number of reasons
> +      // CAs/servers commonly get wrong. This will be treated similarly to an
> +      // expired stapled response.
> +      mOCSPStaplingStatus = CertVerifier::OCSP_STAPLING_INVALID;
> +      MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
> +              ("NSSCertDBTrustDomain: stapled OCSP response: "

Dan's recommendation was that we print to the console here, somehow. Please open a follow-up to figure out how to do that.

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8927072 [details]
bug 1368868 - give up on ocsp stapling strictness because we can't have nice things

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198290/diff/1-2/

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8927072 [details]
bug 1368868 - give up on ocsp stapling strictness because we can't have nice things

https://reviewboard.mozilla.org/r/198290/#review203760

Thanks! I filed a follow-up bug for the console warning. I also added some more tests - could you have another look?

> Dan's recommendation was that we print to the console here, somehow. Please open a follow-up to figure out how to do that.

Filed bug 1416327.

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler]]

ni? to ensure you see comment 21.

Comment 23

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8927072 [details]
bug 1368868 - give up on ocsp stapling strictness because we can't have nice things

https://reviewboard.mozilla.org/r/198290/#review204128

You're right, these are really good tests to add.

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8927072 [details]
bug 1368868 - give up on ocsp stapling strictness because we can't have nice things

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198290/diff/2-3/

Comment 25

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Had to add a minor pref fixup to the modified tests for android:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc43e3f39633
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc1700eb7437
Thanks for the reviews!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c298625e967
give up on ocsp stapling strictness because we can't have nice things r=jcj

Comment 27

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3c298625e967

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval when you're comfortable doing so.

Comment 29

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: ocsp stapling strictness (long-present feature)
[User impact if declined]: potentially unable to access some sites that other browsers can access
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low-to-medium
[Why is the change risky/not risky?]: The code change is minimal, but this is in a complicated, security-sensitive area of the code. There is a precedent for the change we made, however, so I'm fairly confident in it.
[String changes made/needed]: none

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8929555 [details] [diff] [review]
patch for beta

Let's take that in 58. It is a lost cause for now as it is hurting us :/

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9e65f782b6b

Comment 32

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to David Keeler [:keeler] (pto until 27th) from comment #29)
> [Is this code covered by automated tests?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Marking this issue as qe-verify- based that is is covered by automation and manual testing is not required.