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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 + fixed
firefox32 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

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.
Attached patch patchSplinter Review
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.
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
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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?
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.