594547 - (CVE-2010-3772) Investigate crash downstream from [@nsTreeContentView::InsertRow]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Brandon Sterne (:bsterne)]

Attachment: testcase (crashes Firefox 3.6.8)

wushi reported the following Firefox 3.6. crash to [e-mail]:

-------

 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)]

Dveditz will try to reproduce.

Comment 2

[Daniel Veditz [:dveditz]]

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]]

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]]

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)]

jdaggett, can you take a look here?

Comment 6

[John Daggett (:jtd)]

(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)]

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) No longer reading bugmail consistently]

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)]

dveditz will try to repro with a debug build since it was crashing for him.

Comment 11

[wushi]

Have anybody track this one?

Comment 12

[Daniel Veditz [:dveditz]]

What am I supposed to do with a debug build? It repros there as noted in comment 2.

Comment 13

[Johnny Stenback (:jst)]

Attachment: 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 [:bzbarsky]]

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)]

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)]

(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

[[e-mail]]

(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 [:bzbarsky]]

> 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)]

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)]

Fixed.

http://hg.mozilla.org/mozilla-central/rev/99a5d8039d3e

Comment 21

[LegNeato]

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 22

[Johnny Stenback (:jst)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8fe84b5feca6
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4fac36e8cd3e

Comment 23

[Brandon Sterne (:bsterne)]

(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)]

(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.