403680 - CERT_PKIXVerifyCert fails if CRLs are missing, implement cert_pi_revocationFlags

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

When attempting to verify Paypal's cert for EV, I was calling PKIX_VerifyCert.

It always failed during because there was no CRL imported.
This happened even when the caller did not request crl checking.

It turns out that by default a "NIST CRL policy" is set to enabled.
This policy seems to require that a valid CRL is available.

I propose that by default PKIX_VerifyCert should disable the nist-crl-policy.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288548 [details] [diff] [review]
Patch v1

Perhaps I am mistaken, but it appears to me that this patch disables the 
NIST policy always, not only "by default".  I see no way for the caller
to say "I DO want NIST CRL policy".  

I have no problem with saying that the NIST CRL policy should be disabled
BY DEFAULT, but there must be a way to enable it in the interface to this
function.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to comment #2)
> Perhaps I am mistaken, but it appears to me that this patch disables the 
> NIST policy always, not only "by default".  I see no way for the caller
> to say "I DO want NIST CRL policy".  
>
> I have no problem with saying that the NIST CRL policy should be disabled
> BY DEFAULT, but there must be a way to enable it in the interface to this
> function.


Nelson, is saying
  "I do want NIST CRL policy"
equivalent to saying
  "I do want CRL or OCSP checking for each certificate"
?

My suspicion doesn't come out of the blue, but rather because I saw the following:


The interface of CERT_PKIXVerifyCert already allows to request checking for CRL and/or OCSP checking, see attachment 282175 [details] [diff] [review] from bug 294531.

The interface allows an input parameter cert_pi_revocationFlags
and defined values are
  #define CERT_REV_FLAG_OCSP              1
  #define CERT_REV_FLAG_OCSP_LEAF_ONLY    2
  #define CERT_REV_FLAG_CRL               4
  #define CERT_REV_FLAG_CRL_LEAF_ONLY     8
  #define CERT_REV_FAIL_SOFT           0x10
  #define CERT_REV_NIST		(CERT_REV_FLAG_OCSP|CERT_REV_FLAG_CRL)

The fact that Bob/Steve defined a constant CERT_REV_NIST
to mean "check either ocsp or crl"
led me to my suspicion that that's equivalent to the NIST policy.


(And let's ignore for a moment that cert_pi_revocationFlags is not
 yet hooked up :-( this is being addressed in bug 403684.)

Comment 4

[Robert Relyea]

I believe this is an error, and NIST needs to be a separate flag.

Revocation has to be hooked up and the nist flags need to be changed. I'll look into that today... 

bob

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Hmmm.  I didn't notice the CERT_REV_FAIL_SOFT flag before. 
My first thought is that NIST policy is simply "fail hard" (e.g. !FAIL_SOFT)
in which case the existing CERT_REV_NIST seems correct.  
Bob, please tell us how NIST policy is different than that.

Having said that, here are two comments:

I think NIST CRL policy is that there MUST alawys be a valid CRL or
else the cert must be declared invalid.  

- I don't know how (or IF) OCSP affects that.  Does the presence of a 
valid OCSP response satisfy NIST policy, even when no valid CRL is found?  
I don't know.  

- I don't know whether NIST policy requires that a valid CRL be present
ONLY when the cert has a CRLDP extension, or whether it requires that 
revocation checking MUST be done, with or without a CRLDP extension.

I guess what I need is a URL citing the document that states the NIST requirement.

Comment 6

[Robert Relyea]

Comment on attachment 288548 [details] [diff] [review]
Patch v1

I'll attach a patch that figures this out from the parameter flags.

Comment 7

[Robert Relyea]

Attachment: Implement revocation the checking flags.

Some notes:

I haven't testing turning on OCSP.
Nelson, if you feel Alexei is a better reviewer for this patch feel free to bounce it to him.
NIST policy in the pkix code means fail hard if you don't find a CRL. I've made the native flags function this way.
The PKIX_Cert API still doesn't check the leaf, so 'leaf only' is a no-op.

Comment 8

[Kai Engert (:KaiE:)]

Maybe the symbol names for the individual CERT_REV_ bits should be clearly distinguishable from the names that represent a combined group of bits, like CERT_REV_NIST. Maybe add _BITS_ to the bits and _POLICY_ to the groups? Or only doing the latter. Just a thought. It might make things more obvious when reading source code that uses these flags.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 289519 [details] [diff] [review]
Implement revocation the checking flags.

I think this code is correct. Although I don't understand why "leaf checking only" is equivalent to "do not do check crl/ocsp at all".

I have some more suggestions how this code could be changed so it gets easier to read it. I will attach my comments as a text file. But those are not stoppers for landing the current revision.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: review comments

Comment 11

[Robert Relyea]

checked in.
Checking in certdb/certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.40; previous revision: 1.39
done
Checking in certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.60; previous revision: 1.59
done


I'm not closing the bug because I would still like to get an additional review from nelson or alexei, which may generate some additional changes.

bob

Comment 13

[Kai Engert (:KaiE:)]

I asked a related question in bug 405139 comment 6.
I thought I'd repeat the question here:

Bob, what flag would be used to express the following:

- I want either CRL or OCSP check
- one soft failure is allowed
- two soft failures mean => hard failure

Comment 14

[Robert Relyea]

Yes, what we really need is a revocation hard failure.
Currently we have a hard failure for CRL's.
I don't know what kind of failure we have for OCSP.


What I would like is if OCSP is on and CRL's are on, then if we can get the status from either of these, then we are fine. One problem I see is we check CRL's during the build-chain phase, but I think we check OCSP at a later phase.

bob

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I have a question about the definition of the "NIST revocation policy".
Which of these definitions matches it?

A) revocation checking MUST be done *WITH A CRL*, and absence of a CRL is 
a hard failure, OCSP checking is also permitted but not required.
or
B) revocation checking MUST be done with a CRL OR with OCSP.  At least 
one of those revocation methods MUST succeed (e.g. MUST have a CRL or 
MUST get a valid OCSP response, or both) or else the cert is considered 
revoked (hard failure).

I ask because the patch I am reviewing clearly indicates that the NIST
policy requires CRLs (choice A above), but I am not certain that is correct.
I want to clarify this as part of the patch review.

Anyone know?

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289519 [details] [diff] [review]
Implement revocation the checking flags.

This is not a complete review.  I want to discuss this with Alexei, and 
I want him to review it also.  But I did find one leak, for which I would 
give it r-.  The leak is in the code before this patch, so it is not a 
regression in this patch, but this patch should fix it.

With the patch applied, the code reads as shown below.  If function
PKIX_PL_Date_Create_UTCTime returns an error object, it will be leaked.
My proposed fix is in a comment in the code below.

>@@ -2285,30 +2287,97 @@ cert_pkixSetParam(PKIX_ProcessingParams 
> 
> 	case cert_pi_date:
> 	    if (param->value.scalar.time == 0) {
>		error = PKIX_PL_Date_Create_UTCTime(NULL, &date, plContext);
> 	    } else {
>		error = pkix_pl_Date_CreateFromPRTime(param->value.scalar.time,
>						       &date, plContext);
> 		if (error != NULL) {
> 		    PORT_SetError(SEC_ERROR_INVALID_TIME);
> 		    r = SECFailure;
>		    break;
> 		}
> 	    }     // I propose to move this line up 5 lines, and unindent
>                 // the "if (error" block.
>	    error = PKIX_ProcessingParams_SetDate(procParams, date, plContext);
> 	    if (error != NULL) {
> 		PORT_SetError(SEC_ERROR_INVALID_TIME);
> 		r = SECFailure;
> 	    }

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

I just read Kai's review comments in the attachment.  I was having some 
(maybe more) of the very same confusion that Kai was having over why the 
patch disables OCSP when CERT_REV_FLAG_OCSP_LEAF_ONLY is set, and why
it disables CRL checking when CERT_REV_FLAG_CRL_LEAF_ONLY is set.

I guess that, as part of this review, I need to check that the LEAF_ONLY 
checks actually do get checked somewhere, since it's not in this code.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

I see that some of the changes suggested by Kai's review comments were 
made before the patch was committed, so the patch above (which I've 
been reviewing) is not what got checked in.  :-/  It would be nice to 
have a patch attached that is what really got checked in.  

I see that the LEAF OCSP and CRL checks are NOT yet implemented.  
Implementing those in PKIX_VerifyCert is an NSS312B1 blocker, IMO.  
The implementation of the cert_pi_revocationFlags is not complete until 
that is done.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch as checked in

This is the patch that was actually committed.
I'm carrying the review request forward to this patch.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 290830 [details] [diff] [review]
patch as checked in

Perhaps I am mistaken, but I think I see a lot of leaks here.

Most (perhaps all) of the PKIX functions return a pointer to a 
PKIX error object, which is NULL on success, and non-NULL on failure.
That is an allocated object, and it must be destroyed (with a call 
to one of the "decref" functions, IIRC).  The code in these functions,
both before and after the patch, never seem to destroy any of the 
error objects.  That's leakage, unless this code is using the 
"global arena" code, which is another type of leakage.  (I hope we've
stopped doing that.)  Since Mozilla is very sensitive to such leakage, 
let's fix this ASAP.

I'm giving this patch a rather pointless r-, in light of the issues
in this comment and my earlier comments.  I'm sure you will produce
another patch to address those issues.  

I want Alexei to also review this patch for any other leaks or errors
that I misssed.

Comment 21

[Alexei Volkov]

Comment on attachment 290830 [details] [diff] [review]
patch as checked in

My understanding of NIST revocation policy is coincide with what was described in "A" of comment 15. libPkix gives priority to crl checking and does ocsp check only when final chain check happens. But any failures(CRL or OCSP) are resulted in negative outcome of chain validation. 

Leak wise, I agree with Nelson's findings. PKIX_Error pointed by "error" is leaked through out the code. Also, error pointer overwrite indicated in comment #16 should be fixes. DecRef function also returns a pointer to PKIX_Error. We should at least assert that it is equal to NULL to check integrity of our code.

Some functions (like PKIX_List_Create) are also return a pointer to PKIX_Error that should be checked. 

Result of cert_PKIXMakeOIDList is not checked(line 2192). We may end up
checking the chain without a policy list. Should be fixed.

Same with the call of cert_GetTargetCertConstraints(line 2580). Result  is not checked.

Same with the call of CERT_GetCertStores(line 2588). Result is not checked. The name of the function should star start with low case "cert".

Same with the call of CERT_GetCertChainFromCert(line 2507). No check of the return result. But this is ifdefed code.

r+ because the patch does the right thing. The way the error object is handled is the legacy of the original code. New patch should be written to clean up those leaks.

Comment 22

[Kai Engert (:KaiE:)]

This bug is supposed to implement support for cert_pi_revocationFlags.

I think we have agreed that we want to focus on OCSP support, and really make it work.

You might be interested in my findings in bug 375019 comment 8.
It explains the current state of calls into OCSP when the caller to CERT_PKIXVerifyCert requests OCSP checking.


Bob, you might already be aware, but I want to ensure you are:

When the caller requests OCSP checking on the leaf cert, only (and I think that's what we plan to do for OCSP?), then the current code will not trigger any OCSP calls.

As Alexei just explained to me, we must call the classic CERT_VerifyCert to validate the leaf cert, which includes OCSP checking for the leaf.


I still don't see clear what NSS API we will use to implement to decide "ocsp checked, no soft failure".

I wonder if you will explicitly request an OCSP check from within CERT_PKIXVerifyCert.

Comment 23

[Kai Engert (:KaiE:)]

On Wednesday we had a conference call.

In that call, Bob Relyea suggested I should try again, whether the latest NSS trunk now correctly supports the flag CERT_REV_FLAG_OCSP (hard failure, not setting the CERT_REV_FAIL_SOFT_OCSP flag).

And indeed!

I set this flag, then I used a firewall to block access to the OCSP server that is specified in PayPal's server cert.

This gave me a plain SSL UI, no EV UI.

So, from my point of view, this bug is fixed.


I'm leaving this bug open, because the remaining task is to address Nelson's review comments.

Comment 24

[Kai Engert (:KaiE:)]

I am assigning this bug to Bob, because it was his patch, and I assume he will address work on Nelson's review comments.

Comment 25

[Kai Engert (:KaiE:)]

In my opinion the implementation of (cert_pi_revocationFlags & CERT_REV_FLAG_OCSP) is not yet correct.

I filed bug 413997 about getting another detail fixed.