714477 - Extended validation check should not check that OCSP fetching is enabled

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

When OCSP fetching is disabled, we should still validate the certificate, because we might have got the revocation information through some other means, such as through OCSP stapling or through OCSP caching.

Instead, we should disable do the validation but avoid doing the OCSP fetch.

Without this, a site that uses OCSP stapling will be shown as non-EV when OCSP fetching is disabled.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Do not check the OCSP enabled pref during extended validation

Comment 2

[Kai Engert (:KaiE:)]

Brian, shouldn't this change be postponed until we're actually supporting OCSP stapling?

Before we disable this check, shouldn't we test that EV still works with OCSP stapling enabled and this check disabled?

Comment 3

[Kai Engert (:KaiE:)]

Just another note.

I agree it is reasonable to assume that the NSS verification call will check for the presence of fresh revocation information, because that's what we've coded.

But we should test to confirm this is true, before landing this patch.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Kai, it is good to fix this soon because it blocks bug 585706 which blocks some work that the toolkit team wants to do to tighten up the preferences API.

I will figure out how to automated a test for this.

Comment 5

[Kai Engert (:KaiE:)]

An automated test is probably difficult to come up with, since we don't cache OCSP responses across sessions.

A manual test might be sufficient.

Use Firewall rules that prevent connections to the OCSP server (very easy to do on Linux with iptables).

Start Firefox and verify "no EV with OCSP blocked".

Restart Firefox and verify "EV with OCSP connections allowed".

Comment 6

[Nick]

Google Chrome is getting rid of OCSP checks. It really doesn't provide any added security (in fact it introduces privacy issues).

Mozilla, consider that EV Certs should still display green even when OCSP is disabled in settings.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Could we get some traction on the review here and in bug 624514?  PSM is the only thing that is blocking us from making touching the pref service off the main thread fatal.

Comment 8

[Kai Engert (:KaiE:)]

In my understanding EV requires by policy that revocation checking is active.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 585580 [details] [diff] [review]
Do not check the OCSP enabled pref during extended validation

please find a different reviewer

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 585580 [details] [diff] [review]
Do not check the OCSP enabled pref during extended validation

When validating EV certificates, we pass CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE which means the validation will only succeed if we already have a cached OCSP response (or cached CRL) when OCSP fetching is disabled.

The old (current) way did not take into consideration the fact that we may have received revocation information for the certificate from alternate means. This change (and maybe more) will be needed for OCSP stapling.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 585580 [details] [diff] [review]
Do not check the OCSP enabled pref during extended validation

Review of attachment 585580 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +287,3 @@
>     *  Whether OCSP is enabled in preferences.
>     */
> +  /*[deprecated]*/ readonly attribute boolean isOcspOn;

And what about removing it completely?

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Do not check the OCSP enabled pref during extended validation, and remove nsIX509CertDB.isOcspOn

Honza, I don't remember why I didn't remove it, because I wrote that patch so long ago. I have removed it in this version.

Comment 13

[Robert Relyea]

Comment on attachment 680365 [details] [diff] [review]
Do not check the OCSP enabled pref during extended validation, and remove nsIX509CertDB.isOcspOn

Hi Brian,

I think I need a little more information on this patch. 

The previous code checked to see whether or not OCSP was turned on and blew out if it was turned off. The new code skips that, and looks at the certwrapper to see if OCSP was used on the cert? and changes some flags. Without any explanation of the code, it looks like some magic variable (ocsp_on) determines that the ocsp was used, and then we set some flag that "prevents network fetching", while the old code magically just failed to turn on the OCSP indicator. It's not clear that the new code is having the same effect.

(The problem with having me review psm code is you get to explain what is going on;).

bob

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp?rev=b9a8253986f7#1062 and http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp?rev=b9a8253986f7#1095

nsCERTValInParamWrapper just transforms the security.* preferences related to certificate validation into a CERTValInParam structure suitable for passing to CERT_PKIXVerifyCert (though, ironically, we cannot use that pre-built structure for *this* invocation of CERT_PKIXVerifyCert, because it ignores some of those options by design). 

In particular, nsCERTValInParamWrapper::IsOCSPDownloadEnabled() just returns the current value of the "security.OCSP.enabled" pref (true if OCSP is enabled, false if disabled).

It is definitely intentional that the new code has a different effect. The original code effectively disabled EV completely if security.OCSP.enabled=false, even if we have fresh revocation information available from elsewhere (e.g. a CRL or a cached OCSP response, such as one from a stapled OCSP response). This patch tries to rectify that. 

Also, the more urgent part of this patch is simply avoiding the use of the pref service in this method, because the pref service is only allowed to be used on the main thread (it isn't thread safe), and this code executes off of the main thread.

Comment 15

[Robert Relyea]

My question was how did we know we got valid revocation information. I think that is given in comment 8, but I didn't see any code changes that enabled it. Are you saying we already set the 'Require Fresh' option in CERT_PKIXVerifyCert()?

bob

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Robert Relyea from comment #15)
> My question was how did we know we got valid revocation information. I think
> that is given in comment 8, but I didn't see any code changes that enabled
> it. Are you saying we already set the 'Require Fresh' option in
> CERT_PKIXVerifyCert()?

Yes: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsIdentityChecking.cpp?rev=57e047e64019#1232

1232 uint64_t revMethodIndependentFlags = 
1233     CERT_REV_MI_TEST_ALL_LOCAL_INFORMATION_FIRST
1234     | CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE;
1235 
1236   // We need a PRUint64 here instead of a nice int64_t (until bug 634793 is
1237   // fixed) to match the type used in security/nss/lib/certdb/certt.h for
1238   // cert_rev_flags_per_method.
1239   PRUint64 methodFlags[2];
1240   methodFlags[cert_revocation_method_crl] = revMethodFlags;
1241   methodFlags[cert_revocation_method_ocsp] = revMethodFlags;
1242 
1243   CERTRevocationFlags rev;
1244 
1245   rev.leafTests.number_of_defined_methods = cert_revocation_method_ocsp +1;
1246   rev.leafTests.cert_rev_flags_per_method = methodFlags;
1247   rev.leafTests.number_of_preferred_methods = 1;
1248   rev.leafTests.preferred_methods = preferedRevMethods;
1249   rev.leafTests.cert_rev_method_independent_flags =
1250     revMethodIndependentFlags;
1251 
1252   rev.chainTests.number_of_defined_methods = cert_revocation_method_ocsp +1;
1253   rev.chainTests.cert_rev_flags_per_method = methodFlags;
1254   rev.chainTests.number_of_preferred_methods = 1;
1255   rev.chainTests.preferred_methods = preferedRevMethods;
1256   rev.chainTests.cert_rev_method_independent_flags =
1257     revMethodIndependentFlags;

Comment 17

[Robert Relyea]

Comment on attachment 680365 [details] [diff] [review]
Do not check the OCSP enabled pref during extended validation, and remove nsIX509CertDB.isOcspOn

r+ rrelyea

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/304b46261f2b

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/304b46261f2b