Closed Bug 247896 Opened 17 years ago Closed 15 years ago

PR_ParseTimeString use PR_Assert and it calls abort on invalid date

Categories

(NSPR :: NSPR, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: megath, Assigned: wtc)

References

Details

(Keywords: fixed1.8.1, testcase)

Attachments

(4 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040618 MultiZilla/1.6.2.1d
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040618 MultiZilla/1.6.2.1d

PR_ParseTimeString use PR_Assert and it calls abort on invalid date instead of
return PR_FAILURE or smth.

Reproducible: Always
Steps to Reproduce:
1. my chatzilla crashes with mozilla on certain broken irc servers
2.
3.

Actual Results:  
PR_ParseTimeString called abort(); and mozilla/chatzilla exits.
this patch seems to solve problem.
sorry, i forgot about versions.

mozilla-1.7, earlier versions of mozilla did not have this problem.
patch on mozilla/nsprpub/pr/src/misc/prtime.c
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 151346 [details] [diff] [review]
also I guess asserting invalid time zone isnt good idea. patch v2 :)

>                 default:
>-                  PR_ASSERT (0);
>+                  return PR_FAILURE;
this break is now unreachable:
>                   break;
>                 }

>-          PR_ASSERT(tm.tm_month > -1 
>+          if (!(tm.tm_month > -1 
>                                    && tm.tm_mday > 0 
>                                    && tm.tm_hour > -1
>                                    && tm.tm_min > -1
>-                                   && tm.tm_sec > -1);
>+                                   && tm.tm_sec > -1)) return PR_FAILURE;
personally i'd put the return on its own line.
Attachment #151346 - Flags: review?(wchang0222)
This sounds serious.  We call PR_ParseTimeString in Necko to parse HTTP date
valued headers.  Couldn't this be used by evil websites to crash Mozilla? :-(
Flags: blocking1.7.1?
1.7.5 has shipped. Moving request to 1.7.6.
Flags: blocking1.7.5? → blocking1.7.6?
It seems to me the problem exists only in debug builds, and therefore isn't
exploitable . But I agree that we should not call PR_Assert on invalid input
data; this should be reserved for supposedly impossible conditions (i.e. bug
detection).
debug only problem doesn't block 1.7.6 --drivers
Flags: blocking1.7.6? → blocking1.7.6-
Target Milestone: --- → 4.6
Comment on attachment 151346 [details] [diff] [review]
also I guess asserting invalid time zone isnt good idea. patch v2 :)

>--- prtime.old	2004-03-10 04:39:15.000000000 +0600
>+++ prtime.c	2004-06-21 16:03:53.439540832 +0700
>@@ -1506,7 +1506,7 @@
>                 case TT_EET: zone_offset =  2 * 60; break;
>                 case TT_JST: zone_offset =  9 * 60; break;
>                 default:
>-                  PR_ASSERT (0);
>+                  return PR_FAILURE;
>                   break;
>                 }

This assertion should not be removed.  This assertion
detects a bug in the code rather than invalid input
time string.

The other assertion seems to detect invalid input time
string.  If so, it should be replaced by an if statement
with PR_FAILURE return.
Attachment #151346 - Flags: review?(wtchang) → review-
*** Bug 326557 has been marked as a duplicate of this bug. ***
I just encountered this problem on 1.5 Thunderbird non-debug optimised build on Linux.

A mail message was crashing it and preventing me from using mail. I had to remove the message off the server via telnet. So this is pretty much a DoS.

The message contained this header:

Date: Wed, 15 Feb 2006 22: 8: 6  0000
Thunderbird 1.5 (20060207)
Gentoo
linux-2.6.11-mm1
POP3 mail fetch.

(gdb) run /usr/lib/mozilla-thunderbird/thunderbird-bin
Starting program: /usr/lib/mozilla-thunderbird/thunderbird-bin /usr/lib/mozilla-thunderbird/thunderbird-bin
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
[Thread debugging using libthread_db enabled]
[New Thread -1221097248 (LWP 16445)]
[New Thread -1223251024 (LWP 16454)]
[New Thread -1241048144 (LWP 16455)]
[New Thread -1260000336 (LWP 16456)]
[New Thread -1272276048 (LWP 16462)]
[New Thread -1282049104 (LWP 16466)]
Assertion failure: tm.tm_month > -1 && tm.tm_mday > 0 && tm.tm_hour > -1 && tm.tm_min > -1 && tm.tm_sec > -1, at ../../../../mozilla/nsprpub/pr/src/misc/prtime.c:1558

Program received signal SIGABRT, Aborted.
[Switching to Thread -1221097248 (LWP 16445)]
0xffffe410 in __kernel_vsyscall ()

(gdb) bt
#0  0xffffe410 in __kernel_vsyscall ()
#1  0xb7508d4d in raise () from /lib/tls/libc.so.6
#2  0xb750a363 in abort () from /lib/tls/libc.so.6
#3  0xb7df2e55 in PR_Assert () from /usr/lib/nspr/libnspr4.so
#4  0xb75f24a0 in _IO_list_all () from /lib/tls/libc.so.6
#5  0xb7e15cf0 in ?? () from /usr/lib/nspr/libnspr4.so
#6  0xb7e19600 in ?? () from /usr/lib/nspr/libnspr4.so
#7  0xb7e195cc in ?? () from /usr/lib/nspr/libnspr4.so
#8  0x00000616 in ?? ()
#9  0xb7f72210 in _dl_runtime_resolve () from /lib/ld-linux.so.2
#10 0xb7e068e8 in PR_ParseTimeString () from /usr/lib/nspr/libnspr4.so
#11 0x000007d6 in ?? ()
#12 0x089e6838 in ?? ()
#13 0x08978968 in ?? ()
#14 0x30b1bc58 in ?? ()
#15 0x08b1bc80 in ?? ()
#16 0x088ec2f0 in ?? ()
#17 0xbfd79728 in ?? ()
#18 0x089e5154 in ?? ()
#19 0xffffffff in ?? ()
#20 0x00000016 in ?? ()
#21 0x00000007 in ?? ()
#22 0xffffffff in ?? ()
#23 0xffffff68 in ?? ()
#24 0x00000016 in ?? ()
#25 0x0000000f in ?? ()
#26 0xffffffff in ?? ()
#27 0x00000000 in ?? ()


I'll attach the culprit message. Just openign the file in Thunderbird terminates it.
Attached file Thunderbird testcase (obsolete) —
Removed everything from the message leaving only the offending Date: header.
Thunderbird still asserts on it.
Attachment #212139 - Attachment is obsolete: true
Keywords: testcase
Doesn't crash Win32 Thunderbird.
I think I have the same problem here:
http://bugs.gentoo.org/show_bug.cgi?id=122164
personally, having rewritten some piece of this in bug 331216 (yes, i've reviewed the patch in this bug, so i really have no business not duping that bug here), i really don't like the patch here.
Depends on: 331216
Please try this patch and see if it fixes the assertion failure
for you.  See bug 331216 comment 13 for a description of this
patch.
*** Bug 331216 has been marked as a duplicate of this bug. ***
The reviews of this patch were done in bug 331216.

I checked in this patch on the NSPR trunk (NSPR 4.7),
NSPR_4_6_BRANCH (NSPR 4.6.2), NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla 1.9 alpha), and MOZILLA_1_8_BRANCH (Mozilla 1.8.1).
Attachment #217522 - Attachment is obsolete: true
Attachment #218076 - Flags: approval-branch-1.8.1+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Target Milestone: 4.6 → 4.6.2
Although I marked this bug fixed, I only fixed the assertion
failure in the debug build and made the parsed time valid
(but still incorrect) in the optimized build.

One could go further and make PR_ParseTimeString either
parse an incorrect input string like
"Wed, 15 Feb 2006 22: 8: 6  0000" correctly, or make
PR_ParseTimeString fail on such an incorrect input.

It is not hard to make PR_ParseTimeString allow spaces
in the HH:MM:SS part of a time string, but I am not sure
if we want to allow this kind of incorrect inputs.

It is not hard to make PR_ParseTimeString fail on this
kind of invalid inputs, but it is a lot more work to make
PR_ParseTimeString fail on all invalid inputs because
PR_ParseTimeString seems to have been written to be
permissive.  I suspect that most applications would
like PR_ParseTimeString to return an approximate time
rather than failing.

So, this is why I only went as far as fixing the assertion
failure.  If you think we should do any of these, please
open a new bug.
*** Bug 361670 has been marked as a duplicate of this bug. ***
*** Bug 364334 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.