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

RESOLVED FIXED in mozilla33

Status

()

enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

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.
Posted patch parse-GeneralizedTime.patch (obsolete) — Splinter Review
You may find it helpful to use this patch for writing your tests.
Attachment #8437350 - Flags: feedback?(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
Attachment #8437350 - Flags: feedback?(cviecco) → feedback+
Depends on: 998513
Target Milestone: --- → mozilla33
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.
Before I ask for review, I'm going to add some more tests in addition to the tests Camilo wrote in bug 998513.
Attachment #8437350 - Attachment is obsolete: true
Attachment #8444237 - Flags: review?(cviecco)
Attachment #8444238 - Attachment is obsolete: true
Attachment #8444238 - Flags: review?(cviecco)
Attachment #8444250 - Flags: review?(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?
Attachment #8444237 - Flags: review?(cviecco) → review+
Attachment #8444250 - Flags: review?(cviecco) → review+
Posted patch Oops!Splinter Review
Attachment #8446054 - Flags: review?(cviecco)
Comment on attachment 8446054 [details] [diff] [review]
Oops!

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

Good find.
Attachment #8446054 - Flags: review?(cviecco) → review+
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.
Attachment #8448399 - Flags: review?(cviecco)
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!
Attachment #8448457 - Flags: review?(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?
Attachment #8448399 - Flags: review?(cviecco) → review+
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.
Flags: needinfo?(brian)
(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.
Flags: needinfo?(brian)
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.
Attachment #8449549 - Flags: review?(brian)
Attachment #8449549 - Flags: review?(brian) → review+
Attachment #8448457 - Flags: review?(cviecco) → review+
https://hg.mozilla.org/mozilla-central/rev/d2e9bbf6fa8f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1034591
You need to log in before you can comment on or make changes to this bug.