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

VERIFIED FIXED in Firefox 21

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jruderman, Assigned: hsivonen)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla23
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox19 wontfix, firefox20 wontfix, firefox21+ verified, firefox22+ verified, firefox23+ verified, firefox-esr1721+ verified, b2g1821+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

Details

(Whiteboard: [adv-main21+][adv-esr1706+])

Attachments

(4 attachments, 2 obsolete attachments)

Nightly: [@ MOZ_XML_GetCurrentColumnNumber ] 
  bp-66cc9dcd-b536-4a1e-bf23-a39492130310
Debug+Opt: [@ nsXMLContentSink::CloseElement] touching null
ASan+Debug: [@ nsXMLContentSink::CloseElement] reading freed memory
Posted file stack (breakpad)
Posted file stacks (ASan)
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.
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.
Summary: Crash when inline script in <object> removes the <object> → Crash when inline script in an XML doc framed by <object> removes the <object>
johns/Henri, which one of you wants this?
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.
For the "no sink" assertion, see also bug 620265.
Henri can you take this one?
Assignee: nobody → hsivonen
Matt agreed to try for a window.
Flags: needinfo?(mwobensmith)
Correction, not a 'window' just check branches please.
Flags: needinfo?(mwobensmith)
Flags: needinfo?(mwobensmith)
All branches affected, on desktop at least.
Flags: needinfo?(mwobensmith)
I'll take a look.
Status: NEW → ASSIGNED
(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.
Posted patch Add more kung fu (obsolete) — Splinter Review
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.
Attachment #730714 - Flags: review?(bugs)
Comment on attachment 730714 [details] [diff] [review]
Add more kung fu

Oops. Explaining the issue made me realize I added useless kung fu.
Attachment #730714 - Attachment is obsolete: true
Attachment #730714 - Flags: review?(bugs)
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.
Attachment #730714 - Attachment is obsolete: false
Attachment #730714 - Flags: review?(bugs)
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
Attachment #730714 - Flags: review?(bugs) → review+
Posted patch Add more kung fu, v2 (obsolete) — Splinter Review
(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.
Attachment #730714 - Attachment is obsolete: true
Attachment #732282 - Flags: review?(bugs)
Attachment #732282 - Flags: review?(bugs) → review+
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?
Attachment #732282 - Flags: sec-approval?
Comment on attachment 732282 [details] [diff] [review]
Add more kung fu, v2

sec-approval+.

Please also make patches and nom it for affected branches.
Attachment #732282 - Flags: sec-approval? → sec-approval+
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.
Attachment #732282 - Attachment is obsolete: true
Attachment #733816 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/905671d954c5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
Attachment #733816 - Flags: approval-mozilla-esr17?
Attachment #733816 - Flags: approval-mozilla-beta?
Attachment #733816 - Flags: approval-mozilla-b2g18?
Attachment #733816 - Flags: approval-mozilla-aurora?
If the patch get approved for branches, please consider checkin-needed for those branches as approved.
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 .
Attachment #733816 - Flags: approval-mozilla-esr17?
Attachment #733816 - Flags: approval-mozilla-esr17+
Attachment #733816 - Flags: approval-mozilla-beta?
Attachment #733816 - Flags: approval-mozilla-beta+
Attachment #733816 - Flags: approval-mozilla-b2g18?
Attachment #733816 - Flags: approval-mozilla-b2g18+
Attachment #733816 - Flags: approval-mozilla-aurora?
Attachment #733816 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main21+][adv-esr1706+]
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
Group: core-security
You need to log in before you can comment on or make changes to this bug.