Add functions for DER parsing a BIT STRING.

Add functions for DER parsing a BIT STRING.

BUG=410574
Committed: https://crrev.com/5915fe2e06f1df371507836c28873707c77787c1
Cr-Commit-Position: refs/heads/master@{#340594}

[eroman, 2015-07-21 22:38:38]

eroman@chromium.org changed reviewers:
+ nharper@chromium.org, rsleevi@chromium.org

[eroman, 2015-07-21 22:38:38]

@nharper for the primary review

@rsleevi for eventual approval (as Nick is not a full committer yet). Feel free
to disregard this review until Nick signs off.

[nharper, 2015-07-21 23:23:47]

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc
File net/der/parse_values.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new...
net/der/parse_values.cc:173: // It must be in the range 0 - 7 (or else the
encoding is not minimal).
cite spec (this is a requirement for BER, not just DER)

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new...
net/der/parse_values.cc:184: // Ensure that unused bits in the last byte are set
to 0.
cite spec (this is only a requirement for CER and DER)

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h
File net/der/parse_values.h (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h#newc...
net/der/parse_values.h:44: uint8_t* unused_bits) WARN_UNUSED_RESULT;
Should unused_bits be a size_t* instead? I realize that it's on the wire as a
uint8_t, but a size_t makes more sense semantically. (This would apply to the
methods in parser.h as well.)

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte...
File net/der/parse_values_unittest.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte...
net/der/parse_values_unittest.cc:199: TEST(ParseValuesTest,
ParseBitString_Empty_NoUnusedBits) {
Test names shouldn't contain underscores (according to
https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...,
although I noticed that there are several tests in //net that has underscores in
their names.)

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte...
net/der/parse_values_unittest.cc:216: ASSERT_FALSE(ParseBitString(Input(kData),
&bytes, &unused_bits));
I generally prefer the EXPECT_* macros to ASSERT_* where feasible to allow the
test to continue running. I guess it doesn't really matter when it's the last
line of a test case though.

https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc
File net/der/parser.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc#newcode200
net/der/parser.cc:200: return unused_bits == 0;
This approach results in modifying *bytes even if the method returns false. I
believe that the rest of the Parser methods only modify output parameters if
they return true. (ParseBitString does this correctly by declaring Input bytes
(parse_values.cc:180) and only assigning it to the output parameter if the
function is going to return true (parse_values.cc:195).)

https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc
File net/der/parser_unittest.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#...
net/der/parser_unittest.cc:206: ASSERT_FALSE(parser.HasMore());
EXPECT_FALSE

https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#...
net/der/parser_unittest.cc:211: ASSERT_EQ(0xBE, bytes.UnsafeData()[1]);
EXPECT_EQ (for lines 209-211)
Only parser.ReadBitString and bytes.Length need to ASSERT.

[eroman, 2015-07-22 17:05:24]

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc
File net/der/parse_values.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new...
net/der/parse_values.cc:173: // It must be in the range 0 - 7 (or else the
encoding is not minimal).
On 2015/07/21 23:23:47, nharper wrote:
> cite spec (this is a requirement for BER, not just DER)

Done. I changed the comment to:

  // From ITU-T X.690, section 8.6.2.2 (applies to BER, CER, DER):
  //  
  // The initial octet shall encode, as an unsigned binary integer with
  // bit 1 as the least significant bit, the number of unused bits in the final
  // subsequent octet. The number shall be in the range zero to seven.

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new...
net/der/parse_values.cc:184: // Ensure that unused bits in the last byte are set
to 0.
On 2015/07/21 23:23:47, nharper wrote:
> cite spec (this is only a requirement for CER and DER)

Done. I added two citations for this section:

    // From ITU-T X.690, section 11.2.1 (applies to CER and DER, but not BER):
    //
    // Each unused bit in the final octet of the encoding of a bit string value
    // shall be set to zero.

and

    // From ITU-T X.690, section 8.6.2.3 (applies to BER, CER, DER):
    //  
    // If the bitstring is empty, there shall be no subsequent octets,
    // and the initial octet shall be zero.

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h
File net/der/parse_values.h (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h#newc...
net/der/parse_values.h:44: uint8_t* unused_bits) WARN_UNUSED_RESULT;
On 2015/07/21 23:23:47, nharper wrote:
> Should unused_bits be a size_t* instead? I realize that it's on the wire as a
> uint8_t, but a size_t makes more sense semantically. (This would apply to the
> methods in parser.h as well.)

I would argue for a uint8_t on the grounds that it is sufficient to represent
the needed range.

A size_t in my mind implies a length/offset in memory, which is not the case
here. It may also be overkill to use a (potentially) 64-bit number to represent
a total of 8 total values :)

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte...
File net/der/parse_values_unittest.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte...
net/der/parse_values_unittest.cc:199: TEST(ParseValuesTest,
ParseBitString_Empty_NoUnusedBits) {
On 2015/07/21 23:23:47, nharper wrote:
> Test names shouldn't contain underscores (according to
>
https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...,
> although I noticed that there are several tests in //net that has underscores
in
> their names.)

Ah, thanks for pointing this out! (I was not aware of that)

Removed all the underscores.

https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte...
net/der/parse_values_unittest.cc:216: ASSERT_FALSE(ParseBitString(Input(kData),
&bytes, &unused_bits));
On 2015/07/21 23:23:47, nharper wrote:
> I generally prefer the EXPECT_* macros to ASSERT_* where feasible to allow the
> test to continue running. I guess it doesn't really matter when it's the last
> line of a test case though.

Done.

These are the result of copy-paste from success test cases.
(For which if ParseBitString were to fail, I don't want to proceed as the unused
bit length could be invalid memory).

https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc
File net/der/parser.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc#newcode200
net/der/parser.cc:200: return unused_bits == 0;
On 2015/07/21 23:23:47, nharper wrote:
> This approach results in modifying *bytes even if the method returns false. I
> believe that the rest of the Parser methods only modify output parameters if
> they return true. (ParseBitString does this correctly by declaring Input bytes
> (parse_values.cc:180) and only assigning it to the output parameter if the
> function is going to return true (parse_values.cc:195).)

Done.

https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc
File net/der/parser_unittest.cc (right):

https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#...
net/der/parser_unittest.cc:206: ASSERT_FALSE(parser.HasMore());
On 2015/07/21 23:23:47, nharper wrote:
> EXPECT_FALSE

Done.

https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#...
net/der/parser_unittest.cc:211: ASSERT_EQ(0xBE, bytes.UnsafeData()[1]);
On 2015/07/21 23:23:47, nharper wrote:
> EXPECT_EQ (for lines 209-211)
> Only parser.ReadBitString and bytes.Length need to ASSERT.

Done.

[nharper, 2015-07-22 18:38:03]

I'll defer to sleevi for size_t/uint8_t in ParseBitString; otherwise LGTM.

[eroman, 2015-07-27 17:39:00]

eroman@chromium.org changed reviewers:
+ davidben@chromium.org
- rsleevi@chromium.org

[eroman, 2015-07-27 17:39:00]

Tentatively changing reviewer from rsleevi --> davidben.

Because I know David is bored right now and needs more code reviews to keep him
busy.

[Ryan Sleevi, 2015-07-27 20:19:43]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-07-27 20:19:44]

lgtm

[davidben, 2015-07-27 20:19:58]

davidben@chromium.org changed reviewers:
- rsleevi@chromium.org

[davidben, 2015-07-27 20:19:59]

lgtm

https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h
File net/der/parse_values.h (right):

https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h#...
net/der/parse_values.h:34: // octet string into |bytes|, and the number of
unused bits into |unused_bits|.
Probably should do: octet string -> octet string (with bits within each octet
ordered from most to least significant, as in the DER encoding)

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h
File net/der/parser.h (right):

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h#newcod...
net/der/parser.h:155: // bits (in the range 0-7) that are unused.
Probably add: The bits are ordered within each octet of |bytes| from most to
least significant, as in the DER encoding.

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest.cc
File net/der/parser_unittest.cc (right):

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest...
net/der/parser_unittest.cc:201: Parser parser((Input(der)));
[I'm assuming the extra parens here are coming from the Most Vexing Parse.]

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest...
net/der/parser_unittest.cc:229:
EXPECT_FALSE(parser.ReadBitStringNoUnusedBits(&bytes));
I dunno if you want a success test for NoUnusedBits.

[eroman, 2015-07-27 21:19:23]

https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h
File net/der/parse_values.h (right):

https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h#...
net/der/parse_values.h:34: // octet string into |bytes|, and the number of
unused bits into |unused_bits|.
On 2015/07/27 20:19:59, David Benjamin wrote:
> Probably should do: octet string -> octet string (with bits within each octet
> ordered from most to least significant, as in the DER encoding)

Done (although used the other version of your comment, so it matches parser.h).

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h
File net/der/parser.h (right):

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h#newcod...
net/der/parser.h:155: // bits (in the range 0-7) that are unused.
On 2015/07/27 20:19:59, David Benjamin wrote:
> Probably add: The bits are ordered within each octet of |bytes| from most to
> least significant, as in the DER encoding.

Done.

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest.cc
File net/der/parser_unittest.cc (right):

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest...
net/der/parser_unittest.cc:201: Parser parser((Input(der)));
On 2015/07/27 20:19:59, David Benjamin wrote:
> [I'm assuming the extra parens here are coming from the Most Vexing Parse.]

Correct.
I need those parenthesis to avoid a compile problem :(

/me shakes fist at C++ and its function ptr declarations...

https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest...
net/der/parser_unittest.cc:229:
EXPECT_FALSE(parser.ReadBitStringNoUnusedBits(&bytes));
On 2015/07/27 20:19:59, David Benjamin wrote:
> I dunno if you want a success test for NoUnusedBits.

Done.

I also improved the test descriptions.

[eroman, 2015-07-27 21:21:20]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-07-27 21:21:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
nharper@chromium.org, davidben@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1248043002/#ps40001
(title: "Address David's comment")

[commit-bot: I haz the power, 2015-07-27 21:22:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1248043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1248043002/40001

[commit-bot: I haz the power, 2015-07-27 23:05:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-27 23:07:08]

Patchset 3 (id:??) landed as
https://crrev.com/5915fe2e06f1df371507836c28873707c77787c1
Cr-Commit-Position: refs/heads/master@{#340594}