982786 - Test failure for mozilla::pkixder OptionalVersionInvalidTooLong

Status: RESOLVED WORKSFORME

(Core :: Security: PSM, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

There is a flaw in the logic of der::OptionalVersion. It tries to expect an Integer of length 1. If that cannot be found then it assumes that the version is v1. (Because in v1 certificates the version number was optional?)

This also means that if instead of a 1 byte integer there is another type, or a longer integer, present, it will incorrectly assume it is a v1 cert.

This may not be a problem, but I think for the sake of correctness we should make der::OptionalVersion() a little smarter so that it correctly recognizes the possible  absence of the version number.

Comment 1

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

I believe that this test itself is invalid:

> TEST_F(pkixder_tests, OptionalVersionInvalidTooLong)
> {
>   Input input;
>   Result rv1 = input.Init(DER_OPTIONAL_VERSION_INVALID_TOO_LONG,
>                           sizeof DER_OPTIONAL_VERSION_INVALID_TOO_LONG);
>   ASSERT_EQ(Failure, rv1);
> }

It should be something like this:

 TEST_F(pkixder_tests, OptionalVersionInvalidTooLong)
 {
   Input input;
   Result rv1 = input.Init(DER_OPTIONAL_VERSION_INVALID_TOO_LONG,
                           sizeof DER_OPTIONAL_VERSION_INVALID_TOO_LONG);
-  ASSERT_EQ(Failure, rv1);
+  ASSERT_EQ(Success, rv1);
+
+  uint8_t version = 99;
+  ASSERT_EQ(Failure, OptionalVersion(input, version));
 }

Which passes.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

As far as I can tell, the code in question landed much as described by comment 1 in bug 968490, so I think there's nothing more to do here.