Closed Bug 984567 Opened 10 years ago Closed 10 years ago

insanity::pkix: handle/test receiving malformed OCSP responses

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

We should gracefully handle the case where an OCSP response has no responseBytes.

See also bug 979070.
(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
No longer depends on: 977870
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
Attached patch patchSplinter Review
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: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8392484 - Flags: review?(brian)
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)
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 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 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+
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.