Closed Bug 515279 Opened 12 years ago Closed 12 years ago
_PKIXVerify Cert considers a certificate revoked if cert _Process OCSPResponse fails for any reason
1.08 KB, patch
|Details | Diff | Splinter Review|
4.18 KB, patch
|Details | Diff | Splinter Review|
4.73 KB, patch
|Details | Diff | Splinter Review|
1.34 KB, patch
|Details | Diff | Splinter Review|
To reproduce this bug, 1. visit http://www.medicover.com/plpl/152,Strona-glowna.htm 2. click the orange box on the right of the page that says "MEDICOVER on line, Zaloguj sie!". We set the SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE error at ocsp.c:4247: (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/x509_certificate_nss.cc:433 #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/x509_certificate_nss.cc:538 #15 0x0891679f in net::CertVerifier::Request::DoVerify (this=0xf131f7c8) at /usr/local/google/home/wtc/chrome2/src/net/base/cert_verifier.cc:42 #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/worker_pool_linux.cc:78 #19 0x0862bef6 in ThreadFunc (closure=0xf6705fd8) at /usr/local/google/home/wtc/chrome2/src/base/platform_thread_posix.cc:26 #20 0xf76974fb in start_thread () from /lib32/libpthread.so.0 #21 0xf736f09e in clone () from /lib32/libc.so.6 Eventually, we return to pkix_OcspChecker_CheckExternal, and set revStatus to PKIX_RevStatus_Revoked, without looking at the reason of failure (resultCode): http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_ocspchecker.c&rev=1.16&mark=324,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?
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.
Wan-Teh, 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: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_ocspchecker.c&rev=1.16&mark=335-339#323 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: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_ocspchecker.c&rev=1.16&mark=311,319-321#310 For your convenience, here are all the error codes that may be possibly returned by cert_ProcessOCSPResponse: SEC_ERROR_REVOKED_CERTIFICATE SEC_ERROR_INVALID_TIME SEC_ERROR_OCSP_FUTURE_RESPONSE SEC_ERROR_OCSP_OLD_RESPONSE SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE SEC_ERROR_OCSP_UNKNOWN_CERT SEC_ERROR_OCSP_MALFORMED_RESPONSE 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: pkix_pl_OcspResponse_Decode pkix_pl_OcspResponse_GetStatus pkix_pl_OcspResponse_VerifySignature pkix_pl_OcspResponse_GetStatusForCert 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 http://code.google.com/p/chromium/issues/detail?id=13336#c19 and http://code.google.com/p/chromium/issues/detail?id=13336#c20). 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: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_revocationchecker.c&rev=1.10&mark=449#420 2. This one is overly strict: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_ocspchecker.c&rev=1.16&mark=190,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: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/ocsp.c&rev=1.59&mark=4626,4647-4648#4626 ocsp_CertHasGoodStatus may return SECFailure with these error codes: SEC_ERROR_INVALID_TIME SEC_ERROR_REVOKED_CERTIFICATE SEC_ERROR_OCSP_UNKNOWN_CERT SEC_ERROR_OCSP_MALFORMED_RESPONSE (should be impossible) 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 argument: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/ocsp.c&rev=1.59&mark=4626,4640,4647,4662#4626 3. In pkix_pl_Pk11CertStore_CheckRevByCrl, if the cert_CheckCertRevocationStatus call fails, we set pkixRevStatus to PKIX_RevStatus_Revoked: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_pk11certstore.c&rev=1.17&mark=543,548#539 This is overly strict because cert_CheckCertRevocationStatus may fail with these error codes: SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE SEC_ERROR_REVOKED_CERTIFICATE SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE should not cause us 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 certRevocationStatusRevoked - return SECFailure and set error code to SEC_ERROR_REVOKED_CERTIFICATE 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 revoked: http://crbug.com/22796 In this case, it is the SEC_ERROR_OCSP_FUTURE_RESPONSE or SEC_ERROR_OCSP_OLD_RESPONSE error that's mapped to the revoked status.
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 and SEC_ERROR_OCSP_UNKNOWN_CERT. https://learningnetwork.cisco.com is an example of SEC_ERROR_OCSP_UNKNOWN_CERT. (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.
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 great! Note: for SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE, I found 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. Take SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE as an example. If NSS (lib/certhigh/ocsp.c) receives an unauthorized OCSP response, it currently sets the SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE error without 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.
Also implement the SEC_ERROR_OCSP_FUTURE_RESPONSE row from comment 8 as Nelson suggested in comment 14.
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 done 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 done
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.
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.
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 done 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 done
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 SEC_ERROR_REVOKED_CERTIFICATE to PKIX_RevStatus_Revoked and 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)
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 done
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 work.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.