Closed Bug 562542 Opened 11 years ago Closed 11 years ago

An invalid CRL should not cause all certificates issued by that CA to be considered revoked.


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: wtc, Assigned: wtc)



(1 file)

Attached patch Proposed patchSplinter Review
Right now, an invalid CRL (such as a CRL with an invalid signature) causes
all certificates issued by that CA to be considered revoked.

I proposed that we should report "unknown status" instead and let the caller
decide whether the certificate should be considered revoked or good based on
its policy.

We may be able to do better by honoring an old but valid CRL in the cache for
revoking certificates only, but that would require a more thorough
understanding of the CRL cache code.  In fact, thinking about this more, I am
afraid that my patch would allow an attacker to send us an invalid CRL, turning
a revoked certificate into a certificate with unknown status.  So perhaps my
patch still needs work.

Alexei, Bob, Nelson, please let me know if my description of this potential
problem with my patch isn't clear.
Attachment #442289 - Flags: superreview?(rrelyea)
Attachment #442289 - Flags: review?(nelson)
Even though you didn't ask my opinion, I'm not sure if this is such a good idea. Specially, based on what should a user decide? Or do you expect this to be handled at the application level (e.g. the browser / PSM)?
I agree with this bug's summary.
We only store one CRL per issuer.
If we get an invalid CRL, we should not store it.  
We should continue to rely on the previous (last known good) CRL until we 
get a newer good one, but not allow the invalid one to constitute a denial 
of service attack on us.
Comment on attachment 442289 [details] [diff] [review]
Proposed patch

I can't say that I've reviewed this so thoroughly that I'm sure it won't have unintended consequences, but it seems like the right thing to do.
Attachment #442289 - Flags: review?(nelson) → review+
Comment on attachment 442289 [details] [diff] [review]
Proposed patch

r+ rrelyea

I'm pretty convinced this is the right change.

Attachment #442289 - Flags: superreview?(rrelyea) → superreview+
I think we should implement what Nelson described in comment 2.
My patch doesn't go that far.

My patch replaces a denial of service attack with a revocation-suppression
attack, assuming most applications treat the "unknown" status as "good".

Unfortunately I don't have the stamina to live and breathe our CRL code
for two or three days to fully understand it.  Until someone does that, I'm
afraid that we need to advise NSS-based apps to prefer OCSP over CRLs.

I checked in my patch on the NSS trunk for NSS 3.12.7.

Checking in certi.h;
/cvsroot/mozilla/security/nss/lib/certdb/certi.h,v  <--  certi.h
new revision: 1.33; previous revision: 1.32
Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.70; previous revision: 1.69
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.