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

VERIFIED FIXED in Firefox 21

Status

()

Core
XML
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: hsivonen)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla23
x86_64
Mac OS X
crash, csectype-uaf, sec-critical, testcase
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)

(Reporter)

Description

5 years ago
Created attachment 723152 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 723154 [details]
stack (breakpad)
(Reporter)

Comment 2

5 years ago
Created attachment 723155 [details]
stacks (ASan)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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>

Comment 5

5 years ago
johns/Henri, which one of you wants this?

Comment 6

5 years ago
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.
(Reporter)

Comment 7

5 years ago
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)
Keywords: regressionwindow-wanted
Correction, not a 'window' just check branches please.
Flags: needinfo?(mwobensmith)
Keywords: regressionwindow-wanted
Flags: needinfo?(mwobensmith)
All branches affected, on desktop at least.
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox-esr17: --- → affected
Flags: needinfo?(mwobensmith)
(Assignee)

Comment 12

5 years ago
I'll take a look.
Status: NEW → ASSIGNED
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 730714 [details] [diff] [review]
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.
Attachment #730714 - Flags: review?(bugs)
(Assignee)

Comment 15

5 years ago
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)
(Assignee)

Comment 16

5 years ago
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)
tracking-firefox21: --- → ?
tracking-firefox22: --- → +
tracking-firefox-esr17: --- → ?
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+

Updated

5 years ago
tracking-firefox21: ? → +
(Assignee)

Comment 18

5 years ago
Created attachment 732282 [details] [diff] [review]
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.
Attachment #730714 - Attachment is obsolete: true
Attachment #732282 - Flags: review?(bugs)

Updated

5 years ago
Attachment #732282 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

5 years ago
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+
tracking-firefox23: --- → +
status-b2g18: --- → affected
status-firefox19: affected → wontfix
status-firefox20: affected → wontfix
status-firefox23: --- → affected
tracking-b2g18: --- → 21+
tracking-firefox-esr17: ? → 21+
(Assignee)

Comment 21

5 years ago
Created attachment 733816 [details] [diff] [review]
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.
Attachment #732282 - Attachment is obsolete: true
Attachment #733816 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/905671d954c5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
status-firefox23: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 23

5 years ago
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?
(Assignee)

Comment 24

5 years ago
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+

Updated

5 years ago
Keywords: checkin-needed
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
status-b2g18: affected → fixed
status-firefox21: affected → fixed
status-firefox22: affected → fixed
status-firefox-esr17: affected → fixed
Keywords: checkin-needed
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
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified
status-firefox22: fixed → verified
status-firefox23: fixed → verified
status-firefox-esr17: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.