Closed Bug 219539 Opened 21 years ago Closed 21 years ago

Support GeneralizedTime in NSS tools


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: julien.pierre, Assigned: julien.pierre)



(2 files, 1 obsolete file)

Many NSS tools suffer from being hardcoded to UTC.
Taking bug
Assignee: wchang0222 → jpierre
FYI, the patch to signtool to change the expiration date from 3 to 600 months is
to make the test pass. signtool can generate certs, and has a hardcoded validity
period of 3 months. The test suite currently uses that functionality.
Without the ability to set a validity period, I doubt anybody else uses signtool
to create certs.
I didn't want to add a parameter to signtool to set the validity date, since we
should be encouraging people to use certutil to create certs, not signtool.
If we change the expiration date to 600 months from now,
doesn't that mean we will only be testing GeneralizedTime?
No. The notBefore field will be in UTCTime, and the notAfter field in
Signtool is used, a lot, by mozilla developers, so I don't think it's 
appropriate to change it to always generate certs good for 50 years.  
The way it's typically used, they issue a new EE (leaf) cert for each new
JAR file that they sign, and since we check signature validity using the 
signing date, 3 months is more than enough validity period for their use.  

I am concerned about the code to generate and issue certs in signtool.  
It hasn't kept up with the cert generation abilities of certutil, AFAIK.
And I don't see us increasing our investment in signtool maintainence.

Maybe we ought to remove the cert generation ability from signtool and 
have signtool users use certutil to generate their certs, and then use the
certutil-generated certs/keys to sign with signtool.  That's what I'd 
suggest for our QA test scripts.  
Other than the signtool change from 3 to 600 months, are there objections to
this patch ?

I will open a separate bug to improve the test coverage for GeneralizedTime, in
the way of fixing the scripts to use certutil to generate the certs rather than
signtool, since we don't want to add configurability of the validity period for
signtool-created certs.
Attachment #131649 - Flags: superreview?(nelsonbhchan)
Attachment #131649 - Flags: review?(wchang0222)
Attachment #131649 - Flags: superreview?(nelsonbhchan) → superreview?(MisterSSL)
Comment on attachment 131649 [details] [diff] [review]
Patch to fix several UTC vs GeneralizedTime issues

1.  dbck/dbck.c

>+        /* needs to be able to handle GeneralizedTime too */
> 	DER_UTCTimeToTime(&timeBefore, &cert->validity.notBefore);
> 	DER_UTCTimeToTime(&timeAfter, &cert->validity.notAfter);

Why can't these be replaced by the new CERT_DecodeTimeChoice
function?  Is it because it requires a cert DB record format
change (bug 219527)?

2. signtool/certgen.c

>-  printableTime.tm_month += 3;
>+  printableTime.tm_month += 600;

I'll let Nelson or Bob review this change.

3. signver/pk7print.c

The new function sv_PrintTimeByType doesn't seem necessary.
First, its timetype argument seems redundant; it is the same
as t->type.  Second, I believe you can move the code of
sv_PrintTimeByType to sv_PrintTime, and modify sv_PrintUTCTime
and sv_PrintGeneralizedTime to call sv_PrintTime.
Comment on attachment 131649 [details] [diff] [review]
Patch to fix several UTC vs GeneralizedTime issues

I looked at the changes to signver/pk7print.c closer.
I think you should define a new function named sv_PrintTime
(or sv_PrintTimeChoice):

sv_PrintTime(FILE *out, SECItem *t, char *m)
    PRExplodedTime printableTime;
    int64 time;
    char *timeString;
    int rv;

    rv = CERT_DecodeTimeChoice(&time, t);
    if (rv) return rv;

    /* Converse to local time */
    PR_ExplodeTime(time, PR_GMTParameters, &printableTime);

    timeString = (char *)PORT_Alloc(100);

    if ( timeString ) {
	PR_FormatTime( timeString, 100, "%a %b %d %H:%M:%S %Y", &printableTime
;	fprintf(out, "%s%s\n", m, timeString);
	return 0;
    return SECFailure;

and replace sv_PrintUTCTime by this new function.
There is no need to add sv_PrintTimeByType and
Attachment #131649 - Flags: review?(wchang0222) → review-

About dbck.c , I only put a comment and didn't really fix the code, because the
program is not current buildable with NSS, so I didn't see a point in working on
it. I only added the comment because I was doing searches for "UTC" in the
entire NSS tree.
- remove patch for signtool that created certs valid for 600 months
- fix sv_PrintTime
Attachment #131649 - Attachment is obsolete: true
Attachment #132171 - Flags: review?(wchang0222)
This is a review comment about some code in function sv_PrintTime.
The code contains two lines that read thus:

    /* Converse to local time */
    PR_ExplodeTime(time, PR_GMTParameters, &printableTime);

IINM, the comment doesn't accurately describe the code's behavior.
PR_GMTParameters does not convert to local time. It leaves the time in GMT.    
PR_LocalTimeParameters converts to local time.

Comments should document the INTENDED behavior of the code.  If the code
doesn't match the comments, it should be fixed.  But I'm not sure whether
we really want the time shown in local time, or in GMT.

Which ever way we decide, we should make the code and comment agree.
The word Converse should be removed (if no conversion is done), or changed 
to "Convert" if the code is indeed changed to convert to local time.
Attachment #132171 - Flags: superreview?(MisterSSL)
That code wasn't change in my patch, only listed in the diff. I will change the
comment to say "convert" and make it use PR_LocalTimeParameters.
Checking in dbck/dbck.c;
/cvsroot/mozilla/security/nss/cmd/dbck/dbck.c,v  <--  dbck.c
new revision: 1.2; previous revision: 1.1
Checking in lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.49; previous revision: 1.48
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.12; previous revision: 1.11
Checking in p7content/p7content.c;
/cvsroot/mozilla/security/nss/cmd/p7content/p7content.c,v  <--  p7content.c
new revision: 1.9; previous revision: 1.8
Checking in signver/pk7print.c;
/cvsroot/mozilla/security/nss/cmd/signver/pk7print.c,v  <--  pk7print.c
new revision: 1.4; previous revision: 1.3
Comment on attachment 132171 [details] [diff] [review]
incorporate Wan-Teh's feedback

Looks good to me
Attachment #132171 - Flags: superreview?(MisterSSL) → superreview+
Comment on attachment 132171 [details] [diff] [review]
incorporate Wan-Teh's feedback


In dbck.c, you might as well just go ahead and change
the DER_UTCTimeToTime calls to CERT_DecodeTimeChoice calls.

In signver/pk7print.c, it is safer to just delete the comment
"Converse to local time" unless you are sure we should convert
to local time there.
Attachment #132171 - Flags: review?(wchang0222) → review+

I changed the comment and did the conversion because I thought it was
appropriate (there was another place in the code that did it).

I just made the change to dbck.c 

Checking in dbck.c;
/cvsroot/mozilla/security/nss/cmd/dbck/dbck.c,v  <--  dbck.c
new revision: 1.3; previous revision: 1.2

Marking fixed.
Closed: 21 years ago
Resolution: --- → FIXED
Attachment #131649 - Flags: superreview?(MisterSSL)
The patch above (which is checked in) added function SECU_PrintTimeChoice 
to cmd/lib/secutil.c, but did not convert any of the SECU_PrintUTCTime calls
in that file to call SECU_PrintTimeChoice instead.  Julien noticed this 
recently, while reviewing a patch for another bug.  I'm reopening this bug
because of that omission. 
Resolution: FIXED → ---
Target Milestone: --- → 3.9
Priority: -- → P2
I found two more instances of unpatched tools, in the signtool and sslstrength
programs. Please review
Attachment #139536 - Flags: superreview?(MisterSSL)
Attachment #139536 - Flags: review?(wchang0222)
Comment on attachment 139536 [details] [diff] [review]
patched remaining cases

Attachment #139536 - Flags: review?(wchang0222) → review+
Checking in sslstrength/sslstrength.c;
/cvsroot/mozilla/security/nss/cmd/sslstrength/sslstrength.c,v  <--  sslstrength.
new revision: 1.8; previous revision: 1.7
Checking in signtool/list.c;
/cvsroot/mozilla/security/nss/cmd/signtool/list.c,v  <--  list.c
new revision: 1.7; previous revision: 1.6
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Comment on attachment 139536 [details] [diff] [review]
patched remaining cases

Attachment #139536 - Flags: superreview?(MisterSSL) → superreview+
You need to log in before you can comment on or make changes to this bug.