12.99 KB, patch
|Details | Diff | Splinter Review|
1.49 KB, patch
|Details | Diff | Splinter Review|
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.
Wan-Teh, Is this really an exploitable vulnerability? If not for FF and TB, then for what products? How might one exploit it?
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.
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.
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.
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 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.
Attachment #364993 - Flags: review?(nelson) → review+
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
Attachment #364993 - Attachment is obsolete: true
(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.
Thanks, Wan-Teh. Perhaps we should report this to MS?
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.
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).
Attachment #365761 - Flags: review?(nelson)
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?
Attachment #365761 - Flags: review?(nelson) → review+
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 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.
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 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;
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
Attachment #365761 - Attachment is obsolete: true
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?
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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: 220.127.116.11; previous revision: 3.35 done Checking in pr/tests/Makefile.in; /cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v <-- Makefile.in new revision: 18.104.22.168; previous revision: 1.55 done Checking in pr/tests/parsetm.c; /cvsroot/mozilla/nsprpub/pr/tests/parsetm.c,v <-- parsetm.c new revision: 22.214.171.124; previous revision: 126.96.36.199 done Checking in pr/tests/runtests.pl; /cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v <-- runtests.pl new revision: 188.8.131.52; previous revision: 1.3 done Checking in pr/tests/runtests.sh; /cvsroot/mozilla/nsprpub/pr/tests/runtests.sh,v <-- runtests.sh new revision: 184.108.40.206; previous revision: 1.6 done
Target Milestone: 4.8 → 4.7.4
You need to log in before you can comment on or make changes to this bug.