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

RESOLVED FIXED in 3.6.1

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: kaie, Assigned: Julien Pierre)

Tracking

({regression})

3.6.1
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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.
(Reporter)

Updated

15 years ago
Blocks: 171219
Keywords: regression

Comment 1

15 years ago
Bob, could you take a look at this bug?
Assignee: wtc → relyea
Marking P1.  I think all regressions are P1, no?
Priority: -- → P1
Target Milestone: --- → 3.6.1

Updated

15 years ago
Version: unspecified → 3.6

Updated

15 years ago
Keywords: nsbeta1+

Comment 3

15 years ago
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.
Assignee: relyea → jpierre

Comment 4

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

Updated

15 years ago
Blocks: 175225

Comment 5

15 years ago
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.
(Assignee)

Comment 6

15 years ago
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.
(Assignee)

Comment 7

15 years ago
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.
(Reporter)

Comment 8

15 years ago
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?
Keywords: mozilla1.2
(Assignee)

Comment 9

15 years ago
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.
(Assignee)

Comment 10

15 years ago
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/
Attachment #104227 - Attachment is obsolete: true

Comment 11

15 years ago
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.
(Assignee)

Comment 12

15 years ago
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

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

r=relyea
Attachment #104600 - Flags: review+

Comment 14

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

Comment 15

15 years ago
looks good to me.

r=mcgreer
Let's make it unanimous.  r=nelsonb.
(Assignee)

Comment 17

15 years ago
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
(Assignee)

Comment 18

15 years ago
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Reporter)

Comment 19

15 years ago
I tested the patch and confirm it fixes bug 171219.
I will ask drivers for Mozilla 1.2 / client tag checkin approval.
(Reporter)

Updated

15 years ago
Blocks: 174647

Comment 20

15 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 21

15 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Blocks: 178684

Updated

15 years ago
No longer blocks: 174647

Comment 22

15 years ago
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)
Attachment #104600 - Flags: approval+
(Reporter)

Comment 23

15 years ago
Patch was checked in to the 1.2 branch.
(Reporter)

Updated

15 years ago
No longer blocks: 175225
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.
You need to log in before you can comment on or make changes to this bug.