inconsistent OCSP fetching policy when verifying OCSP responses

RESOLVED FIXED in Firefox 28

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: keeler, Assigned: briansmith)

Tracking

(Blocks 1 bug)

trunk
3.15.5
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed, firefox30 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

From bug 923304 comment 11:
> What appears to be happening is if the certificate that signed the response
> is a CA, when verifying that certificate, CERT_VerifyCert will do an OCSP
> lookup for it (given that it has an OCSP AIA and no id-pkix-ocsp-nocheck
> extension). CERT_VerifyOCSPResponseSignature should have some way to specify
> to CERT_VerifyCert that no OCSP lookups should be performed.
> 
> CERT_VerifyOCSPResponseSignature:
> ...
> 4156         if (CERT_IsCACert(signerCert, NULL)) {
> 4157             certUsage = certUsageAnyCA;
> 4158         } else {
> 4159             certUsage = certUsageStatusResponder;
> 4160         }
> 4161         rv = CERT_VerifyCert(handle, signerCert, PR_TRUE,
> 4162                              certUsage, producedAt, pwArg, NULL);
> 4163         if (rv != SECSuccess) {
> 4164             PORT_SetError(SEC_ERROR_OCSP_INVALID_SIGNING_CERT);
> 4165             goto finish;
> 4166         }
> 
> CERT_VerifyCert:
> ...
> 1332     statusConfig = CERT_GetStatusConfig(handle);
> 1333     if (certUsage != certUsageStatusResponder && statusConfig != NULL) {
> 1334 	if (statusConfig->statusChecker != NULL) {
> 1335 	    rv = (* statusConfig->statusChecker)(handle, cert,
> 1336 							 t, wincx);
> 1337 	    if (rv != SECSuccess) {
> 1338 		LOG_ERROR_OR_EXIT(log,cert,0,0);
> 1339 	    }
> 1340       }
> 1341     }

Quoting Brian: "Presumably, we agree that if NSS doesn't check a delegated OCSP response signing certificate for revocation (which it doesn't), then it shouldn't check the CA for revocation either, for consistency's sake."
This patch avoids doing OCSP checks for the CA when we verify an OCSP response signed directly by the CA, just like we already avoid doing OCSP checks for delegated OCSP response signing certs. I changed both libpkix and the classic NSS code.
Assignee: nobody → brian
Status: NEW → ASSIGNED
Attachment #8355020 - Flags: review?(wtc)
If we can get this into NSS 3.15.4, that would be great. Otherwise, if this goes into NSS 3.15.5, we may need to push NSS 3.15.5 to Firefox 27 Beta later.
Target Milestone: --- → 3.15.4
Comment on attachment 8355020 [details] [diff] [review]
stop-doing-OCSP-for-intermediate-signed-OCSP-responses.patch

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

r=wtc. The proposed change seems reasonable, but I am not an expert
in the subtleties of certificate verification, so I suggest Bob or
Ryan should also review this change, at least the behavior if not the
patch itself.

::: lib/certdb/certi.h
@@ +266,5 @@
> + */
> +SECStatus
> +cert_VerifyCertWithFlags(CERTCertDBHandle *handle, CERTCertificate *cert,
> +		         PRBool checkSig, SECCertUsage certUsage, PRTime t,
> +		         void *wincx, CERTVerifyLog *log, PRUint64 flags);

Two nits:

1. |flags| can be a PRUint32. I don't think we will define more than 32 flags
for this function.

2. List |flags| before the |wincx| parameter. By convention |wincx| is
usually the last parameter. |log| already violated this convention, but
we should not make it worse.

@@ +278,5 @@
> +/* Skip all the OCSP checks during certificate verification, regardless of
> + * the global OCSP settings. By default, certificate |cert| will have its
> + * revocation status checked via OCSP according to the global OCSP settings.
> + */
> +#define CERT_VERIFYCERT_SKIP_OCSP 1

We should note that if |certUsage| is certUsageStatusResponder, then
CERT_VERIFYCERT_SKIP_OCSP is assumed to be set.

::: lib/certhigh/certvfy.c
@@ +1247,4 @@
>      SECStatus rv;
>      unsigned int requiredKeyUsage;
>      unsigned int requiredCertType;
> +    unsigned int leafTrustFlags;

Name this variable |failedFlags| or |leafFailedFlags| to match the
parameter name of cert_CheckLeafTrust.

@@ +1333,4 @@
>  
>      /*
>       * Check revocation status, but only if the cert we are checking
>       * is not a status reponder itself.  We only do this in the case

This comment should be updated to describe the new
!(flags & CERT_VERIFYCERT_SKIP_OCSP) check.

Nit: could you fix the spelling error "reponder"? Thanks.

::: lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
@@ +736,5 @@
>              (response->verifyFcn)((PKIX_PL_Object*)response->pkixSignerCert,
>                                    NULL, response->producedAtDate,
>                                    procParams, pNBIOContext,
>                                    state, buildResult,
>                                    NULL, lplContext),

Should we also add a way to suppress OCSP checking to response->verifyFcn?

Or can we just assume that response->verifyFcn, being a special-purpose
verification function, should handle this properly?
Attachment #8355020 - Flags: superreview?(rrelyea)
Attachment #8355020 - Flags: review?(wtc)
Attachment #8355020 - Flags: review?(ryan.sleevi)
Attachment #8355020 - Flags: review+
I note that this is going to diverge from other major implementations, most notably Microsoft's. I can understand the motivation here for performance, but I've not fully considered the correctness.

http://technet.microsoft.com/en-us/library/ee619754(v=ws.10).aspx
http://technet.microsoft.com/en-us/library/ee619784(v=ws.10).aspx

I don't think this patch is a good candidate for 3.15.4 because of these subtleties.
Let's move this bug to NSS 3.15.5.

I thought more about this bug yesterday and would like to note two things.

1. The reason NSS doesn't check a delegated OCSP response signing certificate
for revocation is probably to avoid circular dependency. This doesn't seem to
be an issue for checking the CA's certificate for revocation. This would
argue against this patch.

2. NSS will check the CA's certificate for revocation during certification
path validation. So the OCSP response signature check doesn't need to duplicate
this revocation check. This would argue for this patch. But for this performance
optimization to work, NSS also needs to check that the CA certificate that signs
the OCSP response is the same as the CA certificate that signs the target
certificate (i.e., |signerCert| is the same as |issuer| in
CERT_VerifyOCSPResponseSignature).
Priority: -- → P1
Target Milestone: 3.15.4 → 3.15.5
(In reply to Wan-Teh Chang from comment #5)
> 1. The reason NSS doesn't check a delegated OCSP response signing certificate
> for revocation is probably to avoid circular dependency.

Right.

> This doesn't seem to be an issue for checking the CA's certificate
> for revocation. This would argue against this patch.

The classic NSS certificate verification code does NOT do OCSP fetching for intermediate certificates; see cert_VerifyCertChainOld. Any revocation checking of intermediate certificates is inconsistent with that policy.

Consider the cases:

1. EE                    <- Intermediate <- Root
2. OCSP(EE) <- Responder <- Intermediate <- Root
3. OCSP(EE)              <- intermediate <- Root

In case #1 and case #2, the classic NSS code will NOT do an OCSP request for the intermediate certificate, but in case #2 it will. In case #2, the classic NSS code will NOT do an OCSP request for the Responder certificate. In case #3, the classic NSS code WILL do an OCSP check on the intermediate certificate. However, it doesn't make sense for this to happen in case #3 but not case #1 or case #2.

This patch changes the classic NSS behavior to be consistent in all three cases.

One might argue that NSS should do an OCSP fetch for the intermediate in all cases. However, if an application wants such behavior, they can get it via libpkix. Firefox explicitly does NOT want that behavior.

> 2. NSS will check the CA's certificate for revocation during certification
> path validation. So the OCSP response signature check doesn't need to
> duplicate this revocation check. This would argue for this patch. But for
> this performance optimization to work, NSS also needs to check that the
> CA certificate that signs the OCSP response is the same as the CA
> certificate that signs the target certificate (i.e., |signerCert| is the
> same as |issuer| in CERT_VerifyOCSPResponseSignature).

This check is required for correctness anyway. If it isn't done then that is a separate bug.

(In reply to Ryan Sleevi from comment #4)
> I note that this is going to diverge from other major implementations, most
> notably Microsoft's. I can understand the motivation here for performance,
> but I've not fully considered the correctness.
> 
> http://technet.microsoft.com/en-us/library/ee619754(v=ws.10).aspx
> http://technet.microsoft.com/en-us/library/ee619784(v=ws.10).aspx

We (Gecko) are not trying to be consistent with Microsoft. We've always intended to handle revocations of intermediates via a side channel instead of via OCSP, at least as long as we've been here. (I was once on the opposite side of the argument, when I first arrived here.) If there is disagreement that about the Gecko-desired behavior being the default in the classic NSS code, then I can change the patch so that it exposes a new variant of CERT_VerifyCert with the Gecko-desired OCSP semantics. Note that Gecko can't use libpkix for performance reasons and other reasons.
Comment on attachment 8355020 [details] [diff] [review]
stop-doing-OCSP-for-intermediate-signed-OCSP-responses.patch

r+ assuming you pick up wtc's review comments.

bob
Attachment #8355020 - Flags: superreview?(rrelyea) → superreview+
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #6)
> (In reply to Wan-Teh Chang from comment #5)
> > 1. The reason NSS doesn't check a delegated OCSP response signing certificate
> > for revocation is probably to avoid circular dependency.
> 
> Right.
> 
> > This doesn't seem to be an issue for checking the CA's certificate
> > for revocation. This would argue against this patch.
> 
> The classic NSS certificate verification code does NOT do OCSP fetching for
> intermediate certificates; see cert_VerifyCertChainOld. Any revocation
> checking of intermediate certificates is inconsistent with that policy.
> 
> Consider the cases:
> 
> 1. EE                    <- Intermediate <- Root
> 2. OCSP(EE) <- Responder <- Intermediate <- Root
> 3. OCSP(EE)              <- intermediate <- Root
> 
> In case #1 and case #2, the classic NSS code will NOT do an OCSP request for
> the intermediate certificate, but in case #2 it will. In case #2, the
> classic NSS code will NOT do an OCSP request for the Responder certificate.
> In case #3, the classic NSS code WILL do an OCSP check on the intermediate
> certificate. However, it doesn't make sense for this to happen in case #3
> but not case #1 or case #2.
> 
> This patch changes the classic NSS behavior to be consistent in all three
> cases.
> 
> One might argue that NSS should do an OCSP fetch for the intermediate in all
> cases. However, if an application wants such behavior, they can get it via
> libpkix. Firefox explicitly does NOT want that behavior.

Chromium *does*, in *certain* circumstances, want Intermediate to be checked for all three cases. Chromium already uses libpkix.

Your patch would, as I understand it, *disable* this for the libpkix case (the chagnes to pkix_pl_ocspresponse)

> 
> > 2. NSS will check the CA's certificate for revocation during certification
> > path validation. So the OCSP response signature check doesn't need to
> > duplicate this revocation check. This would argue for this patch. But for
> > this performance optimization to work, NSS also needs to check that the
> > CA certificate that signs the OCSP response is the same as the CA
> > certificate that signs the target certificate (i.e., |signerCert| is the
> > same as |issuer| in CERT_VerifyOCSPResponseSignature).
> 
> This check is required for correctness anyway. If it isn't done then that is
> a separate bug.
> 
> (In reply to Ryan Sleevi from comment #4)
> > I note that this is going to diverge from other major implementations, most
> > notably Microsoft's. I can understand the motivation here for performance,
> > but I've not fully considered the correctness.
> > 
> > http://technet.microsoft.com/en-us/library/ee619754(v=ws.10).aspx
> > http://technet.microsoft.com/en-us/library/ee619784(v=ws.10).aspx
> 
> We (Gecko) are not trying to be consistent with Microsoft.

Gecko may not be, but this is within NSS, and other NSS consumers (and not just browsers) may.

It also diverges from RFC 3280/5280, and while I can understand that Gecko is going to provide revocation checking via alternate means, that should not be *required* of every NSS using app.

> We've always
> intended to handle revocations of intermediates via a side channel instead
> of via OCSP, at least as long as we've been here. (I was once on the
> opposite side of the argument, when I first arrived here.) If there is
> disagreement that about the Gecko-desired behavior being the default in the
> classic NSS code, then I can change the patch so that it exposes a new
> variant of CERT_VerifyCert with the Gecko-desired OCSP semantics. Note that
> Gecko can't use libpkix for performance reasons and other reasons.

I think it'd be much more sensible to make the desired behaviour opt-in. I would recommend *against* changing libpkix to opt-in to this behaviour. That libpkix uses the 'classic' verification for OCSP is certainly an issue (and one we've run into in the past on our own, in Chromium), but that's a separate issue.

If you were to change the default to disable checking, I would argue that would be a break in backwards compatibility / a 'regression' of sorts, rather than a bug fix, and it'd be trickier for version numbers and to get distros to accept.

So yes, strong +1 to making it an additional/new option for Gecko, but not the default.
This is the patch that Wan-Teh and Bob reviews, without the changes to libpkix, and with Wan-Teh's review comments addressed. I will do the changes to libpkix separately.

The current classic NSS logic does not make sense. Further, the current libpkix logic is nonsensical in even more ways. However, if we make the classic NSS logic without fixing the various nonsense in the libpkix code, some libpkix users may feel like we're regressing in some important way. Unfortunately, fixing all the libpkix nonsense requires bigger and riskier changes than what originally proposed. In fact, those changes are what motivated me to start working on insanity::pkix.

Since Gecko needs the change to the classic NSS code temporarily until it can switch to insanity::pkix, and because this is a pure performance enhancement, and because it is important for us to test this patch ASAP and as much as possible so we can uplift it to mozilla-aurora (at least), it is OK for Gecko to patch its private copy of NSS with this patch. When we build against system NSS we will just forgo the optimization, at least until we can all agree on a course of action that will work for the shared NSS.

I have written some tests for this in a patch that I will attach to bug 923304.

We should discuss the various issues with libpkix in a different forum.
Attachment #8355020 - Attachment is obsolete: true
Attachment #8355020 - Flags: review?(ryan.sleevi)
Attachment #8359074 - Flags: superreview+
Attachment #8359074 - Flags: review+
I landed the private patch and the tests (in bug 923304) on Mozilla-Central:
https://hg.mozilla.org/mozilla-central/rev/2e2c276950b9
Whiteboard: [leave open]
https://hg.mozilla.org/integration/mozilla-inbound/rev/633d2aa37305

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12)
> Backed out for xpcshell failures in the push.
> https://hg.mozilla.org/mozilla-central/rev/f356b409d710

Thanks for backing out my patch. I hadn't realized that the tests in bug 923304 work differently for debug builds vs. release builds. I have corrected the tests to pass. This patch is unchanged.
Comment on attachment 8362903 [details] [diff] [review]
Add CERT_SetOCSPFlags to control the behavior from the previous patch

I'd prefer an explicit initialization of the default values of the OCSP_Global variable. Please add a trailing ", 0" at the end of the init block.

r=kaie
Attachment #8362903 - Flags: review?(kaie) → review+
Comment on attachment 8362903 [details] [diff] [review]
Add CERT_SetOCSPFlags to control the behavior from the previous patch

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

This patch seems OK. In general I object to adding an option.
There are already too many options in NSS. Also, assuming NSS
is shared by independent users in the same process that don't
know about each other, changing such a global option may break
other users of NSS in the same process.

So I still hope at least one other NSS developer will invest
the time to understand this bug as well as Brian does. I won't
be able to do that until Thursday.

::: lib/certhigh/ocsp.c
@@ +929,5 @@
> +
> +    PR_EnterMonitor(OCSP_Global.monitor);
> +    
> +    OCSP_Global.flags = flags;
> +    

Nit: the code review tool showed some spaces that should be removed.

::: lib/certhigh/ocsp.h
@@ +75,5 @@
> + * Set additional options for customizing revocation checking. See the
> + * CERT_OCSP_* flags below. The default is to have no flags set.
> + */
> +extern SECStatus
> +CERT_SetOCSPFlags(PRUint32 flags);

Without a corresponding CERT_GetOCSPFlags function, this function cannot be used safely to set just a particular flag.

Another solution is to add a function to just set a specified flag, like this
(similar to SSL_OptionSetDefault):

extern SECStatus
CERT_SetOCSPOption(PRUint32 which, PRBool on);

@@ +80,5 @@
> +
> +/*
> + * Never do revocation checking of the certificate that signed an OCSP
> + * response. By default, NSS will sometimes do revocation checking of the
> + * signer of an OCSP response and sometimes it won't; this inconsistent

"NSS will sometimes do revocation checking of the signer of an OCSP response
and sometimes it won't" should be made more precise. Also, we should remove "inconsistent" because it implies the inconsistency is undesirable or wrong.
Comment on attachment 8359076 [details] [diff] [review]
The patch in attachment 8359074 [details] [diff] [review], packaged as a private patch for Gecko

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Slower performance and potentially less reliable revocation checking.
Testing completed (on m-c, etc.): This landed on mozilla-central on 2014-01-15; see comment 15. This has automated tests in bug 923304 that I'm also asking to uplift.
Risk to taking this patch (and alternatives if risky): This is a very safe patch, as far as stability is concerned.
String or IDL/UUID changes made by this patch: I would never do this to you.

I'm only asking for this to be uplifted to Firefox 28, *not* Firefox 27.

It is important for us to get this change out as far ahead of the Firefox 30 release as is practical, so that we can start discussing OCSP stapling. For example, we need to update WebPageTest.org to support Firefox 28 so that developers can better understand and measure performance difference between OCSP stapling and OCSP fetching. This can be a big performance win in important user scenerios and in important benchmarks.
Attachment #8359076 - Flags: approval-mozilla-aurora?
Note that today the NSS team has decided that we won't work on a 3.15.5 release, because it's time to move on with releasing 3.16

If Mozilla needs a 3.15.5 release, then Mozilla engineers should work on it, but without introducing any new APIs.
Comment on attachment 8362903 [details] [diff] [review]
Add CERT_SetOCSPFlags to control the behavior from the previous patch

I'm withdrawing my review, because this hasn't landed yet, and because Wan-Teh has convinced me this is the wrong approach.

The right approach is to introduce an API that uses a bitwise set/get function.
Attachment #8362903 - Flags: review+ → review-
(In reply to Ryan Sleevi from comment #8)
> (In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if
> you want a response) from comment #6)
> > Consider the cases:
> > 
> > 1. EE                    <- Intermediate <- Root
> > 2. OCSP(EE) <- Responder <- Intermediate <- Root
> > 3. OCSP(EE)              <- intermediate <- Root
> > 
> > In case #1 and case #2, the classic NSS code will NOT do an OCSP request for
> > the intermediate certificate, but in case #2 it will. In case #2, the
> > classic NSS code will NOT do an OCSP request for the Responder certificate.
> > In case #3, the classic NSS code WILL do an OCSP check on the intermediate
> > certificate. However, it doesn't make sense for this to happen in case #3
> > but not case #1 or case #2.
> > 
> > This patch changes the classic NSS behavior to be consistent in all three
> > cases.
> 
> Chromium *does*, in *certain* circumstances, want Intermediate to be checked
> for all three cases. Chromium already uses libpkix.
> 
> Your patch would, as I understand it, *disable* this for the libpkix case
> (the chagnes to pkix_pl_ocspresponse)

Ryan,

I have removed my proposed changes to libpkix. That change was not necessary for Firefox because Firefox only uses libpkix for EV verification, and for EV verification libpkix has already done the OCSP fetch for the intermediate, so the extra OCSP check will (probably or almost) always result in an OCSP cache hit.

Based on our IRC conversation last week, it's my understanding that you now understand that the ocsp.c code does not do OCSP for intermediates except in this one strange case, and that you are now less concerned about my changes to ocsp.c becoming the default. Thus, the patch I wrote to add the option to disable the OCSP checks of intermediates that sign OCSP responses directly (only) is no longer necessary and the patch in attachment 8359074 [details] [diff] [review] can be checked into NSS proper, instead of only being checked into Firefox's copy, as is the case now.

Am I understanding you correctly? Is there anything else I can clarify?
Flags: needinfo?(ryan.sleevi)
Brian:

I took at look at the code in nss/lib/certhigh/ocsp.c last Thursday
for about half an hour. I believe that with some more work, we can
change the code so that if the OCSP response is signed by the issuer,
we can skip the verification of the issuer certificate entirely. To
make this safe, the OCSP cache will need to be keyed by the
(certID, issuer) pair, where |issuer| is the CERTCertificate of the
issuer that either signed the OCSP response, or delegates to an
authorized responder that signed the response.

When we look up an OCSP response from the cache, we will need to
pass to the lookup function not only the certID but also the issuer
certificate (from the certificate chain constructed by the
verification function). A cache item is only a match if both
|certID| and |issuer| match.

In this design, I believe CERT_VerifyOCSPResponseSignature doesn't
need to verify the |issuer| certificate at all; it would duplicate
the work that will be done by the verification function.

If we are worried that some user of CERT_VerifyOCSPResponseSignature
may depend on the current behavior, we can add a new variant of
the function, CERT_VerifyOCSPResponseSignatureV2.

In libpkix, the |issuer| certificate is passed along with the |cert|
to pkix_OcspChecker_CheckLocal, which ultimately is called by
PKIX_RevocationChecker_Check. So this change seems viable. Right now
pkix_OcspChecker_CheckLocal ignores its |issuer| argument. We can
probably change PKIX_PL_OcspCertID_GetFreshCacheStatus to also take
|issuer| as input.
(In reply to Wan-Teh Chang from comment #23)
> Brian:
> 
> I took at look at the code in nss/lib/certhigh/ocsp.c last Thursday
> for about half an hour. I believe that with some more work, we can
> change the code so that if the OCSP response is signed by the issuer,
> we can skip the verification of the issuer certificate entirely. To
> make this safe, the OCSP cache will need to be keyed by the
> (certID, issuer) pair, where |issuer| is the CERTCertificate of the
> issuer that either signed the OCSP response, or delegates to an
> authorized responder that signed the response.

Are you suggesting we merely check if the OCSP response contains a valid signature from the OCSP signer? If the response is anything other than success, we'll typically abort path construction - thus failing to verify the OCSP signer's certificate, yet (by virtue of aborting), treating the response as valid/legitimate.

As far as I can tell, this is only safe if and only if NSS/libpkix uses OCSP during reverse path checking - that is, after it has constructed a path to a trust anchor, validated the trust anchor and the issuer certificate, then calculates the OCSP response on the EE. However, I seem to recall that libpkix uses OCSP in the forward path building case - where it has not yet verified intermediate is valid (yes, this is an altogether weird/bad case, which is where my comments to Brian came from).

See http://mxr.mozilla.org/nss/source/lib/libpkix/pkix/top/pkix_build.c#2463 , where the CRL check happens before the overall validation check happens (in BUILD_CHECKTRUSTED / BUILD_CHECKTRUSTED2).

That is, it's only guaranteed that the forward checkers have run, but not the reverse checkers.

> 
> When we look up an OCSP response from the cache, we will need to
> pass to the lookup function not only the certID but also the issuer
> certificate (from the certificate chain constructed by the
> verification function). A cache item is only a match if both
> |certID| and |issuer| match.
> 
> In this design, I believe CERT_VerifyOCSPResponseSignature doesn't
> need to verify the |issuer| certificate at all; it would duplicate
> the work that will be done by the verification function.
> 
> If we are worried that some user of CERT_VerifyOCSPResponseSignature
> may depend on the current behavior, we can add a new variant of
> the function, CERT_VerifyOCSPResponseSignatureV2.
> 
> In libpkix, the |issuer| certificate is passed along with the |cert|
> to pkix_OcspChecker_CheckLocal, which ultimately is called by
> PKIX_RevocationChecker_Check. So this change seems viable. Right now
> pkix_OcspChecker_CheckLocal ignores its |issuer| argument. We can
> probably change PKIX_PL_OcspCertID_GetFreshCacheStatus to also take
> |issuer| as input.
Flags: needinfo?(ryan.sleevi)
Unclear the the patch posted for uplift still holds good for aurora based on last few comments here. Brian can you please help clarify ?
Flags: needinfo?(brian)
Ryan: thanks for the comment. You're right that I incorrectly assumed
revocation checkers are only run during reverse path checking.

This patch removes the revocation checking during forward path checking.
It removes the BUILD_CRLPREP build state, and the revCheckDelayed boolean
in the libpkix forward builder state, which can be used to suppress
revocation checking during forward path checking (but only if the candidate
cert is not trusted).

The existing revocation checking during reverse path checking is the
pkix_Build_ValidateEntireChain call at pkix_build.c:2556:

http://mxr.mozilla.org/nss/source/lib/libpkix/pkix/top/pkix_build.c#2554

pkix_Build_ValidateEntireChain calls pkix_CheckChain, which calls
PKIX_RevocationChecker_Check.
Attachment #8367671 - Flags: superreview?(rrelyea)
Attachment #8367671 - Flags: review?(ryan.sleevi)
Comment on attachment 8367671 [details] [diff] [review]
libpkix: only run the revocation checkers during reverse path checking

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

::: lib/libpkix/pkix/top/pkix_build.c
@@ -2465,5 @@
> -                PKIX_UInt32 reasonCode;
> -
> -                verifyError =
> -                    PKIX_RevocationChecker_Check(
> -                             state->prevCert, state->candidateCert,

A strange thing about this PKIX_RevocationChecker_Check call is that it is doing revocation checking on state->prevCert rather than state->candidateCert. (The first argument is |cert| and the second argument is |issuer|.)

So, if the output argument revStatus is PKIX_RevStatus_Revoked, we actually need to remove state->prevCert from state->trustChain and backtrack. But we leave the revoked state->prevCert in state->trustChain and go on to try the next candidate cert. This mistake alone convinces me that we can remove this code without making libpkix less correct.
Comment on attachment 8367671 [details] [diff] [review]
libpkix: only run the revocation checkers during reverse path checking

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

r=ryan.sleevi

I've confirmed that revocation checking still proceeds using pkix_Build_ValidateEntireChain, which uses pkix_CheckChain to validate the chain.

This is less than optimal from a path building side, in that a revoked end-entity certificate will still allow forward path building to continue if there are cross-signed intermediates - because it just counts as a node failure against the overall chain - but I'm willing to live with that performance trade-off.

::: lib/libpkix/pkix/top/pkix_build.h
@@ +32,1 @@
>          BUILD_CRL1,

FWIW, BUILD_CRL1, BUILD_CRL2PREP, and BUILD_CRL2PREP are also completely unused.
Attachment #8367671 - Flags: review?(ryan.sleevi) → review+
Comment on attachment 8359074 [details] [diff] [review]
stop-doing-OCSP-for-intermediate-signed-OCSP-responses.patch

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

r=wtc. I now support checking in this patch for NSS 3.15.5.

::: lib/certdb/certi.h
@@ +263,5 @@
>  
> +/* Like CERT_VerifyCert, except with an additional argument, flags. The
> + * flags are defined immediately below.
> + *
> + * OCSP checking is always skipped when certUsage is certUsageStatusResponder.

Perhaps this comment should be moved to the comment block before CERT_VERIFYCERT_SKIP_OCSP?

@@ +271,5 @@
> +                         PRBool checkSig, SECCertUsage certUsage, PRTime t,
> +                         PRUint32 flags, void *wincx, CERTVerifyLog *log);
> +
> +/* Use the default settings.
> + * cert_VerifyCertWithFlags(..., CERT_VERIFYCERT_USE_DEFAULTS) is equivalent

Nit: add ", ..." after CERT_VERIFYCERT_USE_DEFAULTS because |flags| is no longer the last argument.
Attachment #8359074 - Flags: review+
Ryan: thank you. I also removed those three unused forward builder statuses.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/f2132268a92a
Attachment #8367671 - Attachment is obsolete: true
Attachment #8367671 - Flags: superreview?(rrelyea)
Attachment #8368136 - Flags: checked-in+
Attachment #8368136 - Attachment is patch: true
Comment on attachment 8362903 [details] [diff] [review]
Add CERT_SetOCSPFlags to control the behavior from the previous patch

As discussed in the NSS teleconference today, this patch is no longer necessary since we have agreement that changing the default behavior is OK and there's no reason to override it, given other related issues overshadow any problems with the new default behavior.
Attachment #8362903 - Attachment is obsolete: true
Comment on attachment 8359076 [details] [diff] [review]
The patch in attachment 8359074 [details] [diff] [review], packaged as a private patch for Gecko

If I'm reading this right, we no longer need this patch - if I'm wrong, please re-nominate.
Attachment #8359076 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Comment on attachment 8369651 [details] [diff] [review]
Make OCSP fetching for intermediate CA certificates consistent, r=wtc, r=rrelyea

I addressed Wan-Teh's review comments and checked in this patch.
Attachment #8369651 - Attachment description: checked-in.patch → Make OCSP fetching for intermediate CA certificates consistent, r=wtc, r=rrelyea
Attachment #8369651 - Flags: review+
Attachment #8369651 - Flags: checked-in+
Attachment #8359076 - Attachment is obsolete: true
Attachment #8359074 - Attachment is obsolete: true
NSS trunk:
http://hg.mozilla.org/projects/nss/rev/b4a6ef6d459a

NSS 3.15.5 branch:
http://hg.mozilla.org/projects/nss/rev/377dc0ceb7f3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Wan-Teh Chang from comment #30)
> Created attachment 8368136 [details] [diff] [review]
> libpkix: only run the revocation checkers during reverse path checking, v2
> 
> Patch checked in: https://hg.mozilla.org/projects/nss/rev/f2132268a92a

I also landed this on the NSS 3.15.5 branch:
http://hg.mozilla.org/projects/nss/rev/8dd2ca93458a
(In reply to Lukas Blakk [:lsblakk] from comment #32)
> Comment on attachment 8359076 [details] [diff] [review]
> The patch in attachment 8359074 [details] [diff] [review], packaged as a
> private patch for Gecko
> 
> If I'm reading this right, we no longer need this patch - if I'm wrong,
> please re-nominate.

Lukas, now that this is merged into NSS 3.15.5, all we need to do is update Firefox 28 to include NSS 3.15.5. I made the uplift approval request in bug 958916.
The patch "Make OCSP fetching for intermediate CA certificates consistent, r=wtc, r=rrelyea" is in the Gecko test suite in security/manager/ssl/tests/unit/test_ocsp_stapling_with_intermediate.js, via bug 923304.
Flags: in-testsuite+
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.