The default bug view has changed. See this FAQ.

Crash in nsStringBuffer::Realloc() during Feed sniffing (on second visit ?)

VERIFIED FIXED in Firefox 14

Status

()

Firefox
RSS Discovery and Preview
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Radu, Assigned: Ehsan)

Tracking

(Depends on: 1 bug, {crash, testcase})

Trunk
Firefox 14
crash, testcase
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [asan][sg:dos])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 609099 [details]
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.
(Reporter)

Updated

5 years ago
Summary: Crash when pasting a link in the url bar [nsFeedSniffer::AppendSegmentToString] → Crash when pasting a link in the url bar
(Reporter)

Comment 3

5 years ago
Created attachment 610243 [details]
debug build ASAN log
(Reporter)

Comment 4

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

Comment 6

5 years ago
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
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]
(Reporter)

Comment 8

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

Comment 10

5 years ago
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).
tracking-firefox12: --- → -
tracking-firefox13: --- → +
tracking-firefox14: --- → +
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
status-firefox12: affected → wontfix
tracking-firefox-esr10: --- → ?
(Assignee)

Comment 14

5 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

5 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

5 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

5 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

5 years ago
OK, I can repro.
tracking-firefox-esr10: ? → 13+
(Assignee)

Comment 19

5 years ago
Created attachment 616789 [details]
Backtrace
(Assignee)

Comment 20

5 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

5 years ago
Created attachment 616818 [details] [diff] [review]
Patch (v1)
Assignee: gavin.sharp → ehsan
Status: NEW → ASSIGNED
Attachment #616818 - Flags: review?(gavin.sharp)
(Assignee)

Comment 22

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=57d9d64a20cb
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
(Assignee)

Comment 25

5 years ago
Right, I'll submit a new patch.
(Assignee)

Comment 26

5 years ago
Created attachment 616954 [details] [diff] [review]
Patch (v2)

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

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e784d1e53836
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+
(Assignee)

Comment 29

5 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

5 years ago
Attachment #616954 - Flags: review?(gavin.sharp)
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/38110d344363
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Target Milestone: --- → Firefox 14
(Assignee)

Comment 31

5 years ago
Oh, shouldn't be marked as FIXED until it's merged on central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

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

Comment 33

5 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.
Philor says: https://hg.mozilla.org/mozilla-central/rev/38110d344363
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 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 → ---
Created attachment 617186 [details] [diff] [review]
followup fix + test
Attachment #617186 - Flags: review?(mano)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd02041e575
OS: Linux → All
Hardware: x86_64 → All
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.
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
Last Resolved: 5 years ago5 years ago
status-firefox14: affected → fixed
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+
status-firefox-esr10: --- → affected
tracking-firefox14: + → -
(Assignee)

Comment 45

5 years ago
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+
status-firefox11: affected → ---
status-firefox15: --- → fixed
tracking-firefox12: - → ---
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
tracking-firefox-esr10: 13+ → ?
Whiteboard: [asan] → [asan][sg:dos]
Confirmed with dveditz in IRC that we are not going to put this in beta, untracking for 13.
tracking-firefox13: + → -
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+
status-firefox-esr10: affected → wontfix
tracking-firefox-esr10: ? → -
You need to log in before you can comment on or make changes to this bug.