CERT_CheckOCSPStatus / ocsp_GetCachedOCSPResponseStatus can return SECFailure without calling PR_SetError

RESOLVED FIXED in 3.15.5

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

If there is a freshly cached, invalid OCSP response, ocsp_GetCachedOCSPResponseStatus will set rvOcsp to SECFailure, but there is never a call to PR_SetError.
To reproduce, set security.OCSP.require to true, visit https://www.certigna.fr/Certigna/PC (there should be an OCSP failure), and refresh the page (you should also see an error like "###!!! ASSERTION: no error set during certificate validation failure: 'Not Reached', file /home/keeler/mozilla-central/security/manager/ssl/src/SSLServerCertVerification.cpp, line 994" in the console for a debug version of Firefox).

Comment 1

4 years ago
Keeler, Thanks for creating this bug. Note that this is a regression.
Created attachment 8366806 [details] [diff] [review]
fix

Calling PORT_SetError with the cached error code should do the trick.
This is a patch against mozilla-central, but it applies to nss with `patch -p3`.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8366806 - Flags: review?(brian)
Created attachment 8366807 [details] [diff] [review]
test

This tests the fix in PSM.
Attachment #8366807 - Flags: review?(brian)
Blocks: 933109
Attachment #8366806 - Flags: review?(brian) → review+
Comment on attachment 8366807 [details] [diff] [review]
test

Review of attachment 8366807 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/test_ocsp_required.js
@@ +11,5 @@
> +
> +function run_test() {
> +  do_get_profile();
> +  Services.prefs.setBoolPref("security.OCSP.require", true);
> +  Services.prefs.setBoolPref("security.ssl.enable_ocsp_stapling", true);

Why do we need to set security.ssl.enable_ocsp_stapling to true for this test?
Attachment #8366807 - Flags: review?(brian) → review+
Created attachment 8368730 [details] [diff] [review]
test v1.1

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #4)
lPref("security.OCSP.require", true);
> > +  Services.prefs.setBoolPref("security.ssl.enable_ocsp_stapling", true);
> 
> Why do we need to set security.ssl.enable_ocsp_stapling to true for this
> test?

No good reason - I fixed it.

My impression from the NSS meeting was that the fix is going into a release soon - do we want to wait for that or have a private patch until then?
Attachment #8366807 - Attachment is obsolete: true
Attachment #8368730 - Flags: review+
NSS trunk (3.16):
http://hg.mozilla.org/projects/nss/rev/5eddfe83330d

NSS 3.15.5:
http://hg.mozilla.org/projects/nss/rev/4e2f6a631150

Leaving open because Gecko trees are closed, so I can't land the Gecko test.
Target Milestone: --- → 3.15.5
Attachment #8366806 - Flags: checked-in+

Comment 7

4 years ago
Comment on attachment 8366806 [details] [diff] [review]
fix

Review of attachment 8366806 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.
Attachment #8366806 - Flags: review+
Comment on attachment 8368730 [details] [diff] [review]
test v1.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/370064a33b61
Attachment #8368730 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/370064a33b61
Flags: in-testsuite+

Updated

4 years ago
Priority: -- → P2
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c7749cb2702
You need to log in before you can comment on or make changes to this bug.