Last Comment Bug 739040 - Crash in nsStringBuffer::Realloc() during Feed sniffing (on second visit ?)
: Crash in nsStringBuffer::Realloc() during Feed sniffing (on second visit ?)
Status: VERIFIED FIXED
[asan][sg:dos]
: crash, testcase
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: :Ehsan Akhgari
:
:
Mentors:
Depends on: 747315
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-25 03:56 PDT by Radu
Modified: 2012-05-09 12:31 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
affected
-
fixed
fixed
-
wontfix


Attachments
testcase and ASAN log (368.48 KB, application/octet-stream)
2012-03-25 03:57 PDT, Radu
no flags Details
debug build ASAN log (8.59 KB, text/plain)
2012-03-28 12:24 PDT, Radu
no flags Details
Backtrace (13.28 KB, text/plain)
2012-04-19 16:05 PDT, :Ehsan Akhgari
no flags Details
Patch (v1) (1.19 KB, patch)
2012-04-19 17:19 PDT, :Ehsan Akhgari
gavin.sharp: review-
Details | Diff | Splinter Review
Patch (v2) (1.54 KB, patch)
2012-04-20 06:27 PDT, :Ehsan Akhgari
asaf: review+
gavin.sharp: review-
Details | Diff | Splinter Review
followup fix + test (6.72 KB, patch)
2012-04-20 22:52 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
asaf: review+
Details | Diff | Splinter Review
beta/aurora/esr rollup patch (6.95 KB, patch)
2012-04-20 23:32 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description Radu 2012-03-25 03:56:34 PDT
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
Comment 1 Radu 2012-03-25 03:57:27 PDT
Created attachment 609099 [details]
testcase and ASAN log
Comment 2 Christian Holler (:decoder) 2012-03-28 11:49:51 PDT
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.
Comment 3 Radu 2012-03-28 12:24:23 PDT
Created attachment 610243 [details]
debug build ASAN log
Comment 4 Radu 2012-03-28 12:26:16 PDT
Christian -

I have a debug build that crashes, I attached the crash details.
Comment 5 Christian Holler (:decoder) 2012-03-28 12:28:38 PDT
(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.
Comment 6 Radu 2012-03-28 17:20:28 PDT
I can reproduce the crash on another computer, Ubuntu 10.04 i686, latest llvm, mozilla-central c3fd0768d46a
Comment 7 Daniel Veditz [:dveditz] 2012-04-11 10:44:53 PDT
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.
Comment 8 Radu 2012-04-12 04:31:17 PDT
(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 Christian Holler (:decoder) 2012-04-12 10:41:29 PDT
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
Comment 10 :Ehsan Akhgari 2012-04-12 10:56:30 PDT
This has nothing to do with Private Browsing, moving back to General.
Comment 11 Daniel Veditz [:dveditz] 2012-04-12 13:29:42 PDT
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).
Comment 12 Johnathan Nightingale [:johnath] 2012-04-16 06:46:14 PDT
Adding Mano and gavin to suss out the feed-previewness of it
Comment 13 Daniel Veditz [:dveditz] 2012-04-19 13:40:11 PDT
This code hasn't changed in a while, esr10 is probably affected as well
Comment 14 :Ehsan Akhgari 2012-04-19 14:35:39 PDT
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.
Comment 15 Radu 2012-04-19 14:38:02 PDT
(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.
Comment 16 :Ehsan Akhgari 2012-04-19 14:53:35 PDT
OK, and do you absolutely need to build with asan to reproduce this?  I don't get a crash in a regular debug build.
Comment 17 Radu 2012-04-19 14:55:29 PDT
(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.
Comment 18 :Ehsan Akhgari 2012-04-19 16:00:01 PDT
OK, I can repro.
Comment 19 :Ehsan Akhgari 2012-04-19 16:05:49 PDT
Created attachment 616789 [details]
Backtrace
Comment 20 :Ehsan Akhgari 2012-04-19 17:11:47 PDT
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.
Comment 21 :Ehsan Akhgari 2012-04-19 17:19:21 PDT
Created attachment 616818 [details] [diff] [review]
Patch (v1)
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-19 18:28:13 PDT
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?
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-19 18:29:33 PDT
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
Comment 25 :Ehsan Akhgari 2012-04-20 06:13:14 PDT
Right, I'll submit a new patch.
Comment 26 :Ehsan Akhgari 2012-04-20 06:27:30 PDT
Created attachment 616954 [details] [diff] [review]
Patch (v2)

This should fix the problem with the uncompressed feeds, and it passes tests locally.
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-20 07:51:04 PDT
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)
Comment 29 :Ehsan Akhgari 2012-04-20 10:01:32 PDT
(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!
Comment 31 :Ehsan Akhgari 2012-04-20 10:05:07 PDT
Oh, shouldn't be marked as FIXED until it's merged on central.
Comment 32 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-20 10:32:22 PDT
Usually, when a bug is sg-flaged, the commit-comment should only contain the bug number.
Comment 33 :Ehsan Akhgari 2012-04-20 10:59:30 PDT
(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 Boris Zbarsky [:bz] (still a bit busy) 2012-04-20 21:12:09 PDT
Philor says: https://hg.mozilla.org/mozilla-central/rev/38110d344363
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-20 21:54:48 PDT
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).
Comment 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-20 22:52:14 PDT
Created attachment 617186 [details] [diff] [review]
followup fix + test
Comment 37 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-20 23:09:12 PDT
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).
Comment 38 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-20 23:24:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd02041e575
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-20 23:32:51 PDT
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.
Comment 40 Justin Wood (:Callek) 2012-04-21 23:58:55 PDT
per philor from an inbound merge https://hg.mozilla.org/mozilla-central/rev/0dd02041e575
Comment 41 Alex Keybl [:akeybl] 2012-04-22 15:37:58 PDT
(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 Al Billings [:abillings] 2012-04-24 13:49:26 PDT
Christian, can you verify this fix in trunk since you were able to reproduce the crash?
Comment 43 Christian Holler (:decoder) 2012-04-24 14:28:23 PDT
Done :)
Comment 44 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 12:06:48 PDT
Comment on attachment 617189 [details] [diff] [review]
beta/aurora/esr rollup patch

[Triage comment]
Low risk, please go ahead and land.
Comment 45 :Ehsan Akhgari 2012-04-25 18:05:26 PDT
Gavin, can you please take care of landing this?  I hate to mess something up again when landing this on branches.  :/
Comment 47 Daniel Veditz [:dveditz] 2012-04-29 09:56:21 PDT
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.
Comment 48 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-30 08:52:21 PDT
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 Daniel Veditz [:dveditz] 2012-05-07 15:10:48 PDT
Not hearing any objections I'm calling this non-exploitable, and we shouldn't need it on the ESR branch.
Comment 50 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-08 09:36:30 PDT
Confirmed with dveditz in IRC that we are not going to put this in beta, untracking for 13.
Comment 51 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-08 09:37:25 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.