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)

defect
Not set
normal

Tracking

(firefox12 wontfix, firefox13- affected, firefox14- fixed, firefox15 fixed, firefox-esr10- wontfix)

VERIFIED FIXED
Firefox 14
Tracking Status
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)

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
Attached file testcase and ASAN log
Whiteboard: [asan]
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
Attached file debug build ASAN log
Christian -

I have a debug build that crashes, I attached the crash details.
(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
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
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
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
This has nothing to do with Private Browsing, moving back to General.
Component: Private Browsing → General
QA Contact: private.browsing → general
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).
Component: General → RSS Discovery and Preview
Keywords: testcase
QA Contact: general → rss.preview
Adding Mano and gavin to suss out the feed-previewness of it
This code hasn't changed in a while, esr10 is probably affected as well
Assignee: nobody → gavin.sharp
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.
(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.
OK, and do you absolutely need to build with asan to reproduce this?  I don't get a crash in a regular debug build.
(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.
OK, I can repro.
Attached file Backtrace
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.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: gavin.sharp → ehsan
Status: NEW → ASSIGNED
Attachment #616818 - Flags: review?(gavin.sharp)
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-
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
Depends on: 747315
Right, I'll submit a new patch.
Attached patch Patch (v2)Splinter Review
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)
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)
Attachment #616954 - Flags: review+
(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!
Attachment #616954 - Flags: review?(gavin.sharp)
https://hg.mozilla.org/integration/mozilla-inbound/rev/38110d344363
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Oh, shouldn't be marked as FIXED until it's merged on central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #616954 - Flags: approval-mozilla-esr10?
Attachment #616954 - Flags: approval-mozilla-aurora?
Usually, when a bug is sg-flaged, the commit-comment should only contain the bug number.
(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.
Philor says: https://hg.mozilla.org/mozilla-central/rev/38110d344363
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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+
OS: Linux → All
Hardware: x86_64 → All
[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?
Status: REOPENED → NEW
per philor from an inbound merge https://hg.mozilla.org/mozilla-central/rev/0dd02041e575
Status: NEW → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(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.
Christian, can you verify this fix in trunk since you were able to reproduce the crash?
Done :)
Status: RESOLVED → VERIFIED
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+
Gavin, can you please take care of landing this?  I hate to mess something up again when landing this on branches.  :/
Attachment #617189 - Flags: approval-mozilla-aurora+
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]
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.
Not hearing any objections I'm calling this non-exploitable, and we shouldn't need it on the ESR branch.
Group: core-security
Whiteboard: [asan] → [asan][sg:dos]
Confirmed with dveditz in IRC that we are not going to put this in beta, untracking for 13.
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+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: