849597 - Crash when inline script in an XML doc framed by <object> removes the <object>

Status: VERIFIED FIXED

(Core :: XML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Nightly: [@ MOZ_XML_GetCurrentColumnNumber ] 
  bp-66cc9dcd-b536-4a1e-bf23-a39492130310
Debug+Opt: [@ nsXMLContentSink::CloseElement] touching null
ASan+Debug: [@ nsXMLContentSink::CloseElement] reading freed memory

Comment 1

[Jesse Ruderman]

Attachment: stack (breakpad)

Comment 2

[Jesse Ruderman]

Attachment: stacks (ASan)

Comment 3

[Henri Sivonen (:hsivonen)]

This is weird. Attachment 723155 [details] suggests that after the script has been executed, the nsParser object pointed to by mParser in nsContentSink has been freed but the refcounting mParser is still a non-null pointer.

Comment 4

[Henri Sivonen (:hsivonen)]

So removing the <object> manages to destroy the XML parser that's the active parser of the document framed by the <object> even though there is a ref counting reference to the parser from the sink, so that should be enough of a kung fu death grip. I wonder if in fact even the sink itself has been freed at that point, its memory reused and that's why its mParser has gone bad.

Comment 5

[Benjamin Smedberg]

johns/Henri, which one of you wants this?

Comment 6

[Benjamin Smedberg]

With <iframe src=> instead of <object data=>, in a debug build, assertions:

NS_ASSERTION(mSink, "no sink?");

>	xul.dll!nsExpatDriver::HandleError()  Line 929	C++
 	xul.dll!nsExpatDriver::ConsumeToken(aScanner={...}, aFlushTokens=false)  Line 1165	C++
 	xul.dll!nsParser::Tokenize(aIsFinalChunk=true)  Line 2042	C++
 	xul.dll!nsParser::ResumeParse(allowIteration=true, aIsFinalChunk=true, aCanInterrupt=true)  Line 1532	C++
 	xul.dll!nsParser::OnStopRequest(request=0x0b3181c0, aContext=0x00000000, status=NS_OK)  Line 1962	C++
 	xul.dll!nsDocumentOpenInfo::OnStopRequest(request=0x0b3181c0, aCtxt=0x00000000, aStatus=NS_OK)  Line 298	C++
 	xul.dll!nsBaseChannel::OnStopRequest(request=0x0ad6dca8, ctxt=0x00000000, status=NS_OK)  Line 738	C++
 	xul.dll!nsInputStreamPump::OnStateStop()  Line 556	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(stream=0x0b288270)  Line 375	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 83	C++

But no subsequent crash.

I suspect that nsObjectLoadingContent::UnbindFromTree needs logic similar to nsGenericHTMLFrameElement::UnbindFromTree (which calls mFrameLoader->Destroy). Currently that is in nsObjectLoadingContent::DestroyContent.

Comment 7

[Jesse Ruderman]

For the "no sink" assertion, see also bug 620265.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Henri can you take this one?

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Matt agreed to try for a window.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Correction, not a 'window' just check branches please.

Comment 11

[Matt Wobensmith [:mwobensmith][:matt:]]

All branches affected, on desktop at least.

Comment 12

[Henri Sivonen (:hsivonen)]

I'll take a look.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> I suspect that nsObjectLoadingContent::UnbindFromTree needs logic similar to
> nsGenericHTMLFrameElement::UnbindFromTree (which calls
> mFrameLoader->Destroy).

nsObjectLoadingContent::UnbindFromTree already calls nsObjectLoadingContent::UnloadObject which calls mFrameLoader->Destroy.

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: Add more kung fu

So what happens here is that the removal of the browsing context breaks the reference from the sink to the parser and the script element's reference to its creator parser is the last reference to nsParser, so nsParser gets freed from nsScriptLoader and then the sink gets freed from the parser.

The patch adds death grips to appropriate places to make sure that the stack gets to unwind before the objects whose methods are on the stack are freed.

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 730714 [details] [diff] [review]
Add more kung fu

Oops. Explaining the issue made me realize I added useless kung fu.

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 730714 [details] [diff] [review]
Add more kung fu

Never mind. The patch was right after all. Dropping the death grip from the sink made the test case crash again, so both death grips are needed.

Comment 17

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

Comment on attachment 730714 [details] [diff] [review]
Add more kung fu

Is there some reason why caller can't keep 
XMLContentSink and/or Parser alive?
The common XPCOM rule is that caller should keep callee alive, but
that can be tricky to do in some cases.

Assuming this is simpler approach, r=me

Comment 18

[Henri Sivonen (:hsivonen)]

Attachment: Add more kung fu, v2

(In reply to Olli Pettay [:smaug] from comment #17)
> Is there some reason why caller can't keep 
> XMLContentSink and/or Parser alive?
> The common XPCOM rule is that caller should keep callee alive, but
> that can be tricky to do in some cases.

No good reason for the sink. This new patch moves the sink death grip to nsParser, which should be better for performance.

As for why the caller doesn't keep nsParser alive, I don't know. That nsParser needs to use kungFuDeathGrip to keep itself alive seems common enough a pattern that it's probably the easiest to follow the pattern instead of changing how things work.

This patch also takes into account the case where there has been an external script before the crashing object and, therefore, the ResumeParse() call does not come from OnDataAvailable.

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 732282 [details] [diff] [review]
Add more kung fu, v2

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

From the patch, it's easy to conclude that without the patch, there's a way to get the parser and sink freed while there are parser/sink methods on the call stack. That you need to use <object> is not obvious and in no way visible from the patch.

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

Not any more than the actual fix. (The test case is to be landed later separately from the fix.) The check-in comment could be edited not to say XML to avoid giving hints to people who don't realize that nsParser is for XML these days.

Which older supported branches are affected by this flaw?

All of them.

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

The same patch applies.

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

Comment 20

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

Comment on attachment 732282 [details] [diff] [review]
Add more kung fu, v2

sec-approval+.

Please also make patches and nom it for affected branches.

Comment 21

[Henri Sivonen (:hsivonen)]

Attachment: Patch as landed

Landed with the comment changed not to hint at XML:
https://hg.mozilla.org/integration/mozilla-inbound/rev/905671d954c5

Attaching a patch with the edited commit comment and r=smaug in the commit comment for easy landing on branches.

I'll request approvals once the tinderboxes have cycled a bit.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/905671d954c5

Comment 23

[Henri Sivonen (:hsivonen)]

Comment on attachment 733816 [details] [diff] [review]
Patch as landed

[Approval Request Comment]
Fix Landed on Version: 23
Bug caused by (feature/regressing bug #): Problem believed to have been there practically forever.
User impact if declined: There's a reliable way to treat freed memory is be treated as if belonged to live objects, which might be exploitable.
Testing completed: Landed on m-c, didn't bounce. Locally seems to fix the crash.
Risk to taking this patch (and alternatives if risky): Very low risk. Adds a couple of kung fu death grips.
String or UUID changes made by this patch: None.

Comment 24

[Henri Sivonen (:hsivonen)]

If the patch get approved for branches, please consider checkin-needed for those branches as approved.

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 733816 [details] [diff] [review]
Patch as landed

Low risk patch, helps avoid a sec-crit crasher, has had a few days baketime on m-c.Approving on all branches .

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/484d8270f47a
https://hg.mozilla.org/releases/mozilla-beta/rev/60f5c3392c4c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0c6effe4d05f
https://hg.mozilla.org/releases/mozilla-esr17/rev/09bb8f0316e9

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

Reproduced on FF17.0.5esr
Confirmed fixed on FF17.0.6esr candidate build 1
Confirmed fixed on FF21 candidate build 2
Confirmed fixed on FF22 2013-05-09
Confirmed fixed on FF23 2013-05-09