PR_ParseTimeString use PR_Assert and it calls abort on invalid date

RESOLVED FIXED in 4.6.2

Status

defect
--
critical
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: megath, Assigned: wtc)

Tracking

({fixed1.8.1, testcase})

Dependency tree / graph
Bug Flags:
blocking1.7.6 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

15 years ago
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.
Reporter

Comment 1

15 years ago
this patch seems to solve problem.
Reporter

Comment 2

15 years ago
sorry, i forgot about versions.

mozilla-1.7, earlier versions of mozilla did not have this problem.
Reporter

Comment 3

15 years ago
patch on mozilla/nsprpub/pr/src/misc/prtime.c
Assignee

Updated

15 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 4

15 years ago
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)

Comment 5

15 years ago
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? :-(

Updated

15 years ago
Flags: blocking1.7.1?

Comment 6

15 years ago
1.7.5 has shipped. Moving request to 1.7.6.
Flags: blocking1.7.5? → blocking1.7.6?

Comment 7

15 years ago
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-
Assignee

Updated

15 years ago
Target Milestone: --- → 4.6
Assignee

Comment 9

15 years ago
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. ***

Comment 11

14 years ago
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

Comment 12

14 years ago
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.

Comment 13

14 years ago
Posted file Thunderbird testcase (obsolete) —

Comment 14

14 years ago
Removed everything from the message leaving only the offending Date: header.
Thunderbird still asserts on it.
Attachment #212139 - Attachment is obsolete: true

Updated

14 years ago
Keywords: testcase

Comment 15

14 years ago
Doesn't crash Win32 Thunderbird.

Comment 16

14 years ago
I think I have the same problem here:
http://bugs.gentoo.org/show_bug.cgi?id=122164

Comment 17

13 years ago
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
Assignee

Comment 18

13 years ago
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.
Assignee

Comment 19

13 years ago
*** Bug 331216 has been marked as a duplicate of this bug. ***
Assignee

Comment 20

13 years ago
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+
Assignee

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Target Milestone: 4.6 → 4.6.2
Assignee

Comment 21

13 years ago
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.