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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) [:keeler]]

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

See also bug 979070.

Comment 1

[Dana Keeler (she/her) [:keeler]]

(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.

Comment 2

[Dana Keeler (she/her) [:keeler]]

Actually, I'll be handling all cases of malformed ocsp responses here. Sorry for the bugspam.

Comment 3

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

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.

Comment 4

[Dana Keeler (she/her) [:keeler]]

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.

Comment 5

[Dana Keeler (she/her) [:keeler]]

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

[Camilo Viecco (:cviecco)]

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.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 8

[Dana Keeler (she/her) [:keeler]]

Thanks, all.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7999e7135d5

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7999e7135d5