Closed Bug 968490 Opened 10 years ago Closed 10 years ago

Add unit tests for mozilla::pkix DER decoder (pkixder.h/pkixder.c)

Categories

(Core :: Security: PSM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: briansmith, Assigned: mmc)

References

Details

Attachments

(1 file, 7 obsolete files)

      No description provided.
Assignee: nobody → dkeeler
Priority: -- → P3
Hm this test does not work because:

 0:03.18 duplicate symbol __ZN8insanity2_1E in:
 0:03.18 ../../security/manager/ssl/tests/gtest/pkixder_tests.o
 0:03.18 ../../security/certverifier/Unified_cpp_security_certverifier0.o

I am not completely sure how to fix this. This is possibly a bug in insanity/include/insanity/bind.h which is a replacement for <functional> if that is not available. It seems that include exports _1.

This may not have popped up before since the placeholders are only being used in one other place.

I'll try to find a fix or workaround.
See bug 982761 for a fix for the duplicate symbol issue.
Assignee: dkeeler → sarentz
Comment on attachment 8389918 [details] [diff] [review]
968490-add_unit_tests_for_insanity_der_decoder.patch

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

Note there are quite a few lines with trailing whitespace too.

Stefan, could you please update this patch based on my feedback in this bug, the patch I am about to attach, and the other bugs you filed blocking this one, and re-submit it for review?

::: security/manager/ssl/tests/gtest/moz.build
@@ +8,5 @@
>  
>  LIBXUL_LIBRARY = True
>  
>  SOURCES += [
> +	'pkixder_tests.cpp',

I have written a patch on top of this patch to move this to security/insanity/test/gtest, which is a better place for it. I will attach that patch to this bug so you can fold it into your patch. Please do so.

::: security/manager/ssl/tests/gtest/pkixder_tests.cpp
@@ +202,5 @@
> +  0x09,
> +  0x2B, 0x06, 0x01, 0x05, 0x05, 0x07, 0x30, 0x01, 0x01
> +};
> +
> +const uint8_t DER_ALGORITHM_IDENTIFIER_NO_PARAMS[] = {

For these values which are only used in one test, please move then into the TEST_F that they are used in. That makes it much easier to review and debug tests!

@@ +271,5 @@
> +  
> +TEST_F(pkixder_tests, FailWithError)
> +{
> +  Result rv = Fail(SEC_ERROR_BAD_DER);
> +  ASSERT_EQ(Failure, rv);

I suggest that, especially for the functions that return Failure/Success, that we avoid the intermediate variables, like so:

ASSERT_EQ(Failure, Fail(SEC_ERROR_BAD_DER));

This makes the test much easier to read and understand. Also, GTest already has good support for printing the values passed to ASSERT_EQ when the test fails.

Please do this throughout.

@@ +635,5 @@
> +  ASSERT_EQ(sizeof expectedItemData, item.len);
> +  ASSERT_NE(nullptr, item.data);
> +  ASSERT_TRUE(memcmp(item.data, expectedItemData, sizeof expectedItemData) == 0);
> +}
> +

Please split all the tests for der::Input into their own patch. In fact, I think it would be a good idea to split it into its own source file.

Splitting the patches up will make the review process less painful.

@@ +839,5 @@
> +  ASSERT_EQ(Success, rv1);
> +
> +  bool value = false;
> +  Result rv2 = Boolean(input, value);
> +  ASSERT_EQ(Success, rv2);

As explained in bug 982775, this is wrong. it should be ASSERT_EQ(Failure, Boolean(input, value));

@@ +851,5 @@
> +  ASSERT_EQ(Success, rv1);
> +
> +  bool value = false;
> +  Result rv2 = Boolean(input, value);
> +  ASSERT_EQ(Success, rv2);

Ditto.

@@ +904,5 @@
> +
> +// TODO: The DER encoding specification says that Enumerated follows
> +// Integer. So although we only expect single byte values, it would
> +// actually be legal to have an enumerated value encoded in multiple
> +// bytes. How about 'overly long ints' ? (Is 0x00000001 valid ?)

As I explained elsewhere, in DER encoding you must always use the shortest possible encoding of any value. So, overly-long values are explicitly disallowed and the code is (hopefully!) correctly checking that.

@@ +1096,5 @@
> +// PKI Specific Types
> +
> +// These two are a bit odd because AlgorithmIdentifier matches inside
> +// a SEQ. I think the API would be better if it would also expect the
> +// SEQ.

I don't quite understand this comment. It would be good for us to talk about it.

@@ +1258,5 @@
> +}
> +  
> +// TODO: This test works but I think we can be stricter in the parser
> +// by requiring that we find an integer (serial) if the version is not
> +// there.

That is a good idea, but I think that such a test belongs in pkixocsp_test, not here.
Attachment #8389918 - Flags: feedback+
Attached patch move-pkxder_tests.patch (obsolete) — Splinter Review
Stefan, this patch moves the test to security/insanity/test/gtest. It depends on the patches in bug 983853.
Stefan, when going through the bugs you filed and debugging the test failures, I made these changes to your tests. I believe that these changes are the right ones to make. After making these changes, and after fixing bug 982778, the whole test suite passes.

Please integrate whichever of these changes you agree with into your patch.

Also, please make sure that line lengths are <= 80 chars. (It is OK if some lines extend to a little bit beyond column 80 if it helps readability.)
This patch addresses comments made by :briansmith about code style and also includes previous patches. No more long lines, killed trailing whitespace, split up the tests in three files. Also addressed TODOs.
Attachment #8389918 - Attachment is obsolete: true
Attachment #8391562 - Attachment is obsolete: true
Attachment #8391574 - Attachment is obsolete: true
Flags: needinfo?(brian)
Summary: Add unit tests for insanity::pkix DER decoder (pkixder.h/pkixder.c) → Add unit tests for mozilla::pkix DER decoder (pkixder.h/pkixder.c)
Attached patch tests rebased (obsolete) — Splinter Review
I rebased the test patch on top of the renaming changes. There was also an issue with AlgorithmIdentifier not initializing its output, so I fixed that. That can go in a separate bug if need be.
Attachment #8393083 - Attachment is obsolete: true
Attachment #8397449 - Flags: review?(brian)
(In reply to David Keeler (:keeler) from comment #8)
> There was also an
> issue with AlgorithmIdentifier not initializing its output, so I fixed that.
> That can go in a separate bug if need be.

Whoops - that's bug 982778.
Stefan, do you still own this?
Stefan indicated to me that he wouldn't have time to own this, so it needs a new owner.
Assignee: sarentz → nobody
Assignee: nobody → mmc
Attachment #8397449 - Attachment is obsolete: true
Attachment #8397449 - Flags: review?(brian)
Comment on attachment 8408684 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

Address Brian's comments.
Attachment #8408684 - Flags: review?(cviecco)
Attachment #8408684 - Flags: review?(brian)
Comment on attachment 8408684 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

Looks very good, I am uncertain on the decoding of the algorithm (I would let you read the rfc) and would like one extra test for sequences that are too small for their internals (I need my bikeshed)

::: security/pkix/lib/pkixder.h
@@ +474,5 @@
>  //         parameters              ANY DEFINED BY algorithm OPTIONAL  }
>  inline Result
>  AlgorithmIdentifier(Input& input, SECAlgorithmID& algorithmID)
>  {
> +  algorithmID.algorithm = { siBuffer, nullptr, 0 };

I dont think we need to initialize this.

@@ +475,5 @@
>  inline Result
>  AlgorithmIdentifier(Input& input, SECAlgorithmID& algorithmID)
>  {
> +  algorithmID.algorithm = { siBuffer, nullptr, 0 };
> +  algorithmID.parameters = { siBuffer, nullptr, 0 };

It seems like paramteters is not loaded at all move this to replace lines 483 and 484?

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +56,5 @@
> +  0x09,                       // length
> +  0x02, 0x01, 0x01,           // INTEGER length 1 value 0x01
> +  0x02, 0x01, 0x02            // INTEGER length 1 value 0x02
> +  // MISSING DATA HERE ON PURPOSE
> +};

I would also add a case where the internal sequence is marked with a length that extends over the current sequence:

const uint8_t DER_SEQUENCE_TOO_SHORT_OF_INT8[] = {
0x30, // SEQUENCE
0x09, // length
0x02, 0x01, 0x01, // INTEGER length 1 value 0x01
0x02, 0x01, 0x02 // INTEGER length 1 value 0x02 
0x02, 0x02, 0xFF, 0x03 // INTEGER length 2 value 0xFF03
};
Attachment #8408684 - Flags: review?(cviecco) → review-
Comment on attachment 8408684 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

::: security/pkix/lib/pkixder.h
@@ +475,5 @@
>  inline Result
>  AlgorithmIdentifier(Input& input, SECAlgorithmID& algorithmID)
>  {
> +  algorithmID.algorithm = { siBuffer, nullptr, 0 };
> +  algorithmID.parameters = { siBuffer, nullptr, 0 };

I think I added these two lines by mistake - we shouldn't need them. Incidentally, I believe the compiler we use on some B2G builds doesn't allow this inline initialization style.

::: security/pkix/test/gtest/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +LIBRARY_NAME = 'mozillapkix_gtest'
> +
> +EXPORT_LIBRARY = True

Hmmm - I'm not sure this is necessary. Is this what other gtest/moz.build files do?

::: toolkit/library/Makefile.in
@@ +15,5 @@
>    $(NULL)
>  
>  ifdef MOZ_APP_COMPONENT_LIBS
>  COMPONENT_LIBS += $(MOZ_APP_COMPONENT_LIBS)
> +COMPONENT_LIBS += mozillapkix_gtest

This isn't needed - the "FINAL_LIBRARY='xul-gtest'" line in security/pkix/test/gtest/moz.build should be sufficient.
Comment on attachment 8408684 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

::: security/pkix/lib/pkixder.h
@@ +475,5 @@
>  inline Result
>  AlgorithmIdentifier(Input& input, SECAlgorithmID& algorithmID)
>  {
> +  algorithmID.algorithm = { siBuffer, nullptr, 0 };
> +  algorithmID.parameters = { siBuffer, nullptr, 0 };

The bug that this chunk fixes was already fixed in a patch that landed already.

::: security/pkix/test/gtest/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +LIBRARY_NAME = 'mozillapkix_gtest'
> +
> +EXPORT_LIBRARY = True

This is the old way. The current way is to use FINAL_LIBRARY = 'xul-gtest' or similar.

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +34,5 @@
> +  }
> +
> +  virtual ~pkixder_input_tests()
> +  {
> +  }

Nit: this destructor isn't needed.

@@ +68,5 @@
> +TEST_F(pkixder_input_tests, FailWithError)
> +{
> +  ASSERT_EQ(Failure, Fail(SEC_ERROR_BAD_DER));
> +  ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
> +}

We should add another case for Fail(SEC_ERROR_INVALID_ARGS) or similar, to show that Fail() doesn't always set the error code to SEC_ERROR_BAD_DER.

@@ +82,5 @@
> +{
> +  Input input;
> +  ASSERT_EQ(Failure, input.Init(nullptr, 0));
> +  ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
> +}

We should test Init(nullptr, <non-zero-length>) and Init(non-null, 0).

@@ +91,5 @@
> +  // data argument length does not matter, it is not touched, just
> +  // needs to be non-null
> +  ASSERT_EQ(Failure, input.Init((const uint8_t*) "", 0xffff+1));
> +  ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
> +}

Shouldn't we have a case for length 0xffff, if we're testing the boundary?

@@ +112,5 @@
> +
> +  ASSERT_EQ(Success,
> +            input.Init(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
> +  ASSERT_EQ(Success,
> +            input.Expect(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));

I would also ASSERT_TRUE(input.AtEnd());

@@ +115,5 @@
> +  ASSERT_EQ(Success,
> +            input.Expect(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
> +}
> +
> +TEST_F(pkixder_input_tests, ExpectFail)

I think ExpectMismatch would be a clearer name.

@@ +188,5 @@
> +  ASSERT_EQ(Success, input.Read(readByte1));
> +  ASSERT_EQ(0x11, readByte1);
> +
> +  uint8_t readByte2 = 0;
> +  ASSERT_EQ(Failure, input.Read(readByte2));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +219,5 @@
> +  ASSERT_EQ(Success, input.Read(readWord1));
> +  ASSERT_EQ(0x1122, readWord1);
> +
> +  uint16_t readWord2 = 0;
> +  ASSERT_EQ(Failure, input.Read(readWord2));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +259,5 @@
> +  Input input;
> +  const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
> +  ASSERT_EQ(Success, input.Init(der, sizeof der));
> +
> +  ASSERT_EQ(Success, input.Skip(sizeof der));

ASSERT_TRUE(input.AtEnd());

@@ +322,5 @@
> +  SECItem item;
> +  ASSERT_EQ(Success, input.Skip(sizeof expectedItemData, item));
> +  ASSERT_EQ(siBuffer, item.type);
> +  ASSERT_EQ(sizeof expectedItemData, item.len);
> +  ASSERT_NE(nullptr, item.data);

You could ASSERT_EQ(der, item.data);

@@ +333,5 @@
> +  const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
> +  ASSERT_EQ(Success, input.Init(der, sizeof der));
> +
> +  SECItem skippedSECItem;
> +  ASSERT_EQ(Failure, input.Skip(sizeof der * 2, skippedSECItem));

sizeof(der) + 1 would be more useful for testing the boundary condition.

@@ +378,5 @@
> +
> +  SECItem item;
> +  memset(&item, 0x00, sizeof item);
> +
> +  input.GetSECItem(siBuffer, mark, item);

Need to check return value of GetSECItem

@@ +450,5 @@
> +
> +  // Make sure we cannot actually get this number of bytes
> +  ASSERT_EQ(Failure, input.Skip(length));
> +
> +  // TODO(bug 982774): Maybe it is better for ExpectTagAndGetLength to check

bug 982774 is fixed so this test needs to be redone.

@@ +503,5 @@
> +
> +Result NestedOfHelper(Input& input, std::vector<uint8_t>& readValues)
> +{
> +  uint8_t value = 0;
> +  if (Success == input.Read(value)) {

Nit: Sense is inverted from normal style of returning immediately on failure. It should be:

  if (input.Read(value) != Success) {
    return Failure;
  }
  readValues.push_back(value);
  return Success;

@@ +524,5 @@
> +                                 mozilla::pkix::ref(readValues))));
> +  ASSERT_EQ((size_t) 3, readValues.size());
> +  ASSERT_EQ(0x01, readValues[0]);
> +  ASSERT_EQ(0x02, readValues[1]);
> +  ASSERT_EQ(0x03, readValues[2]);

ASSERT_EQ(Success, End(input));

@@ +537,5 @@
> +  std::vector<uint8_t> readValues;
> +  ASSERT_EQ(Failure,
> +    NestedOf(input, SEQUENCE, INTEGER, MustNotBeEmpty,
> +             mozilla::pkix::bind(NestedOfHelper, mozilla::pkix::_1,
> +                                 mozilla::pkix::ref(readValues))));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +541,5 @@
> +                                 mozilla::pkix::ref(readValues))));
> +  ASSERT_EQ((size_t) 0, readValues.size());
> +}
> +
> +TEST_F(pkixder_input_tests, Skip)

It seems like this test and the the SkipWithTruncatedData test should be moved to be with the other tests of Skip()

@@ +557,5 @@
> +  Input input;
> +  ASSERT_EQ(Success, input.Init(DER_TRUNCATED_SEQUENCE_OF_INT8,
> +                                sizeof DER_TRUNCATED_SEQUENCE_OF_INT8));
> +
> +  ASSERT_EQ(Failure, Skip(input, SEQUENCE));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
Comment on attachment 8408684 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

::: security/pkix/test/gtest/pkixder_input_tests.cpp
@@ +13,5 @@
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +

Since this test was written, another method EnsureLength was added to class Input. We might as well test that too.

::: security/pkix/test/gtest/pkixder_pki_types_tests.cpp
@@ +34,5 @@
> +  }
> +
> +  virtual ~pkixder_pki_types_tests()
> +  {
> +  }

Nit: I don't think we need the constructor or the destructor.

@@ +54,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_ALGORITHM_IDENTIFIER_NO_PARAMS,
> +                                sizeof DER_ALGORITHM_IDENTIFIER_NO_PARAMS));
> +
> +  uint16_t length;
> +  ASSERT_EQ(Success, ExpectTagAndGetLength(input, SEQUENCE, length));

What is the purpose of wrapping this in a sequence and then stripping the sequence off? Shouldn't we just get rid of the SEQUENCE wrapper and delete the six lines above?

@@ +70,5 @@
> +  ASSERT_EQ(sizeof expectedAlgorithmID, algorithmID.algorithm.len);
> +  ASSERT_TRUE(memcmp(algorithmID.algorithm.data, expectedAlgorithmID,
> +                     sizeof expectedAlgorithmID) == 0);
> +
> +  ASSERT_EQ((size_t) 0, algorithmID.parameters.len);

0 or size_t(0) if it is really needed.

@@ +71,5 @@
> +  ASSERT_TRUE(memcmp(algorithmID.algorithm.data, expectedAlgorithmID,
> +                     sizeof expectedAlgorithmID) == 0);
> +
> +  ASSERT_EQ((size_t) 0, algorithmID.parameters.len);
> +  ASSERT_EQ(NULL, algorithmID.parameters.data);

nullptr

@@ +90,5 @@
> +  uint16_t length;
> +  ASSERT_EQ(Success, ExpectTagAndGetLength(input, SEQUENCE, length));
> +
> +  Input nested;
> +  ASSERT_EQ(Success, input.Skip(length, nested));

Ditto here; it seems like the wrapping sequence is not needed for this test.

@@ +105,5 @@
> +                     sizeof expectedAlgorithmID) == 0);
> +
> +  ASSERT_EQ((size_t) 0, algorithmID.parameters.len);
> +  ASSERT_EQ(NULL, algorithmID.parameters.data);
> +}

We should have another test that shows that when the AlgorithmIdentifier is followed by a non-NULL value, that the non-NULL value isn't consumed.

(Technically, whether or not we consume that value is supposed to depend on the OID of the algorithm. However, we don't support any algorithms that take non-NULL parameters.)

@@ +161,5 @@
> +                                sizeof DER_CERT_SERIAL_CRAZY_LONG));
> +
> +  SECItem item;
> +  // TODO(bug 982780): CertificateSerialNumber should enforce the restriction
> +  // that serial numbers must be no more than 20 bytes long.

See the discussion in bug 982780. I don't think our parser should enforce this restriction.

@@ +177,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_CERT_SERIAL_ZERO_LENGTH,
> +                                sizeof DER_CERT_SERIAL_ZERO_LENGTH));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, CertificateSerialNumber(input, item));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +184,5 @@
> +TEST_F(pkixder_pki_types_tests, OptionalVersionV1)
> +{
> +  const uint8_t DER_OPTIONAL_VERSION_V1[] = {
> +    0xa0, 0x03,                   // context specific 0
> +    0x02, 0x01, 0x00              // INTEGER(1)

INTEGER(1) is confusing; it is actually an integer with value 0 which means v1. I think you meant INTEGER(0).

@@ +250,5 @@
> +TEST_F(pkixder_pki_types_tests, OptionalVersionInvalidTooLong)
> +{
> +  const uint8_t DER_OPTIONAL_VERSION_INVALID_TOO_LONG[] = {
> +    0xa0, 0x03,                   // context specific 0
> +    0x02, 0x02, 0x12, 0x34        // INTEGER(0x12, 0x34)

INTEGER(0x1234)

@@ +258,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_OPTIONAL_VERSION_INVALID_TOO_LONG,
> +                                sizeof DER_OPTIONAL_VERSION_INVALID_TOO_LONG));
> +
> +  uint8_t version = 99;
> +  ASSERT_EQ(Failure, OptionalVersion(input, version));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +273,5 @@
> +                                sizeof DER_OPTIONAL_VERSION_MISSING));
> +
> +  uint8_t version = 99;
> +  ASSERT_EQ(Success, OptionalVersion(input, version));
> +  ASSERT_EQ(v1, version);

It would be good to demonstrate that OptionalVersion did not consume any input here. Note that the DER encoding is invalid becaues it has length 0x11 but there's only one byte of value.
Comment on attachment 8408684 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

Note: I would be very happy if you would attach an inter-diff to review (i.e. a patch on top of this one), instead of a whole new updated patch, since our review tools such so hard.

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +34,5 @@
> +  }
> +
> +  virtual ~pkixder_universal_types_tests()
> +  {
> +  }

I think the explicit constructor and destructor are not needed.

@@ +55,5 @@
> +  ASSERT_EQ(Success,
> +            input.Init(DER_BOOLEAN_TRUE_01, sizeof DER_BOOLEAN_TRUE_01));
> +
> +  bool value = false;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +71,5 @@
> +  ASSERT_EQ(Success,
> +            input.Init(DER_BOOLEAN_TRUE_42, sizeof DER_BOOLEAN_TRUE_42));
> +
> +  bool value = false;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +120,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_BOOLEAN_INVALID_LENGTH,
> +                                sizeof DER_BOOLEAN_INVALID_LENGTH));
> +
> +  bool value = true;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +135,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_BOOLEAN_INVALID_ZERO_LENGTH,
> +                                sizeof DER_BOOLEAN_INVALID_ZERO_LENGTH));
> +
> +  bool value = true;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +141,5 @@
> +
> +// TODO: The DER encoding specification says that Enumerated follows
> +// Integer. So although we only expect single byte values, it would
> +// actually be legal to have an enumerated value encoded in multiple
> +// bytes. How about 'overly long ints' ? (Is 0x00000001 valid ?)

This needs to be better documented in mozilla::pkix. I added a comment about this to the documentation bug: bug 968556 comment 7. In particular, we are fine (better off) limiting the acceptable range of enumerated values to single-byte positive values, because those are the only enumerated values actually used in OCSP and X.509 certs; we don't need to be able to parse any ASN.1 construct that isn't valid in OCSP or X.509 certs because we're not a general-purpose ASN.1 library.

Separately from that, overly-long values are never legal in DER and so we should test that we are prohibiting them appropriately.

@@ +173,5 @@
> +  uint8_t value = 0;
> +  ASSERT_EQ(Failure, Enumerated(input, value));
> +}
> +
> +TEST_F(pkixder_universal_types_tests, EnumeratedInvalidLength)

I think this should be renamed "EnumeratedOutOfAcceptedRange" and we should add a comment that although this is a valid ENUMERATED value according to ASN.1, we intentionally don't support these large values because there are no ENUMERATED values in X.509 certs or OCSP this large, and we're trying to keep the parser simple and fast.

@@ +186,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_ENUMERATED_INVALID_LENGTH,
> +                                sizeof DER_ENUMERATED_INVALID_LENGTH));
> +
> +  uint8_t value = 0;
> +  ASSERT_EQ(Failure, Enumerated(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +201,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_ENUMERATED_INVALID_ZERO_LENGTH,
> +                                sizeof DER_ENUMERATED_INVALID_ZERO_LENGTH));
> +
> +  uint8_t value = 0;
> +  ASSERT_EQ(Failure, Enumerated(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +213,5 @@
> +  const uint8_t DER_GENERALIZED_TIME_OFFSET[] = {
> +    0x18,
> +    19,
> +    '1', '9', '9', '1', '0', '5', '0', '6', '1', '6', '4', '5', '4', '0', '-',
> +    '0', '7', '0', '0'

This is not a valid GeneralizedTime according to RFC 5280:

  "For the purposes of this profile, GeneralizedTime values MUST be
   expressed in Greenwich Mean Time (Zulu) and MUST include seconds
   (i.e., times are YYYYMMDDHHMMSSZ), even where the number of seconds
   is zero.  GeneralizedTime values MUST NOT include fractional seconds."

It is fine to have leave this test here, but we should add a TODO comment about changing this test to expect failure, referencing a bug against mozilla::pkix about making the parser more strict.

@@ +222,5 @@
> +                                sizeof DER_GENERALIZED_TIME_OFFSET));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Success, GeneralizedTime(input, value));
> +  ASSERT_EQ(673573540000000, value);

OK, sure.

@@ +225,5 @@
> +  ASSERT_EQ(Success, GeneralizedTime(input, value));
> +  ASSERT_EQ(673573540000000, value);
> +}
> +
> +TEST_F(pkixder_universal_types_tests, GeneralizedTimeGMT)

Nit: s/GMT/Z/.

@@ +257,5 @@
> +            input.Init(DER_GENERALIZED_TIME_INVALID_ZERO_LENGTH,
> +                       sizeof DER_GENERALIZED_TIME_INVALID_ZERO_LENGTH));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +258,5 @@
> +                       sizeof DER_GENERALIZED_TIME_INVALID_ZERO_LENGTH));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));
> +}

We should file a follow-up bug for adding more test cases:

(1) truncated at the front so that the four-digit year has only two digits.
(2) truncated at the back so that the 'Z' is stripped off.
(3) truncated at the back so that the seconds are stripped off.
(4) No seconds, but with 'Z'
(5) Invalid month { 0, 13 }, invalid day {0, Feb 30th, 31 in a month with 30 days, 45, etc. }, invalid seconds, etc.
(6) If we support timezone indicators other than 'Z' then test that too.
(7) Leap seconds (seconds >= 60).

@@ +281,5 @@
> +    0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
> +    0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff,
> +    0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
> +    0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff
> +  };

I don't think this test is particularly helpful, but OK.

@@ +288,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_GENERALIZED_TIME_GARBAGE,
> +                                sizeof DER_GENERALIZED_TIME_GARBAGE));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +313,5 @@
> +  ASSERT_EQ(siBuffer, item.type);
> +  ASSERT_EQ((size_t) 4, item.len);
> +  ASSERT_NE(nullptr, item.data);
> +  ASSERT_TRUE(memcmp(item.data, expectedItemData, sizeof expectedItemData)==0);
> +}

All integer values that we encounter in practice are one byte, except for serial numbers. We should make sure we're testing the one-byte case.

@@ +331,5 @@
> +
> +  SECItem item;
> +  memset(&item, 0x00, sizeof item);
> +
> +  EXPECT_EQ(Failure, Integer(input, item));

ASSERT_EQ, not EXPECT_EQ, right? (ditto the two lines below)

Also, ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +349,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_ZERO_LENGTH,
> +                                sizeof DER_INTEGER_ZERO_LENGTH));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +365,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_OVERLY_LONG1,
> +                                sizeof DER_INTEGER_OVERLY_LONG1));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +366,5 @@
> +                                sizeof DER_INTEGER_OVERLY_LONG1));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));
> +}

We should test that the single-byte values 0x00, 0x80, and 0xff work and aren't considered overly-long.

We should test some negative value other than 0xff and 0x80.

@@ +381,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_OVERLY_LONG2,
> +                                sizeof DER_INTEGER_OVERLY_LONG2));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +408,5 @@
> +  Input input;
> +  ASSERT_EQ(Success,
> +            input.Init(DER_NULL_BAD_LENGTH, sizeof DER_NULL_BAD_LENGTH));
> +
> +  ASSERT_EQ(Failure, Null(input));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +428,5 @@
> +  };
> +
> +  ASSERT_EQ(Success, OID(input, expectedOID));
> +}
> +

Let's file a follow-up bug for more comprehensive testing of OIDs. It turns out that OID encoding is complicated.
Attachment #8408684 - Flags: review?(brian) → review-
Comment on attachment 8408684 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

Note: I would be very happy if you would attach an inter-diff to review (i.e. a patch on top of this one), instead of a whole new updated patch, since our review tools such so hard.

Also, please file a bug about testing the new things (e.g. OptionalInteger) that were added since this test was originally written.

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +34,5 @@
> +  }
> +
> +  virtual ~pkixder_universal_types_tests()
> +  {
> +  }

I think the explicit constructor and destructor are not needed.

@@ +55,5 @@
> +  ASSERT_EQ(Success,
> +            input.Init(DER_BOOLEAN_TRUE_01, sizeof DER_BOOLEAN_TRUE_01));
> +
> +  bool value = false;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +71,5 @@
> +  ASSERT_EQ(Success,
> +            input.Init(DER_BOOLEAN_TRUE_42, sizeof DER_BOOLEAN_TRUE_42));
> +
> +  bool value = false;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +120,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_BOOLEAN_INVALID_LENGTH,
> +                                sizeof DER_BOOLEAN_INVALID_LENGTH));
> +
> +  bool value = true;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +135,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_BOOLEAN_INVALID_ZERO_LENGTH,
> +                                sizeof DER_BOOLEAN_INVALID_ZERO_LENGTH));
> +
> +  bool value = true;
> +  ASSERT_EQ(Failure, Boolean(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +141,5 @@
> +
> +// TODO: The DER encoding specification says that Enumerated follows
> +// Integer. So although we only expect single byte values, it would
> +// actually be legal to have an enumerated value encoded in multiple
> +// bytes. How about 'overly long ints' ? (Is 0x00000001 valid ?)

This needs to be better documented in mozilla::pkix. I added a comment about this to the documentation bug: bug 968556 comment 7. In particular, we are fine (better off) limiting the acceptable range of enumerated values to single-byte positive values, because those are the only enumerated values actually used in OCSP and X.509 certs; we don't need to be able to parse any ASN.1 construct that isn't valid in OCSP or X.509 certs because we're not a general-purpose ASN.1 library.

Separately from that, overly-long values are never legal in DER and so we should test that we are prohibiting them appropriately.

@@ +173,5 @@
> +  uint8_t value = 0;
> +  ASSERT_EQ(Failure, Enumerated(input, value));
> +}
> +
> +TEST_F(pkixder_universal_types_tests, EnumeratedInvalidLength)

I think this should be renamed "EnumeratedOutOfAcceptedRange" and we should add a comment that although this is a valid ENUMERATED value according to ASN.1, we intentionally don't support these large values because there are no ENUMERATED values in X.509 certs or OCSP this large, and we're trying to keep the parser simple and fast.

@@ +186,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_ENUMERATED_INVALID_LENGTH,
> +                                sizeof DER_ENUMERATED_INVALID_LENGTH));
> +
> +  uint8_t value = 0;
> +  ASSERT_EQ(Failure, Enumerated(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +201,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_ENUMERATED_INVALID_ZERO_LENGTH,
> +                                sizeof DER_ENUMERATED_INVALID_ZERO_LENGTH));
> +
> +  uint8_t value = 0;
> +  ASSERT_EQ(Failure, Enumerated(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +213,5 @@
> +  const uint8_t DER_GENERALIZED_TIME_OFFSET[] = {
> +    0x18,
> +    19,
> +    '1', '9', '9', '1', '0', '5', '0', '6', '1', '6', '4', '5', '4', '0', '-',
> +    '0', '7', '0', '0'

This is not a valid GeneralizedTime according to RFC 5280:

  "For the purposes of this profile, GeneralizedTime values MUST be
   expressed in Greenwich Mean Time (Zulu) and MUST include seconds
   (i.e., times are YYYYMMDDHHMMSSZ), even where the number of seconds
   is zero.  GeneralizedTime values MUST NOT include fractional seconds."

It is fine to have leave this test here, but we should add a TODO comment about changing this test to expect failure, referencing a bug against mozilla::pkix about making the parser more strict.

@@ +222,5 @@
> +                                sizeof DER_GENERALIZED_TIME_OFFSET));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Success, GeneralizedTime(input, value));
> +  ASSERT_EQ(673573540000000, value);

OK, sure.

@@ +225,5 @@
> +  ASSERT_EQ(Success, GeneralizedTime(input, value));
> +  ASSERT_EQ(673573540000000, value);
> +}
> +
> +TEST_F(pkixder_universal_types_tests, GeneralizedTimeGMT)

Nit: s/GMT/Z/.

@@ +257,5 @@
> +            input.Init(DER_GENERALIZED_TIME_INVALID_ZERO_LENGTH,
> +                       sizeof DER_GENERALIZED_TIME_INVALID_ZERO_LENGTH));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +258,5 @@
> +                       sizeof DER_GENERALIZED_TIME_INVALID_ZERO_LENGTH));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));
> +}

We should file a follow-up bug for adding more test cases:

(1) truncated at the front so that the four-digit year has only two digits.
(2) truncated at the back so that the 'Z' is stripped off.
(3) truncated at the back so that the seconds are stripped off.
(4) No seconds, but with 'Z'
(5) Invalid month { 0, 13 }, invalid day {0, Feb 30th, 31 in a month with 30 days, 45, etc. }, invalid seconds, etc.
(6) If we support timezone indicators other than 'Z' then test that too.
(7) Leap seconds (seconds >= 60).

@@ +281,5 @@
> +    0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
> +    0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff,
> +    0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
> +    0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff
> +  };

I don't think this test is particularly helpful, but OK.

@@ +288,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_GENERALIZED_TIME_GARBAGE,
> +                                sizeof DER_GENERALIZED_TIME_GARBAGE));
> +
> +  PRTime value = 0;
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +313,5 @@
> +  ASSERT_EQ(siBuffer, item.type);
> +  ASSERT_EQ((size_t) 4, item.len);
> +  ASSERT_NE(nullptr, item.data);
> +  ASSERT_TRUE(memcmp(item.data, expectedItemData, sizeof expectedItemData)==0);
> +}

All integer values that we encounter in practice are one byte, except for serial numbers. We should make sure we're testing the one-byte case.

@@ +331,5 @@
> +
> +  SECItem item;
> +  memset(&item, 0x00, sizeof item);
> +
> +  EXPECT_EQ(Failure, Integer(input, item));

ASSERT_EQ, not EXPECT_EQ, right? (ditto the two lines below)

Also, ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +349,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_ZERO_LENGTH,
> +                                sizeof DER_INTEGER_ZERO_LENGTH));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +365,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_OVERLY_LONG1,
> +                                sizeof DER_INTEGER_OVERLY_LONG1));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +366,5 @@
> +                                sizeof DER_INTEGER_OVERLY_LONG1));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));
> +}

We should test that the single-byte values 0x00, 0x80, and 0xff work and aren't considered overly-long.

We should test some negative value other than 0xff and 0x80.

@@ +381,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_OVERLY_LONG2,
> +                                sizeof DER_INTEGER_OVERLY_LONG2));
> +
> +  SECItem item;
> +  ASSERT_EQ(Failure, Integer(input, item));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +408,5 @@
> +  Input input;
> +  ASSERT_EQ(Success,
> +            input.Init(DER_NULL_BAD_LENGTH, sizeof DER_NULL_BAD_LENGTH));
> +
> +  ASSERT_EQ(Failure, Null(input));

ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());

@@ +428,5 @@
> +  };
> +
> +  ASSERT_EQ(Success, OID(input, expectedOID));
> +}
> +

Let's file a follow-up bug for more comprehensive testing of OIDs. It turns out that OID encoding is complicated.
I heard interdiffs work on rbt, let me try that. You might get some spam.

https://reviewboard.allizom.org/r/71/
Attachment #8408684 - Attachment is obsolete: true
Comment on attachment 8409168 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

rbt interdiff: https://reviewboard.allizom.org/r/71/diff/2-3/

I can't figure out how the review functionality actually works. Fixed easy comments, except for the following: 

> What is the purpose of wrapping this in a sequence and then stripping the sequence off? Shouldn't we just get rid of the SEQUENCE wrapper and delete the six lines above?

I got rid of the sequence in the first case, not in the second with the non-null params, which didn't parse properly without the SEQUENCE wrapper.

> We should have another test that shows that when the AlgorithmIdentifier is followed by a non-NULL value, that the non-NULL value isn't consumed.
> It would be good to demonstrate that OptionalVersion did not consume any input here. Note that the DER encoding is invalid becaues it has length 0x11 but there's only one byte of value.

I don't know how to check that no input was consumed.

I added some small tests but deferred the rest to bug 998513 and bug 998482. It's better to land then improve rather than land all the tests at once. Also tests lose utility when they are too long.
Attachment #8409168 - Flags: review?(cviecco)
Attachment #8409168 - Flags: review?(brian)
Some of the build changes that people asked for broke the main build but not gtest.

 1:03.98 Traceback (most recent call last):
 1:03.98   File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
 1:03.98     "__main__", fname, loader, pkg_name)
 1:03.98   File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
 1:03.98     exec code in run_globals
 1:03.98   File "/home/mchew/mozilla-central/python/mozbuild/mozbuild/action/link_deps.py", line 229, in <module>
 1:03.98     main(sys.argv[1:])
 1:03.98   File "/home/mchew/mozilla-central/python/mozbuild/mozbuild/action/link_deps.py", line 219, in main
 1:03.98     linker.add_dependencies(open(f))
 1:03.98   File "/home/mchew/mozilla-central/python/mozbuild/mozbuild/action/link_deps.py", line 134, in add_dependencies
 1:03.98     % (t, self._targets[t][0], depfile))
 1:03.98 Exception: Found target $(DIST)/lib/libxul.so in $(DEPTH)/toolkit/library/build/binaries and $(DEPTH)/toolkit/library/gtest/binaries
Comment on attachment 8409168 [details] [diff] [review]
Add mozilla::pkix::der unit tests (

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

LGTM.(lets land some tests, improve coverage later)
Attachment #8409168 - Flags: review?(cviecco) → review+
Needed to fix a tests after bug 982774, but try is still mysteriously failing:

https://tbpl.mozilla.org/?rev=caa68ce13d9a&tree=Try

17:52:03     INFO -  In file included from ../../../../../gecko/security/pkix/test/gtest/pkixder_input_tests.cpp:21:
17:52:03     INFO -  ../../../../dist/include/gtest/gtest.h: In function 'testing::AssertionResult testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&) [with T1 = int, T2 = unsigned char*]':
17:52:03     INFO -  ../../../../../gecko/security/pkix/test/gtest/pkixder_input_tests.cpp:423:   instantiated from here
17:52:03    ERROR -  ../../../../dist/include/gtest/gtest.h:1445: error: ISO C++ forbids comparison between pointer and integer

The relevant line is comparing a pointer and a pointer, so I'm not sure what's going on.

423   ASSERT_NE(nullptr, item.data);
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #26)
> Needed to fix a tests after bug 982774, but try is still mysteriously
> failing:
> 
> https://tbpl.mozilla.org/?rev=caa68ce13d9a&tree=Try
> 
> 17:52:03     INFO -  In file included from
> ../../../../../gecko/security/pkix/test/gtest/pkixder_input_tests.cpp:21:
> 17:52:03     INFO -  ../../../../dist/include/gtest/gtest.h: In function
> 'testing::AssertionResult testing::internal::CmpHelperNE(const char*, const
> char*, const T1&, const T2&) [with T1 = int, T2 = unsigned char*]':
> 17:52:03     INFO - 
> ../../../../../gecko/security/pkix/test/gtest/pkixder_input_tests.cpp:423:  
> instantiated from here
> 17:52:03    ERROR -  ../../../../dist/include/gtest/gtest.h:1445: error: ISO
> C++ forbids comparison between pointer and integer
> 
> The relevant line is comparing a pointer and a pointer, so I'm not sure
> what's going on.
> 
> 423   ASSERT_NE(nullptr, item.data);

You mean the B2G failures, right? On B2G we're using ancient GCC (4.4 IIRC) and it probably doesn't support native nullptr so there's probably a hidden "#define nullptr 0" somewhere. Try ASSERT_TRUE(item.data) as a workaround.
Flags: needinfo?(brian)
Attachment #8409168 - Attachment is obsolete: true
Attachment #8409168 - Flags: review?(brian)
Thanks, Brian. I'll try that again when trees reopen.
https://hg.mozilla.org/mozilla-central/rev/aad09525748b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.