Closed
Bug 984567
Opened 10 years ago
Closed 10 years ago
insanity::pkix: handle/test receiving malformed OCSP responses
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(1 file)
13.84 KB,
patch
|
briansmith
:
review+
cviecco
:
review+
|
Details | Diff | Splinter Review |
We should gracefully handle the case where an OCSP response has no responseBytes. See also bug 979070.
Assignee | ||
Comment 1•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from bug 979070 comment #8) > ::: security/insanity/lib/pkixocsp.cpp > @@ +395,5 @@ > > + // If responseBytes isn't included (it's optional), this is > > + // an unknown response type. > > + if (der::End(input) == der::Success) { > > + return der::Fail(SEC_ERROR_OCSP_UNKNOWN_RESPONSE_TYPE); > > + } > > We can only get here if responseStatus==0 (Successful), and all successful > responses require responseBytes. Therefore, any > > With your patch for bug 977870, won't the result become > SEC_ERROR_OCSP_MALFORMED_RESPONSE? I think SEC_ERROR_OCSP_MALFORMED_RESPONSE > is the right result, and I also think that we should handle the > SEC_ERROR_BAD_DER case higher up. Sounds good. All we'll need here is the test.
Depends on: 977870
Summary: insanity::pkix: test and handle OCSP responses with no responseBytes → insanity::pkix: test receiving an OCSP response with no responseBytes
Assignee | ||
Comment 2•10 years ago
|
||
Actually, I'll be handling all cases of malformed ocsp responses here. Sorry for the bugspam.
Summary: insanity::pkix: test receiving an OCSP response with no responseBytes → insanity::pkix: handle/test receiving malformed OCSP responses
Assignee | ||
Comment 3•10 years ago
|
||
Brian, if you would have a look at the changes to security/insanity/lib/pkixocsp.cpp, that would be great. I expanded the changes originally made in bug 977870 to change any case where we would return SEC_ERROR_BAD_DER to instead return SEC_ERROR_OCSP_MALFORMED_RESPONSE.
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8392484 [details] [diff] [review] patch Camilo, if you could review the changes to security/insanity/test/... and security/manager/... that would be great.
Attachment #8392484 -
Flags: review?(cviecco)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8392484 [details] [diff] [review] patch Review of attachment 8392484 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/head_psm.js @@ +33,5 @@ > const SEC_ERROR_EXTENSION_NOT_FOUND = SEC_ERROR_BASE + 35; // -8157 > const SEC_ERROR_CA_CERT_INVALID = SEC_ERROR_BASE + 36; > const SEC_ERROR_INADEQUATE_KEY_USAGE = SEC_ERROR_BASE + 90; // -8102 > const SEC_ERROR_CERT_NOT_IN_NAME_SPACE = SEC_ERROR_BASE + 112; // -8080 > +const SEC_ERROR_OCSP_UNKNOWN_RESPONSE_TYPE = SEC_ERROR_BASE + 118; Whoops - don't need this anymore.
Comment 6•10 years ago
|
||
Comment on attachment 8392484 [details] [diff] [review] patch Review of attachment 8392484 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/insanity/lib/pkixocsp.cpp @@ +311,5 @@ > return SECSuccess; > } > > +static inline void > +MaybeIndicateMalformedResponse() rename to : setErrorToMalformedResponseOnBadDer ::: security/manager/ssl/tests/unit/test_ocsp_stapling.js @@ +134,2 @@ > do_check_eq(histogram.counts[3], 2 * 0); // 0 connections with an expired response > + do_check_eq(histogram.counts[4], 2 * 11 + 1); // 11 or 12 connections with bad responses (bug 979070) Is comment OK? I would also add a parenthesis.
Attachment #8392484 -
Flags: review?(cviecco) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8392484 [details] [diff] [review] patch Review of attachment 8392484 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_ocsp_stapling.js @@ +41,5 @@ > add_ocsp_test("ocsp-stapling-good-other.example.com", Cr.NS_OK, false); > add_ocsp_test("ocsp-stapling-none.example.com", Cr.NS_OK, false); > add_ocsp_test("ocsp-stapling-expired.example.com", Cr.NS_OK, false); > add_ocsp_test("ocsp-stapling-expired-fresh-ca.example.com", Cr.NS_OK, false); > + add_ocsp_test("ocsp-stapling-skip-body.example.com", Cr.NS_OK, false); NIT: s/skip-body/skip-responseBytes/ @@ +134,2 @@ > do_check_eq(histogram.counts[3], 2 * 0); // 0 connections with an expired response > + do_check_eq(histogram.counts[4], 2 * 11 + 1); // 11 or 12 connections with bad responses (bug 979070) I would write it 11 + 12. ::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp @@ +43,5 @@ > context.cert = CERT_DupCertificate(aCert); > context.issuerCert = nullptr; > context.signerCert = nullptr; > context.responseStatus = 0; > + context.skipBody = false; I think it would be better to have the constructor of OCSPResponseContext initialize this field, and other fields, to reasonable default values. I am using OCSPResponseContext in my updated patches for bug 916629 and it's likely I would have overlooked the need to initialize skipBody to false there. @@ +97,5 @@ > // context.responseStatus is 0 in all other cases, and it has > // already been initialized, above. > break; > } > + if (aORT == ORTSkipBody) { ORTSkipResponseBytes.
Attachment #8392484 -
Flags: review?(brian) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Thanks, all. https://hg.mozilla.org/integration/mozilla-inbound/rev/a7999e7135d5
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a7999e7135d5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•