Closed
Bug 1019198
Opened 10 years ago
Closed 10 years ago
NSSCertDBTrustDomain::CheckRevocation returns success when given an expired stapled response and the OCSP responder fails
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(1 file)
5.08 KB,
patch
|
briansmith
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
STR: 1. Server staples an expired OCSP response 2. CheckRevocation attempts to fetch a newer response 3. Fetch fails 4. CheckRevocation returns success (because soft-fail is on by default, but more importantly because the expired stapled response is not taken into account)
Assignee | ||
Comment 1•10 years ago
|
||
Hmmm - maybe this isn't as bad as I thought. I came across this in the context of bug 997509 (heeding Unknown or Revoked responses even if they're expired in some situations). I think the behavior I was looking for was that if a stapled response were expired but indicated Unknown/Revoked, if fetching a newer response failed, the handshake should fail. I thought the presence of an expired stapled response alone would already make the handshake fail if a new response could not be fetched, but this seems to not be the case.
Assignee | ||
Comment 2•10 years ago
|
||
It doesn't look like NSS does this, but I think we should.
Comment 3•10 years ago
|
||
You're saying a stale stapled OCSP is worse than no response at all (if soft-fail is on)? If it's stale we shouldn't accept it when hard-fail is on, but if it's a "good" response we can just ignore it for soft-fail, right? Of course if it's stale "don't use this cert" value we may want to believe it anyway.
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #3) > You're saying a stale stapled OCSP is worse than no response at all (if > soft-fail is on)? That was my intuition. I could be convinced otherwise. > If it's stale we shouldn't accept it when hard-fail is on, Right - I'm pretty sure we do this correctly right now. > but if it's a "good" response we can just ignore it for soft-fail, right? I guess? > Of > course if it's stale "don't use this cert" value we may want to believe it > anyway. Right - that's bug 997509.
Comment 5•10 years ago
|
||
Comment on attachment 8433698 [details] [diff] [review] patch Review of attachment 8433698 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +385,1 @@ > Don't we need to do the same thing or similar here (at the end of the function)?: PRErrorCode error = PR_GetError(); if (error == SEC_ERROR_OCSP_UNKNOWN_CERT || error == SEC_ERROR_REVOKED_CERTIFICATE) { return rv; } PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("NSSCertDBTrustDomain: end of CheckRevocation")); return SECSuccess; Also, the "return SECSuccess;" at the end of the function should have the "// Soft fail -> success :(" comment too.
Attachment #8433698 -
Flags: review?(brian) → review+
Comment 6•10 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #3) > You're saying a stale stapled OCSP is worse than no response at all (if > soft-fail is on)? As part of preparing for implementing Must-Staple, we decided to be strict regarding the validation of stapled OCSP responses, to see if Must-Staple is even feasible given current web server implementations, and to encourage more careful implementations of OCSP stapling in servers. Thus, we are conservative about what types of errors we soft fail on for stapling, whereas we are (too) liberal about what types of errors we soft fail on for fetching.
Assignee | ||
Comment 7•10 years ago
|
||
Thanks, Brian. https://tbpl.mozilla.org/?tree=Try&rev=e747552edd65
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3697556d43f7
Assignee | ||
Comment 9•10 years ago
|
||
Seems like the consensus is this isn't really security-sensitive.
Group: core-security
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3697556d43f7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8433698 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): mozilla::pkix User impact if declined: this is necessary for bug 997509 Testing completed (on m-c, etc.): landed in 32 (now aurora) Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #8433698 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8433698 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in
before you can comment on or make changes to this bug.
Description
•