1093947 - Make JS callers of ios.newChannel call ios.newChannel2 in uriloader/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_4_uriloader.patch

Tanvi, and here as well :-) We have to make sure to pass the right arguments!

Comment 2

[Tanvi Vyas[:tanvi]]

Comment on attachment 8538861 [details] [diff] [review]
js_4_uriloader.patch

Hi Olli,

We are going through and updating js callers of newChannel and want to pass the correct loadInfo.  Can you look at the callsite in the attached patch and provide answers to the question below.

uriloader/exthandler/nsWebHandlerApp.js
Looks like we can use aWindowContext as the node.  But what about the content type?  Is this TYPE_DOCUMENT, or could it be an iframe or something else?
https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsWebHandlerApp.js#63

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8538861 [details] [diff] [review]
js_4_uriloader.patch

I'm not too familiar with this code, and I would just need to look at the source code as you would need to. (Well I did start to look at the callers, and couldn't see anything guaranteeing that it would be TYPE_DOCUMENT always.)

But since http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?mark=1690-1690&rev=48f4e72f94cb#1682 uses TYPE_OTHER, I think we should use the same here, for now.

(Per documentation TYPE_OTHER shouldn't be ever used.)

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8538861 [details] [diff] [review]
js_4_uriloader.patch

Dan, since Smaug is not completely sure, maybe you can help review since you wrote this part?

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 uriloader/ 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#202
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#76

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I'll try and have a look at this tomorrow.  FWIW, I think :bz may be the only "expert" in that code, if there are any around at all.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Additional review note: One thing worth mentioning is that when a uri is coming from a webpage we should not use the systemPrincipal as the loadingPrincipal when calling NewChannel2.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8538861 [details] [diff] [review]
js_4_uriloader.patch

Just chatted with dmose on IRC, he thinks it would be better if Boris can take a look at it. Boris whenever you get some time can you please have a look at this patch? Thanks!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8538861 [details] [diff] [review]
js_4_uriloader.patch

r=me

Comment 9

[Tanvi Vyas[:tanvi]]

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8538861 [details] [diff] [review]
> js_4_uriloader.patch
> 
> I'm not too familiar with this code, and I would just need to look at the
> source code as you would need to. (Well I did start to look at the callers,
> and couldn't see anything guaranteeing that it would be TYPE_DOCUMENT
> always.)
> 
> But since
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp?mark=1690-1690&rev=48f4e72f94cb#1682 uses TYPE_OTHER, I think we should
> use the same here, for now.


Boris, is TYPE_OTHER right here?  Or should it be TYPE_DOCUMENT?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Does it matter, given that this is a system load?

In practice, this is used for opening system helper apps, so if we're getting here we know it's not a URI we'll deal with ourselves.  I think.  I'm not exactly an expert on this code.

Comment 11

[Tanvi Vyas[:tanvi]]

Comment on attachment 8538861 [details] [diff] [review]
js_4_uriloader.patch

(In reply to Boris Zbarsky [:bz] from comment #10)
> Does it matter, given that this is a system load?
> 
> In practice, this is used for opening system helper apps, so if we're
> getting here we know it's not a URI we'll deal with ourselves.  I think. 
> I'm not exactly an expert on this code.

Okay, sounds like TYPE_OTHER then.  You are right - in all the use cases of loadInfo we can think of today, if the loadingPrincipal is system the content policy type doesn't matter.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70bc10b845af

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70bc10b845af