Closed Bug 987295 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(2 files, 3 obsolete files)

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).
Attached patch fix patch (obsolete) — Splinter Review
The issue is apparent on https://qvsslrca2-v.quovadisglobal.com/.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8395840 - Flags: review?(brian)
(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/
Attached patch test patch (obsolete) — Splinter Review
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.
Attachment #8396588 - Flags: review?(cviecco)
Attachment #8396588 - Flags: feedback?(brian)
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 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
Attachment #8396588 - Flags: review?(cviecco) → review-
(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.
Attached patch test patch v2 (obsolete) — Splinter Review
How's this?
Attachment #8396588 - Attachment is obsolete: true
Attachment #8396588 - Flags: feedback?(brian)
Attachment #8397350 - Flags: review?(cviecco)
Attachment #8397350 - Flags: feedback?(brian)
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.
Attachment #8397350 - Flags: review?(cviecco) → review+
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?)
Flags: needinfo?(brian)
Comment on attachment 8395840 [details] [diff] [review]
fix patch

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

See review comments in comment 5.
Attachment #8395840 - Flags: review?(brian) → review-
(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.
Flags: needinfo?(brian)
Attached patch fix patch v2Splinter Review
Camilo, since we're trying to get this done today, I'd appreciate you making sure I addressed the changes Brian suggested.
Attachment #8395840 - Attachment is obsolete: true
Attachment #8399538 - Flags: review?(cviecco)
Attached patch test patch v2.1Splinter Review
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
Attachment #8397350 - Attachment is obsolete: true
Attachment #8397350 - Flags: feedback?(brian)
Attachment #8399539 - Flags: review+
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)
Attachment #8399538 - Flags: review?(cviecco) → review+
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 |").
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
https://hg.mozilla.org/mozilla-central/rev/b591f6b11bec
https://hg.mozilla.org/mozilla-central/rev/6b570ab31d19
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.