1019770 - Use mozilla::pkix::der to decode certificate validity period (notAfter and notBefore)

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

Goals:

1. Remove NSS dependency.
2. Clarify time zone decoding.
3. Make it easy (easier) to write unit tests for testing notAfter/notBefore processing (expiration).
4. Pave the way for decoding the rest of the certificate using mozilla::pkix::der to remove the NSS CERTCertificate dependency.

Comment 1

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

Attachment: parse-GeneralizedTime.patch

You may find it helpful to use this patch for writing your tests.

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8437350 [details] [diff] [review]
parse-GeneralizedTime.patch

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

This is good.

::: security/pkix/lib/pkixder.cpp
@@ +168,5 @@
> +}
> +
> +inline int
> +daysBeforeYear(int year)
> +{

This is not quite days before year. But I cannot think of a better name.

@@ +177,5 @@
> +}
> +
> +Result
> +TimeChoice(SECItemType type, Input& input, /*out*/ PRTime& time)
> +{

Add comment something stating that we only decode times that match RFC5280 and not genertic ASN1 times?

@@ +232,5 @@
> +  static const int aug = 31;
> +  static const int sep = 30;
> +  static const int oct = 31;
> +  static const int nov = 30;
> +  static const int dec = 31;

why not replace this with an an array
daysInMonth = [ 0,  // null entry, to make calls simple
                31, //jan
              ...
                31 //Dec
];

and then
for (i=1;i<=month;i++) {
  days+=daysinMonth[i];
}

////
Maybe is just my fascination with for loops.

@@ +277,5 @@
> +  if (ReadTwoDigits(input, 0, 59, minutes) != Success) {
> +    return Failure;
> +  }
> +  int seconds;
> +  if (ReadTwoDigits(input, 0, 59, seconds) != Success) {

// comment here: no leap seconds

Comment 3

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

Comment on attachment 8437350 [details] [diff] [review]
parse-GeneralizedTime.patch

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

::: security/pkix/lib/pkixder.cpp
@@ +177,5 @@
> +}
> +
> +Result
> +TimeChoice(SECItemType type, Input& input, /*out*/ PRTime& time)
> +{

OK.

@@ +232,5 @@
> +  static const int aug = 31;
> +  static const int sep = 30;
> +  static const int oct = 31;
> +  static const int nov = 30;
> +  static const int dec = 31;

I did it this way for efficiency. In theory the compiler may unroll that loop into what I've done but since I already ground out the explicit unrolling we might as well use it.

@@ +277,5 @@
> +  if (ReadTwoDigits(input, 0, 59, minutes) != Success) {
> +    return Failure;
> +  }
> +  int seconds;
> +  if (ReadTwoDigits(input, 0, 59, seconds) != Success) {

OK.

Comment 4

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

Before I ask for review, I'm going to add some more tests in addition to the tests Camilo wrote in bug 998513.

Comment 5

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

Attachment: new-CheckTimes-p1-implementation.patch

Comment 6

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

Attachment: new-CheckTimes-p2-modify-existing-tests.patch

Comment 7

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

Attachment: new-CheckTimes-p2-modify-existing-tests-2.patch

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 8444237 [details] [diff] [review]
new-CheckTimes-p1-implementation.patch

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

::: security/pkix/lib/pkixder.h
@@ +503,5 @@
> +//
> +// and parse the tag (and length and value) from the input like the other
> +// functions here. However, currently we get TimeChoices already partially
> +// decoded by NSS, so for now we'll have this signature, where the input
> +// parameter contains the value of the time choice.

Maybe add something here about the valid time formats and the limit of tumes to be >= epoch?

Comment 9

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

Attachment: Oops!

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 8446054 [details] [diff] [review]
Oops!

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

Good find.

Comment 11

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

Attachment: add-more-time-tests.patch

This patch adds more tests for how we parse times. The existing tests mostly test that we're correctly rejecting times with invalid syntax. These tests are mostly testing that we correctly parse times with valid syntax.

Comment 12

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

Attachment: add-notAfter-notBefore-tests.patch

This is the final set of tests. It checks that we're actually checking notAfter and notBefore against the given time. Pretty crazy that we didn't already have said tests!

Comment 13

[Camilo Viecco (:cviecco)]

Comment on attachment 8448399 [details] [diff] [review]
add-more-time-tests.patch

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

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +547,5 @@
> +  // Note that by using the last second of the last day of the year, we're also
> +  // effectively testing all the accumulated conversions from Gregorian to to
> +  // Julian time, including in particular the effects of leap years.
> +
> +  for (uint16_t i = 1970; i <= 9999; ++i) {

overkill much?

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea9b7bd2db5
https://hg.mozilla.org/integration/mozilla-inbound/rev/981f8345edd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/91b03a644dbe

Comment 15

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

Thanks for the reviews. I addressed your review comments before checking in the patches. I folded the "oops!" patch into the main checkin. I added leave-open since there is still one testing patch in the bug that needs to be reviewed and landed.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0a6c01c429
https://hg.mozilla.org/integration/mozilla-inbound/rev/56344d2ab608
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0bba462a5d because something from this push (this bug, or bug 1031952 or bug 1032947) broke the build: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=42894750&tree=Mozilla-Inbound

Comment 17

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

(In reply to Wes Kocher (:KWierso) from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0a6c01c429
> https://hg.mozilla.org/integration/mozilla-inbound/rev/56344d2ab608
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0bba462a5d because
> something from this push (this bug, or bug 1031952 or bug 1032947) broke the
> build: 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42894750&tree=Mozilla-
> Inbound

It was cset 91b03a644dbe from this bug. Here is the fix:

-#define TWO_CHARS(t) ('0' + ((t) / 10u)), ('0' + ((t) % 10u))
+#define TWO_CHARS(t) static_cast<uint8_t>('0' + ((t) / 10u)), \
+                     static_cast<uint8_t>('0' + ((t) % 10u))

Will land this again with this fix after the tree opens.

Comment 18

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

Re-pushed with the aforementioned fix, after verifying clean tryserver run:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ed7720fc40
https://hg.mozilla.org/integration/mozilla-inbound/rev/278f163756bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a83d5a927c90

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2ed7720fc40
https://hg.mozilla.org/mozilla-central/rev/278f163756bd
https://hg.mozilla.org/mozilla-central/rev/a83d5a927c90

Comment 20

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

Attachment: remove unused GENERALIZED_TIME_LENGTH

Looks like this breaks clang builds with --enable-warnings-as-errors:

 0:23.27 pkixder_universal_types_tests.o
 0:23.96 /home/keeler/mozilla-central/security/pkix/test/gtest/pkixder_universal_types_tests.cpp:359:23: error: unused variable 'GENERALIZED_TIME_LENGTH' [-Werror,-Wunused-const-variable]
 0:23.96 static const uint16_t GENERALIZED_TIME_LENGTH = 17; // tvYYYYMMDDHHMMSSZ
 0:23.96                       ^
 0:23.96 1 error generated.

Comment 21

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

Great - thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5e337cfa83

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f5e337cfa83

Comment 24

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

Thanks!

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

Comment 25

[Carsten Book [:Tomcat]]

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