175115 - Setting member CERTCertificate.timeOK does no longer cause cert to verify

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert [:KaiE:]]

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.

Comment 1

[Wan-Teh Chang]

Bob, could you take a look at this bug?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Marking P1.  I think all regressions are P1, no?

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Wan-Teh Chang]

Can we fix this bug by passing PR_TRUE as the
allowOverride argument to CERT_CheckCertValidTimes?

Comment 5

[Wan-Teh Chang]

Attachment: 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.

Comment 6

[Julien Pierre]

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.

Comment 7

[Julien Pierre]

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.

Comment 8

[Kai Engert [:KaiE:]]

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?

Comment 9

[Julien Pierre]

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.

Comment 10

[Julien Pierre]

Attachment: 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 11

[Wan-Teh Chang]

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.

Comment 12

[Julien Pierre]

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 13

[Robert Relyea]

Comment on attachment 104600 [details] [diff] [review]
fix problems in CRL cache

r=relyea

Comment 14

[Wan-Teh Chang]

Julien, please ask Nelson or Ian to review your patch too.
Then, check it into the trunk of NSS for some testing.

Comment 15

[Ian McGreer]

looks good to me.

r=mcgreer

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Let's make it unanimous.  r=nelsonb.

Comment 17

[Julien Pierre]

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

Comment 18

[Julien Pierre]

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: 1.26.2.1; previous revision: 1.26
done

Comment 19

[Kai Engert [:KaiE:]]

I tested the patch and confirm it fixes bug 171219.
I will ask drivers for Mozilla 1.2 / client tag checkin approval.

Comment 20

[Arthur]

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.

Comment 21

[Kai Engert [:KaiE:]]

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 22

[Asa Dotzler [:asa]]

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)

Comment 23

[Kai Engert [:KaiE:]]

Patch was checked in to the 1.2 branch.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

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.