Bug 485941 (CVE-2009-1232)

Stack overflow using overly-deep XML tree (DoS)

NEW
Unassigned

Status

()

Core
Layout
P1
critical
8 years ago
a month ago

People

(Reporter: reed, Unassigned)

Tracking

(Blocks: 4 bugs, {crash, testcase})

Trunk
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.0.x +

Firefox Tracking Flags

(blocking2.0 -, status2.0 ?, status1.9.2 ?, status1.9.1 wanted)

Details

(Whiteboard: [sg:dos][stack-exhaustion][needs patch][tbird crash], crash signature, URL)

Attachments

(8 attachments)

(Reporter)

Description

8 years ago
Created attachment 370016 [details]
PoC

I tested this both via HTTP and via file:// on trunk (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090323 Minefield/3.6a1pre), 3.5b4pre (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090330 Shiretoko/3.5b4pre), and 3.0.9pre (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.9pre) Gecko/2009033004 GranParadiso/3.0.9pre).

In all three (x2) cases, all I got was "XML Parsing Error: no element found", "<file path>", and "Line Number 1, Column 228891:". No crash or anything...
(Reporter)

Updated

8 years ago
Whiteboard: [sg:investigate]
Related to bug 456954 and bug 323394?

Comment 2

8 years ago
Created attachment 370051 [details]
stack

I get this stack on 64 bit linux/debug/1.9.0 when reloading the testcase.
1.9.1 and trunk work fine.

Stack 'loops' here:
0x00002aaab35eb36b in nsCSSFrameConstructor::ConstructFrameInternal (this=0x1f0e390, aState=@0x7fffea9e8830, 
    aContent=0x2aaab86866b0, aParentFrame=0x2aaab9666ca8, aTag=0x2aaab8686650, aNameSpaceID=0, aStyleContext=0x2aaab9666d08, 
    aFrameItems=@0x7fffe9dee510, aXBLBaseTag=0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsCSSFrameConstructor.cpp:7758
0x00002aaab35eb72c in nsCSSFrameConstructor::ConstructFrame (this=0x1f0e390, aState=@0x7fffea9e8830, 
    aContent=0x2aaab86866b0, aParentFrame=0x2aaab9666ca8, aFrameItems=@0x7fffe9dee510)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsCSSFrameConstructor.cpp:7580
0x00002aaab35eb948 in nsCSSFrameConstructor::ProcessInlineChildren (this=0x1f0e390, aState=@0x7fffea9e8830, 
    aContent=0x2aaab86864f0, aFrame=0x2aaab9666ca8, aCanHaveGeneratedContent=1, aFrameItems=@0x7fffe9dee510, 
    aKidsAllInline=0x7fffe9dee52c) at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsCSSFrameConstructor.cpp:12851
0x00002aaab35e9f6d in nsCSSFrameConstructor::ConstructInline (this=0x1f0e390, aState=@0x7fffea9e8830, 
    aDisplay=<value optimized out>, aContent=0x2aaab86864f0, aParentFrame=0x2aaab9666bb0, aStyleContext=0x2aaab9666c10, 
    aIsPositioned=0, aNewFrame=0x2aaab9666ca8)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsCSSFrameConstructor.cpp:12622
#20 0x00002aaab35ea989 in nsCSSFrameConstructor::ConstructFrameByDisplayType (this=0x1f0e390, aState=@0x7fffea9e8830, 
    aDisplay=0x2aaab94ac270, aContent=0x2aaab86864f0, aNameSpaceID=<value optimized out>, aTag=<value optimized out>, 
    aParentFrame=0x2aaab9666bb0, aStyleContext=0x2aaab9666c10, aFrameItems=@0x7fffe9deebf0, aHasPseudoParent=0)
    at /home/smaug/mozilla/mozilla_cvs/mozilla/layout/base/nsCSSFrameConstructor.cpp:6636
We should probably get this fixed sometime soon since it was posted publicly.

Dan, what say you?

Also: This looks like CSS stuff. bz or dbaron: Can you take a look at this and see what fixed it on trunk/1.9.1 so we can backport it?
Flags: blocking1.9.0.9?
Flags: blocking1.9.0.10?

Comment 4

8 years ago
OK, some really quick comments without having managed to reproduce this yet:

1)  There is no XUL involved anywhere here.  The fact that there is a claim
    that it's involved makes me suspect whoever's making said claims.
2)  nsCSSFrameConstructor has to do with "CSS" only insofar as it's the thing
    that builds the frame tree.
3)  The poc.xml file is a malformed XML file which is chiefly notable for having
    absolutely no end tags.  It does have 30000 start tags, though.

If we should happen to actually try to lay the document out before detecting that it's malformed, it's quite likely that we will get a stack overflow during the recursive frame construction process.  Is that what's being reported?  Or something else?  The url in the url field is short on detail.  ;)

Comment 5

8 years ago
Created attachment 370116 [details]
Modification of the XML file that triggers stack overflow reliably on trunk

Comment 6

8 years ago
For what it's worth, on my testcase Opera seems to hang for a few minutes, then crash, while Safari gives an XML parsing error that's completely unrelated to the actual way the file is malformed.

In any case, I'd love to know whether anyone's claiming that there's anything here other than us being unable to make a function call due to C stack exhaustion, and if so what the basis for claiming it is...

Comment 7

8 years ago
Oh, and to be clear, directly loading attachment 370116 [details] is sufficient to crash any Gecko that does incremental XML parsing (3.0 and later).  No need for the silly HTML iframe thing.

Comment 8

8 years ago
Note that the Opera thing is also http://www.milw0rm.com/exploits/8320 and explicitly points to this testcase.

So my general take on this is that yes, this is a crash, no it's not exploitable, unless I'm seriously missing something here.
So the problem might be that we don't cap the depth of an XML tree like we do for HTML? Should probably add that to the Expat driver.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9.2a1
Created attachment 370209 [details] [diff] [review]
v1

Do we want to just ignore things above a certain level of nesting or show the error page? This throws a generic error, if we do that we can probably add a better error message on trunk.

Note that pretty printing doubles or triples the nesting level, but we don't have to account for that because the XSLT processor already cuts things off.
(Reporter)

Updated

8 years ago
Duplicate of this bug: 486251
Not for 1.9.0.9 since it's just a DoS and opening up because it was reported publicly.
Group: core-security
Flags: blocking1.9.0.9?
Flags: wanted1.9.1?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10?
Flags: blocking1.9.0.10+
Whiteboard: [sg:investigate] → [sg:dos]
Whiteboard: [sg:dos] → [sg:dos] stack-overflow
Alias: CVE-2009-1232
Attachment #370209 - Flags: superreview?(jonas)
Attachment #370209 - Flags: review?(mrbkap)
Comment on attachment 370209 [details] [diff] [review]
v1

Not sure who comment 10 was directed at, trying parser guys who weren't CC'd on the bug and probably didn't see the question.
(no evidence of memory corruption here).
Summary: Possible XML XUL parser memory corruption (DoS) → Stack overflow using overly-deep XUL XML (DoS)

Comment 15

8 years ago
Or XUL for that matter.
Summary: Stack overflow using overly-deep XUL XML (DoS) → Stack overflow using overly-deep XML tree (DoS)
Comment on attachment 370209 [details] [diff] [review]
v1

I *don't* think we should do this. It's not stopping any DoS attacks anyway since you can create arbitrarily deep trees using the DOM.

Additionally the current patch will affect pure data documents, such as DOMParser and XMLHttpRequest documents. Where it not only makes a lot more sense to create very deeply nested DOMs, it's also much safer.

The limit we have in the HTML parser makes a lot more sense IMHO since it's easy there to create tag soup that produces very deeply nested trees by accident.

Feel free to re-request reviews if you disagree.
Attachment #370209 - Flags: superreview?(jonas)
Attachment #370209 - Flags: superreview-
Attachment #370209 - Flags: review?(mrbkap)
So I guess you're arguing for wontfixing this bug on branch (and on trunk?). I personally don't think it makes much sense to allow overly-deep trees. There are a number of places within our code that will just cause a stack overflow with them, so their usefulness will be limited anyway.
Hi,
I've found that bug and posted it to milw0rm - when I attached with OllyDBG to Firefox I'v found out that I'm within xul module after that crash. Also ESP register can be modified, however I couldn't find any interesting attack vector other then DoS (and yes I know that ESP is volatile by the nature ;). I did some tests and Opera crashed, Mozilla crashed, however Internet Explorer parsed that malformed XML and... it recovered after nearly 10 minutes. Interesting...
(Reporter)

Updated

8 years ago
Duplicate of this bug: 489755

Comment 20

8 years ago
> I'm within xul module

If you mean libxul, that's just the big "statically link everything together" module.  It contains all the DOM, layout, networking, etc, etc code.  Most of it is not XUL related.

> Also ESP register can be modified

You mean the stack frame on which the overflow happens varies depending on precisely what's on the stack?  Sure thing!
So maybe change name from libxul to libeverything :) Also as I said - ESP is volatile by default and unstable, however it gets really strange address after that crash. I tried to play with it for a while but I couldn't find any idea how to use it for code exection purposes.
Flags: blocking1.9.0.11+ → blocking1.9.0.12+
Whiteboard: [sg:dos] stack-overflow → [sg:dos] stack-overflow [needs patch]
Flags: wanted1.9.1?
Flags: wanted1.9.1.x+
Flags: blocking1.9.0.13+
Flags: blocking1.9.0.12+
Sicking: see comment 17, are you arguing for wontfixing this? If not, what do you think we should do?
status1.9.1: --- → wanted
Flags: wanted1.9.1.x+
Flags: blocking1.9.0.14+
Jonas: An update here would be nice. I really don't think we should WONTFIX a bug that's clearly a crash. Stability issues are important.

Anyway, this isn't exploitable and is, thus, no longer blocking our 1.9.0 or 1.9.1 releases. We do, however, *want* it if a patch has landed on trunk and baked. Please request approval after it lands on m-c.
Arg, wrote a comment here but apparently it got lost.

I don't think fixing this in the XML parser is the right solution. That doesn't solve the sg:dos problem here since you can just build a deep DOM using appendChild etc.

It would solve the problem of people accidentally feeding deeply nested XML documents and crashing themselves/their users, but I haven't heard of this being a real-world problem. (This is where HTML parser differs since it's easy to accidentally create a HTML document that produces deeply nested DOM trees).

If we want to reduce the number of accidental crashes, then IMHO we should look at making the layout code stop creating frames beyond a certain depth since layout is extra sensitive to deeply nested trees due to placing large amounts of data on the stack.

We could also look into creating a tree-iterator that lets you iterate non-recursively over a subtree. Obviously this wouldn't work everywhere, but could possibly reduce the number of accidental crashes.
This needs someone from layout to do comment 24 then. Should this move to a different component?
Assignee: peterv → nobody
Status: ASSIGNED → NEW
Sure.

One thing I forgot to add is that we could also add code to the DOM that checks that we never produce trees deeper than X levels. That would be the only way to truly fix the sg:dos problem here. However I'm worried about perf issues with that, though maybe it's not a big deal.

However I still think that we should make layout stop creating frames beyond a certain depth in order to not for X above to be limited by layout.
Component: XML → Layout
QA Contact: xml → layout

Comment 27

8 years ago
How is solving that problem in layout any easier than limiting DOM tree depth?  I agree it might be more appropriate, but it's not any easier.  It's the same problem, in fact.
And aren't there plenty of things in content that are recursive, e.g., BindToTree?
The main reason isn't that it's easier. It's that I'm pretty sure that layout is the most sensitive to deep DOM trees.

Say that we fixed this bug by only changing the DOM to prevent trees deeper than X. This means that we'd have to make X whatever limit that the layout code has (seemingly 200 based on the current MAX_REFLOW_DEPTH define).

This even for things like XMLHttpRequest loaded documents and other types of non-displayed data documents, which I think would be unfortunate.


If we do want to fix people unexpectedly running into crashes due to too deep DOMs, then I think layout is where we should start fixing that.

On ther other hand if we are worried about the sg:dos problem then I think we should fix both layout and the DOM.
Duplicate of this bug: 543774
Duplicate of this bug: 544188
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
status1.9.2: --- → ?
status2.0: --- → ?

Updated

8 years ago
Duplicate of this bug: 547226
Maybe the thing to do is to limit DOM depth to some fixed limit for non-data documents, and ensure that all the algorithms we use to walk data documents are non-recursive.

Comment 34

7 years ago
We could try doing that...  That said, the depth check on each DOM mutation could get a little expensive.

Another option is to keep track of "current depth" in all DOM nodes (parent depth + 1), which should be doable via Bind/Unbind without too much trouble.  Then recursive algorithms could check depth and stop as needed...

Having depth in tree stored on nodes would actually be possibly-helpful for other things too (e.g. parallelizing selector matching).
blocking2.0: ? → -
http://niebezpiecznik.pl/PoC/Firefox_3.6/poc.html also for search goodness.
Blocks: 532972
(Reporter)

Updated

7 years ago
Duplicate of this bug: 584465

Comment 37

7 years ago
Is this url related to this bug?

http://static.nvd.nist.gov/feeds/xml/cve/nvdcve-2.0-2008.xml
Duplicate of this bug: 501381
Blocks: 507452
Duplicate of this bug: 622346
Duplicate of this bug: 644526
Duplicate of this bug: 657455
Duplicate of this bug: 657108
Duplicate of this bug: 664397
Comment hidden (abuse-reviewed)
Target Milestone: mozilla1.9.2a1 → ---
Version: unspecified → Trunk

Updated

6 years ago
Duplicate of this bug: 682877
People keep testing this. Hooking up Socorro...

From Sept 1 - October 4 http://niebezpiecznik.pl/PoC/Firefox_3.6

    356 nsGenericElement::UnbindFromTree(int, int)
     53 nsBindingManager::RemovedFromDocument(nsIContent*, nsIDocument*)
     19 xul.dll@0x120a7e
     17 xul.dll@0x91ed4
     13 xul.dll@0x13e23e
     12 nsGenericElement::UnbindFromTree(bool, bool)/poc.html
      1 nsGenericElement::GetChildAt(unsigned int)
      1 SelectorMatches
      1 txXPathNodeUtils::localNameEquals(txXPathNode const&, nsIAtom*)
      1 xul.dll@0x196731
Crash Signature: int) ] [@ nsXMLElement::UnbindFromTree(int, int) ] [@ xXPathNodeUtils::localNameEquals(txXPathNode const&, nsIAtom*) ] [@ SelectorMatches ] [@ nsBindingManager::RemovedFromDocument(nsIContent*, nsIDocument*) ] [@ nsGenericElement::GetChildAt(un&hellip;

Updated

6 years ago
Keywords: crash

Comment 47

6 years ago
When do you think it will be fixed? It still works on 10.0.2.

Comment 48

6 years ago
Not very fast 13.0a1 have the same crash .
Duplicate of this bug: 513224

Updated

5 years ago
Blocks: 561277
Duplicate of this bug: 376862

Comment 51

5 years ago
Hi all.  I have a crash case that I found as part of my project. Basically when creating a grid generator algorithm I stumbled in a recursion case where JS is creating DIVs ( float and clear ) types. This reminded me of 1997, februray when I attempted to write pixels using 1x1 divs in an attempt to make a canvas app. 

Here we are: 

The use case is: 
http://www.telasocial.com/p/d/grid-layout/

The crash case is: 
https://github.com/taboca/TelaSocial-Grid-Type/issues/1

The motivation for me to write in this bug is: 

1) bClary pointed me of 3 possible bugs; 
2) Comment 33 comment #c33 is interesting as I feel this goes all the way to user experience — understood it does not happen a lot online 
3) I am attaching experience from chrome browser simply as a case of the UI they place when the detect the condition.

Comment 52

5 years ago
Created attachment 607724 [details]
Popup that showed when the testcase was loaded in chrome

Comment 53

5 years ago
Created attachment 607725 [details]
After page experience
Created attachment 627097 [details]
some js that triggers this crash reliably (WARNING: will crash your browser if using ff)

still causes crash on current release version (12.0) with win7.

Updated

5 years ago
Attachment #627097 - Attachment mime type: text/plain → text/html

Updated

5 years ago
Attachment #627097 - Attachment description: some js that triggers this crash reliably → some js that triggers this crash reliably (WARNING: will crash your browser if using ff)
Blocks: 679905
Whiteboard: [sg:dos] stack-overflow [needs patch] → [sg:dos][stack-overflow][needs patch][tbird crash]
Duplicate of this bug: 893036
Whiteboard: [sg:dos][stack-overflow][needs patch][tbird crash] → [sg:dos][stack-exhaustion][needs patch][tbird crash]

Comment 56

4 years ago
Please remember may this bug duplicate but still works and crash Mozilla even 26 version .

1.http://niebezpiecznik.pl/PoC/Firefox_3.6/poc.html

2.http://niebezpiecznik.pl/PoC/Opera_10.10/poc.html

3.http://taboca.github.com/TelaSocial-Grid-Type/test.html

4.https://bugzilla.mozilla.org/attachment.cgi?id=539481&action=edit
Duplicate of this bug: 1100059

Updated

2 years ago
Crash Signature: [@ SelectorMatches ] [@ nsBindingManager::RemovedFromDocument(nsIContent*, nsIDocument*) ] [@ nsGenericElement::GetChildAt(unsigned int) ] [@ nsGenericElement::UnbindFromTree(bool, bool) ] [@ nsGenericElement::UnbindFromTree(int, int) ] [@ nsX&hellip; → [@ SelectorMatches ] [@ nsBindingManager::RemovedFromDocument(nsIContent*, nsIDocument*) ] [@ nsGenericElement::GetChildAt(unsigned int) ] [@ nsGenericElement::UnbindFromTree(bool, bool) ] [@ nsGenericElement::UnbindFromTree(int, int) ] [@ nsX&hellip;
Blocks: 1196167

Comment 58

a year ago
Created attachment 8759052 [details]
stack_overflow_test_case.zip

Investigating a crash a friend encountered when visiting this URL http://forum.palemoon.org/ appears to be very similar in nature to this current bug, Added test case attachment.

Crash report in latest nightly https://crash-stats.mozilla.com/report/index/09dcb3fc-8108-49fb-9e23-848e82160602 but affects right down to 38.0 maybe lower.

This issue also affects google chrome.

Updated

a year ago
Duplicate of this bug: 1277517

Comment 60

a year ago
More crash reports that are related to this issue:
https://crash-stats.mozilla.com/report/index/077b961b-178c-4e8a-9e0b-9ed172160601
https://crash-stats.mozilla.com/report/index/12c1fac2-a5ce-4c84-98ea-025fa2160601
https://crash-stats.mozilla.com/report/index/c81db66e-6e13-4f64-962b-683192160601

These crashes also occurred on http://forum.palemoon.org/. The site owner has confirmed that this bug was the cause of the crashes, and has modified the web site to work around the problem, so that particular site will no longer cause crashes.

The same problem was experienced with version FF 46.0.1 and 38.8.0 ESR.
I think this is showing up in Nightly as [@ mozilla::dom::Element::UnbindFromTree ], which is a stack overflow, with Pale Moon URLs.
Crash Signature: [@ SelectorMatches ] [@ nsBindingManager::RemovedFromDocument(nsIContent*, nsIDocument*) ] [@ nsGenericElement::GetChildAt(unsigned int) ] [@ nsGenericElement::UnbindFromTree(bool, bool) ] [@ nsGenericElement::UnbindFromTree(int, int) ] [@ nsX&hellip; → [@ SelectorMatches ] [@ nsBindingManager::RemovedFromDocument(nsIContent*, nsIDocument*) ] [@ nsGenericElement::GetChildAt(unsigned int) ] [@ nsGenericElement::UnbindFromTree(bool, bool) ] [@ nsGenericElement::UnbindFromTree(int, int) ] [@ nsX&hellip;

Comment 62

a year ago
The attached test case https://bugzilla.mozilla.org/show_bug.cgi?id=485941#c58 was extracted from the sites html, It contains the iframe and the xml file that was embedded in there forums html code.

<iframe width="970" height="2" style="opacity:0;" src="/BingSiteData.xml"></iframe>

They have removed this iframe form there forum html resolving the crash on there site.

So if you need to test for the crash, The attached case can reproduce it for you.
Duplicate of this bug: 1354861
Duplicate of this bug: 1355411

Updated

2 months ago
Duplicate of this bug: 1367922

Updated

a month ago
Duplicate of this bug: 1373982
You need to log in before you can comment on or make changes to this bug.