See bug 171219. Actual behaviour: PSM sets member timeOK, but certificate does not verify with an "expired" error. Expected behaviour: Cert should verify. This is a regressions.
Bob, could you take a look at this bug?
Marking P1. I think all regressions are P1, no?
Bob found that the bug is caused by the CERT_CheckCertValidTimes(issuer, t, PR_FALSE) call in the CERT_CheckCRL function in lib/certdb/crl.c. Reassigned the bug to Julien.
Can we fix this bug by passing PR_TRUE as the allowOverride argument to CERT_CheckCertValidTimes?
Created attachment 104227 [details] [diff] [review] Proposed patch: pass PR_TRUE to CERT_CheckCertValidTimes This fixes the problem and does not affect certs whose timeOK field is PR_FALSE, which is the common case. By the way, removing the CERT_CheckCertValidTimes call also fixes the problem.
Wan-Teh, This fix is dangerous for the CRL cache. I had included another patch earlier in the week. Perhaps I attached it to the wrong bug.
I see several problems here. Main problem : The main cause of this bug is that I was checking for the CA certificate's validity in the CRL check. The logic was that if the CA expired, then any CRL found would not verify, so there was no point in looking one up. In fact, since the CRL check was only performed in the context of a certificate verification, I just returned a failure. However, I wasn't aware of the possibility of overriding the certificate expiration through timeOK, so I didn't make a provision for it. Second problem : Bob pointed out that checking for the expiration of the CA was incorrect in the first place. The correct check is to verify if the CRL was issued during the validity period of the CA certificate. I added this erroneous check because the CRL was being verified with a call to CERT_VerifySignedData using the time "t" passed in from the certificate verification function, rather than the issuing time of the CRL. This was a pre-existing problem in 3.5 and before. So the incorrect check I added matched the incorrect way the CRL was being verified before. Since we need to look up the CRL in order to verify its time against the CA cert, this requires a CRL lookup. Therefore, the short-circuit check I have at the beginning of CERT_CheckCRL must be deleted altogether. The signature verification code later in the CRL cache also needs to be modified to pass the correct time during signature verification, not the time t being passed on the stack from the certificate function verification. However, this opens a whole new can of worms : The reason for the "short-circuit" is that I made the assumption that, given a valid CA (ie. CRL issuer) certificate, I would always be able to verify a CRL I had fetched, and from that result decide whether it was a good or bad CRL. For the purpose of being able to verify CRLs, the CRL cache stores a copy of the issuer certificate. It uses a copy of the first certificate available when a cache is created for the issuer during the first certificate verification on that issuer. Once fetched, that certificate never changes in the cache. It now occurs to me that the CA certificate itself can change over time. The CA can request a new certificate from its root, possibly using not just a new validity date but also a new key. This may happen with or without the old CA cert being revoked. So not only can the issuer cert change, but for a given time t there may be more than one valid CA certificate for an issuer. This means the cache needs to be able to store more than one CA certificate per issuer. An array of CA certs should be used. The biggest problem is to add the logic to avoid re-verification of CRLs. I went to great pains while writing the cache to make sure that the decoding and verification, successful or failed, would only occur once. While caching the decoded version is OK and already implemented, some cases of failed verifications are tricky. It seems to me that if two completely different CA certs with the same subject can be valid at any time, then we can never truly consider any CRL invalid. We can merely delay the verification until a matching CA cert becomes available to verify that CRL. This means a CA cert that has a validity date encompassing the CRL issuing date, and a key allowing the signature to verify. Dealing will this problem would require much new code and new states to be added to the CRL objects and the CRL cache.
Julien, your comment sounds like a lot of work. Since this is an annyoing regression and the release of Mozilla 1.2 is close (in a couple of days), are you able to produce a fix that reverts the code back to the previous behaviour?
To revert the behavior to be exactly like the previous one, I would need to take out the entire CRL cache. I am working on a smaller patch that will solve the problem, but I won't be updating the issuer cert in the cache, or maintaining a list of issuer certs. I'll just do the signature verification with the correct check and without the short-circuiting, and that should solve it. I'm about to test this fix now.
Created attachment 104600 [details] [diff] [review] fix problems in CRL cache 1) Verify CRL signature against last update time, not the time passed to the certificate verification function 2) remove erroneous check for expired CA 3) remove time parameter from many functions where it wasn't needed I have verified that this solves the PSM problem of not being able to visit to https://webmail.marcanoonline.com/
Comment on attachment 104600 [details] [diff] [review] fix problems in CRL cache I verified that this patch does what you described. (The patch is simpler than it looks. Reviewers might want to apply the patch and do a cvs diff -uw to get rid of the diffs from indentation changes.) Someone else should review it for the correctness of the fix.
Wan-Teh, This patch is simpler than the one I had coded last week - it is totally different. The old patch is in the PSM bug (and marked obsolete).
Comment on attachment 104600 [details] [diff] [review] fix problems in CRL cache r=relyea
Julien, please ask Nelson or Ian to review your patch too. Then, check it into the trunk of NSS for some testing.
looks good to me. r=mcgreer
Let's make it unanimous. r=nelsonb.
I have checked this into the branch. I will monitor tinderbox and if all goes well it will also go to NSS_3_6_BRANCH later today. Checking in crl.c; /cvsroot/mozilla/security/nss/lib/certdb/crl.c,v <-- crl.c new revision: 1.28; previous revision: 1.27 done
Everything was fine on tinderbox. Checking to NSS_3_6_BRANCH and marking fixed. Checking in crl.c; /cvsroot/mozilla/security/nss/lib/certdb/crl.c,v <-- crl.c new revision: 188.8.131.52; previous revision: 1.26 done
I tested the patch and confirm it fixes bug 171219. I will ask drivers for Mozilla 1.2 / client tag checkin approval.
Reopening because the fix isn't checked in the mozilla trunk. Please don't mark bugs fixed if they're not fixed on the trunk.
Arthur, you do not understand how things are working here. Mozilla and NSS are separate products. Mozilla uses a snapshot of the external product NSS. In the trunk of NSS, this bug is fixed. Marking fixed again.
Comment on attachment 104600 [details] [diff] [review] fix problems in CRL cache a=asa for checkin to the 1.2 branch (on behalf of drivers)
Patch was checked in to the 1.2 branch.
Julien, A new comment in bug 171219 claims that this problem has regressed, and is now reproducible again. Revision 1.28 to crl.c removed the pre-emptive check of the issuer's cert validity date from CERT_CheckCRL. About a month later, John Unruh Verified that the previously-reproducible test case for this bug could no longer be reproduced. But the preemptive test of the issuer's cert validity date was put back into CERT_CheckCRL in revision 1.36 in August 2003 (9 months later). That checkin cites bug 216701. I suspect (er, guess) that putting that test back reintroduced the problem reported in this bug and in bug 171219. Please confirm or refute that suspicion. If revision 1.36 did reintroduce that problem, we should file a new regression bug rather than reopening these old bugs, because the old problem apparently really was fixed at one time.