421228 - [FIX]Content policies and CheckLoadURI don't apply to DOMParser or XHR from chrome

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

If someone uses DOMParser or XHR from system code (e.g. chrome JS), we stomp the system principal on the document being loaded before we even start to parse it.  As a result that document will be able to bypass CheckLoadURI and will bypass the data document content policy as a result of bug 388597.

CheckLoadURI is an issue on branch too, but at least there we'd stop things that get a content policy check.... which means everything except XML DTDs.  And I guess we're pretty limited in what we allow to be loaded as those, so this whole thing might not be an issue on branch.

Can we possibly set the principal after the data document is fully loaded?  Even then you'd be in trouble in system code if you mutate that document...

Perhaps data documents loaded from system code shouldn't get a system principal?  That's what would make the most sense to me, to be honest.

Comment 1

[Johnny Stenback (:jst)]

why does it matter that we bypass checkloaduri? Seems like chrome should be
able to load anything if it so desires.

Bypassing the data document content policy does seem wrong though. Do we in
general not apply content policies to chrome stuff as a perf optimization? I
seem to recall something to that effect.

Seems wrong to not give chrome XHR documents the system principal. Not doing
that could be a fix, but it does seems like a hack to me.

If the worse thing that'll happen is that we bypass the data document content
policy this doesn't seem like a blocker. -- jonas.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Seems like chrome should be able to load anything if it so desires.

Yes.  But should a document that chrome loads via DOMParser or XMLHttpRequest be able to load anything it desires?  I would think not.  Otherwise, any time chrome uses DOMParser on a string it didn't create itself or uses XMLHttpRequest on any non-chrome URI, we have random web-generated content doing arbitrary loads.  It can't read the resulting data, but even just doing the load is bad in many cases.

> Do we in general not apply content policies to chrome stuff as a perf
> optimization?

I believe so, yes.

> Seems wrong to not give chrome XHR documents the system principal.

What would make the most sense for me is to give them null principals.  That way chrome could still touch them, no one else could, and they couldn't perform any actions.

> If the worse thing that'll happen ... doesn't seem like a blocker

Renominating, per explanation above: the worst that will happen is worse than just bypassing the data document content policy, because we'll have untrusted data being able to bypass CheckLoadURI.

Comment 3

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

Well, untrusted data can only bypass it if it manages to get chrome code to parse that data, right? And it can't get it to do anything dangerous with that data since all you can accomplish is GET requests to chrome and file uris which is harmless as the data can't be read.

Still think this isn't a blocker. Though a patch to make these documents have a null principal would be nice.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Well, untrusted data can only bypass it if it manages to get chrome code to
> parse that data

That would be any extension that ever does an XMLHttpRequest to an http:// URI.  At that point, whoever owns the wireless access point has just gotten their content of choice parsed by chrome.

> And it can't get it to do anything dangerous with that data

It can hang the app, trivially (read from /dev/tty, that sort of thing).

> which is harmless as the data can't be read.

GET requests to file:// are really bad even if you can't read the resulting data.  That's why we have CheckLoadURI instead of just same-origin checks.

Note also that this allows access to ALL URIs, not just chrome and file.  Some of those URIs can be easily used to do really bad things if run in the wrong places; that's why they're not publicly available.  Some can be used to trigger exploitable crashes if run the "wrong" way, and the CheckLoadURI is all that's protecting against it.

Basically, you're severely underestimating the severity of a CheckLoadURI bypass.  It's really really bad...

Renominating one last time.  I just don't have time to get this done in the near term, and we really do need this fixed...  If you still think this shouldn't block given the above, it's your call, I guess...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to do null principals

This is completely untested past the "yeah, it compiles and the browser starts" phase.  I'm not going to be able to find time to test it, but maybe we can get some resources on that?  It should be possible to use chrome mochitests for this: do an XMLHttpRequest or use DOMParser and then look at the nodePrincipal of the resulting document, at least.  Better yet would be verifying that the data document policy is not being bypassed.  Not sure how best to do that.

Comment 6

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

I still don't think this is a blocker. We're talking about chrome doing this so it's not really "bypassing" CheckLoadURI. It's by design that we let chrome load anything. Chrome code should always check anything they accept from content.

That said, I think we should try to get the patch in.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Uh... How exactly is chrome supposed to check something?  The very process of parsing the string into a document will make the requests.  Are you suggesting that the DOMParser or XHR consumer should do a bunch of regexp matches looking for URIs before parsing?  Can that even be done with XHR?  If I recall correctly it just goes and parses no matter what if the type is something it can parse?

Or are you saying extensions should never use DOMParser on data from an untrusted source or use XHR on http:// URIs?  I suspect that's a lost battle; no sane extension developer would think that an extension that does those and nothing else is exploitable.

I should note that the code _we_ ship by default doesn't seem to use XHR or DOMParser from chrome.  But I'm pretty sure extensions do....

Comment 8

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

Ok, i'm just not sure if this is a blocker or not. But it seems like we'd spend as much time figuring out if it is as we would checking the patch in.

If there are regressions etc, we should really evaluate if this is a blocker or if there is a better fix

Comment 9

[Damon Sicore (:damons)]

Who owns this?  We need to make a decision on this immediately.

Comment 10

[Boris Zbarsky [:bzbarsky]]

It's my patch, so I guess I own it.  Sicking and I talked about it last night, and decide we should get this patch in.  Just needs reviews and perhaps approval...

Comment 11

[Damon Sicore (:damons)]

Is this blocking or wanted?  It's marked as both.

Comment 12

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

Should be blocking.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in (with a minor tweak to DOMParser to avoid the baseURI problem there too).

I'll try to write some tests when I can, I guess...

Comment 14

[Boris Zbarsky [:bzbarsky]]

The DOMParser tweak wasn't enough, sadly.  See bug 429785.

Comment 15

[Daniel Veditz [:dveditz]]

Do we still agree with the assessment in comment 0 that we don't really need this on the 1.8 branch?