998513 - Test GeneralizedTime encodings

Status: RESOLVED FIXED

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

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

+++ This bug was initially created as a clone of Bug #998482 +++

+++ This bug was initially created as a clone of Bug #968490 +++

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).

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: more-generalized-time-tests

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8437230 [details] [diff] [review]
more-generalized-time-tests

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

DER_GeneralizedTimeToTime uses simple bound checking for the values of seconds and days, so we dont allows leap seconds, but will allow any month of the year to have up to 31 days (including Feb).

Comment 3

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

Comment on attachment 8437230 [details] [diff] [review]
more-generalized-time-tests

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

Looks good. Let's add more comments so we don't have just these opaque values.
How about a test for a month that doesn't exist? Or hours or minutes? What happens if there's a negative value? Or if there's other non-numerical symbols? What if the year is 5 digits? What happens if the Zulu indicator is a lower-case z?
Do you have a test in here for fractional seconds?
What if the time-zone is not Zulu and it's specified? (e.g. -0800) What if the time-zone isn't "Z" but it is -0000?

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +365,5 @@
> +{
> +  // ISO 8601 says that the year resresentation MUST be of at least
> +  // four digits. 1BC is labeled 0000
> +  const uint8_t DER_GENERALIZED_TIME_INVALID_TWO_DIGIT_YEAR[] = {
> +    0x18,

add a comment: // GeneralizedTime

@@ +366,5 @@
> +  // ISO 8601 says that the year resresentation MUST be of at least
> +  // four digits. 1BC is labeled 0000
> +  const uint8_t DER_GENERALIZED_TIME_INVALID_TWO_DIGIT_YEAR[] = {
> +    0x18,
> +    13,

comment: length 13

@@ +367,5 @@
> +  // four digits. 1BC is labeled 0000
> +  const uint8_t DER_GENERALIZED_TIME_INVALID_TWO_DIGIT_YEAR[] = {
> +    0x18,
> +    13,
> +    '9', '1', '0', '5', '0', '6', '1', '6', '4', '5', '4', '0', 'Z'

Let's label the parts of this (e.g. which part is the two-digit year?)

@@ +373,5 @@
> +
> +  Input input;
> +  Result rv1 = input.Init(DER_GENERALIZED_TIME_INVALID_TWO_DIGIT_YEAR,
> +                          sizeof DER_GENERALIZED_TIME_INVALID_TWO_DIGIT_YEAR);
> +  ASSERT_EQ(Success, rv1);

Apparently "ASSERT_EQ(Success, input.Init(...));" is the expected style.

@@ +375,5 @@
> +  Result rv1 = input.Init(DER_GENERALIZED_TIME_INVALID_TWO_DIGIT_YEAR,
> +                          sizeof DER_GENERALIZED_TIME_INVALID_TWO_DIGIT_YEAR);
> +  ASSERT_EQ(Success, rv1);
> +
> +  PRTime value = 0;

nit: probably don't need to initialize value here

@@ +388,5 @@
> +  // Zulu
> +  const uint8_t DER_GENERALIZED_TIME_INVALID_LOCAL[] = {
> +    0x18,
> +    14,
> +    '1', '9', '9', '1', '0', '5', '0', '6', '1', '6', '4', '5', '4', '0'

again - label parts

@@ +394,5 @@
> +
> +  Input input;
> +  Result rv1 = input.Init(DER_GENERALIZED_TIME_INVALID_LOCAL,
> +                          sizeof DER_GENERALIZED_TIME_INVALID_LOCAL);
> +  ASSERT_EQ(Success, rv1);

ASSERT_EQ(Success, input.Init(...));

@@ +427,5 @@
> +TEST_F(pkixder_universal_types_tests, GeneralizedTimeNoSeconds)
> +{
> +  // While this is a valid generalized time (ISO 8601) it is NOT
> +  // a valid RFC5280 Generalized time (Section 4.1.2.5.2) as it does not
> +  // contain seconds value. However we are accepting this (sad panda)

so... bug #?
Similarly, does the code enforce that we require minutes, hours, days, months, years?

@@ +479,5 @@
> +
> +  PRTime value = 0;
> +  Result rv2 = GeneralizedTime(input, value);
> +  ASSERT_EQ(Success, rv2);
> +  // Sad panda, this is Sat, 02 Mar 1991 16:45:40 GMT

File a bug

Comment 4

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

(In reply to Camilo Viecco (:cviecco) from comment #2)
> DER_GeneralizedTimeToTime uses simple bound checking for the values of
> seconds and days, so we dont allows leap seconds, but will allow any month
> of the year to have up to 31 days (including Feb).

Please write the tests that we are *supposed* to pass, including checking that we reject things like "February 30th". I have already written a patch for bug 1019770 that will fix those for mozilla::pkix, so you can just reference 1019770 in your TODO comments.

Comment 5

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

Also, if our time parser is really as broken as Camilo says it is here, in that it accepts several types of invalid values, then there's probably not anything we can do about it for Firefox 31, since there would be a big compatibility risk. How important is it for this test to be done for Firefox 31? If you guys don't think it is that important for Firefox 31, I'd be happy to take over this bug if Camilo doesn't feel like writing a bunch more of the tests, since I already wrote the patch for bug 1019770--I just can't promise it will be done for Firefox 31. But, I definitely don't want to steal this bug from Camilo if he wants to keep owning it :).

Comment 6

[Camilo Viecco (:cviecco)]

Brian, you know how much I enjoy writing tests.(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #5)
> Also, if our time parser is really as broken as Camilo says it is here, in
> that it accepts several types of invalid values, then there's probably not
> anything we can do about it for Firefox 31, since there would be a big
> compatibility risk. How important is it for this test to be done for Firefox
> 31?
Not important at all, this is to really finish our: things that should be done for FF31 (we discussed and agreed it would be OK for them to land on the current central only without need to uplift)

 If you guys don't think it is that important for Firefox 31, I'd be
> happy to take over this bug if Camilo doesn't feel like writing a bunch more
> of the tests, since I already wrote the patch for bug 1019770--I just can't
> promise it will be done for Firefox 31. But, I definitely don't want to
> steal this bug from Camilo if he wants to keep owning it :).

I just finished another round of tests, but all of them should pass with your new
parser. Lets assume I can get an r+ for this (and thanks for offering the taking
this bug, you know that writing tests like this (with no automation) ranks to me
close to needing to go to the dentist.

Comment 7

[Camilo Viecco (:cviecco)]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #3)
> Comment on attachment 8437230 [details] [diff] [review]
> more-generalized-time-tests
> 
> Review of attachment 8437230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Let's add more comments so we don't have just these opaque
> values.
> How about a test for a month that doesn't exist? 
The patch included such test (see invalid month)
Or hours or minutes? What
> happens if there's a negative value? Or if there's other non-numerical
> symbols? What if the year is 5 digits? What happens if the Zulu indicator is
> a lower-case z?
> Do you have a test in here for fractional seconds?
> What if the time-zone is not Zulu and it's specified? (e.g. -0800) What if
> the time-zone isn't "Z" but it is -0000?
That test was already there.

Comment 8

[Camilo Viecco (:cviecco)]

Attachment: more-generalized-time-tests (v2)

Comment 9

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

Comment on attachment 8439312 [details] [diff] [review]
more-generalized-time-tests (v2)

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

Looking good. I think it would be nice to comment each of the tests with why the input is what it is and how we're expecting it to fail. See the example, below.
Also, I just thought of another test: what if there's leading/trailing whitespace?
r=me with comments addressed.

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +322,5 @@
> +//   defined in Section 2.4.  The format for GeneralizedTime is as
> +//   specified in Section 4.1.2.5.2 of [RFC5280].
> +//
> +// So while we can could accept other ASN1 (ITU-T X.680) encodings for
> +// GeneralizedTime  we should not accept them and breaking reading of these

nit: double-space

@@ +323,5 @@
> +//   specified in Section 4.1.2.5.2 of [RFC5280].
> +//
> +// So while we can could accept other ASN1 (ITU-T X.680) encodings for
> +// GeneralizedTime  we should not accept them and breaking reading of these
> +// other encodings is actually encouraged :).

I'm not a fan of emoticons in comments.

@@ +325,5 @@
> +// So while we can could accept other ASN1 (ITU-T X.680) encodings for
> +// GeneralizedTime  we should not accept them and breaking reading of these
> +// other encodings is actually encouraged :).
> +
> +// Bug 1023605 Times with offset are should not be considered valid

nit: "TODO(bug 1023605): a GeneralizedTime with a timezone offset should not be considered valid"

@@ +330,5 @@
>  TEST_F(pkixder_universal_types_tests, GeneralizedTimeOffset)
>  {
>    const uint8_t DER_GENERALIZED_TIME_OFFSET[] = {
>      0x18,
>      19,

Comment this too?

@@ +418,5 @@
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));
> +  ASSERT_EQ(SEC_ERROR_INVALID_TIME, PR_GetError());
> +}
> +
> +// Bug 1023605 Times without seconds should not be considered valid

TODO(bug 1023605): a GeneralizedTime...

@@ +433,5 @@
> +            input.Init(DER_GENERALIZED_TIME_NO_SECONDS,
> +                       sizeof DER_GENERALIZED_TIME_NO_SECONDS));
> +  PRTime value = 0;
> +  Result rv2 = GeneralizedTime(input, value);
> +  ASSERT_EQ(Success, rv2);

ASSERT_EQ(Success, GeneralizedTime(...));

@@ +471,5 @@
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));
> +  ASSERT_EQ(SEC_ERROR_INVALID_TIME, PR_GetError());
> +}
> +
> +// Bug 1023437 invalid days for a given month should be rejected

TODO(bug 1023437): ...

@@ +486,5 @@
> +            input.Init(DER_GENERALIZED_TIME_INVALID_DAY_FEB,
> +                       sizeof DER_GENERALIZED_TIME_INVALID_DAY_FEB));
> +  PRTime value = 0;
> +  Result rv2 = GeneralizedTime(input, value);
> +  ASSERT_EQ(Success, rv2);

ASSERT_EQ(Success, GeneralizedTime(...));

@@ +487,5 @@
> +                       sizeof DER_GENERALIZED_TIME_INVALID_DAY_FEB));
> +  PRTime value = 0;
> +  Result rv2 = GeneralizedTime(input, value);
> +  ASSERT_EQ(Success, rv2);
> +  // Bug Bug 1023437. The returned value is Sat, 02 Mar 1991 16:45:40 GMT

I'm not sure two comments are necessary for this test. In either case: "TODO(bug 1023437): ..."

@@ +496,5 @@
> +{
> +  const uint8_t DER_GENERALIZED_TIME_INVALID_DAY_DEC[] = {
> +    0x18,                           // Generalized Time
> +    15,                             // Length = 15
> +    '1', '9', '9', '1', '1', '2', '3', '2', '1', '6', '4', '5', '4', '0', 'Z'

Without commenting these values, it's a bit hard to see exactly what this is testing. Why not do something like this:
'1', '9', '9', '1', // YYYY (1991)
'1', '2', // MM (December)
'3', '2', // DD (the 32nd, which doesn't exist)
... etc.

@@ +531,5 @@
> +{
> +  const uint8_t DER_GENERALIZED_TIME_INVALID_HOURS[] = {
> +    0x18,                           // Generalized Time
> +    15,                             // Length = 15
> +    '2', '0', '1', '2', '0', '6', '3', '0', '2', '5', '5', '9', '0', '1', 'Z'

Again, hard to spot what's being tested here - document the input.

@@ +594,5 @@
> +TEST_F(pkixder_universal_types_tests, GeneralizedTimeInvalidExtraData)
> +{
> +  const uint8_t DER_GENERALIZED_TIME_INVALID_EXTRA_DATA[] = {
> +    0x18,                           // Generalized Time
> +    16,                             // Length = 15

This comment is out of date.

@@ +606,5 @@
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));
> +  ASSERT_EQ(SEC_ERROR_INVALID_TIME, PR_GetError());
> +}
> +
> +TEST_F(pkixder_universal_types_tests, GeneralizedTimeInvalidCenturyChar)

Technically I think it's the millennium char that's invalid...

@@ +692,5 @@
> +  ASSERT_EQ(Failure, GeneralizedTime(input, value));
> +  ASSERT_EQ(SEC_ERROR_INVALID_TIME, PR_GetError());
> +}
> +
> +

nit: unnecessary blank line

Comment 10

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

Comment on attachment 8439312 [details] [diff] [review]
more-generalized-time-tests (v2)

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

The main thing that we're missing from these tests is a confirmation that we are actually parsing the time to the correct value. That is, how do we know for syntactically-correct GeneralizedTimes that the PRTime value returned by the GeneralizedTime function is correct? There need to be some comparisons between the PRTime value returned by GeneralizedTime and the PRTime constructed by PR_ImplodeTime.

Comment 11

[Camilo Viecco (:cviecco)]

Attachment: more-generalized-time-tests (v3)

Keeping r+ from keeler

Comment 12

[Camilo Viecco (:cviecco)]

Attachment: more-generalized-time-tests (v3.1)

Keeping r+ from keeler
fixed two missed nits.

Comment 13

[Camilo Viecco (:cviecco)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #10)
> Comment on attachment 8439312 [details] [diff] [review]
> more-generalized-time-tests (v2)
> 
> Review of attachment 8439312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The main thing that we're missing from these tests is a confirmation that we
> are actually parsing the time to the correct value. That is, how do we know
> for syntactically-correct GeneralizedTimes that the PRTime value returned by
> the GeneralizedTime function is correct? There need to be some comparisons
> between the PRTime value returned by GeneralizedTime and the PRTime
> constructed by PR_ImplodeTime.

We do have one of those for 1991

Comment 14

[Camilo Viecco (:cviecco)]

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

Comment 15

[Phil Ringnalda (:philor)]

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