770684 - (CVE-2012-3975) DOMParser does subresource loads when used from privileged code

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[vsemozhetbyt]

If DOMParser parses HTML string to DOM, should not it omit not only script processing but also recourse loading (as the pure XHR does with responseType = "document")?

I run this code from JavaScript Editor (Scratchpad, in browser context):

var parser = new DOMParser();
var xhr = new XMLHttpRequest();
xhr.mozBackgroundRequest = true;
xhr.open("GET", "https://addons.mozilla.org/en-US/firefox/", true);
xhr.timeout = 10000;
xhr.channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE;
xhr.onload = function() {
	var doc = parser.parseFromString(this.responseText, "text/html");
	var title = doc.evaluate("//title", doc, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue;
	if (title) {
		alert(title.textContent);
	}
	else {
		alert("Parsing error");
	}
}
xhr.ontimeout = function() {
	alert("Timeout");
}
xhr.onerror = function() {
	alert("HTTP error");
}
xhr.send(null);

HttpFox (https://addons.mozilla.org/ru/firefox/addon/httpfox/) shows additional recourse loading. Here some HTTP logs for three sites:

Started, Time, Sent, Received, Method, Result, Type, URL

https://addons.mozilla.org/en-US/firefox/

00:00:01.221	1.197	603		316		GET	200		text/html	https://addons.mozilla.org/en-US/firefox/
00:00:02.492	0.171	332		(34312)	GET	(Cache)	text/css	https://static-ssl-cdn.addons.mozilla.net/media/css/zamboni/impala-min.css?build=2628081
00:00:02.548	0.143	321		(824)	GET	(Cache)	text/css	https://static-ssl-cdn.addons.mozilla.net/media/css/impala/nojs.css?b=78073c4
00:00:02.618	0.464	968		201		GET	303		Redirect to: /dcso6de4r0000082npfcmh4rf_4b1e/njs.gif?dcsredirect=108&dcstlh=1298194945&dcstlv=1298194945&dcsuri=/nojavascript&WT.js=No&WT.tv=8.6.2	https://statse.webtrendslive.com/dcso6de4r0000082npfcmh4rf_4b1e/njs.gif?dcsuri=/nojavascript&WT.js=No&WT.tv=8.6.2
00:00:03.383	0.622	1050	327		GET	200		image/gif	https://statse.webtrendslive.com/dcso6de4r0000082npfcmh4rf_4b1e/njs.gif?dcsredirect=108&dcstlh=1298194945&dcstlv=1298194945&dcsuri=/nojavascript&WT.js=No&WT.tv=8.6.2

http://www.urbandictionary.com/

00:00:02.218	0.163	893	6678	GET	200		text/html	http://www.urbandictionary.com/
00:00:02.470	0.582	844	21059	GET	200		text/css	http://static2.urbandictionary.com/rel-30747e0/assets/base-datauri.css
00:00:02.527	0.487	859	8094	GET	200		image/png	http://static0.urbandictionary.com/rel-30747e0/images/logo-holiday.png
00:00:02.581	0.461	864	613		GET	200		image/gif	http://static2.urbandictionary.com/rel-30747e0/images/left_browse_arrow.gif
00:00:02.639	0.377	865	613		GET	200		image/gif	http://static1.urbandictionary.com/rel-30747e0/images/right_browse_arrow.gif
00:00:02.698	0.112	853	(165)	GET	(Cache)	image/gif	http://static1.urbandictionary.com/rel-30747e0/images/quotes.gif
00:00:02.756	0.324	352	231		GET	200		image/gif	http://pixel.quantserve.com/pixel/p-77H27_lnOeCCI.gif

http://www.imdb.com/

00:00:18.897	0.283	1433	243		GET	200		text/html	http://www.imdb.com/
00:00:19.236	0.339	335		(28286)	GET	(Cache)	text/css	http://i.media-imdb.com/images/SF3997b2af7161e82df725bc7ca86f84ce/css/min/consumerhome.css
00:00:19.290	0.448	597		188		GET	302		Redirect to: http://s0.2mdn.net/viewad/817-grey.gif	http://ad.doubleclick.net/ad/imdb2.consumer.homepage/;tile=2;sz=1008x150,1008x200,1008x30,9x1;p=t;p=top;ct=com;bpx=1;ab=a;ka=0;ord=717635184750?
00:00:19.344	0.261	346		(96)	GET	(Cache)	text/css	http://i.media-imdb.com/images/SF52e6b9f11712d3ec552179f6c869b63a/css2/site/consumer-navbar-no-js.css
00:00:19.396	0.340	582		188		GET	302		Redirect to: http://s0.2mdn.net/viewad/817-grey.gif	http://ad.doubleclick.net/ad/imdb2.consumer.homepage/;tile=5;sz=1008x60,1008x66,7x1;p=ns;ct=com;bpx=1;ab=a;ka=0;ord=717635184750?
00:00:19.449	0.288	580		188		GET	302		Redirect to: http://s0.2mdn.net/viewad/817-grey.gif	http://ad.doubleclick.net/ad/imdb2.consumer.homepage/;tile=3;sz=300x250,11x1;p=tr;p=tc;ct=com;bpx=1;ab=a;ka=0;ord=717635184750?
00:00:19.504	0.146	425		220		GET	200		image/gif	http://b.scorecardresearch.com/p?c1=2&c2=6034961&c3=&c4=http%3A%2F%2Fwww.imdb.com%2F&c5=c6=&15=&cj=1
00:00:19.899	0.418	328		(43)	GET	(Cache)	image/gif	http://s0.2mdn.net/viewad/817-grey.gif
00:00:19.957	0.362	328		(43)	GET	(Cache)	image/gif	http://s0.2mdn.net/viewad/817-grey.gif
00:00:20.015	0.305	328	(	43)		GET	(Cache)	image/gif	http://s0.2mdn.net/viewad/817-grey.gif

Why DOMParser needs these files for just DOM parsing?

Comment 1

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

It shouldn't load those files.

(Btw, why do you parse responseText and not just load HTML using XHR?)

Comment 2

[vsemozhetbyt]

Olli Pettay

Because I have a problem with some sites which I describe in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=770684

And I have been advised to use DOMParser in my extension untill this bug will be fixed.

Comment 3

[vsemozhetbyt]

Sorry, wrong link. See: https://bugzilla.mozilla.org/show_bug.cgi?id=765620

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm not convinced that this is a bug.  If you parse the markup as HTML, you can adopt the nodes into the document and they should behave like normal HTML nodes.  It seems weird to only kick off the loads on adoption.

Comment 5

[vsemozhetbyt]

Kyle Huey

But DOMParser does not load all the images and so on. Just some.

Comment 6

[vsemozhetbyt]

Kyle Huey

And I think we must discern page rendering and DOM parsing. For background parsing we don't need any image loading or styles.

Comment 7

[Boris Zbarsky [:bzbarsky]]

If I run the code from comment 0 in the scratchpad, I get an exception because xhr.channel is undefined, as expected.  I assume that the "in browser context" bit hides some crucial part of the steps to reproduce?  I'd really appreciate actually being told what that is.

Kyle, DOMParser is not supposed to do subresource loads.  See bug 421228.  ;)

Comment 8

[vsemozhetbyt]

2 Boris Zbarsky

http://blog.mozilla.org/devtools/2011/08/15/introducing-scratchpad/

See the part "A Word about Scopes", the last paragraph.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, looks like the "devtools.chrome.enable" pref needs to be set to true.

This is a bad bug in the patch for bug 102699.  Before that patch, the only codepath that could lead to parsing looked like this, in order:

1)  Create a document with the DOMParser's mOriginalPrincipal.
2)  Call EnableXULXBL() on the document if needed
3)  Call StartDocumentLoad()
4)  Set the document's base URI
5)  Reset the document's principal to mPrincipal.
6)  Feed data into the parser.

That sequence of steps was pretty clearly documented (at least in terms of the whole principal dance) and _very_ critical.

When that bug was fixed, the XML codepath stayed as above, but HTML codepath was written more like this:

1)  Create a document with the DOMParser's mOriginalPrincipal.
2)  Feed data into the parser.
3)  Call EnableXULXBL() on the document if needed
4)  Set the document's base URI
5)  Reset the document's principal to mPrincipal.

But the whole point of resetting to mPrincipal is that it MUST happen before any data goes in.  Otherwise you're parsing with the system principal.  Also, this is never calling StartDocumentLoad, so afaict it's not setting up whatever state that would normally set up (e.g. the document URI) the same way as the XML path.  And it's calling EnableXULXBL() too late, of course.  Not like this matters much for text/html.

This bug means that using DOMParser on text/html is pretty unsafe from chrome: It allows whatever string you're parsing to poke any URI it wants, including ones that web content normally can't access.  (On a Unix system it allows at minimum a DoS attack by reading from file:///dev/tty.)

Henri, Olli, can one of you grab this?  Or should I try to?

Comment 10

[Alex Keybl [:akeybl]]

Our final beta build is tomorrow EOD pacific time. Is there any reason to consider blocking that build on this FF12 regression?

Comment 11

[Boris Zbarsky [:bzbarsky]]

No, though this might be worth taking in a chemspill if we do one for some reason: every extension using DOMParser is basically insecure at the moment.

Comment 12

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

I think Henri will be back from vacation real soon.

Comment 13

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

What is the critical part of this bug?
The created document is a data document, so it itself shouldn't load anything.
HTML parser may speculatively load something. (It shouldn't enable speculative loads for data documents)
Or what am I missing here.

Comment 14

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

Though, I really don't understand why anything is loaded. Document is a data document, so
the data document content policy should prevent all the loads.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Content policy is never consulted for the system principal.  See the CHECK_PRINCIPAL bit in NS_CheckContentLoadPolicy.  So the document is in fact loading things here.

Comment 16

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

bug 388597 added a bizarre inconsistency to content policy handling.
(Another option would be to not have data document cp at all, but prevent such loads in some other way.)

Comment 17

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

I guess I've looked at the code enough to write a patch.

Comment 18

[vsemozhetbyt]

//every extension using DOMParser is basically insecure at the moment

I use it only because of the bug 765620. I just have given a simple testcase in the last comment there. May be somebody can confirm the bug.

Comment 19

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

Attachment: v1

Like this?

Comment 20

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

Comment on attachment 640106 [details] [diff] [review]
v1

Unfortunately this seems to leak. We're not supposed to call
StartDocumentLoad in this case for HTML documents.

Comment 21

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

Attachment: v2

Comment 22

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

Attachment: simple approach

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 640187 [details] [diff] [review]
simple approach

r=me

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/15f4ed9607b3

Comment 25

[Alex Keybl [:akeybl]]

Tracking for FF14, for possible 14.0.1 consideration.

Comment 26

[vsemozhetbyt]

Thank you, people.

Comment 27

[Alex Keybl [:akeybl]]

(In reply to Olli Pettay [:smaug] (limited connectivity July 26-29) from comment #24)
> https://hg.mozilla.org/mozilla-central/rev/15f4ed9607b3

Is this low risk enough to uplift to FF15 on mozilla-beta?

Comment 28

[Henri Sivonen (:hsivonen)]

(In reply to Alex Keybl [:akeybl] from comment #27)
> (In reply to Olli Pettay [:smaug] (limited connectivity July 26-29) from
> comment #24)
> > https://hg.mozilla.org/mozilla-central/rev/15f4ed9607b3
> 
> Is this low risk enough to uplift to FF15 on mozilla-beta?

Seems so to me. (Looks like smaug is away right now.)

Comment 29

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

Comment on attachment 640187 [details] [diff] [review]
simple approach

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 102699, bug 388597
User impact if declined: security issues 
Testing completed (on m-c, etc.): Landed m-c 2012-07-09 
Risk to taking this patch (and alternatives if risky): Should be very low risk
String or UUID changes made by this patch: NA

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 640187 [details] [diff] [review]
simple approach

[Triage Comment]
Low risk sg:moderate fix - let's take this for the next beta (going to build tomorrow). Please land as soon as possible.

Comment 31

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

https://hg.mozilla.org/releases/mozilla-beta/rev/029cd9d5d85f

Comment 32

[u279076]

Reporter, can you see if this is fixed for you now? Can you try the latest Firefox 15 release, 16 beta, and 17 aurora? Thanks.

Comment 33

[vsemozhetbyt]

Yes, it is fixed in these versions. Thank you.

Comment 34

[u279076]

Thanks vvsemozhetbyt@. Marking this bug verified based on comment 33.