390502 - libpkix fails cert validation when no valid CRL (NIST validation policy is always enforced)

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Alexei Volkov]

During a cert validation libpkix tries to do revocation check with CA's existing/valid crl. If such crl was not found, libpkix treats a subject cert as revoked and fails to build the chain.

This this behavior should be optional depending on user application revocation
policies.

Comment 1

[Julien Pierre]

Alexei, this behavior surprises me. I thought the code would fall back and try to do OCSP if CRL was not found. Does it not ? But it may well be that it fails currently if no revocation is available.

Please also see bug 233806 about this subject. The new NSS PKIX interface should have a way of specifying the CRL policy the application wants.

Comment 2

[Alexei Volkov]

Attachment: patch v1

Apply the patch after patch for bug 309888 was applied and remove PKIX_NOTDEF for
    PKIX_CHECK(
        PKIX_ProcessingParams_SetNISTRevocationPolicyEnabled(procParams,
                                                             PKIX_FALSE,
                                                             plContext),
        PKIX_PROCESSINGPARAMSSETNISTREVOCATIONENABLEDFAILED);

Also patch for 391454 most probably will be needed if validating certs that have VeriSign cert(v1) CA as trust anchor.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 276313 [details] [diff] [review]
patch v1

I want to consult with Julien before giving this patch its final review result, but I am inclined to give it r-, because I believe that the code still enforces parts of the NIST policy even when the new NIST policy boolean is set to PKIX_FALSE.  

I have emailed review comments to Alexei.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 276313 [details] [diff] [review]
patch v1

Alexei, I emailed you review comments on this patch.  
Regarding comment 4 in that message, I think Julien and I have agreed 
that when we're not enforcing NIST's CRL policy, then the ThisUpdate
and NextUpdate CRL field values should not be able to cause a cert 
revocation check to fail, by themselves.  They should be valid dates,
but the actual values of those dates should not matter when NIST policy
is not enforced.  So I think you need to change  the code shown below.

>          /* Check for Date */
>          if (selDate != NULL){
>  
> +            PKIX_CHECK(PKIX_ComCRLSelParams_GetNISTPolicyEnabled
> +                    (params, &nistPolicyEnabled, plContext),
> +                    PKIX_COMCRLSELPARAMSGETNISTPOLICYENABLEDFAILED);
> +
>                  result = PKIX_FALSE;
>  
>                  PKIX_CHECK(PKIX_PL_CRL_VerifyUpdateTime
> -                            (crl, selDate, &result, plContext),
> +                            (crl, selDate, nistPolicyEnabled,
> +                             &result, plContext),
>                              PKIX_CRLVERIFYUPDATETIMEFAILED);
>  
>                  if (result == PKIX_FALSE) {

I think the above test needs to be changed to 
                   if (result == PKIX_FALSE && nistPolicyEnabled) {

>                          PKIX_CRLSELECTOR_DEBUG("DateAndTime match Failed\n");
>                          *pMatch = PKIX_FALSE;
>                          goto cleanup;
>                  }
>          }

Comment 6

[Kai Engert (:KaiE:)]

Nelson, in your previous comment, I think you missed to list your new proposed code.

You're saying "above test needs to be changed to"...
but then you include a piece of code that is context, which is identical with the existing code.

Comment 7

[Kai Engert (:KaiE:)]

I was trying the code from 390888 and I figured I need the patch from this bug, too. After applying them both and following Alexei's instructions from comment 2, I get the following:

BUILD ERROR:
*** CERTVFYPKIX Error- cert_BuildAndVerifyChain: Unable to build chain
*** Cause (2): *** BUILD Error- PKIX_BuildChain: pkix_Build_InitiateBuildChain failed
*** Cause (3): *** BUILD Error- pkix_Build_InitiateBuildChain: Unable to build chain
###!!! ASSERTION: why did NSS call our bad cert handler if all looks good?: 'Not Reached', file /home/kaie/moz/ev/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp, line 2511


However, despite the above error messages, NSS seems to return a positive verification result, because Firefox continues its connection to the site.

And why is NSS calling the bad cert callback, if the cert returns valid verification result?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Kai, please don't test the patch for 390888 any more until a lot more pkix
bugs get fixed.  

libSSL calls the bad cert callback for one reason only, which is that the 
cert verification callback returned SECFailure.

Comment 9

[Kai Engert (:KaiE:)]

Ok, my previous comment is due to some other bug.

NSS fails to verify a server cert, it calls back into PSM's bad-cert-handler. PSM tries to use the cert error log to obtain the details of the failure, but it ends up with an empty error log...

In this case I had attempted to "continue" with the connection, which eventually produced the other error messages.

So the real bug, and I probably should file one for this: On Validation failure, libpkix returns an empty error log. But maybe I'd need to use different functions to obtain the error log, when having pkix-verify enabled? Anyway, I'll take this to a separate bug.

Comment 10

[Alexei Volkov]

Kai, for this patch to work you will need to uncomment the following code in certvfypkix.c:

+#ifdef PKIX_NOTDEF
+    /* Code will be enabled with integration of a patch for bug 390502 */
+    PKIX_CHECK(
+        PKIX_ProcessingParams_SetNISTRevocationPolicyEnabled(procParams,
+                                                             PKIX_FALSE,
+                                                             plContext),
+        PKIX_PROCESSINGPARAMSSETNISTREVOCATIONENABLEDFAILED);
+#endif /* PKIX_NOTDEF */

Also VerifyLog generation depends on bug 391183 and bug 390527. Before we fix those bugs we would not get proper results from the library.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: patch v1 b

This is a version of patch that is merged to the latest trunk. It can be combined with patch v4 c from bug 390888.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v1 c

This patch applies on top of what Alexei has checked in very recently, and it includes the change that Alexei mentioned in comment 10

Comment 13

[Alexei Volkov]

Attachment: patch v2

review adjustments. PKIX_PL_CRL_VerifyUpdateTime is called only when NIST policy is enforced. Comments for the function is changed.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278875 [details] [diff] [review]
patch v2

Alexei,  
Your patch faithfully implements the review comments that I emailed to 
you back when I wrote comment 3.   However, as you know, Julien and I
had an email discussion about my proposal, and he persuaded me that:

1) Your first patch was right to always call PKIX_PL_CRL_VerifyUpdateTime
regardless of nistPolicyEnabled, and 

2) PKIX_PL_CRL_VerifyUpdateTime should check the nextUpdate field (if it 
exists) and the thisUpdate field (which the code inexplicably calls 
lastUpdate) for correct parseable syntax, even if not using NIST policy,
and should throw an error if those are not parseable, even if not using
NIST policy.  It should always check all those fields to see that they
are parseable first, and not check to see that the values are in acceptable
ranges until it knows they're all valid and parseable.

3) If PKIX_PL_CRL_VerifyUpdateTime throws an error, we should treat that
as cause of cert validation failure, even if not using NIST policy, and

4) If PKIX_PL_CRL_VerifyUpdateTime does NOT throw an error, and we're 
not doing NIST policy, then we should ignore the result reported by 
PKIX_PL_CRL_VerifyUpdateTime.

So, in light of that, I wrote comment 5, which suggested a very small
change to pkix_CRLSelector_DefaultMatch from the way it was in your 
first patch.  

The only additional change to PKIX_PL_CRL_VerifyUpdateTime is to 
reorder it so that it doesn't check nextUpdate or lastUpdate to see that
they are in range until after it has parsed all the dates in the CRL,
which is how it worked before your first patch.  

SO, I'm going to ask you to implement that alternative.

Comment 15

[Julien Pierre]

Nelson,

Re: comment 14 issue 2), in the discussion we had, I only meant to always ensure that the CRL and all its fields was properly DER-encoded. It appears that the main CRL template CERT_CrlTemplate already takes care of part of that check, at least for tag and length, though not for the time value and the proper formatting of the time value. I realize there was some confusion since DER actually dictates the format of the value. I would be OK with not calling DER_DecodeTimeChoice in cases where the value is not needed for those fields since at least the tag was checked. Alexei pointed out that thisUpdate (lastUpdate in the code) is always needed in the CRL cache so it is already being decoded. I am OK with not decoding the nextUpdate value in the non-NIST case.

Comment 16

[Alexei Volkov]

Nelson, since there is no need to do decoding and time checks for lastUpdate and nextUpdate fields of CRL, I propose we skip calling PKIX_PL_CRL_VerifyUpdateTime for the case where NIST policies are not enforced.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

OK, in light of comments 15 and 16, the only additional change needed to 
this latest patch is to reorder it so that it doesn't check nextUpdate or 
lastUpdate to see that they are in range until after it has parsed all 
the dates in the CRL, which is how it worked before your first patch.  

Comment 18

[Alexei Volkov]

Attachment: patch v3. Restore order of calls in PKIX_PL_CRL_VerifyUpdateTime

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279001 [details] [diff] [review]
patch v3. Restore order of calls in PKIX_PL_CRL_VerifyUpdateTime

r=nelson

Comment 20

[Alexei Volkov]

integrated into trunk.