The default bug view has changed. See this FAQ.

Support GeneralizedTime option in certificate validity field

RESOLVED FIXED in 3.9

Status

NSS
Libraries
P2
enhancement
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Terry Hayes, Assigned: Julien Pierre)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cert])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

15 years ago
The validity value in X.509 certificates now supports a CHOICE of UTCTime or
GeneralizedTime for the notBefore and notAfter fields.  GeneralizedTime MUST be
used for dates beginning in 2050.

While most certificates will not contain dates beyond 2049, in the next few
years certificate authority (CA) certificates (especially roots) may be issued
with expiration dates that far in the future.

When processing certificates, NSS should accept the GeneralizedTime format for
all dates.  When generating certificates NSS should choise the GeneralizedTime
option for dates beginning in 2050.

See X.509 and IETF RFC 2459 for details.

Updated

15 years ago
Severity: normal → enhancement
Priority: -- → P1

Updated

15 years ago
Whiteboard: [cert]

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → 3.6

Comment 1

15 years ago
Moved to Future.  Will consider this for NSS 3.7.
Target Milestone: 3.6 → Future

Updated

15 years ago
Target Milestone: Future → 3.7

Updated

15 years ago
Target Milestone: 3.7 → Future
We really ought to outlaw the target milestone of "Future".  
This bug needs to be fixed NOW.  We're starting to see more and more certs
with GeneralizedTime dates and we really ought to handle them.  (IMO :-)
Priority: P1 → P2
Target Milestone: Future → 3.9

Comment 3

14 years ago
Let's implement GeneralizedTime support in NSS 3.9.
Julien, want to take a stab at this?

I worked on this a little bit during NSS 3.5
development.  That was when Terry and I discovered
some problems with our ASN.1 encoder and decoder's
handling of CHOICE.  The bugs have been fixed on
the trunk (see bug 161580).  The support of
GeneralizedTime requires correct handling of CHOICE.
Assignee: wtc → jpierre
Status: ASSIGNED → NEW
(Assignee)

Comment 4

14 years ago
Wan-Teh,

Does this change apply only to certificates or also to other objects, eg. CRLs ?

Regardless, it sounds like something I could work on. It would be useful to have
a list of functions that need to support this. I am assuming the CERT_*Validity
functions and the CERTValidity structure are the primary ones.

Comment 5

14 years ago
I suspect that this changes applies to CRLs as well,
but you should consult RFC 3280 to be sure.

You can start with the structure that represents
a decoded Validity, and look for the functions that
operate on that structure.

Comment 6

14 years ago
Thanks for looking at this problem, but until it is fixed could you please
change the error message.

I am writing a server application which creates it's own certs and I don't want
to think about the number of days I spent going through my code to try to figure
out why I got this message when I tried to connect :

"Error establishing an encrypted connection. Error code -8183"

Eventually I gave up and switched to running mozilla with gdb to track down the
problem was with the date being generalized not utc.

Please display a more meaningful message, or at least print some debug log line
to say what has happened.

Anyway, thanks again for looking at it.
Greg.
Greg,  This bug report is against the NSS libraries, which are used by the
mozilla browser and by numerous other software products.  The NSS libraries
provide NO UI (and NO error dialogs) for the browser.  
Complaints about the browser's error dialogs would be the subject of a bug
against PSM, not a bug against the NSS security libraries.  
I invite you to search bugzilla for PSM bugs about error messages and vote
for them, or better yet, contribute code to fix them.
(Assignee)

Comment 8

14 years ago
Actually, RFC3280 says the following :

   CAs conforming to this profile MUST always encode certificate
   validity dates through the year 2049 as UTCTime; certificate validity
   dates in 2050 or later MUST be encoded as GeneralizedTime.

Therefore the problem only exists for dates after 2049. The RFC does not allow
dates before 2050 to be encoded as GeneralizedTime.
The problem is only relevant if a certificate expires after 2050 .
(Reporter)

Comment 9

14 years ago
It's true that RFC3280 says that the GeneralizedTime format must be used only 
for dates beginning in 2050.  This is to allow compatibility with existing 
implementations (like NSS) that do not yet support the format.

However, there is no reason for an implementation that supports 
GeneralizedTime to reject a certificate (or other object) that contains dates 
before 2050 that are encoded using that format.

Dates generated by NSS should use the RFC guideline to ensure interoperability 
with other RFC3280-conformant implementations.
(Assignee)

Comment 10

14 years ago
Terry,

I agree that when we support GeneralizedTime in NSS, we should have no
restriction on dates.
 
I only made this quote from RFC3280 to shed some light on the urgency of making
this change to NSS. People have been using certs with GeneralizedTime validity
format which didn't decode in mozilla, and that was one reason that we wanted to
implement this support.

I'm guessing that the problematic certs we have encountered so far were either :

1) not following the RFC3280 guidelines, ie. they had dates before 2049 in
GeneralizedTime format.

2) dummy self-signed rootswith 100 year validity date, generated with OpenSSL by
some wannabe CA administrator in their bedroom

Perhaps I am wrong and there are already valid reasons in 2003 to issue certs
expiring so long in the future, but even the trusted roots aren't going as far
into the future yet. Actually, the longest expiring root I can find is the AOL
TW root, and it expires in 2037, so perhaps this problem was part of the reason
for limiting the expiration date .

Regardless, this will get fixed in 3.9 .
(Assignee)

Comment 11

14 years ago
More findings about GeneralizedTime :

RFC3280 also specifies the use of GeneralizedTime for CRLs thisUpdate and
nextUpdate fields, in the same way it does for certificates' notBefore and
notAfter fields . It is also legal for the revocation date in CRLs.

To implement this for certificates, I need to modify the CERTValidityStr
structure. Unfortunately, that is included in the CERTCertificate as a struct
member. I would break binary compatibility by doing this change.
To preserve binary compatibility, I will need to add a new validity structure at
the end of CERTCertificate. The old one structure would have to stay as a
placeholder.

Another way would be to do away with the entire CERT_ValidityTemplate, add the
new fields at the end of the CERTCertificate structure, and do the decoding in
the CERTCertificate's main template instead of subtemplate. I am leaning towards
that option, because the whole "validity structure" doesn't actually exist, it
is just a representation of the 2 fields in the certificate.
I think you should be able to keep binary compatibility by placing the choice
discriminant in the type member of the secitem for each of the time values.
(Assignee)

Comment 13

14 years ago
Do we have any certs encoded with GeneralizedTime for testing purposes ?
(Assignee)

Comment 14

14 years ago
I have written some code that allows decoding of CERTCertificate by NSS of
either the UTC or GeneralizedTime. I used the SECItem type field for the choice
result, as suggested by Nelson.

However, by quickly reviewing the various CERTValidity* manipulation functions
in util/sectime.c, I find that many of these functions appear to be broken.

For instance :
- CERT_CreateValidity calls DER_TimeToUTCTime, which allocates memory on the
main heap, rather than in the CERTValidity's arena . 

- CERT_DestroyValidity does not attempt to free the memory in notBefore.data and
notAfter.data ; it only destroys the CERTValidity's arena, thus leaking memory .

- CERT_CopyValidity first tries to destroy the destination validity structure
before doing the copy and assigning fields into it ! In the only actual usage of
this function, in CERT_CreateCertificate, the target validity structure does not
have an arena, so it cannot get freed, and this bug does not actually manifest
itself.

In addition to fixing the above bugs, I have identified the following remaining
tasks :

- fix CERT_CreateValidity so that it encodes the GeneralizedTime type for dates
past 2049 .

- fix the softoken cert validity decoding. Softoken uses its own "hand" decoder
called nsslowcert . Currently it only supports UTC time.
(Assignee)

Comment 15

14 years ago
Created attachment 130946 [details] [diff] [review]
preliminary patch - decode either UTCTime or GeneralizedTime in validity fields of CERTCertificate
(Assignee)

Comment 16

14 years ago
Looks like the attached patch breaks the NSS tests . I still got some work to do.

Also, I need to add the support for GeneralizedTime in CRLs.
(Assignee)

Comment 17

14 years ago
I finally found out why this patch was breaking the tests. The same template
gets used for decoding and encoding. The encoder was failing on the CHOICE,
because I hadn't modified the cert encoding function to assign the type field of
the SECItem to match one of the IDs in the template. Therefore, it didn't know
whether to use the  component type of UTC or Generalized when encoding.

I have made a patch that passes the NSS tests.
It works for both encoding and decoding.

The only thing missing still is the nsslowcert support in softoken, and the CRL
support (which should probably be a separate bug from this one, since this is
about certificates).

I was able to run the tests in year 2050, by setting my machine to that date.
Presumably the CA certs are being created with the GeneralizedTime encoding in
this case, though I had no way to verify it. Apparently, the fact that
nsslowcert doesn't understand this format is not a problem. The only problem I
saw in the test is that gmake rolls over to 1969 when I set my computer's date
to 2050, and thinks my NSS built in 2003 is "in the future".
(Assignee)

Comment 18

14 years ago
Created attachment 131114 [details] [diff] [review]
updated patch to fix both encoder and decoder
(Assignee)

Updated

14 years ago
Attachment #130946 - Attachment is obsolete: true
(Assignee)

Comment 19

14 years ago
Actually, with my patch, there were still three failures when setting the
computer to year 2050 :

Signing a set of files (signtool -Z) 	Failed
Listing signed files in jar (signtool -v) 	Failed
Show who signed jar (signtool -w)

However, if I run the tip of NSS (no patch) in year 2050, I get far more
failures : every SSL test fails, except the negative ones. Half the S/MIME tests
do as well. 2 cert verification tests fail, including one of the FIPS tests. The
three test tool failures mentioned above also occur .
All those failures except the last 3 tool failures are fixed by my patch in 2050.

Comment 20

14 years ago
Interesting test results.

We need a way to test your new code without setting the
system clock to 2050 for two reasons.  1. We can't change
the system clock during our normal QA testing.  2. Setting
the system clock to 2050 will test more than the
support of GeneralizedTime.  The test failures may
be caused by the 32-bit time_t overflow rather than
the lack of support for GeneralizedTime.
As you probably know, certutil's commands to create a cert take two optional
arguments:
   -w warp-months      (default 0)
   -v validity-months  (default 0)

The notBefore date is "now" + "warp-months" months.
The notAfter  date is the notBefore date + ("validity-months" + 3) months.

So, by using a value of, say, 600 (12*50) validity months in the QA scripts,
it should be possible to ensure that we generate certs with both UTCTime and
GeneralizedTime.
(Assignee)

Comment 22

14 years ago
Created attachment 131154 [details] [diff] [review]
changes for the softoken lowcert layer
(Assignee)

Updated

14 years ago
Attachment #131114 - Flags: superreview?(wchang0222)
Attachment #131114 - Flags: review?(relyea)
(Assignee)

Updated

14 years ago
Attachment #131154 - Flags: superreview?(wchang0222)
Attachment #131154 - Flags: review?(relyea)
(Assignee)

Comment 23

14 years ago
Bob, Wan-Teh,

The patches to add support for GeneralizedTime on certificates are complete.
Please review them. I will open a separate bug for the CRL support.

Comment 24

14 years ago
Comment on attachment 131114 [details] [diff] [review]
updated patch to fix both encoder and decoder

This patch looks good.	I just wanted to suggest some changes.

The 'type' field of a SECItem structure has the enumeration
type SECItemType, so you should add new enumeration constants
(siGeneralizedTime and siUTCTime) to SECItemType rather than
using SEC_ASN1_GENERALIZED_TIME and SEC_ASN1_UTC_TIME.

>+int DecodeValidityTime(PRTime* output, SECItem* input)

This function should return SECStatus.

> SECStatus
> CERT_GetCertTimes(CERTCertificate *c, PRTime *notBefore, PRTime *notAfter)
> {
>     int rv;
>+
>+    if (!c || !notBefore || !notAfter)
>+        return SECFailure;

We should set an error code before returning SECFailure.
(BTW, 'rv' should have the SECStatus type.)

CERT_TimeChoice should be renamed CERT_TimeChoiceTemplate.

>+SECStatus EncodeValidityTime(SECItem* output, PRTime input, PRArenaPool* arena)
>+{
...
>+    if (printableTime.tm_year>=2050) {
>+        rv = DER_TimeToGeneralizedTimeArena(output, input, arena);
>+        output->type = SEC_ASN1_GENERALIZED_TIME;
>+        return rv;
>+    } else {
>+        rv = DER_TimeToUTCTimeArena(output, input, arena);
>+        output->type = SEC_ASN1_UTC_TIME;
>+        return rv;
>+    }

Should we set output->type in the DER_TimeToXXXTimeArena functions?
Attachment #131114 - Flags: superreview?(wchang0222)
Attachment #131114 - Flags: superreview-
Attachment #131114 - Flags: review?(relyea)

Updated

14 years ago
Attachment #131154 - Flags: superreview?(wchang0222) → superreview+

Comment 25

14 years ago
Comment on attachment 131114 [details] [diff] [review]
updated patch to fix both encoder and decoder

Some questions about the first patch.

>+int DecodeValidityTime(PRTime* output, SECItem* input)
>+{
>+    switch (input->type) {
>+        case SEC_ASN1_GENERALIZED_TIME:
>+            return DER_GeneralizedTimeToTime(output, input);
>+
>+        case SEC_ASN1_UTC_TIME:
>+        default:
>+            return DER_UTCTimeToTime(output, input);
>+    }
>+}

If input->type is neither GenerailzedTime nor UTCTime, we treat
it as UTCTime.	Why?  Why doesn't this function fail in that case?

I suspect it's because some existing code doesn't set input->type.
Can we determine the type of input by its length?

>+extern SECStatus DER_TimeToUTCTimeArena(SECItem *dst, int64 gmttime,
>+                                        PRArenaPool* arenaOpt);
>+

>+extern SECStatus DER_TimeToGeneralizedTimeArena(SECItem *dst, int64 gmttime,
>+                                                PRArenaPool* arenaOpt);

It is more consistent with existing functions to have the PRArenaPool*
as the first argument.

Declare DecodeValidityTime and EncodeValidityTime as static.
(Assignee)

Comment 26

14 years ago
Comment on attachment 131154 [details] [diff] [review]
changes for the softoken lowcert layer

Nelson pointed out that there are some ambiguous cases where a UTCtime value
cannot be distinguished from a GeneralizedTime . This is because seconds are 
optional.
The following value for example :
20040101010101Z
Can mean either january 1st 2004 in GeneralizedTime, or April 1st 2020 in
UTCTime ...

So, we must distinguish them by ASN.1 component tag.

The hand decoder built into the softokn does not check any component tag value,
unfortunately.
Therefore, this patch must be reworked.
(Assignee)

Comment 27

14 years ago
Created attachment 131367 [details] [diff] [review]
updated patch
Attachment #131114 - Attachment is obsolete: true
Attachment #131154 - Attachment is obsolete: true
(Assignee)

Comment 28

14 years ago
Comment on attachment 131367 [details] [diff] [review]
updated patch

All the NSS tests pass with these patches at the current date.
The changes include:
- adding and using siUtcTime and siGeneralizedTime to SECItemType

- using SECStatus instead of int for return types

- moving some functions areound to util

- renaming of the time choice template to CERT_TimeChoiceTemplate

- addition of CERT_InlineTimeChoiceTemplate (needed for some of my other
forthcoming patches)

- setting of SECItem.type in the encoder functions

- asserting and failing if the SECItem.type isn't set, since we can't safely
guess the type with the value at decoding time

- reordering arena arguments

- in softoken, add an argument to return the ASN.1 tag so that the correct
decoding of the time can be done

- remove "UTC" from comments in several places where PR_Now() is called, since
this is somewhat ambiguous given the DER UTCtime issues (I was searching on
that for flaws)

- probably more ...
Attachment #131367 - Flags: superreview?(wchang0222)
Attachment #131367 - Flags: review?(nelsonb)
(Assignee)

Comment 29

14 years ago
I forgot to mention this patch also includes the fix for CRLs. I didn't touch
the KRLs.

Wan-Teh, Nelson, I know this is a very long patch, but I would appreciate a review.
(Assignee)

Comment 30

14 years ago
Created attachment 131637 [details] [diff] [review]
incorporate Nelson's feedback

- fix bug in lowcert.c where notBefore was used where notAfter was meant
- use dst->len in lowcert.c rather than repeating constants for memory
allocation
- use PRTime instead of int64 for new code in sectime and dertime
- optimize the year 2050 by adding a constant PRTime to compare against,
instead of calling PR_ExplodeTime
(Assignee)

Updated

14 years ago
Attachment #131367 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131637 - Flags: superreview?(MisterSSL)
Attachment #131637 - Flags: review?(wchang0222)
(Assignee)

Comment 31

14 years ago
Note: the stanpcertdb.c fix shouldn't go in because it changes the DB format by
allowing a GeneralizedTime to be saved for the S/MIME profile time. See bug
219527 for that issue.
(Assignee)

Updated

14 years ago
Blocks: 219524
(Assignee)

Comment 32

14 years ago
Created attachment 131643 [details] [diff] [review]
remove stanpcertdb.c from the patch

The change to stanpcertdb would have saved either a GeneralizedTime or UTCTime
S/MIME profile time, and we can't do that without breaking the database format
for backwards compatibility.
Attachment #131637 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131643 - Flags: superreview?(MisterSSL)
Attachment #131643 - Flags: review?(wchang0222)
(Assignee)

Comment 33

14 years ago
Created attachment 131654 [details] [diff] [review]
more fixes

Hopefully this is the last patch needed here .
The changes from the last patch are :
- revert from sizeof(SECItemType) to sizeof(SECItem) in the
CERT_TimeChoiceTemplate
- remove CERT_InlineTimeChoiceTemplate

Thanks to Nelson, I also included the changes for the test scripts to extend
the validity date of certs to 600 months, past year 2050.
Attachment #131643 - Attachment is obsolete: true
(Assignee)

Comment 34

14 years ago
Comment on attachment 131654 [details] [diff] [review]
more fixes

Note that all that the NSS tests now pass, both at current date and after 2050,
so we should be set as far as this feature goes, at least for everything
currently tested by the test suite.
Attachment #131654 - Flags: superreview?(MisterSSL)
Attachment #131654 - Flags: review?(wchang0222)
(Assignee)

Updated

14 years ago
Blocks: 218793

Comment 35

14 years ago
Comment on attachment 131654 [details] [diff] [review]
more fixes

This patch is very good.  Most of my comments are style or minor
issues.  The two biggies are:

1. In lib/nss/nss.def, the new functions need to be added to a
new NSS_3.9 section rather than the NSS_3.8 section.

2. If I understand it correctly, the changes to tests/cert/cert.sh
means we will only be testing GeneralizedTime and won't be testing
UTCTime in some cases.	We want to test both.  This may mean
repeating some of the tests for GeneralizedTime testing.

I will give you the rest of my comments offline.
Attachment #131654 - Flags: review?(wchang0222) → review-
(Assignee)

Comment 36

14 years ago
In response to comment #35, issue 2 :
The certs created by certutil will have a notBefore encoded in UTCTime and a
notAfter encoded in GeneralizedTime, so we will indeed be testing both encodings.
If we want to test both fields with the different encodings, we will have to
reset the system time after 2050. Setting notBefore as GeneralizedTime implies
that the notBefore is after 2050, and I believe some of the tests (SSL) can only
run at the current system date, so they would fail.
Some of the certs generated in the QA tests are generated by signtool. 
signtool always produces certs with a hard-coded 3 month validity period.  
So, presently, the certs produced by signtool will NOT test GeneralizedTime.
But I think GeneralizedTime will get adequate testing in other cases.

Apart from the nss.def issue, I think this patch is ready to checkin.  

Comment 38

14 years ago
We don't need to go to great length (such as setting the
system clock to Year 2050) to test a notBefore encoded
in GeneralizedTime, but we need to test a notAfter encoded
in UTCTime.
(Assignee)

Comment 39

14 years ago
One way to test certs with both notAfter in UTCTime and GeneralizedTime without
adding to the test suite would be to just have one of the certs (the CA) in the
existing cert.sh script use -v 60, and another one (the end-entity) use -v 600 . 

(Assignee)

Comment 40

14 years ago
Created attachment 131714 [details] [diff] [review]
as checked in . incorporates Wan-Teh's feedback

Changes are :
- fixing nss.def to use the 3.9 section
- renaming siUtcTime to siUTCTime .
- add a missing PORT_SetError in DER_TimeChoiceDayToAscii if SECItem type isn't
set

Checking in lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.55; previous revision: 1.54
done
Checking in lib/certdb/certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.24; previous revision: 1.23
done
Checking in lib/certdb/crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.37; previous revision: 1.36
done
Checking in lib/certhigh/certhtml.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhtml.c,v  <--  certhtml.c
new revision: 1.4; previous revision: 1.3
done
Checking in lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v	<--  certvfy.c
new revision: 1.35; previous revision: 1.34
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.111; previous revision: 1.110
done
Checking in lib/softoken/lowcert.c;
/cvsroot/mozilla/security/nss/lib/softoken/lowcert.c,v	<--  lowcert.c
new revision: 1.15; previous revision: 1.14
done
Checking in lib/util/dertime.c;
/cvsroot/mozilla/security/nss/lib/util/dertime.c,v  <--  dertime.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/util/seccomon.h;
/cvsroot/mozilla/security/nss/lib/util/seccomon.h,v  <--  seccomon.h
new revision: 1.4; previous revision: 1.3
done
Checking in lib/util/secder.h;
/cvsroot/mozilla/security/nss/lib/util/secder.h,v  <--	secder.h
new revision: 1.3; previous revision: 1.2
done
Checking in lib/util/sectime.c;
/cvsroot/mozilla/security/nss/lib/util/sectime.c,v  <--  sectime.c
new revision: 1.2; previous revision: 1.1
done
Checking in tests/cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.25; previous revision: 1.24
done
(Assignee)

Updated

14 years ago
Attachment #131654 - Attachment is obsolete: true
(Assignee)

Comment 41

14 years ago
Resolved.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Blocks: 219082

Updated

14 years ago
Attachment #131154 - Flags: review?(rrelyea0264) → review+
(Assignee)

Updated

14 years ago
Attachment #131637 - Flags: superreview?(MisterSSL)
Attachment #131637 - Flags: review?(wchang0222)
(Assignee)

Updated

14 years ago
Attachment #131367 - Flags: superreview?(wchang0222)
Attachment #131367 - Flags: review?(MisterSSL)
(Assignee)

Updated

14 years ago
Attachment #131643 - Flags: superreview?(MisterSSL)
Attachment #131643 - Flags: review?(wchang0222)
(Assignee)

Updated

14 years ago
Attachment #131654 - Flags: superreview?(MisterSSL)
You need to log in before you can comment on or make changes to this bug.