825070 - systemXHR loads should be subject to checkLoadURI checks

Status: RESOLVED FIXED

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

Description

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

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.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 3

[Andrew Overholt [:overholt]]

CCing some people who may be interested in taking this.

Comment 4

[Philipp von Weitershausen [:philikon]]

I can take this.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: v1

https://tbpl.mozilla.org/?tree=Try&rev=a41fc1dee9e0

Comment 6

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

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.

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: v1 nits addressed r=sicking

Comment 8

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b89fcf827e4

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/5b89fcf827e4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc061a528ad3
https://hg.mozilla.org/releases/mozilla-b2g18/rev/581dc264f49c