219539 - Support GeneralizedTime in NSS tools

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

Many NSS tools suffer from being hardcoded to UTC.

Comment 1

[Julien Pierre]

Attachment: Patch to fix several UTC vs GeneralizedTime issues

Comment 2

[Julien Pierre]

Taking bug

Comment 3

[Julien Pierre]

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.

Comment 4

[Wan-Teh Chang]

If we change the expiration date to 600 months from now,
doesn't that mean we will only be testing GeneralizedTime?

Comment 5

[Julien Pierre]

No. The notBefore field will be in UTCTime, and the notAfter field in
GeneralizedTime.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 7

[Julien Pierre]

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.

Comment 8

[Wan-Teh Chang]

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 9

[Wan-Teh Chang]

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

int
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);
	PORT_Free(timeString);
	return 0;
    }
    return SECFailure;
}

and replace sv_PrintUTCTime by this new function.
There is no need to add sv_PrintTimeByType and
sv_PrintGeneralizedTime.

Comment 10

[Julien Pierre]

Wan-Teh,

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.

Comment 11

[Julien Pierre]

Attachment: incorporate Wan-Teh's feedback

- remove patch for signtool that created certs valid for 600 months
- fix sv_PrintTime

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 13

[Julien Pierre]

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.

Comment 14

[Julien Pierre]

Checking in dbck/dbck.c;
/cvsroot/mozilla/security/nss/cmd/dbck/dbck.c,v  <--  dbck.c
new revision: 1.2; previous revision: 1.1
done
Checking in lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.49; previous revision: 1.48
done
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.12; previous revision: 1.11
done
Checking in p7content/p7content.c;
/cvsroot/mozilla/security/nss/cmd/p7content/p7content.c,v  <--  p7content.c
new revision: 1.9; previous revision: 1.8
done
Checking in signver/pk7print.c;
/cvsroot/mozilla/security/nss/cmd/signver/pk7print.c,v  <--  pk7print.c
new revision: 1.4; previous revision: 1.3
done

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 132171 [details] [diff] [review]
incorporate Wan-Teh's feedback

Looks good to me

Comment 16

[Wan-Teh Chang]

Comment on attachment 132171 [details] [diff] [review]
incorporate Wan-Teh's feedback

r=wtc.

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.

Comment 17

[Julien Pierre]

Wan-Teh,

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
done

Marking fixed.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

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. 

Comment 19

[Julien Pierre]

Attachment: patched remaining cases

I found two more instances of unpatched tools, in the signtool and sslstrength
programs. Please review

Comment 20

[Wan-Teh Chang]

Comment on attachment 139536 [details] [diff] [review]
patched remaining cases

r=wtc.

Comment 21

[Julien Pierre]

Checking in sslstrength/sslstrength.c;
/cvsroot/mozilla/security/nss/cmd/sslstrength/sslstrength.c,v  <--  sslstrength.
c
new revision: 1.8; previous revision: 1.7
done
Checking in signtool/list.c;
/cvsroot/mozilla/security/nss/cmd/signtool/list.c,v  <--  list.c
new revision: 1.7; previous revision: 1.6

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139536 [details] [diff] [review]
patched remaining cases

sr=MisterSSL