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

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Danil Ivanov]

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

Comment 1

[Danil Ivanov]

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>

Comment 2

[Boris Zbarsky [:bzbarsky]]

The parsing error is correct for now; you can't use XUL from untrusted content anymore.  The crash is clearly a bug.

Comment 3

[Max]

In the example above the DOMParser is accessed from a JS XPCOM component, so it should not be an "untrusted content". Should it?

Comment 4

[Boris Zbarsky [:bzbarsky]]

If you create the DOMParser using createInstance and then never call init() on it, it defaults to a sandboxed parser with no privileges whatsoever.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Though you might need bug 593026 to get the right principal to initialize with.

Comment 6

[Max]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Max]

OK, I think I get it now. So you mean, a web page will be unable to parse XML containig nodes in XUL namespace?

Comment 9

[Danil Ivanov]

Boris, thank you. createInstance with init() is working.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 12

[Max]

> > > 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.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Assigning to jst@ since it's a b7 blocker without an owner ...

Comment 14

[Wayne Mery (:wsmwk)]

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

Comment 15

[Johnny Stenback (:jst)]

Over to Jonas who did all the remote XUL disabling work.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

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.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

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

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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).

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

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

Thanks again for finding this and making a minimized testcase.