vfychain -p reports revoked cert, but with -pp or no -p reports good chain

RESOLVED FIXED in 3.12.4

Status

NSS
Libraries
P1
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Alexei Volkov)

Tracking

trunk
3.12.4
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX MOZ)

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 369796 [details]
zip file with 3 certs

I have a chain of 3 certs from a live web server www.facebook.com
Using today's trunk NSS build, when I try to verify that chain using vfychain
I get different results depending on the number of -p arguments given.
With 0 or 2 -p arguments, I get a report that the chain is good.
But with 1 -p argument, I get a report that there is a revoked cert.  
I should get the same result from all 3 tests.  

The 3 test commands are:
vfychain -u 1 -v   -d DB cert.000 cert.001 cert.002
vfychain -u 1 -vp  -d DB cert.000 cert.001 cert.002
vfychain -u 1 -vpp -d DB cert.000 cert.001 cert.002

The attached zip file contains the 3 certs.  
Test this with an empty cert and key DB pair, and with trunk nssckbi.
 
It would be nice if we could fix it for Monday's release.
I will attempt to debug this a bit.
It's reporting that the root has been revoked!  :-o
Here is the stack where things go wrong.

nss3.dll!pkix_BuildForwardDepthFirstSearch() 	 Line 2742	C
nss3.dll!pkix_Build_InitiateBuildChain() 	 Line 3512 + 0x18 bytes	C
nss3.dll!PKIX_BuildChain()			 Line 3685 + 0x1d bytes	C
nss3.dll!cert_BuildAndValidateChain()		 Line  823 + 0x1d bytes	C
nss3.dll!cert_VerifyCertChainPkix()		 Line 1259 + 0x15 bytes	C
nss3.dll!cert_VerifyCertChain()			 Line  880 + 0x29 bytes	C
nss3.dll!CERT_VerifyCertChain()			 Line  892 + 0x29 bytes	C
nss3.dll!CERT_VerifyCert()			 Line 1488 + 0x25 bytes	C
nss3.dll!pkix_pl_OcspResponse_VerifyResponse()	 Line  721 + 0x28 bytes	C
nss3.dll!pkix_pl_OcspResponse_VerifySignature()	 Line  900 + 0x28 bytes	C
nss3.dll!pkix_OcspChecker_CheckExternal()	 Line  317 + 0x1d bytes	C ***
nss3.dll!PKIX_RevocationChecker_Check()		 Line  422 + 0x30 bytes	C
nss3.dll!pkix_CheckChain()			 Line  801 + 0x36 bytes	C
nss3.dll!pkix_Build_ValidateEntireChain()	 Line 1369 + 0x64 bytes	C
nss3.dll!pkix_BuildForwardDepthFirstSearch()	 Line 2501 + 0x23 bytes	C
nss3.dll!pkix_Build_InitiateBuildChain()	 Line 3512 + 0x18 bytes	C
nss3.dll!PKIX_BuildChain()			 Line 3685 + 0x1d bytes	C
nss3.dll!cert_BuildAndValidateChain()		 Line  823 + 0x1d bytes	C
nss3.dll!cert_VerifyCertChainPkix()		 Line 1259 + 0x15 bytes	C
nss3.dll!cert_VerifyCertChain()			 Line  880 + 0x29 bytes	C
nss3.dll!CERT_VerifyCertificate()		 Line 1296 + 0x2d bytes	C
vfychain.exe!main()				 Line  623 + 0x2a bytes	C


It's doing an OCSP check!  Nothing on the command line told it to do that!
I think it's failing to construct a valid issuer chain for the signature 
on the OCSP response.  But that should work.  

So there are TWO mysteries here:
1) Why is it failing to construct a chain for the OCSP response signature?
2) Why is it doing OCSP at all?!
Alexei, you asked about bugs that block the adoption of libPKIX to the 
exclusion of the old library code.  This is one of them.
Priority: -- → P1
(Reporter)

Updated

9 years ago
Blocks: 479393
(Assignee)

Updated

9 years ago
Whiteboard: PKIX MOZ
(Assignee)

Comment 5

9 years ago
Created attachment 370718 [details] [diff] [review]
Patch v1 - disable ocsp checking when ocsp statucConfig or statusChecker are not set in db handle

libpkix wrapper for the old code sets incorrect ocsp method flags. For compatibility reasons, ocsp check should be disallowed in case when 
statucConfig or statusChecker are not set in db handle. This patch fixes this.

Still looking for the reason, why libpkix filed to validate an ocsp response from ocsp.digicert.com.
Attachment #370718 - Flags: review?(nelson)
Comment on attachment 370718 [details] [diff] [review]
Patch v1 - disable ocsp checking when ocsp statucConfig or statusChecker are not set in db handle

Two issues with this patch:

> +    /* For compatibility with the old code, need to check that
> +     * statusConfig is set in the db handle and status checker
> +     * is defined befor allow ocsp status check on the leaf cert.*/
> +    statusConfig = CERT_GetStatusConfig(CERT_GetDefaultCertDB());
> +    if (statusConfig == NULL || statusConfig->statusChecker != NULL) {
 
1) That last test should be == NULL.
2) If that test is true, then instead of merely zeroing methodFlags,
   I think this should skip all the code shown below.

> +        methodFlags &= PKIX_REV_M_DO_NOT_TEST_USING_THIS_METHOD;
> +    }
> +
>      /* Disabling ocsp fetching when checking the status
>       * of ocsp response signer. Here and in the next if,
>       * adjust flags for ocsp signer cert validation case. */
>      if (disableOCSPRemoteFetching) {
>          methodFlags |= PKIX_REV_M_FORBID_NETWORK_FETCHING;
>      }
>  
>      if (ocsp_FetchingFailureIsVerificationFailure()
>          && !disableOCSPRemoteFetching) {
>          methodFlags |=
>              PKIX_REV_M_FAIL_ON_MISSING_FRESH_INFO;
>      }
>  
>      /* add OCSP revocation method to check only the leaf certificate.*/
>      PKIX_CHECK(
>          PKIX_RevocationChecker_CreateAndAddMethod(revChecker, procParams,
>                                       PKIX_RevocationMethod_OCSP, methodFlags,
>                                       1, NULL, PKIX_TRUE, plContext),
>          PKIX_REVOCATIONCHECKERADDMETHODFAILED);
>
Attachment #370718 - Flags: review?(nelson) → review-
(Reporter)

Updated

9 years ago
Target Milestone: 3.12.3 → 3.12.4
(Assignee)

Comment 7

9 years ago
Created attachment 371511 [details] [diff] [review]
370718: Patch v2 - disable ocsp checking when ocsp statucConfig or statusChecker are not set in db handle(committed)

Address review comments.
Attachment #370718 - Attachment is obsolete: true
Attachment #371511 - Flags: review?(nelson)
(Reporter)

Updated

9 years ago
Attachment #371511 - Flags: review?(nelson) → review+
Comment on attachment 371511 [details] [diff] [review]
370718: Patch v2 - disable ocsp checking when ocsp statucConfig or statusChecker are not set in db handle(committed)

r=nelson
(Assignee)

Updated

9 years ago
Attachment #371511 - Attachment description: 370718: Patch v2 - disable ocsp checking when ocsp statucConfig or statusChecker are not set in db handle → 370718: Patch v2 - disable ocsp checking when ocsp statucConfig or statusChecker are not set in db handle(committed)
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.