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

VERIFIED FIXED in mozilla1.9beta2

Status

()

Core
DOM
P1
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: ted, Assigned: dveditz)

Tracking

(4 keywords)

Trunk
mozilla1.9beta2
x86
Windows XP
crash, fixed1.8.0.15, regression, verified1.8.1.12
Points:
---
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] 1-byte overrun on trunk due to bug 239371, crash signature, URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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]
(Assignee)

Comment 2

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

Updated

10 years ago
Severity: normal → critical
Keywords: crash
Summary: Buffer overrun [@ nsDocument::RetrieveRelevantHeaders ] at provided URL → Buffer overrun [@ nsDocument::RetrieveRelevantHeaders] at provided URL
(Assignee)

Comment 3

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

Updated

10 years ago
Assignee: nobody → dveditz
(Assignee)

Comment 4

10 years ago
Created attachment 287076 [details] [diff] [review]
patch v1
Attachment #287076 - Flags: superreview?
Attachment #287076 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #287076 - Flags: superreview?(jst)
Attachment #287076 - Flags: superreview?
Attachment #287076 - Flags: review?(jst)
Attachment #287076 - Flags: review?
(Reporter)

Comment 5

10 years ago
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.
(Reporter)

Comment 6

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

Comment 7

10 years ago
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+
(Reporter)

Comment 8

10 years ago
Created attachment 287128 [details] [diff] [review]
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.
Attachment #287128 - Flags: review?
(Reporter)

Updated

10 years ago
Attachment #287128 - Flags: review? → review?(jst)

Comment 9

10 years ago
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+

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Attachment #287128 - Flags: review?(jst) → review+

Comment 11

10 years ago
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.
(Assignee)

Comment 12

10 years ago
Created attachment 287556 [details] [diff] [review]
update w/review comments

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+
"formattedTime" :(
(Reporter)

Comment 15

10 years ago
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
Priority: -- → P1
(Assignee)

Updated

10 years ago
Blocks: 239371
Flags: wanted1.8.1.x+
Whiteboard: [sg:critical] → [sg:moderate] 1-byte overrun on trunk due to bug 239371
(Assignee)

Updated

10 years ago
Group: security
(Assignee)

Comment 19

10 years ago
FIXED: attachment 287556 [details] [diff] [review] checked in
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
The Mochitest needs to be committed as well.
Flags: in-testsuite?
(Reporter)

Comment 21

10 years ago
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
(Assignee)

Comment 23

10 years ago
Clearing branch wanted flag assuming the regressing change will not land in older NSPR versions.
Flags: wanted1.8.1.x+
Version: unspecified → Trunk

Comment 24

10 years ago
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.
(Assignee)

Comment 25

10 years ago
fair enough, it's certainly not going to hurt anything.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
(Assignee)

Comment 26

10 years ago
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+
(Assignee)

Comment 27

10 years ago
not really blocking, but approving the patch
Flags: blocking1.8.1.12?

Comment 28

10 years ago
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
Keywords: fixed1.8.1.12
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).
(Reporter)

Comment 30

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

9 years ago
Stephen: you can verify that I checked in the patch on the MOZILLA_1_8_BRANCH correctly.
(I already did that.)
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12

Comment 33

9 years ago
distro patch blocks 1.8.0.15
Flags: blocking1.8.0.15+

Comment 34

9 years ago
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]
You need to log in before you can comment on or make changes to this bug.