1087737 - Make JS callers of ios.newChannel call ios.newChannel2 in mobile/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_10_mobile.patch

And there are more to come where we have to make sure we pass the right arguments

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_10_mobile.patch

Wesley, in Bug 1038756 we started attaching a loadInfo to every channel whenever the channel gets created through NS_NewChannel in nsNetUtil.h. Now we are expanding the loadInfo attachment to also include channels that get created within JS to ultimately end up having a loadInfo on every channel.  Please find a description of all the required arguments to create a channel here [1] or alternatively here [2].

In the attached patch we tried to pass the right arguments to newChannel() to the best of our knowledge. It's quite complicated to provide the most accurate arguments (node, principal, contentType, etc.) because we are not experts within mobile/ code. Probably we have to change some more function signatures to pass the correct principal/node around so we finally end up having the right arguments in the function that finally creates the channel.

Please take a look at the patch. Since this is security critical code please look closely and let us know if we need additional reviewers/help for certain sub components/files.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#209
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#73

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8540867 [details] [diff] [review]
js_10_mobile.patch

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

::: mobile/android/chrome/content/CastingApps.js
@@ +301,5 @@
>  
> +  _getContentTypeForURI: function(aURI, aElement, aCallback) {
> +    let channel = Services.io.newChannelFromURI2(aURI,
> +                                                 aElement,
> +                                                 null, // aLoadingPrincipal

I assume this is guessed from the element? The docs don't make that clear to me (same for triggering principal, although triggering principal notes that it could be left out).

@@ +315,5 @@
>            case 303:
>              request.cancel(0);
>              let location = channel.getResponseHeader("Location");
> +            CastingApps._getContentTypeForURI(
> +              CastingApps.makeURI(location), aElement, aCallback);

Don't wrap this. We don't use 80 line in mobile/ directories.

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_10_mobile.patch

(In reply to Wesley Johnston (:wesj) from comment #3)
> > +    let channel = Services.io.newChannelFromURI2(aURI,
> > +                                                 aElement,
> > +                                                 null, // aLoadingPrincipal
> 
> I assume this is guessed from the element? The docs don't make that clear to
> me (same for triggering principal, although triggering principal notes that
> it could be left out).

Yes, that is correct, you you pass an element/node - then we query the principal from that element/node.

Carrying over r+ from wesj.

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e285881c00c5

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e285881c00c5