Closed Bug 592829 Opened 9 years ago Closed 9 years ago

Crash: DOMParser.parseFromString [@ nsXMLContentSink::CloseElement ]

Categories

(Core :: XML, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: mash, Assigned: sicking)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100901 Firefox/4.0b6pre
Build Identifier: 

Components.classes["@mozilla.org/xmlextras/domparser;1"].getService(Components.interfaces.nsIDOMParser).parseFromString('<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"/>', "text/xml");
=> crash



Reproducible: Always




Talkback crash ID: 79d414fa-83ba-43aa-87f5-bda8a2100901
Status: UNCONFIRMED → NEW
blocking2.0: --- → beta6+
Ever confirmed: true
Keywords: crash
Also: parsing error

Components.classes["@mozilla.org/xmlextras/xmlserializer;1"].getService(Components.interfaces.nsIDOMSerializer).serializeToString(Components.classes["@mozilla.org/xmlextras/domparser;1"].getService(Components.interfaces.nsIDOMParser).parseFromString('<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"></overlay>', "text/xml"));

XML Parsing Error: 
Location: moz-nullprincipal:{c50036f4-d4b7-5c41-baf7-88c1ea500b38}
Line Number 1, Column 80:<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"></overlay>
The parsing error is correct for now; you can't use XUL from untrusted content anymore.  The crash is clearly a bug.
In the example above the DOMParser is accessed from a JS XPCOM component, so it should not be an "untrusted content". Should it?
If you create the DOMParser using createInstance and then never call init() on it, it defaults to a sandboxed parser with no privileges whatsoever.
Though you might need bug 593026 to get the right principal to initialize with.
What if I pass null as principal to parser's init?
What all this fuss is about if a code (XPCOM component) already runs in privileged mode? It can access a whole lot of unsafe things. Such as Components.classes etc. It can erase files and eat my dog. So why can't it simply parse a piece of XML without piercing through superfluous obstacles?
BTW I could not find a sane documentation on nsIPrincipal concerning what it actually is and how it can be obtained.
> What if I pass null as principal to parser's init?

Then you'll get the untrusted behavior.

> What all this fuss is about

The assumption is that you're possibly parsing data you don't trust and don't want to give it chrome permissions by accident.  So if you trust the data you're parsing, you need to explicitly say so.
OK, I think I get it now. So you mean, a web page will be unable to parse XML containig nodes in XUL namespace?
Boris, thank you. createInstance with init() is working.
> a web page will be unable to parse XML containig nodes in XUL namespace?

At the moment, that is correct.  There's some argument happening about it, though.
(In reply to comment #10)
> > a web page will be unable to parse XML containig nodes in XUL namespace?
> 
> At the moment, that is correct.  There's some argument happening about it,
> though.

It's unlikely to be changed. Allowing nodes in the XUL namespace to be instantiated would be an enormous amount of work, both now and ongoing in the future. It does not appear that this amount of work will be worth it.
> > > a web page will be unable to parse XML containig nodes in XUL namespace?
> > At the moment, that is correct.
> It's unlikely to be changed.
This is unfair from my point of view. Parsing means just getting a DOM. This DOM does not necessary need to be "executed" (like displayings a XUL window). I understand that security is a primary objective, though disalowing to parse _some_ kinds of XML looks... weird. It reminds me a joke that it's a syntax error to write fortran code while not wearing a blue tie.
Assigning to jst@ since it's a b7 blocker without an owner ...
Assignee: nobody → jst
same as crashes below?  which are 90% startup crashes

nsXMLContentSink::CloseElement(nsIContent*) (windows)
bp-330f41ca-4b41-415c-bec8-3a5762100910

nsXMLContentSink::CloseElement (linux)
bp-ab1790c0-3d7d-4da2-9a06-07b102100909


comment 0 bp-79d414fa-83ba-43aa-87f5-bda8a2100901 is
0    XUL    nsXMLContentSink::CloseElement     nsCOMPtr.h:800
1    XUL    nsXMLContentSink::HandleEndElement    content/xml/document/src/nsXMLContentSink.cpp:1137
2    XUL    nsXMLContentSink::HandleEndElement    content/xml/document/src/nsXMLContentSink.cpp:1102
3    XUL    Driver_HandleEndElement    parser/htmlparser/src/nsExpatDriver.cpp:425
4    XUL    doContent    parser/expat/lib/xmlparse.c:2470
5    XUL    contentProcessor    parser/expat/lib/xmlparse.c:2095
6    XUL    doProlog    parser/expat/lib/xmlparse.c:4075
7    XUL    prologInitProcessor    parser/expat/lib/xmlparse.c:3809
8    XUL    MOZ_XML_Parse    parser/expat/lib/xmlparse.c:1528
9    XUL    nsExpatDriver::ParseBuffer    parser/htmlparser/src/nsExpatDriver.cpp:1003
10    XUL    nsExpatDriver::ConsumeToken    parser/htmlparser/src/nsExpatDriver.cpp:1101
Severity: normal → critical
Over to Jonas who did all the remote XUL disabling work.
Assignee: jst → jonas
Attached patch Patch to fix (obsolete) — Splinter Review
Thanks to the excellent testcase, this was indeed a trivial fix.

The basic problem is that we don't have very good error handling in the expat driver, which means that even after we've failed to parse the <overlay> start tag, we keep parsing. However since the state isn't what we expect it to be, we fall over.

Ideally we should revamp our XML parser error handling, but that's post-FF4 work.
Attachment #477621 - Flags: review?(jst)
Attachment #477621 - Flags: review?(jst) → review+
I'm bumping this to beta8. There are *no* crashes in crash-stats with this signature so I don't think people are hitting it a lot.

An update on the patch: I landed it but it caused aborts. I know why it causes the abort, but when I fixed that problem, tryserver showed leaks which may or may not have been intermittent. Looking into that now.
blocking2.0: beta7+ → beta8+
Attached patch Patch v2Splinter Review
This fixes two things:

1. Clears the mLoopingForSyncLoad flag if we error during parsing in the DOMParser. This avoids a debug-only abort in the DOMParser dtor

2. Collapse most error codes to NS_ERROR_HTMLPARSER_STOPPARSING in the expat driver so that we properly call Terminate() and thus avoid leaking if there are errors originating in the XML sink.

got r=peterv over irl
Attachment #477621 - Attachment is obsolete: true
Attachment #480781 - Flags: review+
I was also asked to put this in beta7 if we had a fix in time. This fix feels very secure, especially compared to current m-c code. (Only reason it was backed out was due to test case exercising code that is already there).
blocking2.0: beta8+ → beta7+
Checked in

http://hg.mozilla.org/mozilla-central/rev/517ccfdc4f5a

Thanks again for finding this and making a minimized testcase.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsXMLContentSink::CloseElement ]
You need to log in before you can comment on or make changes to this bug.