930281 - (CVE-2013-6671) SEGV in libxul.so!nsGfxScrollFrameInner::IsLTR()

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: av_1.html

Found by the BlackBerry Security Automated Analysis Team's fuzzing framework ALF.

==30229==ERROR: AddressSanitizer: SEGV on unknown address 0x00011fff8000 (pc 0x7f45f6cafc26 sp 0x7fff5a2f0220 bp 0x7fff5a2f02d0 T0)
AddressSanitizer can not provide additional info.
    #0 0x7f45f6cafc25 (libxul.so!nsGfxScrollFrameInner::IsLTR() const+0x2c5)
        Line 3411 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/generic/nsGfxScrollFrame.cpp"
    #1 0x7f45f6c91da3 (libxul.so!nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&)+0x643)
        Line 4129 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/generic/nsGfxScrollFrame.cpp"
    #2 0x7f45f6c93cad (libxul.so!nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&)+0xbad)
        Line 795 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/generic/nsGfxScrollFrame.cpp"
    #3 0x7f45f6c16a33 (libxul.so!nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*)+0x113)
        Line 961 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/generic/nsContainerFrame.cpp"
    #4 0x7f45f6db94a7 (libxul.so!ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&)+0x6e7)
        Line 221 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/generic/nsViewportFrame.cpp"
    #5 0x7f45f6b02cea (libxul.so!PresShell::DoReflow(nsIFrame*, bool)+0x96a)
        Line 7905 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp"
    #6 0x7f45f6b1441b (libxul.so!PresShell::ProcessReflowCommands(bool)+0x25b)
        Line 8046 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp"
    #7 0x7f45f6b13d35 (libxul.so!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)+0xa25)
        Line 3869 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp"
    #8 0x7f45f6b456e4 (libxul.so!nsRefreshDriver::Tick(long, mozilla::TimeStamp)+0x1d54)
        Line 1159 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #9 0x7f45f6b4a4e0 (libxul.so!mozilla::RefreshDriverTimer::Tick()+0x1f0)
        Line 168 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #10 0x7f45fa638c31 (libxul.so!nsTimerImpl::Fire()+0x6d1)
        Line 546 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #11 0x7f45fa6392d6 (libxul.so!nsTimerEvent::Run()+0x66)
        Line 630 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #12 0x7f45fa630019 (libxul.so!nsThread::ProcessNextEvent(bool, bool*)+0xaa9)
        Line 622 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp"
    #13 0x7f45fa55c371 (libxul.so!NS_ProcessNextEvent(nsIThread*, bool)+0xb1)
        Line 251 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp"
    #14 0x7f45f91a9091 (libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+0x311)
        Line 85 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp"
    #15 0x7f45fa74b653 (libxul.so!MessageLoop::Run()+0x1c3)
        Line 220 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc"
    #16 0x7f45f8f87cac (libxul.so!nsBaseAppShell::Run()+0x5c)
        Line 161 of "/builds/slave/m-in-l64-asan-0000000000000000/build/widget/xpwidgets/nsBaseAppShell.cpp"
    #17 0x7f45f8989d9e (libxul.so!nsAppStartup::Run()+0xbe)
        Line 268 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/components/startup/nsAppStartup.cpp"
    #18 0x7f45f5f131c5 (libxul.so!XREMain::XRE_mainRun()+0x1e05)
        Line 3886 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #19 0x7f45f5f140fa (libxul.so!XREMain::XRE_main(int, char**, nsXREAppData const*)+0x4fa)
        Line 3954 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #20 0x7f45f5f1502b (libxul.so!XRE_main+0x3ab)
        Line 4156 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #21 0x459d1d (firefox!main+0x94d)
        Line 275 of "/builds/slave/m-in-l64-asan-0000000000000000/build/browser/app/nsBrowserApp.cpp"
    #22 0x7f460568876c (libc.so.6!__libc_start_main+0xec)
        Line 226 of "libc-start.c"
    #23 0x45929c (firefox!_start+0x28)
==30229==ABORTING

Comment 1

[Daniel Veditz [:dveditz]]

crashes in a nightly build, too-- no need for a special asan build to debug
bp-0cd68a22-a922-490c-9425-a20d02131024

Comment 2

[Daniel Veditz [:dveditz]]

This is the same stack as bug 906301 which we fixed on trunk in nightly 26 and shipped with Firefox 24. Regression? or just coincidentally similar? Will have to check older releases to know.

Comment 3

[Daniel Veditz [:dveditz]]

Just tested in today's Aurora (26.0a2 2013-10-13) and bug 906301 remains fixed but the testcase in this bug does still cause a crash bp-0c5fa274-2515-4df9-952a-d0ec22131024

Interestingly the crash address is the same. Is that controllable from the testcase, or a constant?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Looking

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Interesting piece is here, I think.

#8  0x00007fe6ea932455 in NS_DebugBreak (aSeverity=1, aStr=0x7fe6ec4da340 "Inserting node that already has parent", aExpr=0x7fe6ec4da323 "!aKid->GetParentNode()", aFile=
    0x7fe6ec4d9bf0 "/home/smaug/mozilla/hg/mozilla/content/base/src/nsINode.cpp", aLine=1349) at /home/smaug/mozilla/hg/mozilla/xpcom/base/nsDebugImpl.cpp:417
#9  0x00007fe6e8de72c7 in nsINode::doInsertChildAt (this=0x7fe6cbe64340, aKid=0x7fe6cbe64660, aIndex=2, aNotify=false, aChildArray=...) at /home/smaug/mozilla/hg/mozilla/content/base/src/nsINode.cpp:1348
#10 0x00007fe6e8cd7728 in mozilla::dom::FragmentOrElement::InsertChildAt (this=0x7fe6cbe64340, aKid=0x7fe6cbe64660, aIndex=2, aNotify=false)
    at /home/smaug/mozilla/hg/mozilla/content/base/src/FragmentOrElement.cpp:954
#11 0x00007fe6e8845540 in nsINode::AppendChildTo (this=0x7fe6cbe64340, aKid=0x7fe6cbe64660, aNotify=false) at ../../dist/include/nsINode.h:572
#12 0x00007fe6e964d7b8 in nsHtml5TreeOperation::Append (this=0x7fe6c28dd458, aNode=0x7fe6cbe64660, aParent=0x7fe6cbe64340, aBuilder=0x7fe6bd4fbb40)
    at /home/smaug/mozilla/hg/mozilla/parser/html/nsHtml5TreeOperation.cpp:189
#13 0x00007fe6e964dac6 in nsHtml5TreeOperation::Perform (this=0x7fe6c28dd458, aBuilder=0x7fe6bd4fbb40, aScriptElement=0x7fff51ad3de8) at /home/smaug/mozilla/hg/mozilla/parser/html/nsHtml5TreeOperation.cpp:238
#14 0x00007fe6e9648dd6 in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x7fe6bd4fbb40) at /home/smaug/mozilla/hg/mozilla/parser/html/nsHtml5TreeOpExecut

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

This seems to go down deep into the parser land.
We're adding ol to body when ol is already in document.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch for the crash

Henri, I think we really must do at least this (parser is using those
low level DOM APIs which don't do this kind of checks), but
should we do something else too in the parser?

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 821640 [details] [diff] [review]
patch for the crash

>diff --git a/parser/html/nsHtml5TreeOperation.cpp b/parser/html/nsHtml5TreeOperation.cpp
>--- a/parser/html/nsHtml5TreeOperation.cpp
>+++ b/parser/html/nsHtml5TreeOperation.cpp
>@@ -179,16 +179,18 @@ nsHtml5TreeOperation::Append(nsIContent*
>                              nsHtml5TreeOpExecutor* aBuilder)
> {
>   nsresult rv = NS_OK;
>   nsIDocument* executorDoc = aBuilder->GetDocument();
>   NS_ASSERTION(executorDoc, "Null doc on executor");
>   nsIDocument* parentDoc = aParent->OwnerDoc();
>   NS_ASSERTION(parentDoc, "Null owner doc on old node.");
> 
>+  NS_ENSURE_STATE(!aNode->GetParentNode());

An early return is not OK. Is nsINode::AppendChildTo not guaranteed to remove from old parent like the public DOM API? If not, we should remove the node from the old parent if there is one.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

AppendChildTo doesn't guarantee anything.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Could we use the less-low level DOM API in this case which does reparenting?

But I still don't understand how parser can still hold to <ol> when it has been in the document
in such way that getElementById finds it.

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Could we use the less-low level DOM API in this case which does reparenting?

Possibly. I'll need to take a closer look at the AAA.

> But I still don't understand how parser can still hold to <ol> when it has
> been in the document
> in such way that getElementById finds it.

When the second <nobr> is seen, <ol> should be in the parser stack. I'm not sure if I'm replying to what you meant to ask.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Henri, could you take this?

If that code in parser starts to use the less low-level DOM APIs, 
###!!! ASSERTION: Want to fire DOMNodeRemoved event, but it's not safe: '(aChild->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aChild)-> IsInNativeAnonymousSubtree()) || IsSafeToRunScript() || sDOMNodeRemovedSuppressCount', file /home/smaug/mozilla/hg/mozilla/content/base/src/nsContentUtils.cpp, line 3700
starts to fire

(In reply to Henri Sivonen (:hsivonen) from comment #11)
> When the second <nobr> is seen, <ol> should be in the parser stack. I'm not
> sure if I'm replying to what you meant to ask.
Well, apparently the script has accessed the <ol> and can then do whatever with it.
Why does the parser still want to append it to some element?

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Henri, could you take this?

OK.

> If that code in parser starts to use the less low-level DOM APIs, 
> ###!!! ASSERTION: Want to fire DOMNodeRemoved event, but it's not safe:
> '(aChild->IsNodeOfType(nsINode::eCONTENT) &&
> static_cast<nsIContent*>(aChild)-> IsInNativeAnonymousSubtree()) ||
> IsSafeToRunScript() || sDOMNodeRemovedSuppressCount', file
> /home/smaug/mozilla/hg/mozilla/content/base/src/nsContentUtils.cpp, line 3700
> starts to fire

That's the reason not to use lower-level APIs.

> (In reply to Henri Sivonen (:hsivonen) from comment #11)
> > When the second <nobr> is seen, <ol> should be in the parser stack. I'm not
> > sure if I'm replying to what you meant to ask.
> Well, apparently the script has accessed the <ol> and can then do whatever
> with it.
> Why does the parser still want to append it to some element?

If this is about <ol>  being the elements that is appended, that's indeed  weird.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #8)
> If not, we should remove the node from the old parent if there is one.

We try to, but the relevant code only works if the parent is nsIContent. Here, the parent is a document.
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#243

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> If this is about <ol>  being the elements that is appended, that's indeed 
> weird.

Actually not weird. Just AAA as usual, when the second <nobr> triggers the AAA.

Comment 15

[Henri Sivonen (:hsivonen)]

Attachment: Ask for nsINode parent instead of nsIContent parent

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Crashtest to be landed later

Comment 17

[Henri Sivonen (:hsivonen)]

Safe to say that all branches are affected.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Please ask for sec-approval? before checkin.

Comment 20

[Henri Sivonen (:hsivonen)]

Comment on attachment 825306 [details] [diff] [review]
Ask for nsINode parent instead of nsIContent parent

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Once you realize that you can't access the bug whose number the commit message gives, it's easy to guess that the change is security-relevant and then it's very easy to work back from the change to HTML+JS that triggers the crash. It's unclear to me how easy it is to write an exploit more malicious than the denial of service resulting from the crash, but the sec-critical rating, of course, assumes that it's plausible that you can write one.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The test is to be landed later, because it makes things even more obvious than the fix.

> Which older supported branches are affected by this flaw?

All. This bug goes all the way back to Firefox 4.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The same patch should apply to all branches.

> How likely is this patch to cause regressions; how much testing does it need?

Very unlikely to cause regressions. The fix doesn't affect previously non-crashing cases at all (other than the vtable difference of using a superclass type).

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 825306 [details] [diff] [review]
Ask for nsINode parent instead of nsIContent parent

sec-approval+ for landing on November 12 or later. 

You should probably prepare and nominate Aurora, Beta, and ESR24 patches since this is a sec-critical.

Comment 22

[Henri Sivonen (:hsivonen)]

Comment on attachment 825306 [details] [diff] [review]
Ask for nsINode parent instead of nsIContent parent

(In reply to Al Billings [:abillings] from comment #21)
> You should probably prepare and nominate Aurora, Beta, and ESR24 patches
> since this is a sec-critical.

The same patch applies cleanly to all those branches. Can I expect RyanVM to take care of figuring out what to do with B2G?

[Approval Request Comment]
> Bug caused by (feature/regressing bug #): 

Bug 487949.

> User impact if declined: 

Web content can crash the browser in a potentially exploitable way.

> Testing completed (on m-c, etc.): 

Not landed on m-c due to security embargo, but the fix is so simple it shouldn't really need testing.

> Risk to taking this patch (and alternatives if risky): 

Very low risk. Previously non-crashing cases are unaffected.

> String or IDL/UUID changes made by this patch:

None.

Comment 23

[Henri Sivonen (:hsivonen)]

(In reply to Al Billings [:abillings] from comment #21)
> sec-approval+ for landing on November 12 or later. 

Thanks. Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/e93c7c45903f

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e93c7c45903f

Comment 25

[Henri Sivonen (:hsivonen)]

Thanks for the approvals. Landed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb748b480402
https://hg.mozilla.org/releases/mozilla-beta/rev/14ed3e7e606e
https://hg.mozilla.org/releases/mozilla-esr24/rev/faf5b54bb7cf

And thanks to the reporter for finding this subtle bug!

RyanVM, will you land this to the appropriate B2G branches?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5a2f9444ccf5

Comment 27

[Ryan VanderMeulen [:RyanVM]]

s/GetParentNode/GetNodeParent because b2g18 is "special" like that.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7c3cfc0936ca

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5a2f9444ccf5
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7c3cfc0936ca

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/14ed3e7e606e

Comment 30

[Ioana (away)]

Reproduced the crash on a Firefox 25 ASAN build and on a Firefox 24ESR debug build.

When reproducing the crash, I didn't get the error in comment 0, I just got the following assertions:
###!!! ASSERTION: Inserting node that already has parent: '!aKid->GetParentNode()', file ../../../../content/base/src/nsINode.cpp, line 1351
###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc()', file ../../../../content/base/src/Element.cpp, line 907
###!!! ASSERTION: Shouldn't be here!: '!(IsNodeOfType(eCONTENT) && IsInDoc())', file ../../../dist/include/nsINode.h, line 1464
###!!! ASSERTION: aDocument must be current doc of aParent: '!aParent || aDocument == aParent->GetCurrentDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 446
###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc() && !IsInDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 448
###!!! ASSERTION: These should always be in sync!: 'slowNode == node', file ../../../dist/include/nsINode.h, line 794
###!!! ASSERTION: Shouldn't be here!: '!(IsNodeOfType(eCONTENT) && IsInDoc())', file ../../../dist/include/nsINode.h, line 1464
###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 521
###!!! ASSERTION: aDocument must be current doc of aParent: '!aParent || aDocument == aParent->GetCurrentDoc()', file ../../../../content/base/src/Element.cpp, line 906
###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc()', file ../../../../content/base/src/Element.cpp, line 907
###!!! ASSERTION: These should always be in sync!: 'slowNode == node', file ../../../dist/include/nsINode.h, line 794
###!!! ASSERTION: Shouldn't be here!: '!(IsNodeOfType(eCONTENT) && IsInDoc())', file ../../../dist/include/nsINode.h, line 1464
###!!! ASSERTION: aDocument must be current doc of aParent: '!aParent || aDocument == aParent->GetCurrentDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 446
###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc() && !IsInDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 448
###!!! ASSERTION: These should always be in sync!: 'slowNode == node', file ../../../dist/include/nsINode.h, line 794
###!!! ASSERTION: Shouldn't be here!: '!(IsNodeOfType(eCONTENT) && IsInDoc())', file ../../../dist/include/nsINode.h, line 1464
###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 521
###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file ../../../../content/base/src/Element.cpp, line 1075
###!!! ASSERTION: aDocument must be current doc of aParent: '!aParent || aDocument == aParent->GetCurrentDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 446
###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc() && !IsInDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 448
###!!! ASSERTION: These should always be in sync!: 'slowNode == node', file ../../../dist/include/nsINode.h, line 794
###!!! ASSERTION: Shouldn't be here!: '!(IsNodeOfType(eCONTENT) && IsInDoc())', file ../../../dist/include/nsINode.h, line 1464
###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file ../../../../content/base/src/nsGenericDOMDataNode.cpp, line 521
###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file ../../../../content/base/src/Element.cpp, line 1075

The assertions and crash are gone on:
ASAN builds:
Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (20131113065829)
Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (20131114004000)
Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (20131113030205)
Debug build:
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20131113 Firefox/24.0 (20131113074329)

Considering that the assertions look quite different to those in comment 0, I need someone to confirm whether this is the same bug or I verified something else.

Comment 31

[Henri Sivonen (:hsivonen)]

(In reply to Ioana Budnar, QA [:ioana] from comment #30)
> When reproducing the crash, I didn't get the error in comment 0, I just got
> the following assertions:
> ###!!! ASSERTION: Inserting node that already has parent:
> '!aKid->GetParentNode()', file ../../../../content/base/src/nsINode.cpp,
> line 1351

This is expected behavior when the fix for this bug has not yet been applied.