Closed
Bug 739040
Opened 12 years ago
Closed 12 years ago
Crash in nsStringBuffer::Realloc() during Feed sniffing (on second visit ?)
Categories
(Firefox Graveyard :: RSS Discovery and Preview, defect)
Firefox Graveyard
RSS Discovery and Preview
Tracking
(firefox12 wontfix, firefox13- affected, firefox14- fixed, firefox15 fixed, firefox-esr10- wontfix)
People
(Reporter: radu.stanca, Assigned: ehsan.akhgari)
References
Details
(Keywords: crash, testcase, Whiteboard: [asan][sg:dos])
Attachments
(6 files, 1 obsolete file)
368.48 KB,
application/octet-stream
|
Details | |
8.59 KB,
text/plain
|
Details | |
13.28 KB,
text/plain
|
Details | |
1.54 KB,
patch
|
asaf
:
review+
Gavin
:
review-
|
Details | Diff | Splinter Review |
6.72 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
6.95 KB,
patch
|
Details | Diff | Splinter Review |
mozilla-central, c71845b3b2a6 built with ASAN(https://developer.mozilla.org/en/Building_Firefox_with_Address_Sanitizer) on Ubuntu 11.10 x86_64 Steps to reproduce: 1. extract testcase.tar.gz somewhere on a webserver, in my case the link is http://127.0.0.1/~rs/test.html The .ogg file is just a sample taken from Wikipedia, I did not modify it 2. open that link in another browser, I use Chrome, copy the link from Chrome and paste it in Firefox to crash it. On a new profile it will not crash the first time you visit that link only the second time and subsequently. I tested mozilla-release built with ASAN but I could not reproduce the crash. Attached is the testcase archive and the ASAN log
Updated•12 years ago
|
Whiteboard: [asan]
Comment 2•12 years ago
|
||
I tried reproducing this as described, but I did not succeed. Maybe a developer can give some hint about what might be the key to reproduction, then I can try again with a debug build.
Summary: Crash when pasting a link in the url bar [nsFeedSniffer::AppendSegmentToString] → Crash when pasting a link in the url bar
Christian - I have a debug build that crashes, I attached the crash details.
Comment 5•12 years ago
|
||
(In reply to Radu from comment #4) > Christian - > > I have a debug build that crashes, I attached the crash details. Thanks, that'll greatly help the developers to isolate the cause.
I can reproduce the crash on another computer, Ubuntu 10.04 i686, latest llvm, mozilla-central c3fd0768d46a
Comment 7•12 years ago
|
||
Although we haven't yet reproduced the symptoms in the log look sg:critical -- assuming the worst until we can rule it out. Don't know if the problem is in the string code or the feed sniffer, and have no idea how the .ogg file from comment 0 comes into play (if at all) since that doesn't show in the stack.
Status: UNCONFIRMED → NEW
status-firefox11:
--- → affected
status-firefox12:
--- → affected
status-firefox13:
--- → affected
status-firefox14:
--- → affected
Component: General → Private Browsing
Ever confirmed: true
Keywords: crash
QA Contact: general → private.browsing
Summary: Crash when pasting a link in the url bar → Crash in nsStringBuffer::Realloc() during Feed sniffing (on second visit ?)
Whiteboard: [asan] → [sg:critical][asan]
(In reply to Daniel Veditz [:dveditz] from comment #7) > Although we haven't yet reproduced the symptoms in the log look sg:critical > -- assuming the worst until we can rule it out. Daniel - Have you tried restarting Firefox after pasting the link the first time? This is how I usually do it: - paste link from Chrome, Firefox will not crash - restart Firefox - paste link, Firefox will crash
Comment 9•12 years ago
|
||
I managed to reproduce this, the URL I was using did seem to be inappropriate. Using the url "starlog.ro/test1.html" did the trick for me. Here's my debug trace: ==50377== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f8339b1558b at pc 0x42bb1b bp 0x7fff9608b1e0 sp 0x7fff9608b1c8 READ of size 1 at 0x7f8339b1558b thread T0 #0 0x42bb1b in memcmp ??:0 #1 0x7fff9608b520 in ([stack]+0xca520) 0x7f8339b1558b is located 0 bytes to the right of 11-byte region [0x7f8339b15580,0x7f8339b1558b) allocated by thread T0 here: #0 0x42d727 in realloc ??:0 #1 0x7f83805ef618 in moz_realloc /srv/repos/browser/mozilla-central/memory/mozalloc/mozalloc.cpp:129 #2 0x7f8371d05f9e in nsStringBuffer::Realloc(nsStringBuffer*, unsigned long) /srv/repos/browser/mozilla-central/xpcom/string/src/nsSubstring.cpp:239 #3 0x7f8371d1fd72 in nsACString_internal::MutatePrep(unsigned int, char**, unsigned int*) /srv/repos/browser/mozilla-central/xpcom/string/src/nsTSubstring.cpp:135 #4 0x7f8371d21d7c in nsACString_internal::ReplacePrepInternal(unsigned int, unsigned int, unsigned int, unsigned int) /srv/repos/browser/mozilla-central/xpcom/string/src/nsTSubstring.cpp:198 #5 0x7f8371d24baf in nsACString_internal::ReplacePrep(unsigned int, unsigned int, unsigned int) /srv/repos/browser/mozilla-central/objdir-ff-asan64dbg/xpcom/string/src/../../../dist/include/nsTSubstring.h:695 #6 0x7f8371d2902e in nsACString_internal::Replace(unsigned int, unsigned int, char const*, unsigned int) /srv/repos/browser/mozilla-central/xpcom/string/src/nsTSubstring.cpp:480 #7 0x7f83651fd8cf in nsACString_internal::Append(char const*, unsigned int) /srv/repos/browser/mozilla-central/objdir-ff-asan64dbg/xpcom/base/../../dist/include/nsTSubstring.h:384 #8 0x7f83718534e6 in NS_CStringSetDataRange_P /srv/repos/browser/mozilla-central/xpcom/build/nsXPCOMStrings.cpp:325 #9 0x7f835d3b41ee in NS_CStringSetDataRange /srv/repos/browser/mozilla-central/xpcom/stub/nsXPComStub.cpp:497 #10 0x7f83554c9d1e in nsACString::Replace(unsigned int, unsigned int, char const*, unsigned int) /srv/repos/browser/mozilla-central/objdir-ff-asan64dbg/browser/components/feeds/src/../../../../dist/include/nsStringAPI.h:507 #11 0x7f83554c1d3a in nsACString::Append(char const*, unsigned int) /srv/repos/browser/mozilla-central/objdir-ff-asan64dbg/browser/components/feeds/src/../../../../dist/include/nsStringAPI.h:522 #12 0x7f83554c18e4 in nsFeedSniffer::AppendSegmentToString(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) /srv/repos/browser/mozilla-central/browser/components/feeds/src/nsFeedSniffer.cpp:373 #13 0x7f8371a908f1 in nsStringInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) /srv/repos/browser/mozilla-central/xpcom/io/nsStringStream.cpp:261 #14 0x7f83554c21ed in nsFeedSniffer::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) /srv/repos/browser/mozilla-central/browser/components/feeds/src/nsFeedSniffer.cpp:383 #15 0x7f83554c23bf in non-virtual thunk to nsFeedSniffer::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) ???:0 #16 0x7f83657db677 in nsHTTPCompressConv::do_OnDataAvailable(nsIRequest*, nsISupports*, unsigned int, char const*, unsigned int) /srv/repos/browser/mozilla-central/netwerk/streamconv/converters/nsHTTPCompressConv.cpp:377 #17 0x7f83657d5327 in nsHTTPCompressConv::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) /srv/repos/browser/mozilla-central/netwerk/streamconv/converters/nsHTTPCompressConv.cpp:309 #18 0x7f83554ba371 in nsFeedSniffer::ConvertEncodedData(nsIRequest*, unsigned char const*, unsigned int) /srv/repos/browser/mozilla-central/browser/components/feeds/src/nsFeedSniffer.cpp:115 #19 0x7f83554bee6b in nsFeedSniffer::GetMIMETypeFromContent(nsIRequest*, unsigned char const*, unsigned int, nsACString&) /srv/repos/browser/mozilla-central/browser/components/feeds/src/nsFeedSniffer.cpp:313 #20 0x7f8365d1691c in CallTypeSniffers /srv/repos/browser/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:689 #21 0x7f8365459eb0 in CallPeekFunc /srv/repos/browser/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:127 #22 0x7f8371a49acc in nsPipeInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) /srv/repos/browser/mozilla-central/xpcom/io/nsPipe3.cpp:799
Assignee | ||
Comment 10•12 years ago
|
||
This has nothing to do with Private Browsing, moving back to General.
Component: Private Browsing → General
QA Contact: private.browsing → general
Comment 11•12 years ago
|
||
Sorry, was off by one in the drop-down: meant "RSS Discovery and Preview" based on FeedSniffer in the stack (although it could be the code string routines I suppose).
tracking-firefox12:
--- → -
tracking-firefox13:
--- → +
tracking-firefox14:
--- → +
Component: General → RSS Discovery and Preview
Keywords: testcase
QA Contact: general → rss.preview
Comment 12•12 years ago
|
||
Adding Mano and gavin to suss out the feed-previewness of it
Comment 13•12 years ago
|
||
This code hasn't changed in a while, esr10 is probably affected as well
Assignee | ||
Comment 14•12 years ago
|
||
I think reproducing this requires a specific HTTP header to be sent for the ogg file. I can't get the feed sniffer to run at all when testing this.
Reporter | ||
Comment 15•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #14) > I think reproducing this requires a specific HTTP header to be sent for the > ogg file. I can't get the feed sniffer to run at all when testing this. Ehsan, the testcase from starlog.ro/test1.html does not have the .ogg file in it anymore.
Assignee | ||
Comment 16•12 years ago
|
||
OK, and do you absolutely need to build with asan to reproduce this? I don't get a crash in a regular debug build.
Reporter | ||
Comment 17•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #16) > OK, and do you absolutely need to build with asan to reproduce this? I > don't get a crash in a regular debug build. I was able to reproduce the crash only on asan build.
Assignee | ||
Comment 18•12 years ago
|
||
OK, I can repro.
Updated•12 years ago
|
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 years ago
|
||
OK, I figured the bug out. nsFeedSniffer::GetMIMETypeFromContent decodes the received data, and then uses the length of the encoded buffer as the length of the decoded data. This means that it doesn't look far enough in the stream if the length of the encoded data is greater than the length of the decoded data, and reads random memory off the heap looking for things like "<rss" otherwise.
Assignee | ||
Comment 21•12 years ago
|
||
Assignee: gavin.sharp → ehsan
Status: NEW → ASSIGNED
Attachment #616818 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 22•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=57d9d64a20cb
Comment 23•12 years ago
|
||
Comment on attachment 616818 [details] [diff] [review] Patch (v1) >diff --git a/browser/components/feeds/src/nsFeedSniffer.cpp b/browser/components/feeds/src/nsFeedSniffer.cpp >+ length = NS_MIN(mDecodedData.Length(), MAX_BYTES); Lack of context in this patch isn't ideal, but this function seems to handled mDecodedData being empty, by falling back to just using the original data: const char* testData = mDecodedData.IsEmpty() ? (const char*)data : mDecodedData.get(); So if mDecodedData.length is 0, you actually want to continue using the original "length" here, right? Otherwise it seems like you'll break sniffing for feeds that aren't compressed. Are there tests that catch that?
Attachment #616818 -
Flags: review?(gavin.sharp) → review-
Comment 24•12 years ago
|
||
Ah, looks like there is! From your try run: 1115 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_bug364677.html | Feed served as text/xml without a channel/link should have been sniffed - got undefined, expected feedHandler
Assignee | ||
Comment 25•12 years ago
|
||
Right, I'll submit a new patch.
Assignee | ||
Comment 26•12 years ago
|
||
This should fix the problem with the uncompressed feeds, and it passes tests locally.
Attachment #616818 -
Attachment is obsolete: true
Attachment #616954 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 27•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e784d1e53836
Comment 28•12 years ago
|
||
Feed-preview has such a bad record, that I would like everyone to know this bug involves code I never touched, dating to the initial rss-feature landing ;) Anyway, *at least for trunk* I filed bug 747315 about porting this lean component to an even leaner js component. That would avoid this kind of bugs in the first place (the behavior might have been wrong but it wouldn't crash)
Updated•12 years ago
|
Attachment #616954 -
Flags: review+
Assignee | ||
Comment 29•12 years ago
|
||
(In reply to Mano from comment #28) > Anyway, *at least for trunk* I filed bug 747315 about porting this lean > component to an even leaner js component. That would avoid this kind of bugs > in the first place (the behavior might have been wrong but it wouldn't crash) That's virtuous, we should totally do that!
Assignee | ||
Updated•12 years ago
|
Attachment #616954 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 30•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/38110d344363
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → Firefox 14
Assignee | ||
Comment 31•12 years ago
|
||
Oh, shouldn't be marked as FIXED until it's merged on central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•12 years ago
|
Attachment #616954 -
Flags: approval-mozilla-esr10?
Attachment #616954 -
Flags: approval-mozilla-aurora?
Comment 32•12 years ago
|
||
Usually, when a bug is sg-flaged, the commit-comment should only contain the bug number.
Assignee | ||
Comment 33•12 years ago
|
||
(In reply to Mano from comment #32) > Usually, when a bug is sg-flaged, the commit-comment should only contain the > bug number. I usually don't do that, because that would make spotting security patches extremely easy. I just type in a commit message which would describe what's evident by just reading the patch, but in a way that does not give away more information than can be obtained by merely reading the patch. I believe other developers use this approach as well.
Comment 34•12 years ago
|
||
Philor says: https://hg.mozilla.org/mozilla-central/rev/38110d344363
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 35•12 years ago
|
||
Comment on attachment 616954 [details] [diff] [review] Patch (v2) This looks wrong too... it removes the "only sniff 512 bytes" restriction when dealing with uncompressed data (we need to clamp to MAX_BYTES regardless of whether mDecodedData.IsEmpty).
Attachment #616954 -
Flags: review-
Attachment #616954 -
Flags: approval-mozilla-esr10?
Attachment #616954 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 36•12 years ago
|
||
Attachment #617186 -
Flags: review?(mano)
Comment 37•12 years ago
|
||
Comment on attachment 617186 [details] [diff] [review] followup fix + test Ugh, thanks Gavin (btw, in the js version I made convertEncodedData return aData to simplify this code path).
Attachment #617186 -
Flags: review?(mano) → review+
Updated•12 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Comment 39•12 years ago
|
||
[Approval Request Comment] Fix Landed on Version: 14 (just landed on trunk) Risk to taking this patch (and alternatives if risky): should be quite safe. test coverage is decent, logic fairly straightforward. This bug is easy to spot from monitoring security-sensitive bug landings, and relatively easy to exploit remotely.
Attachment #617189 -
Flags: approval-mozilla-esr10?
Attachment #617189 -
Flags: approval-mozilla-beta?
Attachment #617189 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Status: REOPENED → NEW
Comment 40•12 years ago
|
||
per philor from an inbound merge https://hg.mozilla.org/mozilla-central/rev/0dd02041e575
Status: NEW → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 41•12 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #39) > Created attachment 617189 [details] [diff] [review] > beta/aurora/esr rollup patch > > [Approval Request Comment] > Fix Landed on Version: 14 (just landed on trunk) > Risk to taking this patch (and alternatives if risky): should be quite safe. > test coverage is decent, logic fairly straightforward. This bug is easy to > spot from monitoring security-sensitive bug landings, and relatively easy to > exploit remotely. I'm going to leave these nominations on the bug, since we may be approving for Beta 13 after the 4/24 merge, rather than Aurora 13 prior to the merge. That being said, in email (with the security@ list) we collectively decided that this did not need to make it into FF12 based upon the difficulty to exploit.
Comment 42•12 years ago
|
||
Christian, can you verify this fix in trunk since you were able to reproduce the crash?
Comment 44•12 years ago
|
||
Comment on attachment 617189 [details] [diff] [review] beta/aurora/esr rollup patch [Triage comment] Low risk, please go ahead and land.
Attachment #617189 -
Flags: approval-mozilla-esr10?
Attachment #617189 -
Flags: approval-mozilla-esr10+
Attachment #617189 -
Flags: approval-mozilla-beta?
Attachment #617189 -
Flags: approval-mozilla-beta+
Attachment #617189 -
Flags: approval-mozilla-aurora?
Attachment #617189 -
Flags: approval-mozilla-aurora+
Updated•12 years ago
|
status-firefox-esr10:
--- → affected
Assignee | ||
Comment 45•12 years ago
|
||
Gavin, can you please take care of landing this? I hate to mess something up again when landing this on branches. :/
Updated•12 years ago
|
Attachment #617189 -
Flags: approval-mozilla-aurora+
Updated•12 years ago
|
Comment 47•12 years ago
|
||
I can't reconcile 'sg:critical' with comment 20 and the eventual patch. The data being read is treated as string data so there's not going to be any misinterpreting data as pointers. The data is only used to answer feed/not-feed but isn't returned to the content so there's no possibility of info leak -- at worst we get slapped by the OS for an access-violation, or we possibly misidentify a feed. Clearing the rating for re-triage.
Whiteboard: [sg:critical][asan] → [asan]
Comment 48•12 years ago
|
||
If the conclusion is that this isn't exploitable, then we probably don't need to land it on ESR/beta (it's already on Aurora), so I guess I'll hold off on landing this until you guys triage it.
Comment 49•12 years ago
|
||
Not hearing any objections I'm calling this non-exploitable, and we shouldn't need it on the ESR branch.
Comment 50•12 years ago
|
||
Confirmed with dveditz in IRC that we are not going to put this in beta, untracking for 13.
Comment 51•12 years ago
|
||
Comment on attachment 617189 [details] [diff] [review] beta/aurora/esr rollup patch clearing the approval flags to keep this out of our release management queries.
Attachment #617189 -
Flags: approval-mozilla-esr10+
Attachment #617189 -
Flags: approval-mozilla-beta+
Updated•12 years ago
|
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•