480740 - When compiled by VC 2005, PR_ParseTimeString may crash on out-of-range time strings.

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Proposed patch with regression test

Note: This bug doesn't affect official Firefox releases because
they use a custom CRT mozcrt19.dll rather than the CRT msvcr80.dll
that comes with VC 2005.

If you compile with Visual C++ 2005 (8.0), and you pass an input
date on or after 3001-01-01 00:00:00 to libc time functions
(such as localtime or mktime), the invalid input handler is
invoked, causing the application to crash.  This has been reported
in these web pages:
http://securityvulns.com/advisories/year3000.asp
http://www.securiteam.com/windowsntfocus/5MP0D0UKKO.html
http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=266036

PR_ParseTimeString is affected by this problem when you
pass to it a time string without a time zone and
default_to_gmt=PR_FALSE.  Under that condition, PR_ParseTimeString
calls mktime().  If the input time string is out of range,
PR_ParseTimeString crashes inside the mktime() call.

To review the proposed patch:
- First verify that it is correct to move the PR_NormalizeTime
  call up.  We need the parsed time result to be normalized
  before we call mktime(), which simplifies the check for
  out-of-range dates.
- Then, review the check for out-of-range dates before calling
  mktime(), for Visual C++ 2005 or later only.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh,
Is this really an exploitable vulnerability?  
If not for FF and TB, then for what products?
How might one exploit it?

Comment 2

[Wan-Teh Chang]

These days application crashes caused by invalid external inputs
seem to be classified as denial of service attacks by default.
If the burden is on the bug owners to prove they are not
exploitable, it's much less work to just fix the crashes.

The date strings parsed by PR_ParseTimeString are used in many
text-based network protocols, for example the Date: email headers
and the expires attributes of HTTP cookies.  So the date strings
are often external inputs that may be invalid.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I think the security group policy is that mere DOS attacks are 
not considered "security sensitive" unless they are exploitable to cause
some greater damage sun as remote code execution, etc.

I have partially reviewed the patch, and am continuing.

Comment 4

[Wan-Teh Chang]

I found that mktime won't invoke the invalid parameter handler if the
date is far out of range (the year is > 3001).  Here is the relevant
source code from the _make__time64_t function in mktime64.c in the CRT.
_BASE_YEAR is 70 (year 1970), and _MAX_YEAR64 is 1100 (year 3000).

        /*
         * First, make sure tm_year is reasonably close to being in range.
         */
        if ( ((tmptm1 = tb->tm_year) < _BASE_YEAR - 1) || (tmptm1 > _MAX_YEAR64
          + 1) )
            goto err_mktime;
        ...
err_mktime:
        /*
         * All errors come to here
         */

        errno = EINVAL;
        return (__time64_t)(-1);

So our regression tests have to use dates in year 3001.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch with regression test v2

I also found that the magic maximum date is
23:59:59, December 31, 3000, US Pacific Time, not
23:59:59, December 31, 3000, UTC as documented
in MSDN.  (This mistake is not surprising because
Redmond is in the US Pacific Time Zone.)

This updated patch reflects the new findings.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 364703 [details] [diff] [review]
Proposed patch with regression test

r=nelson

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 364993 [details] [diff] [review]
Proposed patch with regression test v2

r=nelson, with some comments.

>+                   * mktime will return (time_t) -1 if the input is a date
>+                   * after 23:59:59, December 31, 3000, US Pacific Time (not
>+                   * UTC as documented): 

Is it always Pacific Time?  Or is it local time, whatever that is?

This version of the test program does not test the use of non-normalized 
date strings.  I think that non-normalized test was a good idea.

Comment 8

[Wan-Teh Chang]

Attachment: Proposed patch with regression test v3 (checked in)

The non-normalized date string I used only makes sense in the US
Pacific Time timezone.  Since I wanted the test program to work
in any timezone, I removed that date string.

But I found that I can set the TZ environment variable and then
call _tzset() to set the timezone for the test program, so that's
what I did in this patch.

I checked this in on the NSPR trunk (NSPR 4.8).

Checking in pr/src/misc/prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.37; previous revision: 3.36
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.58; previous revision: 1.57
done
RCS file: /cvsroot/mozilla/nsprpub/pr/tests/parsetm.c,v
done
Checking in pr/tests/parsetm.c;
/cvsroot/mozilla/nsprpub/pr/tests/parsetm.c,v  <--  parsetm.c
initial revision: 1.1
done
Checking in pr/tests/runtests.pl;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v  <--  runtests.pl
new revision: 1.4; previous revision: 1.3
done
Checking in pr/tests/runtests.sh;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.sh,v  <--  runtests.sh
new revision: 1.7; previous revision: 1.6
done

Comment 9

[Wan-Teh Chang]

(In reply to comment #7)
> Is it always Pacific Time?  Or is it local time, whatever that is?

Originally I thought it's local time.  The first patch was
written under that assumption.  Although local time is still
not the documented UTC, I thought that perhaps the input to
mktime doesn't have the timezone specification, so mktime
just did an approximate check.

Today I single-stepped the test program in the debugger and
found that mktime adds the timezone offset (for our timezone,
it's 28800 seconds) before comparing the date with the magic
maximum (_MAX__TIME64_T).  That made me wonder how 00:00:00
Jan 1, 3001 in our timezone (US Pacific Time) can possibly be
the first out-of-range date.  I did more experiments, including
changing the timezone of my computer, and confirmed that the
magic maximum should be 23:59:59, December 31, 3000, US Pacific
Time.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Wan-Teh.  Perhaps we should report this to MS?

Comment 11

[Wan-Teh Chang]

It's already reported to Microsoft by someone else.  See
the connect.microsoft.com URL in comment 0.  I'll add my
own findings there.

Comment 12

[Wan-Teh Chang]

Attachment: Limit workaround to Visual C++ 2005 only

I verified that the Visual C++ 2005 bug has been fixed in
Visual C++ 2008, so limit our workaround to Visual C++ 2005.

Added the actual _MAX__TIME64_T value to our test program
to show that it is incorrect (in US Pacfic Time rather than
UTC).

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 365761 [details] [diff] [review]
Limit workaround to Visual C++ 2005 only

r+ for the change to mozilla/nsprpub/pr/src/misc/prtime.c only.

As for the change to mozilla/nsprpub/pr/tests/parsetm.c, 
I have a bunch of questions.  

a) According to my copy of the file, it's src/crt/ctime.h,
and in it the relevant #define has the value 0x100000000000i64
which is not the same as 0x793406fffi64 * 1000000i64
So, I don't understand what this patch is really doing.

The comment sounds like it is defining max_time to be the "wrong"
value taken from the ctime.h source file.  
Is it actually trying to define the correct value?  
Should the comment say 
"The value in src\crt\ctime.h is wrong. This is the true value." ?
If you really are trying to use the value from the ctime.h header,
shouldn't you just use the symbol defined there?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

As an aside, if the comment in ctime.h is to be believed, they are saying
that the number of seconds (or is it microseconds?) from the beginning of
the year 1970 to the beginning of the year 3000 is exactly a power of two!
Somehow, I find that very hard to believe.

Comment 15

[Wan-Teh Chang]

Comment on attachment 365761 [details] [diff] [review]
Limit workaround to Visual C++ 2005 only

I'm trying to demonstrate that the actual value of _MAX__TIME64_T
in the CRT sources of Visual C++ 2005 and Visual C++ 2008 is
slightly different from the value documented in MSDN.  The MSDN
documented value is 23:59:59, December 31, 3000 UTC, but the
actual value in Visual C++ 2005 and 2008 CRT sources is
23:59:59, December 31, 3000 US Pacific Time.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

According to 
http://aspn.activestate.com/ASPN/Mail/Message/perl5-porters/3646205

> The CRT src headers include a ctime.h in which we have:
> 
> #define _MAX__TIME64_T     0x793406fffi64       /* number of seconds from
>                                                  00:00:00, 01/01/1970 UTC to
>                                                  23:59:59. 12/31/3000 UTC */

But in my copy of the MS PSDK, it is 

> #define _MAX__TIME64_T     0x100000000000i64    /* number of seconds from
>                                                  00:00:00, 01/01/1970 UTC to
>                                                  23:59:59. 12/31/2999 UTC */

So, that's why I was confused.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 365761 [details] [diff] [review]
Limit workaround to Visual C++ 2005 only

Wan-Teh, r+ for this whole patch. 
I'd suggest that you change one part of it (not required)

>+    /* Wrong _MAX__TIME64_T value from crt\src\ctime.h */
>+    PRTime max_time = 0x793406fffi64 * 1000000i64;

Change to 

>+    /* Wrong _MAX__TIME64_T value from src\crt\ctime.h */
>+    /* MS ctime.h declares it to be 0x793406fffi64     */
>+    PRTime max_time = _MAX__TIME64_T * 1000000i64;

Comment 18

[Wan-Teh Chang]

Attachment: Limit workaround to Visual C++ 2005 only (as checked in)

Thanks for the help, Nelson.  I'm abandoning my change
to the parsetm.c test.  It was only intended to illustrate
a discrepancy between MSDN docs and MSVCRT implementation.
(If I have time, I'll send a standalone program to Microsoft.)
I can't use _MAX__TIME64_T in our test program because
crt\src\ctime.h is an internal header file of MSVCRT.

I checked in the change to prtime.c on the NSPR trunk (NSPR 4.8).

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.39; previous revision: 3.38
done

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

ctime.h is part of the freely available MS "Platform SDK".
I thought we were already dependent on files from the psdk.
Am I mistaken about that?

Comment 20

[Wan-Teh Chang]

ctime.h is an internal header of the CRT.  It is in the
CRT's src directory.  An application can't include ctime.h,
just like an NSS-based app can't include sslimpl.h even
though sslimpl.h is in the NSS source tree.

Comment 21

[Wan-Teh Chang]

I backported the fix to the NSPR_4_7_BRANCH for NSPR 4.7.4.

Checking in pr/src/misc/prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.35.4.1; previous revision: 3.35
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.55.4.1; previous revision: 1.55
done
Checking in pr/tests/parsetm.c;
/cvsroot/mozilla/nsprpub/pr/tests/parsetm.c,v  <--  parsetm.c
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in pr/tests/runtests.pl;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v  <--  runtests.pl
new revision: 1.3.4.1; previous revision: 1.3
done
Checking in pr/tests/runtests.sh;
/cvsroot/mozilla/nsprpub/pr/tests/runtests.sh,v  <--  runtests.sh
new revision: 1.6.28.1; previous revision: 1.6
done