Closed Bug 402150 Opened 12 years ago Closed 12 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: ted, Assigned: dveditz)

References

()

Details

(4 keywords, Whiteboard: [sg:moderate] 1-byte overrun on trunk due to bug 239371)

Crash Data

Attachments

(2 files, 1 obsolete file)

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.
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.
Flags: blocking1.9?
Keywords: regression
Whiteboard: [sg:critical]
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.
Severity: normal → critical
Keywords: crash
Summary: Buffer overrun [@ nsDocument::RetrieveRelevantHeaders ] at provided URL → Buffer overrun [@ nsDocument::RetrieveRelevantHeaders] at provided URL
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
Assignee: nobody → dveditz
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #287076 - Flags: superreview?
Attachment #287076 - Flags: review?
Attachment #287076 - Flags: superreview?(jst)
Attachment #287076 - Flags: superreview?
Attachment #287076 - Flags: review?(jst)
Attachment #287076 - Flags: review?
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.
Doesn't happen on Fx2008, we get "2007" in the .tm_year.
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.
Attachment #287076 - Flags: review+
Attached patch mochitestSplinter Review
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.
Attachment #287128 - Flags: review?
Attachment #287128 - Flags: review? → review?(jst)
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 on attachment 287076 [details] [diff] [review]
patch v1

r+sr=jst
Attachment #287076 - Flags: superreview?(jst)
Attachment #287076 - Flags: superreview+
Attachment #287076 - Flags: review?(jst)
Attachment #287076 - Flags: review+
Flags: blocking1.9? → blocking1.9+
Attachment #287128 - Flags: review?(jst) → review+
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.
Updated with review formatting suggestions, carrying over r/sr+.
Attachment #287076 - Attachment is obsolete: true
Attachment #287556 - Flags: superreview+
Attachment #287556 - Flags: review+
Attachment #287556 - Flags: approvalM9?
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=endgame drivers for M9
Attachment #287556 - Flags: approvalM9? → approvalM9+
This didn't get landed yet?  We probably shouldn't ship this in b1.  :-/
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 on attachment 287556 [details] [diff] [review]
update w/review comments

a=beltzner for 1.9 once the tree re-opens
Attachment #287556 - Flags: approval1.9+
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.
Attachment #287556 - Flags: approvalM9+ → approvalM9-
Target Milestone: --- → mozilla1.9 M10
Blocks: 239371
Flags: wanted1.8.1.x+
Whiteboard: [sg:critical] → [sg:moderate] 1-byte overrun on trunk due to bug 239371
Group: security
FIXED: attachment 287556 [details] [diff] [review] checked in
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
The Mochitest needs to be committed as well.
Flags: in-testsuite?
Mochitest checked in.
Flags: in-testsuite? → in-testsuite+
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.
Status: RESOLVED → VERIFIED
Clearing branch wanted flag assuming the regressing change will not land in older NSPR versions.
Flags: wanted1.8.1.x+
Version: unspecified → Trunk
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.
fair enough, it's certainly not going to hurt anything.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #287556 - Flags: approval1.8.1.12+
not really blocking, but approving the patch
Flags: blocking1.8.1.12?
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
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).
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.
Stephen: you can verify that I checked in the patch on the MOZILLA_1_8_BRANCH correctly.
(I already did that.)
distro patch blocks 1.8.0.15
Flags: blocking1.8.0.15+
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=asac for 1.8.0.15

(unmodified distro patch)
Attachment #287556 - Flags: approval1.8.0.15+
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
Keywords: fixed1.8.0.15
Crash Signature: [@ nsDocument::RetrieveRelevantHeaders]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.