Closed Bug 515279 Opened 12 years ago Closed 12 years ago

CERT_PKIXVerifyCert considers a certificate revoked if cert_ProcessOCSPResponse fails for any reason


(NSS :: Libraries, defect, P1)


(Not tracked)



(Reporter: wtc, Assigned: alvolkov.bgs)



(4 files, 2 obsolete files)

Attached patch Proposed patch (obsolete) — Splinter Review
To reproduce this bug,
1. visit,Strona-glowna.htm
2. click the orange box on the right of the page that says
"MEDICOVER on line, Zaloguj sie!".


(gdb) where
#0  ocsp_AuthorizedResponderForCertID (handle=0xf67f6698, 
    signerCert=0xf1344b40, certID=0xf1344120, thisUpdate=1252455413000000)
    at ocsp.c:4247
#1  0xf771fd24 in ocsp_VerifySingleResponse (single=0xf13440f0, 
    handle=0xf67f6698, signerCert=0xf1344b40, producedAt=1252455413000000)
    at ocsp.c:4344
#2  0xf77209b4 in ocsp_GetVerifiedSingleResponseForCertID (handle=0xf67f6698, 
    response=0xf1341cc0, certID=0xf13414a8, signerCert=0xf1344b40, 
    time=1252455410470483, pSingleResponse=0xf5637a34) at ocsp.c:4963
#3  0xf7720a8a in cert_ProcessOCSPResponse (handle=0xf67f6698, 
    response=0xf1341cc0, certID=0xf13414a8, signerCert=0xf1344b40, 
    time=1252455410470483, certIDWasConsumed=0xf5637acc, 
    cacheUpdateStatus=0xf5637ad0) at ocsp.c:5019
#4  0xf7805935 in pkix_pl_OcspResponse_GetStatusForCert (cid=0xf1311900, 
    response=0xf67c5c50, validity=0xf13267b0, pPassed=0xf5637b64, 
    pReturnCode=0xf5637b6c, plContext=0xf1327488)
    at pkix_pl_ocspresponse.c:1002
#5  0xf779e4b9 in pkix_OcspChecker_CheckExternal (cert=0xf1324b60, 
    issuer=0xf1332720, date=0xf13267b0, checkerObject=0xf1323ef8, 
    procParams=0xf13266e8, methodFlags=5, pRevStatus=0xf5637bf4, 
    pReasonCode=0xf1335208, pNBIOContext=0xf5637c04, plContext=0xf1327488)
    at pkix_ocspchecker.c:323
#6  0xf779fa9d in PKIX_RevocationChecker_Check (cert=0xf1324b60, 
    issuer=0xf1332720, revChecker=0xf131ed50, procParams=0xf13266e8, 
    chainVerificationState=1, testingLeafCert=1, pRevStatus=0xf5637c94, 
    pReasonCode=0xf1335208, pNbioContext=0xf5637ca0, plContext=0xf1327488)
    at pkix_revocationchecker.c:426
#7  0xf77bea27 in pkix_CheckChain (certs=0xf1338b38, numCerts=2, 
    anchor=0xf1339bd0, checkers=0xf1338aa0, revChecker=0xf131ed50, 
    removeCheckedExtOIDs=0xf1339fe0, procParams=0xf13266e8, 
    pCertCheckedIndex=0xf13351f4, pCheckerIndex=0xf13351f8, 
    pRevChecking=0xf1335218, pReasonCode=0xf1335208, pNBIOContext=0xf5637d48, 
    pFinalSubjPubKey=0xf5637d54, pPolicyTree=0xf5637d50, pVerifyTree=0x0, 
    plContext=0xf1327488) at pkix_validate.c:791
#8  0xf77c5c3f in pkix_Build_ValidateEntireChain (state=0xf13351d8, 
    anchor=0xf1339bd0, pNBIOContext=0xf5637e00, pValResult=0xf5637e28, 
    verifyNode=0xf1333b98, plContext=0xf1327488) at pkix_build.c:1339
#9  0xf77c93e6 in pkix_BuildForwardDepthFirstSearch (pNBIOContext=0xf5637f6c, 
    state=0xf13351d8, pValResult=0xf5637f64, plContext=0xf1327488)
    at pkix_build.c:2530
#10 0xf77cd5af in pkix_Build_InitiateBuildChain (procParams=0xf13266e8, 
    pNBIOContext=0xf5638020, pState=0xf5638028, pBuildResult=0xf5638024, 
    pVerifyNode=0xf5638088, plContext=0xf1327488) at pkix_build.c:3551
#11 0xf77ce2a6 in PKIX_BuildChain (procParams=0xf13266e8, 
    pNBIOContext=0xf563809c, pState=0xf5638098, pBuildResult=0xf56380a0, 
    pVerifyNode=0xf5638088, plContext=0xf1327488) at pkix_build.c:3719
#12 0xf7728fea in CERT_PKIXVerifyCert (cert=0xf131bf38, usages=2, 
    paramsIn=0xf56380e8, paramsOut=0xf5638294, wincx=0x0) at certvfypkix.c:2148
#13 0x0889a225 in PKIXVerifyCert (cert_handle=0xf131bf38, policy_oids=0x0, 
    num_policy_oids=0, cvout=0xf5638294)
    at /usr/local/google/home/wtc/chrome2/src/net/base/
#14 0x0889b08e in net::X509Certificate::Verify (this=0xf1311a78, 
    hostname=@0xf131f7d4, flags=3, verify_result=0xf131f808)
    at /usr/local/google/home/wtc/chrome2/src/net/base/
#15 0x0891679f in net::CertVerifier::Request::DoVerify (this=0xf131f7c8)
    at /usr/local/google/home/wtc/chrome2/src/net/base/
#16 0x089163ac in DispatchToMethod<net::CertVerifier::Request, void (net::CertVerifier::Request::*)()> (obj=0xf131f7c8, 
    method=0x8916762 <net::CertVerifier::Request::DoVerify()>, arg=@0xf1311d6c)
    at /usr/local/google/home/wtc/chrome2/src/base/tuple.h:412
#17 0x089163e7 in RunnableMethod<net::CertVerifier::Request, void (net::CertVerifier::Request::*)(), Tuple0>::Run (this=0xf1311d50)
    at /usr/local/google/home/wtc/chrome2/src/base/task.h:307
#18 0x096c15ba in ThreadMain (this=0xf6705fd8)
    at /usr/local/google/home/wtc/chrome2/src/base/
#19 0x0862bef6 in ThreadFunc (closure=0xf6705fd8)
    at /usr/local/google/home/wtc/chrome2/src/base/
#20 0xf76974fb in start_thread () from /lib32/
#21 0xf736f09e in clone () from /lib32/

Eventually, we return to pkix_OcspChecker_CheckExternal, and
set revStatus to PKIX_RevStatus_Revoked, without looking at
the reason of failure (resultCode):,328-329#323

The attached patch sets revStatus to PKIX_RevStatus_Revoked
only if resultCode is SEC_ERROR_REVOKED_CERTIFICATE.  For
all other errors, it leaves revStatus unchanged, with the
initial value PKIX_RevStatus_NoInfo.

Is this the right fix?
Attachment #399367 - Flags: review?(alexei.volkov.bugs)
Apply this patch to inject a DER_GeneralizedTimeToTime failure
at the right place.  Then you can reproduce this bug with any
HTTPS site whose certificate has an OCSP responder.
The CERT_PKIXVerifyCert API has some flags that tell it how to interpret 
various OCSP response errors.  Are you saying that the various settings 
of those flags have no effect?  Or are you merely saying that the particular
setting of those flags that PSM uses always has this effect?  The latter is 
not necessarily an NSS bug.
Those flags have an effect only if revStatus is PKIX_RevStatus_NoInfo:

If we set revStatus to PKIX_RevStatus_Revoked, then it's a done deal.
There are no flags for ignoring a revoked certificate.

Note that even a pkix_pl_OcspResponse_VerifySignature failure leave
revStatus unchanged at PKIX_RevStatus_NoInfo:,319-321#310

For your convenience, here are all the error codes that may
be possibly returned by cert_ProcessOCSPResponse:

Perhaps some other error codes should also cause the certificate
to be considered revoked.
> If we set revStatus to PKIX_RevStatus_Revoked, then it's a done deal.
> There are no flags for ignoring a revoked certificate.
Correct. Revocation flags can only change the meaning of no_info status or, in case when a method has received status "cert valid", can enforce checking revocation by another methods in the list. If any of the methods receive revoked status, it gets immediately returned to a caller of revocation function.

Wan-Teh, I'm not quite agree with your patch since it makes pretty much any ocsp response that would not pass acceptance check to be treated as if we have no info about the certificate. Such response can be negative as well.

However I would agree, that is it wrong to treat a response as a negative one for the incorrect date in it, or a failure of our function to decode a time. I think the solution must be more complex, than just checking for revocation status after the return of cert_ProcessOCSPResponse function.
We may want to get a status for a cert on the response and when see if it passes a response acceptance test and based on these two returns judge about the cert status.
Alexei, thank you for looking at this bug.

After we create an OCSP response object, we go through these steps:

Only the failure of the last step (pkix_pl_OcspResponse_GetStatusForCert)
causes us to set revStatus to PKIX_RevStatus_Revoked.  If any of the
first three steps fails, revStatus is still KIX_RevStatus_NoInfo.
The weight we give to errors in various steps doesn't seem consistent.
In particular, the third step (pkix_pl_OcspResponse_VerifySignature)
sounds very important.
We can not treat first three calls equally with the last one, since we need to protect from adversary changes to the response. And therefore until the signature is verified, we follow the path in case of failure that would set "no info" status for a cert.
Currently, any error returned after validation of a signature on a response is considered to be fatal and causes libpkix to revoke the cert. I propose that we read received status of the cert any way, whenever it is possible. If ocsp_GetVerifiedSingleResponseForCertID return an error, then libpkix will adjust cert status.

Consider the following table. The first column has error codes returned by ocsp_GetVerifiedSingleResponseForCertID. Second - the adjustment of status of the cert in case, when error code was returned and the response(in received CERTOCSPSingleResponse structure) indicates, that the cert was revoked.
The third, adjustment to the status of a cert when response has VALID status.
  Error Codes                                       REVOKED   |  VALID
  SEC_ERROR_INVALID_TIME                            REV       |    NoInfo
  SEC_ERROR_OCSP_FUTURE_RESPONSE                    NoInfo    |    NoInfo
  SEC_ERROR_OCSP_OLD_RESPONSE                       REV       |    NoInfo
  SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE              NoInfo    |    NoInfo
  SEC_ERROR_OCSP_UNKNOWN_CERT                       NoInfo    |    NoInfo
  SEC_ERROR_OCSP_MALFORMED_RESPONSE                 NoInfo    |    NoInfo

Appreciate for your comments.
I like the proposal in comment 8.  

I would propose these refinements.  As I understand it, today, we issue 
SEC_ERROR_INVALID_TIME, if we find that any of the following 3 times 
values is present, but cannot be decoded properly:
- producedAt time
- thisUpdate time
- nextUpdate time  (which is optional)

I propose that we change that so that

a) if nextUpdate is invalid, we simply ignore it.  Treat it as if it had 
not been provided at all, and do not throw SEC_ERROR_INVALID_TIME. and

b) if thisUpdate is invalid but we have a valid productedAt, use the 
producedAt value as a substitute for the thisUpdate value, rather than 
throwing SEC_ERROR_INVALID_TIME  (this suggestion may be more controversial).
Alexei: We're still getting random reports of "certificate revoked"
errors for all certificates (see and  So
I searched for "PKIX_RevStatus_Revoked" in our source tree and
inspected all occurrences.  If there is any other string I should
search for, please let me know.

I found two other issues.  Let me know if you want me to file
separate bugs about them.

1. This one I'm not sure about.  Please review it: 

2. This one is overly strict:,196-198,201#189

If hasFreshStatus is PKIX_TRUE but statusIsGood is PKIX_FALSE,
the ocsp_CertHasGoodStatus call in ocsp_GetCachedOCSPResponseStatusIfFresh
must have returned SECFailure:,4647-4648#4626

ocsp_CertHasGoodStatus may return SECFailure with these
error codes:

Only the SEC_ERROR_REVOKED_CERTIFICATE error should cause
us to set revStatus to PKIX_RevStatus_Revoke.

Note that the error code set by ocsp_CertHasGoodStatus is
not returned to the caller via the missingResponseError

3. In pkix_pl_Pk11CertStore_CheckRevByCrl, if the
cert_CheckCertRevocationStatus call fails, we set
pkixRevStatus to PKIX_RevStatus_Revoked:,548#539

This is overly strict because cert_CheckCertRevocationStatus
may fail with these error codes:

to set pkixRevStatus to PKIX_RevStatus_Revoked.

cert_CheckCertRevocationStatus could use some cleanup. It
may report that the cert is revoked in two ways:
- return SECSuccess and set *revStatus to
- return SECFailure and set error code to
This is confusing and probably wrong.  Perhaps it should
only use the first method.  Please review
cert_CheckCertRevocationStatus carefully.

Thank you for your help!
Alexei, I confirmed that this bug and an incorrect system time
can also cause CERT_PKIXVerifyCert to consider a certificate

In this case, it is the SEC_ERROR_OCSP_FUTURE_RESPONSE or
SEC_ERROR_OCSP_OLD_RESPONSE error that's mapped to the revoked
Attached patch Proposed patch v2 (obsolete) — Splinter Review
Alexei, please review this patch, which implements partially the table in
your comment 8.  A partial fix for this bug is much better than no fix
because many Chromium users are getting SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE
(The OCSP response for its intermediate CA certificate has "unknown" OCSP
certificate status.)

In ocsp.c, it's necessary to pass the error code up to the caller of
ocsp_GetCachedOCSPResponseStatusIfFresh if the ocsp_CertHasGoodStatus
call fails (for example, because the OCSP certiticate status is "unknown").

In pkix_ocspchecker.c, the error code mapping table in your comment 8
needs to be applied to both cached and newly obtained OCSP responses.
Attachment #399367 - Attachment is obsolete: true
Attachment #417609 - Flags: review?(alexei.volkov.bugs)
Attachment #399367 - Flags: review?(alexei.volkov.bugs)
Comment on attachment 417609 [details] [diff] [review]
Proposed patch v2

Nelson, could you review this patch?  This is a partial implementation
of Alexei and your proposal in comment 8 and comment 9, but on machines
with correct system time, this fix is good enough.  Thanks.
Attachment #417609 - Flags: superreview?(nelson)
Comment on attachment 417609 [details] [diff] [review]
Proposed patch v2

Wan-Teh, I see that your function 
pkix_OcspChecker_MapResultCodeToRevStatus implements the
last 3 of the 6 rows from comment 8, for the "revoked"
column only, but not for the valid column.  

I'm surprised that it does not also implement the second row.  
I would think that would be helpful to your users also.  

It would be quite easy to implement that entire table.
Would you like that?

I don't think we can say this bug is RESOLVED/FIXED when 
this patch is committed, but at least improved.
Attachment #417609 - Flags: superreview?(nelson) → superreview+
Nelson: for these three error codes, the table is:

  Error Codes                                       REVOKED   |  VALID
  SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE              NoInfo    |    NoInfo
  SEC_ERROR_OCSP_UNKNOWN_CERT                       NoInfo    |    NoInfo
  SEC_ERROR_OCSP_MALFORMED_RESPONSE                 NoInfo    |    NoInfo

For these three errors, the "revoked" and "valid" columns
are the same, so the actual certificate status in the
OCSP response doesn't matter.  This is why I chose to
implement those three rows of the table first.

You're right that SEC_ERROR_OCSP_FUTURE_RESPONSE row is
also like this, so I can add it to my patch.

Do you want to implement the entire table?  That'll be

that the problem is that the OCSP responder's cert is
issued by the root CA rather than by the intermediate
CA that issued the cert in question.  This violates
RFC 5280, but is a minor violation (someone unfamiliar
with RFC 5280 is likely to think this is allowed).
I should clarify a point.

If NSS (lib/certhigh/ocsp.c) receives an unauthorized
OCSP response, it currently sets the
inspecting the certificate status in the response.
This is why my patch seems to ignore the "valid"
column.  My patch actually does implement the valid
column for those three rows.
from comment 8 as Nelson suggested in comment 14.
Attachment #417609 - Attachment is obsolete: true
Attachment #418328 - Flags: review?(nelson)
Attachment #417609 - Flags: review?(alexei.volkov.bugs)
Comment on attachment 418328 [details] [diff] [review]
Proposed patch v3 (checked in)

r+=nelson, under same assumptions as before.
Attachment #418328 - Flags: review?(nelson) → review+
Comment on attachment 418328 [details] [diff] [review]
Proposed patch v3 (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.60; previous revision: 1.59
Checking in libpkix/pkix/checker/pkix_ocspchecker.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/checker/pkix_ocspchecker.c,v  <--  pkix_ocspchecker.c
new revision: 1.17; previous revision: 1.16
Attachment #418328 - Attachment description: Proposed patch v3 → Proposed patch v3 (checked in)
This patch implements the second row in the table in comment 8.

I don't think we should handle SEC_ERROR_INVALID_TIME.  It is
a kind of DER encoding error, so it should receive the same
treatment as SEC_ERROR_OCSP_MALFORMED_RESPONSE.  If there is
a new signature weakness, we don't want the time fields in an
OCSP response to be used as free-formed blocks an attacker
can use to create hash collisions.

So I suggest that we also add SEC_ERROR_INVALID_TIME to
the switch statement in the pkix_OcspChecker_MapResultCodeToRevStatus
function, or alternatively, only SEC_ERROR_REVOKED_CERTIFICATE should
be mapped to the Revoked status, all other errors should be
mapped to NoInfo.
Attachment #418399 - Flags: review?(nelson)
Comment on attachment 418399 [details] [diff] [review]
Allow an old response with the "revoked" cert status (checked in)

Now I agree with you. Invalid time should not be treated differently from malformed response.
I liked the patch. r=alexei
Attachment #418399 - Flags: superreview+
Comment on attachment 418399 [details] [diff] [review]
Allow an old response with the "revoked" cert status (checked in)

Thanks for your review, Alexei.
Attachment #418399 - Flags: review?(nelson)
Comment on attachment 418399 [details] [diff] [review]
Allow an old response with the "revoked" cert status (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

Checking in lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.61; previous revision: 1.60
Checking in lib/libpkix/pkix/checker/pkix_ocspchecker.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/checker/pkix_ocspchecker.c,v  <--  pkix_ocspchecker.c
new revision: 1.18; previous revision: 1.17
Attachment #418399 - Attachment description: Allow an old response with the "revoked" cert status → Allow an old response with the "revoked" cert status (checked in)
Rather than adding SEC_ERROR_INVALID_TIME to the switch statement,
I think we should change the switch statement to map only
map all other errors to PKIX_RevStatus_NoInfo.

This is essentially my original proposed patch (attachment 399367 [details] [diff] [review]).
Now that we honor old OCSP responses with the "revoked" cert
status, we are ready to do this.
Attachment #420651 - Flags: review?(alexei.volkov.bugs)
Target Milestone: 3.12.5 → 3.12.6
Comment on attachment 420651 [details] [diff] [review]
Map only SEC_ERROR_REVOKED_CERTIFICATE to PKIX_RevStatus_Revoked (checked in)

Completely agree with you now. r=alexei
Attachment #420651 - Flags: review?(alexei.volkov.bugs) → review+
Comment on attachment 420651 [details] [diff] [review]
Map only SEC_ERROR_REVOKED_CERTIFICATE to PKIX_RevStatus_Revoked (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

Checking in pkix_ocspchecker.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/checker/pkix_ocspchecker.c,v  <--  pkix_ocspchecker.c
new revision: 1.19; previous revision: 1.18
Attachment #420651 - Attachment description: Map only SEC_ERROR_REVOKED_CERTIFICATE to PKIX_RevStatus_Revoked → Map only SEC_ERROR_REVOKED_CERTIFICATE to PKIX_RevStatus_Revoked (checked in)
In comment 3 I described three other problems.  Problem #2
has been fixed by my proposed patch v3 (attachment 418328 [details] [diff] [review]).
Problem #1 and problem #3 are still open.  We should
investigate them.  I opened bug 540049 for the remaining
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.