Closed Bug 1232904 Opened 4 years ago Closed 4 years ago

Use channel.asyncOpen2 within dom/media/IdpSandbox.jsm

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → mozilla
Blocks: 1232430
Status: NEW → ASSIGNED
Hey Martin, we are about to use asyncOpen2() for IdpSandbox.jsm. Question is: what is this code doing exactly so we can determine the right security policy for this callsite. What kind of security checks do we need to perform?

Using the systemPrincipal as the triggeringPrincipal even though we have a loadingNode available doesn't seem quite right to me. Is there any particular reason?
Flags: needinfo?(martin.thomson)
I'll confess that I don't know all the consequences of setting a different triggering principal here, but maybe my initial assessment was incorrect on that basis.

The point of this is that the browser loads the code and interacts with it.  It has to be as though this is a little sandbox where the browser talks to the origin.  The script is not run with the principal of the loading page, it's run in its own origin.  I have no problem with the site learning the origin or referer (we give it the origin immediately in any case), but I think we need to avoid CSP checks here.  There is a loose relationship with the page and the script that we load here, so requiring something like a script hash or something would make this difficult.
Flags: needinfo?(martin.thomson)
Martin, finally I got a chance to look at your answer and to be honest I am slightly confused. So please answer me the following question which will clear everything up:

Can the URL of the channel be influenced by the web or is it provided by the browser only?

If the URL is basically a hardcoded URL in the browser then your code is fine. If not, then we have to change the setup.

Please note that what you have right now is not doing any harm as long as we are not using asyncOpen2.
Flags: needinfo?(martin.thomson)
Also, I forgot to ask:
a) are there any other tests than: dom/media/tests/mochitest/identity?
b) do we have to worry that the channel might be redirected?
Hmm, the site can influence the URL.

and a) no other tests
b) yes, it might be redirected


I guess it's hard to provide a good answer to your questions without understanding the purpose of setting the triggering principal.  Can you explain what that affects and why a system principal would be bad?
Flags: needinfo?(martin.thomson) → needinfo?(mozilla)
(In reply to Martin Thomson [:mt:] from comment #5)
> I guess it's hard to provide a good answer to your questions without
> understanding the purpose of setting the triggering principal.  Can you
> explain what that affects and why a system principal would be bad?

If the site can influence the URL, then using a loadingPrincipal of doc.nodePrincipal sounds right to me. But as you pointed out, that will cause the CSP of the page to be applied to that load once we switch to use asyncOpen2.

In most cases the loadingPrincipal and the triggeringPrincipal are/should be identical. If you discard the triggeringPrincipal from NewChannel, then the loadingPrincipal will also be triggeringPrincipal. I converted the code to:
>     let ioChannel = ioService.newChannelFromURI2(uri, doc, doc.nodePrincipal,
>-                                                 systemPrincipal, 0,
>+                                                 null, 0,
but still using AsyncOpen() and ran all tests within dom/media/tests/mochitest/identity/ - works fine.


Now, there are only a few exceptions where you need to set a different triggeringPrincipal. E.g. in case a stylesheet loads an image, then the loadingPrincipal will be the principal of the document but the triggeringPrincipal [1] will be the principal of the stylesheet. We are perfoming contentPolicy checks using the loadingPrincipal but still performing content accessibility checks using the triggeringPrincipal. From what I've read, I don't think your case requires a different triggeringPrincipal. It seems to me that the loadingPrincipal and the triggeringPrincipal are identical in your case.

Further, using systemPrincipal as the loadingPrincipal will bypass all security checks, because it's the system that performs the load. E.g. Updating/fetching the safebrowsing list is triggered by the browser and can't be influenced by any webpage. That channel creation is triggered by the system - hence systemPrincipal.

At the moment, using a loadingPrincipal of doc.nodePrincipal and in addition using a trigginerPrincipal of systemPrincipal has no effects to security checks, but we are about to change that so securitychecks will also be bypassed when the triggeringPrincipal is the systemPrincipal (which is needed for XBL user stylesheets :-) ).

Anyway, I think we need to be careful about using the systemPrincipal, because we are loading script here, which when redirected could potnetially end up loading malicious script.

Does that make things clearer?

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#179
Flags: needinfo?(mozilla) → needinfo?(martin.thomson)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> (In reply to Martin Thomson [:mt:] from comment #5)
> Anyway, I think we need to be careful about using the systemPrincipal,
> because we are loading script here, which when redirected could potnetially
> end up loading malicious script.

s/end up loading malicious script/ends up exposing user-private data to the webpage.
I'm sorry to say that this makes me less certain that you are doing the right thing.

> Now, there are only a few exceptions where you need to set a different
> triggeringPrincipal. E.g. in case a stylesheet loads an image, then the
> loadingPrincipal will be the principal of the document but the
> triggeringPrincipal [1] will be the principal of the stylesheet. We are
> perfoming contentPolicy checks using the loadingPrincipal but still performing
> content accessibility checks using the triggeringPrincipal. From what I've
> read, I don't think your case requires a different triggeringPrincipal. It
> seems to me that the loadingPrincipal and the triggeringPrincipal are identical
> in your case.

If this is the case, then CSP on the page can block the script load.  That's a little unfortunate, but not a huge problem, since the site basically can understand what is going on.

However, what accessibility checks are being guided by the triggering principal?  If this creates a situation where the script load might be blocked due to being cross origin that might be a problem: the load needs to have all the cookies that a first party page load would get so that the script can access that information.  That said, since this is a script load, that should be OK here, right?  Because script loads don't get blocked by CORS.

If that's all OK, then I think that your change is fine.
Flags: needinfo?(martin.thomson)
(In reply to Martin Thomson [:mt:] from comment #8)
> I'm sorry to say that this makes me less certain that you are doing the
> right thing.

I am not sure what you mean by that? What makes you less certain that we are doing the right thing?

> If this is the case, then CSP on the page can block the script load.  That's
> a little unfortunate, but not a huge problem, since the site basically can
> understand what is going on.

Yes, the CSP of the page would stop the script from loading if not whitelisted by the CSP.
 
> However, what accessibility checks are being guided by the triggering
> principal?

Content accessibility checks are performed for the loadingPrincipal and if the triggeringPrincipal differs from the loadingPrincipal, then also for the triggeringPrincipal.

> If this creates a situation where the script load might be
> blocked due to being cross origin that might be a problem: the load needs to
> have all the cookies that a first party page load would get so that the
> script can access that information.  That said, since this is a script load,
> that should be OK here, right? 

Yes that is fine, we are using the security flag SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS which means that the SOP is *not* enforced (allowing cross origin) but still making sure that no chrome: or file: uris are loaded. That brings me to my next question. If a data: URI is loaded, should that data: URI inherit the principal of the loading document (this is what I am proposing in the patch) or should it rather get it's own NullPrincipal in which case we would have to change the security flag to SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL.

> If that's all OK, then I think that your change is fine.

Should be all good in my opinion, all of the tests within dom/media/tests/mochitest/identity/ are passing.
Attachment #8698796 - Attachment is obsolete: true
Attachment #8710267 - Flags: review?(martin.thomson)
This will never load a data: URL.  The URL is always https:// and I ensure that redirects are all https://.

I would be happy with ..._DATA_IS_NULL there.  It seems safest.
Comment on attachment 8710267 [details] [diff] [review]
bug_1232904_asyncopen2_dom_media_idpsandbox.patch

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

::: dom/media/IdpSandbox.jsm
@@ +47,5 @@
>      let listener = new ResourceLoader(resolve, reject);
> +    let ioChannel = NetUtil.newChannel({
> +      uri: uri,
> +      loadingNode: doc,
> +      securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,

Let's got with ..._DATA_IS_NULL
Attachment #8710267 - Flags: review?(martin.thomson) → review+
(In reply to Martin Thomson [:mt:] from comment #11)
> Let's got with ..._DATA_IS_NULL

_DATA_IS_NULL it is - thanks!
https://hg.mozilla.org/mozilla-central/rev/3f330afda9f4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.