987295 - mozilla::pkix: fix and test OCSP response extension decoding

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

In bug 986156, we discovered that mozilla::pkix does not properly decode OCSP response extensions (I think there are multiple bugs behind bug 986156, so I spun this bug off separately).

Comment 1

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

Attachment: fix patch

The issue is apparent on https://qvsslrca2-v.quovadisglobal.com/.

Comment 2

[Kathleen Wilson]

(In reply to David Keeler (:keeler) from comment #1)
> Created attachment 8395840 [details] [diff] [review]
> fix patch
> 
> The issue is apparent on https://qvsslrca2-v.quovadisglobal.com/.

That site works for me (using Firefox Nightly Debug) when I have "security.use_mozillapkix_verification" set to true and OCSP hard-fail on.

However, the following CA test websites give me the sec_error_ocsp_malformed_response error:
https://publicca.hinet.net/
https://ev.omniroot.com
https://certdemo-ev-valid.ssl.d-trust.net/
https://certdemo-ov-valid.ssl.d-trust.net/
https://root-ca2.test.telesec.de
https://assured-id-root.digicert.com/testroot/
https://ev-root.digicert.com/testroot/
https://testssl-valid-r2i1.disig.sk/
https://testssl-valid.disig.sk/index.en.html
https://testssl-valid-r1i1.disig.sk/
https://secure.omniroot.com
https://evsslrca2-v.quovadisglobal.com
https://mail.procert.net.ve/exchange
https://www.elkk.amsterdam.nl/
https://www.digid.nl/
https://certum-ctnca-ev.certum.pl
https://nerys-m1.wellsfargo.com/test.html
https://secure.certifyid.com

Also, I don't know if this is the same bug, but the following VeriSign test sites give me the sec_error_bad_der error:
https://ssltest23.bbtest.net/
https://ssltest24.bbtest.net/
https://ssltest23.bbtest.net/

Comment 3

[Kathleen Wilson]

(In reply to Kathleen Wilson from comment #2)
> (In reply to David Keeler (:keeler) from comment #1)
> > Created attachment 8395840 [details] [diff] [review]
> > fix patch
> > 
> > The issue is apparent on https://qvsslrca2-v.quovadisglobal.com/.
> 
> That site works for me (using Firefox Nightly Debug) when I have
> "security.use_mozillapkix_verification" set to true and OCSP hard-fail on.

I take that back. That site gives me the sec_error_ocsp_malformed_response
I was mistakenly thinking it was this site: https://qvsslrca-v.quovadisglobal.com/

Comment 4

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

Attachment: test patch

Let me know what you think. This isn't a full implementation of all the various tests we could do - it's more of the minimum possible to convince us we've fixed the immediate issue.

Comment 5

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

One concern I have is that the explicit tagging [1], [2], [3], etc. is supposed to enable extensibility. Should an OCSP response that has [2] <Some unknown thing> or [3] <Another unknown thing> be accepted or rejected? My current thinking is that for OCSPv1 it is OK to reject anything after the extensions, so we don't need to worry about it. I think it would also be OK to allow arbitrary things to be after the extensions. (And, since extensions are optional, I really mean "at the end, after the stuff mentioned in the spec".)

However, it is different within the "[1] Extensions" structure. There, you use ExpectTagAndIgnoreLength to consume the tag and length of the "explicit [1]". And, you use NestedOf to consume a sequence of sequences. But, we don't check that there is anything inside the "explicit [1]" after the sequence of sequences. I think you could fix that by calling der::End(input) after calling der::NestedOf. However, then you've just re-implemented der::Nested.

It seems instead of calling CheckExtensionsForCriticality directly, we should call it as:

    der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
                CheckExtensionsForCriticality);

and have the body of CheckExtensionsForCriticality be just:

    return der::NestedOf(input, der::SEQUENCE, der::SEQUENCE,
                         der::MustNotBeEmpty,
                         CheckExtensionForCriticality);

Comment 6

[Camilo Viecco (:cviecco)]

Comment on attachment 8396588 [details] [diff] [review]
test patch

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

Couple of nits.

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +101,5 @@
>    }
> +  OCSPResponseExtension extension;
> +  if (aORT == ORTCriticalExtension || aORT == ORTNoncriticalExtension) {
> +    // Technically this is { id-pkix-ocsp 10 }, but I don't know if that's
> +    // available for use as a test value. The idea is this is an unknown OID.

If any oid is sufficient, try using one from the mozilla space. For testing I have requested the 
1.3.6.1.4.1.13769.666.666.666 space. (I am infact using "1.3.6.1.4.1.13769.666.666.666.1.500.9.1 for testing of x509).

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +405,5 @@
> +//   critical         BOOLEAN DEFAULT FALSE
> +//   value            OCTET STRING
> +// }
> +SECItem*
> +Extension(OCSPResponseContext& context, OCSPResponseExtension* extension)

Please rename to OCSPExtension or ResponseExtension

Comment 7

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #5)
> It seems instead of calling CheckExtensionsForCriticality directly, we
> should call it as:
> 
>     der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
>                 CheckExtensionsForCriticality);
> 
> and have the body of CheckExtensionsForCriticality be just:
> 
>     return der::NestedOf(input, der::SEQUENCE, der::SEQUENCE,
>                          der::MustNotBeEmpty,
>                          CheckExtensionForCriticality);

Sounds good to me.

Comment 8

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

Attachment: test patch v2

How's this?

Comment 9

[Camilo Viecco (:cviecco)]

Comment on attachment 8397350 [details] [diff] [review]
test patch v2

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

Just a couple of nits that I had previously missed.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +405,5 @@
> +//   critical         BOOLEAN DEFAULT FALSE
> +//   value            OCTET STRING
> +// }
> +SECItem*
> +OCSPExtension(OCSPResponseContext& context, OCSPResponseExtension* extension)

nit that I missed, this can be made static

@@ +438,5 @@
> +//   SEQUENCE OF Extension
> +// }
> +SECItem*
> +Extensions(OCSPResponseContext& context)
> +{

missed nit in previous review, make it static.

Comment 10

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

Brian, do you have further concerns beyond what you've mentioned in comment 5? (As in, if I implement what you suggest, do you think that's sufficient?)

Comment 11

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

Comment on attachment 8395840 [details] [diff] [review]
fix patch

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

See review comments in comment 5.

Comment 12

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

(In reply to David Keeler (:keeler) from comment #10)
> Brian, do you have further concerns beyond what you've mentioned in comment
> 5? (As in, if I implement what you suggest, do you think that's sufficient?)

It looks good to me besides what I mentioned in comment 5.

Comment 13

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

Attachment: fix patch v2

Camilo, since we're trying to get this done today, I'd appreciate you making sure I addressed the changes Brian suggested.

Comment 14

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

Attachment: test patch v2.1

Addressed comments, carrying over r+.
Here's how try went:
https://tbpl.mozilla.org/?tree=Try&rev=d224a23e1c2c
https://tbpl.mozilla.org/?tree=Try&rev=6ab5ae5374bb

Comment 15

[Camilo Viecco (:cviecco)]

Comment on attachment 8399538 [details] [diff] [review]
fix patch v2

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

r+ with comments addressed.

::: security/pkix/lib/pkixocsp.cpp
@@ +586,5 @@
>    }
>  
>    if (!input.AtEnd()) {
> +    if (der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
> +                    CheckExtensionsForCriticality) != der::Success) {

Nit: add a comment explaining why we are doing this. (same on the other changed line)

Comment 16

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

Well, we're doing this because it didn't follow the specified format before, and now it does. The applicable documentation is on line 531 ("//    responseExtensions  [1] EXPLICIT Extensions OPTIONAL }") and line 603 ("//    singleExtensions     [1]     EXPLICIT Extensions{{re-ocsp-crl |").

Comment 17

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

13:18      keeler | also, for bug 987295, the issue is that the implementation didn't match the specification - do you think we need to specifically document
                  | that?
13:18     cviecco | yes.
13:18     cviecco | If possible add a bug number on when we are going to remove this exception.
13:19      keeler | oh, sorry - I wasn't being clear. The change fixes an actual bug in our implementation, so now it matches the spec. The spec was already
                  | documented in the code
13:22      keeler | cviecco: ^
13:22     cviecco | I am fine with that
13:22     cviecco | I tought you were talking about the BC issue
13:22      keeler | ok

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b591f6b11bec
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b570ab31d19

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b591f6b11bec
https://hg.mozilla.org/mozilla-central/rev/6b570ab31d19