Last Comment Bug 594547 - (CVE-2010-3772) Investigate crash downstream from [@nsTreeContentView::InsertRow]
(CVE-2010-3772)
: Investigate crash downstream from [@nsTreeContentView::InsertRow]
Status: RESOLVED FIXED
[sg:critical?]
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: 1.9.2 Branch
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-08 14:26 PDT by Brandon Sterne (:bsterne)
Modified: 2014-09-04 09:40 PDT (History)
10 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.13+
.13-fixed
.16-fixed


Attachments
testcase (crashes Firefox 3.6.8) (25.06 KB, application/xhtml+xml)
2010-09-08 14:26 PDT, Brandon Sterne (:bsterne)
no flags Details
Likely fix. (1.14 KB, patch)
2010-11-17 15:19 PST, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
tnikkel: review+
jst: approval2.0+
christian: approval1.9.2.13+
christian: approval1.9.1.16+
Details | Diff | Review

Description Brandon Sterne (:bsterne) 2010-09-08 14:26:45 PDT
Created attachment 473219 [details]
testcase (crashes Firefox 3.6.8)

wushi reported the following Firefox 3.6. crash to security@mozilla.org:

-------

 Hi,
    I think I found another exploitable vuln for firefox 3.6.8, the stack like this:

(960.858): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=03c7de48 ebx=03c7de50 ecx=3ffdf793 edx=00000000 esi=03cffffc edi=03d00000
eip=7815023a esp=0012f68c ebp=0012f694 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010202
*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\Program Files\Mozilla Firefox\MOZCRT19.dll -
MOZCRT19!memmove+0x5a:
7815023a f3a5            rep movs dword ptr es:[edi],dword ptr [esi]
*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\Program Files\Mozilla Firefox\xul.dll -
ChildEBP RetAddr
WARNING: Stack unwind information not available. Following frames may be wrong.
0012f694 100eab13 MOZCRT19!memmove+0x5a
0012f6b0 1076138d xul!gfxIntSize::gfxIntSize+0x49d7
0012f6d0 107613bf xul!gfxFontTestStore::NewStore+0x6cff
00000000 00000000 xul!gfxFontTestStore::NewStore+0x6d31



When you check the POC, you can found a lot of no-use things, these things just make the file size > 26k , To reproduce this case, maybe you need make your PC slowly and refresh some times ,haha. I can give you the dump file If you need .

wushi
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2010-09-21 13:43:34 PDT
Dveditz will try to reproduce.
Comment 2 Daniel Veditz [:dveditz] 2010-09-21 23:12:39 PDT
Got a different stack on a debug Mac 1.9.2 build after reloading a few times (crash in the parser). Haven't seen a problem on trunk or an opt 1.9.2 builds.

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000001d8ffffc
Crashed Thread:  0

Thread 0 Crashed:
0 __memcpy + 1908 (cpu_capabilities.h:246)
1 nsTArray_base::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int) + 197 (nsTArray.cpp:175)
2 Row** nsTArray<Row*>::ReplaceElementsAt<Row*>(unsigned int, unsigned int, Row* const*, unsigned int) + 129
3 Row** nsTArray<Row*>::InsertElementsAt<Row*>(unsigned int, nsTArray<Row*> const&) + 65
4  nsTreeContentView::InsertRow(int, int, nsIContent*) + 375 (nsTreeContentView.cpp:1447)
5  nsTreeContentView::ContentInserted(nsIDocument*, nsIContent*, nsIContent*, int) + 1342 (nsTreeContentView.cpp:1076)
6  nsTreeContentView::ContentAppended(nsIDocument*, nsIContent*, int) + 105 (nsTreeContentView.cpp:990)
7  nsNodeUtils::ContentAppended(nsIContent*, int) + 324 (nsNodeUtils.cpp:135)
8  nsContentSink::NotifyAppend(nsIContent*, unsigned int) + 146 (nsContentSink.cpp:1381)
9  nsXMLContentSink::HandleEndElement(unsigned short const*, int) + 752 (nsXMLContentSink.cpp:1200)
10 nsXMLContentSink::HandleEndElement(unsigned short const*) + 32 (nsXMLContentSink.cpp:1146)
11 nsExpatDriver::HandleEndElement(unsigned short const*) + 233 (nsExpatDriver.cpp:450)
12 Driver_HandleEndElement(void*, unsigned short const*) + 93
13 doContent + 3677 (xmlparse.c:2550)
14 contentProcessor + 81 (xmlparse.c:2095)
15 MOZ_XML_ParseBuffer + 192 (xmlparse.c:1618)
16 MOZ_XML_Parse + 1218 (xmlparse.c:1589)
17 nsExpatDriver::ParseBuffer(unsigned short const*, unsigned int, int, unsigned int*) + 544 (nsExpatDriver.cpp:1027)
18 nsExpatDriver::ConsumeToken(nsScanner&, int&) + 940 (nsExpatDriver.cpp:1126)
19 nsParser::Tokenize(int) + 349 (nsParser.cpp:3120)
20 nsParser::ResumeParse(int, int, int) + 515 (nsParser.cpp:2336)
21 nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 776 (nsParser.cpp:2985)
22 nsDocumentOpenInfo::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 99 (nsURILoader.cpp:306)
23 nsStreamListenerTee::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 624 (nsStreamListenerTee.cpp:108)
24 nsHttpChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 802 (nsHttpChannel.cpp:5391)
25 nsInputStreamPump::OnStateTransfer() + 777 (nsInputStreamPump.cpp:510)
26 nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) + 138 (nsInputStreamPump.cpp:400)
27 nsInputStreamReadyEvent::Run() + 100 (nsStreamUtils.cpp:113)
28 nsThread::ProcessNextEvent(int, int*) + 676 (nsThread.cpp:527)
29 NS_ProcessPendingEvents_P(nsIThread*, unsigned int) + 146
30 nsBaseAppShell::NativeEventCallback() + 181 (nsBaseAppShell.cpp:126)
31 nsAppShell::ProcessGeckoEvents(void*) + 518 (nsAppShell.mm:426)
32 CFRunLoopRunSpecific + 3141
33 CFRunLoopRunInMode + 88
34 RunCurrentEventLoopInMode + 283
35 ReceiveNextEventCommon + 175
36 BlockUntilNextEventMatchingListInMode + 106
37 _DPSNextEvent + 657
38 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
39 -[NSApplication run] + 795
40 nsAppShell::Run() + 288 (nsAppShell.mm:778)
41 nsAppStartup::Run() + 148 (nsAppStartup.cpp:183)
42 XRE_main + 14942 (nsAppRunner.cpp:3483)
43 main + 709 (nsBrowserApp.cpp:158)
44 _start + 209
45 start + 41
Comment 3 Daniel Veditz [:dveditz] 2010-09-21 23:39:42 PDT
I didn't see any useful assertions before the above crash, just some NS_WARNINGs about javascript errors in the page.
Comment 4 Daniel Veditz [:dveditz] 2010-09-22 00:01:03 PDT
Opt Windows 3.6.10pre, on the other hand, goes down pretty easily.

bp-6066425b-a2cd-445c-95c2-ef2412100921
bp-e0059276-fcdb-4860-85c5-0376c2100921

The first crash only took 3 or 4 reloads, the second took at least twenty, plus closing and reopening the testcase in the middle. The stacks look like what I saw on Mac debug.

I don't see gfxIntSize anywhere unlike comment 0. Two different bugs?

wushi: what fonts are you using on your system? Maybe that's where the difference lies.
Comment 5 Damon Sicore (:damons) 2010-09-28 13:52:47 PDT
jdaggett, can you take a look here?
Comment 6 John Daggett (:jtd) 2010-09-28 17:48:38 PDT
(In reply to comment #4)

> I don't see gfxIntSize anywhere unlike comment 0. Two different bugs?

I'm not sure this is actually a crash in gfx code at all, my guess is that this is a bug in content code.  The offset from gfxIntSize in the original description is quite large, I think you're seeing a crash in other code.

  gfxIntSize::gfxIntSize+0x49d7
  gfxFontTestStore::NewStore+0x6cff

Looking at the source for the 1.9.2 branch, that's testing code that will only ever get called by a test tool that is independent from xul.lib.  We should probably #ifdef out this code but that's not really relevant to this bug.
Comment 8 Damon Sicore (:damons) 2010-10-12 14:01:44 PDT
If we ifdef out the test code, try to reproduce, is there any chance that will change the stack a bit and reveal the source of the problem?
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2010-10-12 14:03:14 PDT
Given the offsets in comment 6, it seems like this is totally unrelated to gfxIntSize. Need to get a better stack which contains correct symbols
Comment 10 Damon Sicore (:damons) 2010-10-19 13:51:13 PDT
dveditz will try to repro with a debug build since it was crashing for him.
Comment 11 wushi 2010-11-03 05:11:45 PDT
Have anybody track this one?
Comment 12 Daniel Veditz [:dveditz] 2010-11-12 10:16:06 PST
What am I supposed to do with a debug build? It repros there as noted in comment 2.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-17 15:19:36 PST
Created attachment 491346 [details] [diff] [review]
Likely fix.

While I don't fully understand all of the code involved here, what seems to be happening here is that we're notifying for having appended a new child in the sink, the child in question is the <option> element in the testcase, where the relevant parts look like this:

                            <ther:tree insertafter="" >
                              <ther:treechildren >
                                <div id="div2">X
                                  <xht:optgroup >
                                    <xht:option>1111111111 """"""""""</xht:option>

We have a XUL tree, with a treechildren child, containing an HTML div with an optgroup, containing the option that we're adding during the crash. When we're notifying for the option, we try to find the row index (FindContent(), passing in the optgroup), which is not a direct child of the treechildren, nor an actual row, so we get -1 back. Doing nothing in this case seems to fix this, but it's non-trivial to reproduce this here, so I can't claim to know for sure.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-11-17 20:14:51 PST
Comment on attachment 491346 [details] [diff] [review]
Likely fix.

This seems ok (though it would be good for Timothy to double-check)... but weren't we going to rip all the HTML stuff out of this code?  It's unused (a carryover from XBL form controls), and just causes issues like this one.

I don't see an existing bug on this; we should get one filed (and ideally fixed).
Comment 15 Timothy Nikkel (:tnikkel) 2010-11-18 00:13:28 PST
Comment on attachment 491346 [details] [diff] [review]
Likely fix.

I wonder why this is the only place that doesn't check the return value of FindContent.
Comment 16 Timothy Nikkel (:tnikkel) 2010-11-18 00:15:29 PST
(In reply to comment #14)
> but
> weren't we going to rip all the HTML stuff out of this code?  It's unused (a
> carryover from XBL form controls), and just causes issues like this one.
> 
> I don't see an existing bug on this; we should get one filed (and ideally
> fixed).

I don't know the history of this, but are we fairly confident no one has used these tags inside a tree?
Comment 17 neil@parkwaycc.co.uk 2010-11-18 04:28:33 PST
(In reply to comment #16)
> I don't know the history of this, but are we fairly confident no one has used
> these tags inside a tree?
Since work stopped on XBL form controls I've only seen them in crash tests.
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-11-18 05:10:37 PST
> I wonder why this is the only place that doesn't check the return value of
> FindContent.

Probably because outside of fuzzers this is dead code?

> I don't know the history of this, but are we fairly confident no one has used
> these tags inside a tree?

Confident enough that I'm totally willing to break any such users, who must have gone to some lengths to thus weirdly mix namespaces.
Comment 19 Timothy Nikkel (:tnikkel) 2010-11-18 12:11:48 PST
Ok, so let's get a bug on that, although it's less important now that we've disabled remote xul.
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-18 14:42:07 PST
Fixed.

http://hg.mozilla.org/mozilla-central/rev/99a5d8039d3e
Comment 21 christian 2010-11-18 14:43:24 PST
Comment on attachment 491346 [details] [diff] [review]
Likely fix.

a=LegNeato for 1.9.2.13 and 1.9.1.16. Just a reminder that code freeze is tonight.
Comment 23 Brandon Sterne (:bsterne) 2010-12-03 10:17:38 PST
(In reply to comment #19)
> Ok, so let's get a bug on that, although it's less important now that we've
> disabled remote xul.

Was this bug filed yet?
Comment 24 Timothy Nikkel (:tnikkel) 2010-12-04 15:59:36 PST
(In reply to comment #23)
> (In reply to comment #19)
> > Ok, so let's get a bug on that, although it's less important now that we've
> > disabled remote xul.
> 
> Was this bug filed yet?

I filed bug 616746, thanks for the reminder.

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