398693 - DER_AsciiToTime produces incorrect output for dates 1950-1970

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Binary DER cert with expiration date in 1955

Attached is a certificate with an expiration date in 1955.  
The validity dates in the certificate are:
       UTCTime '041215145453Z'   <- 2004-12-15 14:54:53 GMT
       UTCTime '550315145453Z'   <- 1955-03-15 14:54:53 GMT

But the NSS pp program, in all presently released versions of NSS, 
displays those dates as:
            Not Before: Wed Dec 15 14:54:53 2004
            Not After : Wed Mar 16 14:54:53 1955   (wrong date)

The date of the month is off by 1.  
I believe this happens in dates in the range of 1950-1969.

I have written a patch to DER_AsciiToTime that fixes this, and is more
provably correct than the present code (IMO).  It produces this output:
            Not Before: Wed Dec 15 14:54:53 2004
            Not After : Tue Mar 15 14:54:53 1955

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

I think this patch is a little more "obviously" correct,
and the results speak for themselves. 

This patch applies to the trunk.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 283692 [details] [diff] [review]
patch v1

Wan-Teh, please review this small patch.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Of course, I see one comment I didn't change, it should be changed as:
-     /* Compute the number of seconds in the years elapsed since 1970 */
+     /* Compute the number of seconds in the years elapsed since 1950 */

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 283692 [details] [diff] [review]
patch v1

requesting second review, for branch.

Comment 5

[Wan-Teh Chang]

Comment on attachment 283692 [details] [diff] [review]
patch v1

If year is the number of elapsed years since January 1, 1950, the
calculation below is wrong:

>     if (((year % 4) == 0) && (month < 3)) {
> 	days--;
>     }

Please try your patch on the date 1952-02-01 (a leap year, and the
month is before March).

I believe you can fix this bug with just this change:

-    days += (year - 68)/4;
+    days += year/4 - 68/4;

Comment 6

[Wan-Teh Chang]

It's clearer to write that line as:

-    days += (year - 68)/4;
+    days += year/4 - 70/4;

Comment 7

[Wan-Teh Chang]

We should rewrite this function to parse the input string into
a PRExplodedTime structure, and then call PR_ImplodeTime to
get the output.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 283692 [details] [diff] [review]
patch v1

(In reply to comment #5)

> >     /* compute number of elapsed leap days since January 1, 1950 */
> >     days += (year + 2)/4;  
> If year is the number of elapsed years since January 1, 1950, the
> calculation below is wrong:
> >     if (((year % 4) == 0) && (month < 3)) {
> > 	    days--;
> >     }

You're right.  I could fix that by adding 2 to the year, as I do on
the previous line, but I could also rewrite this entire function by 
prepending either "19" or "20" to the entire input string, and then
calling DER_GeneralizedTimeToTime, which calls PR_ImplodeTime.
I think I'll do that.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - rewrite

This patch rewrites DER_AsciiToTime.  
The function now converts the UTCTime into a GeneralizedTime by 
prepending either "19" or "20", and then calls DER_GeneralizedTimeToTime 
to do the actual work.   It also moves some code from DER_UTCTimeToTime 
to DER_AsciiToTime, to avoid duplicating that code. 

Bob & Wan-Teh, please review.

Comment 10

[Wan-Teh Chang]

Comment on attachment 283834 [details] [diff] [review]
patch v2 - rewrite

You should move the comment:
     /*
     ** Maximum valid UTCTime is yymmddhhmmss+0000 which is 17 bytes.
     ** 20 should be large enough for all valid encoded times. 
     */
to DER_AsciiToTime to explain the size of localBuf.

You didn't copy the memset call, which ensures localBuf is null terminated
for the PORT_Strlen(localBuf) call later to succeed.  Also,

>+    PORT_Strncpy(localBuf+2, string, 17);

may read beyond the end of the 'string' buffer because 'string' doesn't
need to be null terminated.  Consider an input of 11-byte yymmddhhmmZ,
not null terminated, with nonzero bytes after the 'Z'.

I like this patch but it still needs work.  Sorry.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 283834 [details] [diff] [review]
patch v2 - rewrite

Wan-Teh, localBuf IS guaranteed to be NUL terminated.

+    PORT_Strncpy(localBuf+2, string, 17);
+    localBuf[19] = '\0';

However, I agree that there is a problem with the patch to
DER_UTCTimeToTime, so I will submit another patch.

Comment 12

[Wan-Teh Chang]

Comment on attachment 283834 [details] [diff] [review]
patch v2 - rewrite

Please ignore my comment about not copying the memset call.
The only problem is the PORT_Strncpy call.

Your change to DER_UTCTimeToTime may introduce subtle changes
because it loses the zero padding for time->len < sizeof localBuf.
This could also allow DER_AsciiToTime to read beyond the end
of the time->data buffer, for example, if time->data is
"0710060800+1" and time->len is 12, the statement
    CAPTURE(hourOff,string+11,loser);
in DER_AsciiToTime (original version) will read one byte beyond
the end of buffer.  (In your version the PORT_Strncpy call will
do that.)

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: even bigger rewrite

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284255 [details] [diff] [review]
even bigger rewrite

This rewrite combines the code for parsing both UTCTime and 
GeneralizedTime into a single function, which calls PR_ImplodeTime.
I believe it preserves all the semantics of the 3 rewritten functions:
DER_AsciiToTime, DER_UTCTimeToTime, and DER_GeneralizedTimeToTime
including the bizarre rule rule that the input string for 
DER_AsciiToTime need not be NUL terminated as long as it contains a
valid UTCTime.

Comment 15

[Wan-Teh Chang]

Comment on attachment 284255 [details] [diff] [review]
even bigger rewrite

r=wtc.

>+    p += 2; \

It'd be better to put the macro parameter p in parentheses.

>+    while (len < sizeof localBuf) {
>+        localBuf[len++] = '\0';

We need only one null byte after the copy of time->data in localBuf.
So you can replace while by if.

>+    if (string == NULL || dst == NULL) {
>+	PORT_SetError(SEC_ERROR_INVALID_ARGS);
>+	return SECFailure;
>+    }

For maximum backward compatibility, if string is NULL, we
should go to loser to set the SEC_ERROR_INVALID_TIME error code.

Would be nice to document that der_TimeStringToTime reads at most
17+generalized bytes, or until the first null byte,  from string.

Comment 16

[Julien Pierre]

Comment on attachment 284255 [details] [diff] [review]
even bigger rewrite

This looks OK except for a few nits, in order of those I care the most about to least :

1) In DER_UTCTimeToTime, there is code that attempts to make a copy and NULL-terminate the input. But it will fail if time->len is 20 (the size of localBuf).

This is not fatal because the subsequent code will not read more than 20 bytes even if the string is not NULL terminated. But it would be nice to fix still.

The problem is :

len = PR_MIN(time->len, sizeof localBuf);

In that case, the 20 byte string will be copied, but not terminated.

The second argument to PR_MIN should probably simply be sizeof localBuf -1 .

The same problem exists in DER_GeneralizedTimeToTime.

2) Since the 2 functions appear to be identical except for the values 11/13 and UTC_STRING / GEN_STRING, I think there is an opportunity to move the remaining common code to der_TimeStringToTime.

3) The CAPTURE macro does not check boundaries of the string it extracts characters from. It appears the logic is OK because der_TimeStringToTime will terminate if CAPTURE encounters non-digits, and the string is always NULL-terminated, except for the case I mentioned earlier, in which the code doesn't try to read all the way to the end of the string.

Comment 17

[Wan-Teh Chang]

Julien,

The string in localBuf doesn't need to be null-terminated if it
is 17+generalized bytes or longer, where generalized is 0 or 2.
The original code doesn't truncate long input.  Nelson's code
truncates long input to 20 bytes.  Since we're truncating, we
might as well truncate to 19 bytes so we can always null terminate
localBuf.

You're right that the CAPTURE macro and the der_TimeStringToTime
function work together to prevent us from reading beyond the
boundary of the string.  This is not obvious, which is why I
suggested at the end of comnment 14 to document this with a comment.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Yeah, the old DER_AsciiToTime function and the new der_TimeStringToTime 
function don't need or care about trailing NUL characters, as such.
They don't do strcpy or strlen or other string operations that depend on 
trailing NULs.  

They parse until they get a complete time string, or until they come to a 
character that is not one of the 13 valid characters allowed in a time 
string (which are "01234567489+-Z".  

The new function will never attempt to parse more than 19 characters of
input, which is why there's no need to add a trailing pad character to 
the string if it's maximum length.  I agree with Wan-Teh that the buffer
could have been 19 characters instead of 20.  20 just seemed like a nice
round number.  :)

Padding is only needed in the case where the input SECItem buffer is 
shorter than the maximum length time string.  When we copy it into the 
buffer, a valid time string character past the end of the original string 
might make the resultant string look valid even if the original was not.  
So, if the original string was shorter than maximum length, we want to 
ensure that the first character after the original string is a character 
not from the set of valid time string characters.  A trailing blank or 
comma would do just as well as a trailing NUL.  

There certainly could be more comments in the code.  This entire comment, 
for example, could go in there.  Maybe it's best to put in a URL that 
refers to this bug instead. ?

Comment 19

[Julien Pierre]

Wan-Teh,

I am aware that the string doesn't really need to be NULL-terminated except if it's short. But I had to figure this out for myself by reading the code. It was not obvious that it was OK not to NULL-terminate when the string is 20 bytes or longer.

Nelson,

Some extra comments would be useful. I think they should be in the source, not a bugzilla URL, to improve readability.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

I extended the comment for the DER_AsciiToTime function and committed the fix
on the trunk only.

Checking in util/dertime.c;
new revision: 1.10; previous revision: 1.9