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

RESOLVED FIXED

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] stepping stone?)

Attachments

(1 attachment)

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.
Flags: blocking1.9?
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.

Updated

10 years ago
Flags: blocking1.9? → blocking1.9-
> 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.
Flags: blocking1.9- → blocking1.9?
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.
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
> 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...
Flags: blocking1.9- → blocking1.9?
Created attachment 309184 [details] [diff] [review]
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.
Attachment #309184 - Flags: superreview?
Attachment #309184 - Flags: review?(jonas)
Attachment #309184 - Flags: superreview? → superreview?(jst)
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.
Flags: blocking1.9? → blocking1.9-
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....
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
Flags: blocking1.9- → blocking1.9+
Who owns this?  We need to make a decision on this immediately.
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...
Assignee: nobody → bzbarsky
Summary: Content policies and CheckLoadURI don't apply to DOMParser or XHR from chrome → [FIX]Content policies and CheckLoadURI don't apply to DOMParser or XHR from chrome
Is this blocking or wanted?  It's marked as both.
Should be blocking.
Flags: wanted1.9.0.x+

Updated

10 years ago
Priority: -- → P2

Updated

10 years ago
Attachment #309184 - Flags: superreview?(jst) → superreview+
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...
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
The DOMParser tweak wasn't enough, sadly.  See bug 429785.
Do we still agree with the assessment in comment 0 that we don't really need this on the 1.8 branch?
Whiteboard: [sg:moderate] stepping stone?

Updated

10 years ago
Depends on: 467123
Group: core-security
Flags: wanted1.8.1.x-
You need to log in before you can comment on or make changes to this bug.