402150 - Buffer overrun [@ nsDocument::RetrieveRelevantHeaders] at provided URL

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[(not currently active) Ted Mielczarek]

Loading this URL in a Win32 trunk build crashes reliably in nsDocument::RetrieveRelevantHeaders.  It's a buffer overrun that gets caught by the VC8 CRT.  The overrun is a sprintf of a date, the PRExplodedTime .tm_year is 19107.  Looks like we're probably getting a bogus date value from the server, I haven't looked into it further.

Comment 1

[Daniel Veditz [:dveditz]]

This is old code (circa 2005), why is it breaking now? Firefox 2 and a trunk from a couple months back don't crash (but current trunk does), I'll try to narrow down a regression range.

Regardless of what might have changed elsewher, we should absolutely NOT be using sprintf here or anywhere. If PR_FormatTime() doesn't work for some reason we should use (PR_)snprintf.

Comment 2

[Daniel Veditz [:dveditz]]

2007100705 doesn't crash, 2007101405 does. Sorry for the big range but it's taking me 4 minutes a pop to download these with my current flaky connection.

Comment 3

[Daniel Veditz [:dveditz]]

crash was introduced between 10/11 and 10/12:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-11+&maxdate=2007-10-12+04%3A00&cvsroot=%2Fcvsroot

No doubt it's the one-line NSPR tag bump hiding a bunch of changes, one of which is in prtime.c to fix bug 239371 (support 5-digit years).

In this case the server is sending a "last-modified" time of
"Fri, 2 Nov 19107 00:00:01 GMT". [haven't seen years like that since we were fixing y2k bugs]

We can't fault NSPR here but we can certainly worry about where else we're assuming years fit in four digits

Comment 4

[Daniel Veditz [:dveditz]]

Attachment: patch v1

Comment 5

[(not currently active) Ted Mielczarek]

Did we pull that NSPR change onto the 1.8 branch?  We wouldn't see a crash there, since this is the VC8 CRT checking stack overflow.

Comment 6

[(not currently active) Ted Mielczarek]

Doesn't happen on Fx2008, we get "2007" in the .tm_year.

Comment 7

[Wan-Teh Chang]

Comment on attachment 287076 [details] [diff] [review]
patch v1

r=wtc.  I would indent the format string and arguments for PR_snprintf
to align with the open parenthesis.

We have the option of resolving NSPR bug 239371 WONTFIX.  The bug
fix to parse five-digit years hasn't appeared in any NSPR final
release yet.

Another option is for the callers of PR_ParseTimeString to validate
that the parsed time is within a reasonable range.

In any case, changing sprintf to PR_snprintf is a good change.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: mochitest

This is a dead simple mochitest that just sends the same Last-Modified header as the URL, and has a no-op test inside the document to ensure that the document loaded without crashing.  Browser crashes for me on stock trunk, passes with dveditz' patch.

Comment 9

[Wan-Teh Chang]

Comment on attachment 287076 [details] [diff] [review]
patch v1

dveditz, for PR_snprintf, the format string should ideally be:

                  "%02ld/%02ld/%04hd %02ld:%02ld:%02ld"

NSPR uses ld for PRInt32 and hd for PRInt16.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 287076 [details] [diff] [review]
patch v1

r+sr=jst

Comment 11

[Wan-Teh Chang]

In comment #3 dveditz asked
> We can't fault NSPR here but we can certainly worry about where else we're
> assuming years fit in four digits

I looked at all occurrences of "tm_year" in the SeaMonkey LXR:
http://lxr.mozilla.org/seamonkey/search?string=tm_year

I examined all direct uses of tm_year.  I didn't examine some of
the data whose values come from tm_year.  I found only two other
potential problems.  In both cases we pass tm_year to sprintf:
http://lxr.mozilla.org/seamonkey/source/calendar/libical/src/test/regression-utils.c#14
http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/ParseFTPList.cpp#1711

In the first case, the buffer is 1024-byte long.  So buffer overflow
is impossible.

The second case is in test code (function do_it) that is commented out.
It's not easy to see what the buffer size is, but this is in dead code.

Comment 12

[Daniel Veditz [:dveditz]]

Attachment: update w/review comments

Updated with review formatting suggestions, carrying over r/sr+.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=endgame drivers for M9

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

"formattedTime" :(

Comment 15

[(not currently active) Ted Mielczarek]

This didn't get landed yet?  We probably shouldn't ship this in b1.  :-/

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Please land it immediately on GECKO190_20071106_RELBRANCH and then contact either me or cf in IRC, or email release-drivers@

We'll leave it as approved1.9+ as well for when the tree re-opens.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=beltzner for 1.9 once the tree re-opens

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 287556 [details] [diff] [review]
update w/review comments

After a quick IRC chat with dveditz, we've decided to leave this for after M9 so as to keep forward motion.

Comment 19

[Daniel Veditz [:dveditz]]

FIXED: attachment 287556 [details] [diff] [review] checked in

Comment 20

[Jeff Walden [:Waldo]]

The Mochitest needs to be committed as well.

Comment 21

[(not currently active) Ted Mielczarek]

Mochitest checked in.

Comment 22

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112905 Minefield/3.0b2pre. No crash when loading the URL in the bug.

Comment 23

[Daniel Veditz [:dveditz]]

Clearing branch wanted flag assuming the regressing change will not land in older NSPR versions.

Comment 24

[Wan-Teh Chang]

Dan, it's a good idea to land this patch on the Mozilla 1.8 branch for two reasons.
1. The Mozilla 1.8 branch may upgrade to NSPR 4.7.x.
2. Linux distributions may use the Mozilla 1.8 branch with a system NSPR at
version 4.7.x.

Comment 25

[Daniel Veditz [:dveditz]]

fair enough, it's certainly not going to hurt anything.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 287556 [details] [diff] [review]
update w/review comments

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 27

[Daniel Veditz [:dveditz]]

not really blocking, but approving the patch

Comment 28

[Wan-Teh Chang]

Comment on attachment 287556 [details] [diff] [review]
update w/review comments

I checked in this patch on the MOZILLA_1_8_BRANCH for 1.8.1.12.

Checking in nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.566.2.37; previous revision: 3.566.2.36
done

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

I _cannot_ crash with the 2.0.0.11 Win32 builds (Vista/XP), using http://man.he.net/man2/lseek as the URL, so obviously can't verify this in the .12 candidates (comment 6 seems to indicate that branch builds didn't (wouldn't?) crash, since they get "2007"--rather than 19107--returned).

Comment 30

[(not currently active) Ted Mielczarek]

Even if the NSPR changes went into the 1.8 branch, you still wouldn't crash there, because the crash is actually triggered by the stack safety stuff in VC8.  You'd just get silent stack corruption.

Comment 31

[Wan-Teh Chang]

Stephen: you can verify that I checked in the patch on the MOZILLA_1_8_BRANCH correctly.
(I already did that.)

Comment 32

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified using Bonsai: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsDocument.cpp&branch=MOZILLA_1_8_BRANCH&root=/cvsroot&subdir=mozilla/content/base/src&command=DIFF_FRAMESET&rev1=3.566.2.36&rev2=3.566.2.37, for MOZILLA_1_8_BRANCH.

Replacing fixed1.8.1.12 with verified1.8.1.12.

Comment 33

[Alexander Sack]

distro patch blocks 1.8.0.15

Comment 34

[Alexander Sack]

Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=asac for 1.8.0.15

(unmodified distro patch)

Comment 35

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.566.2.6.2.20; previous revision: 3.566.2.6.2.19
done