PR_ParseTimeString use PR_Assert and it calls abort on invalid date

RESOLVED FIXED in 4.6.2

Status

--
critical
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: megath, Assigned: wtc)

Tracking

({fixed1.8.1, testcase})

other
4.6.2
fixed1.8.1, testcase
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
Created attachment 151345 [details] [diff] [review]
patch for mozilla/nsprpub/pr/src/misc/prtime.c 

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
Created attachment 151346 [details] [diff] [review]
also I guess asserting invalid time zone isnt good idea. patch v2 :)

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

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

Comment 7

14 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

14 years ago
Target Milestone: --- → 4.6
(Assignee)

Comment 9

14 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

13 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

13 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

13 years ago
Created attachment 212139 [details]
Thunderbird testcase

Comment 14

13 years ago
Created attachment 212143 [details]
Simplified Thunderbird testcase

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

Updated

13 years ago
Keywords: testcase

Comment 15

13 years ago
Doesn't crash Win32 Thunderbird.

Comment 16

13 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
Created attachment 217522 [details] [diff] [review]
patch to fix the assertion failure

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
Created attachment 218076 [details] [diff] [review]
patch to fix the assertion failure (as checked in)

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
Last Resolved: 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.