143334 - Support GeneralizedTime option in certificate validity field

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Terry Hayes]

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.

Comment 1

[Wan-Teh Chang]

Moved to Future.  Will consider this for NSS 3.7.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Julien Pierre]

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

[Wan-Teh Chang]

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

[Greg]

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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Julien Pierre]

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 .

Comment 9

[Terry Hayes]

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.

Comment 10

[Julien Pierre]

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 .

Comment 11

[Julien Pierre]

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.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 13

[Julien Pierre]

Do we have any certs encoded with GeneralizedTime for testing purposes ?

Comment 14

[Julien Pierre]

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.

Comment 15

[Julien Pierre]

Attachment: preliminary patch - decode either UTCTime or GeneralizedTime in validity fields of CERTCertificate

Comment 16

[Julien Pierre]

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.

Comment 17

[Julien Pierre]

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

Comment 18

[Julien Pierre]

Attachment: updated patch to fix both encoder and decoder

Comment 19

[Julien Pierre]

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

[Wan-Teh Chang]

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.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 22

[Julien Pierre]

Attachment: changes for the softoken lowcert layer

Comment 23

[Julien Pierre]

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

[Wan-Teh Chang]

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?

Comment 25

[Wan-Teh Chang]

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.

Comment 26

[Julien Pierre]

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.

Comment 27

[Julien Pierre]

Attachment: updated patch

Comment 28

[Julien Pierre]

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

Comment 29

[Julien Pierre]

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.

Comment 30

[Julien Pierre]

Attachment: 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

Comment 31

[Julien Pierre]

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.

Comment 32

[Julien Pierre]

Attachment: 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.

Comment 33

[Julien Pierre]

Attachment: 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.

Comment 34

[Julien Pierre]

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.

Comment 35

[Wan-Teh Chang]

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.

Comment 36

[Julien Pierre]

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.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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.

Comment 39

[Julien Pierre]

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 . 

Comment 40

[Julien Pierre]

Attachment: 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

Comment 41

[Julien Pierre]

Resolved.