Closed Bug 825070 Opened 7 years ago Closed 7 years ago

systemXHR loads should be subject to checkLoadURI checks

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: sicking, Assigned: philikon)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now we don't do *any* security checks for the URL passed to a systemXHR object.

We should at least do a checkLoadURI check to make sure that you can't load from file:// and other protocols which content generally has no business loading from.
More info from dup

(In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> In b2g, this enables some super-powered OS fingerprinting, but doesn't put
> any user data at risk if we got the OS security model right.
> 
> We would *absolutely* need to fix this before enabling this interface for
> desktop though.
> 
> This is kind of a scary hole though and I'm thinking we should fix this for
> b2g v1.
> 
> philikon/sicking how hard would it be to summarily deny file:// for system
> XHR in v1?  There's no use case.
CCing some people who may be interested in taking this.
blocking-basecamp: ? → +
I can take this.
Assignee: nobody → philipp
Comment on attachment 696444 [details] [diff] [review]
v1

Review of attachment 696444 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, just one nit.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1638,5 @@
>  {
> +  // A system XHR (chrome code or a web app with the right permission) can
> +  // always perform cross-site requests. In the web app case, however, we
> +  // must still check for protected URIs like file:///.
> +  if (mIsSystem) {

This isn't technically correct since I think that if both mIsSystem and nsContentUtils::IsSystemPrincipal(mPrincipal) is true, then we don't want to do the below.

Maybe make this
|if(IsSystemXHR() && !nsContentUtils::IsSystemPrincipal(mPrincipal)) {|

Or move it to inside the if-statement below.
Attachment #696444 - Flags: review?(jonas) → review+
Attachment #696444 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5b89fcf827e4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.