1059924 - Centralize tag and length decoding in mozilla::pkix's DER decoder

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: fundamental-DER-decoder.patch

This patch adds the function ReadTagAndGetLength, which will be needed by the new name processing code (bug 970542 and others).

The patch also centralizes all of the tag and length parsing into ReadTagAndGetLength so that 
there is only place place where it is done. This should make it easier to verify (formally and semi-formally) that the DER decoder is safe and correct.

Comment 1

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

Comment on attachment 8480733 [details] [diff] [review]
fundamental-DER-decoder.patch

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

Looks good with comments addressed.

::: security/pkix/lib/pkixder.cpp
@@ +39,5 @@
>    if (rv != Success) {
>      return rv;
>    }
> +  if ((tag & 0x1F) == 0x1F) {
> +    // TODO: test for this?

Yes, a test would be nice. Should be pretty simple, right?

@@ +81,5 @@
>      // We don't support lengths larger than 2^16 - 1.
>      return Result::ERROR_BAD_DER;
>    }
>  
>    // Ensure the input is long enough for the length it says it has.

This comment is a little out of date, now.

::: security/pkix/lib/pkixder.h
@@ +124,5 @@
>  ExpectTagAndGetTLV(Reader& input, uint8_t tag, /*out*/ Input& tlv)
>  {
>    Reader::Mark mark(input.GetMark());
> +  Input unusedValue;
> +  Result rv = ExpectTagAndGetValue(input, tag, unusedValue);

ExpectTagAndSkipValue?

Comment 2

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

Attachment: Part 1: Refactor the code and add the new function, addressing review comments

Comment 3

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

Attachment: Part 2: Add the tests

The tests weren't as trivial as I initially expected.

Comment 4

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

I pushed part 1 without part 2, which I probably shouldn't have, but I did:

https://hg.mozilla.org/integration/mozilla-inbound/rev/88c41feb981d

Comment 5

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

Comment on attachment 8483293 [details] [diff] [review]
Part 2: Add the tests

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

Looks good - I just have a question about some of the comments.

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +551,5 @@
> +{
> +  // High tag number form is not allowed (legal 1 byte tag).
> +  //
> +  // Even if we comment out ReadTagAndGetValue's check to prohibit the high tag
> +  // number form, this test will pass, because 31 will be interpreted as a

Was this intended to be "If we comment out..." or "... this test will not pass..." or something else? As it is, I'm not understanding what you're saying.

@@ +573,5 @@
> +{
> +  // High tag number form is not allowed (2 byte legal tag)
> +  //
> +  // Even if we comment out ReadTagAndGetValue's check to prohibit the high tag
> +  // number form, this test will pass, because 31 will be interpreted as a

Same here

Comment 6

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

Comment on attachment 8483293 [details] [diff] [review]
Part 2: Add the tests

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

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +551,5 @@
> +{
> +  // High tag number form is not allowed (legal 1 byte tag).
> +  //
> +  // Even if we comment out ReadTagAndGetValue's check to prohibit the high tag
> +  // number form, this test will pass, because 31 will be interpreted as a

When I originally wrote this test, the test passed right away, as expected. Then, in order to make sure the test was testing the code in question, I commented out (i.e. removed) the check that prohibits the high tag number form in pkixder.cpp. I was surprised that the test still passed, because the second byte of the tag was being interpreted as a length that happened to be correct given the input. Consequently, this test is really about "make sure we don't write code in the future to accept the high tag number form," not "make sure we don't mis-interpret the "high tag number form." I will clarify this before checkin.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88c41feb981d

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f919a6bbfef0

Comment 9

[Carsten Book [:Tomcat]]

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