NSSCertDBTrustDomain::CheckRevocation returns success when given an expired stapled response and the OCSP responder fails

RESOLVED FIXED in Firefox 31

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31+ fixed, firefox32 fixed)

Details

Attachments

(1 attachment)

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)
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.
Created attachment 8433698 [details] [diff] [review]
patch

It doesn't look like NSS does this, but I think we should.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8433698 - Flags: review?(brian)
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.
(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 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+
(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.
Thanks, Brian.
https://tbpl.mozilla.org/?tree=Try&rev=e747552edd65
https://hg.mozilla.org/integration/mozilla-inbound/rev/3697556d43f7
Seems like the consensus is this isn't really security-sensitive.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/3697556d43f7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Assignee)

Updated

3 years ago
Blocks: 997509
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

3 years ago
status-firefox31: --- → affected
status-firefox32: --- → fixed
tracking-firefox31: --- → ?
tracking-firefox31: ? → +
Attachment #8433698 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/56ef7a74c77d
status-firefox31: affected → fixed
You need to log in before you can comment on or make changes to this bug.