1087723 - Make JS callers of ios.newChannel call ios.newChannel2 in addon-sdk

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1087723_js_calles_in_addon-sdk.patch

Jonas, in all of the dependents of bug [1] we are going to convert JS callers of ios.newChannel() and also ios.newChannelFromURI() to use the new API including the loadInfo params. Can you take a look at this patch, because all the other dependents of [1] will look pretty similar.

Also, do you think we have to update callers of NetUtil.newChannel()? [1] Or is it fine that we call ios.NewChannel2() within NetUtil.newChannel()? There are not that many callers of Netutil.newChannel(). If we decide to update those callsites as well, we have to file new bugs to do that. Just wanted to bring that to your attention as well.


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1087720
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#203

Comment 2

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

Comment on attachment 8510499 [details] [diff] [review]
bug_1087723_js_calles_in_addon-sdk.patch

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

This patch looks good. But please see below for an assertion we should add to the ioservice.

::: addon-sdk/source/app-extension/bootstrap.js
@@ +53,5 @@
> +  let channel = ioservice.newChannel2(uri,
> +                                      'UTF-8',
> +                                      null,
> +                                      null,      // aLoadingNode
> +                                      Services.scriptSecurityManager.getSystemPrincipal(),

Can we add an assertion to ioservice.newChannel*2 that makes sure that either a loadingNode *or* a loadingPrincipal is passed.

Passing both should cause newChannel*2 to throw an exception.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1087723_addon_sdk.patch

Based on the comments in the touched files, e.g:
* Utility function that synchronously reads local resource from the given `uri` and returns content string.
* Takes chrome URI and returns content under that URI.
* ...
I think this patch is already what we want, using the systemPrincipal, SEC_NORMAL, TYPE_OTHER. I manually inspected all of the callsites and I don't think there is any other information (like node, etc) available which is more appropriate to use then the default arguments.

Since this is alphabetically the first dependency of Bug 1087720 I am going to run this by Jonas to see if his understanding of how the patches should look like and mine align.

In general, for (almost) all of the testing where we create a channel in JS, we can use the default arguments. In almost all of those cases we don't really care about the security checks.

Interesting update in that patch: We now also have to pass laodinfo-arguments to NetUtil.newChannel2 and also NetUtil.asyncFetch2.
If ascyncFetch2's first argument is a channel however, those arguments are not going to be used. In fact they should not be used, hence I am setting all of them to *null* on purpose.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_1_addon-sdk.patch

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8538856 [details] [diff] [review]
js_1_addon-sdk.patch

Tanvi, we have to revisit all those callsites to make sure we pass the right arguments!

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_1_addon-sdk.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_1_addon-sdk_tests.patch

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8542222 [details] [diff] [review]
js_1_addon-sdk.patch

Dietrich, 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 addon-sdk/ 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 9

[Christoph Kerschbaumer [:ckerschb]]

Thanks Erik for the speedy response, one thing I forgot to mention is that when a uri is coming from a webpage we should not use the systemPrincipal as the loadingPrincipal when calling NewChannel2.

Having that said, can you please take another look at /source/lib/sdk/net/url.js which has a function readURI - not sure what uri that is loading.

Sorry for not having provided that information initially.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Thanks Erik for the speedy response, one thing I forgot to mention is that
> when a uri is coming from a webpage we should not use the systemPrincipal as
> the loadingPrincipal when calling NewChannel2.
> 
> Having that said, can you please take another look at
> /source/lib/sdk/net/url.js which has a function readURI - not sure what uri
> that is loading.
> 
> Sorry for not having provided that information initially.

so the sdk/net/url is used by the sdk/addon/bootstrap.js (which is the same context as the change made in app-extension/bootstrap.js) and also sdk/l10n/loader

https://github.com/mozilla/addon-sdk/search?utf8=%E2%9C%93&q=readURI&type=Code

It could be used by the main.js of an add-on too, which afaict would be the same as using it from the bootsrap.js context as far as this bug is concerned.

Does this answer your question?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #10)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> > Thanks Erik for the speedy response, one thing I forgot to mention is that
> > when a uri is coming from a webpage we should not use the systemPrincipal as
> > the loadingPrincipal when calling NewChannel2.
> > 
> > Having that said, can you please take another look at
> > /source/lib/sdk/net/url.js which has a function readURI - not sure what uri
> > that is loading.
> > 
> > Sorry for not having provided that information initially.
> 
> so the sdk/net/url is used by the sdk/addon/bootstrap.js (which is the same
> context as the change made in app-extension/bootstrap.js) and also
> sdk/l10n/loader
> 
> https://github.com/mozilla/addon-sdk/
> search?utf8=%E2%9C%93&q=readURI&type=Code
> 
> It could be used by the main.js of an add-on too, which afaict would be the
> same as using it from the bootsrap.js context as far as this bug is
> concerned.
> 
> Does this answer your question?

Erik, thanks for your answer. The problem is, that we are changing the API from using newChannel to newChannel2 all over the tree. We are not experts in all of the different parts of the tree and since the information passed in to the channel is security critical we are slightly scared. E.g. systemPrincipal bypasses all security checks. So once we move security checks into AsyncOpen() we wanna make sure that we have the correct info set on each channel.

Presumably, from what I can read for you answer, is that readURI is only used for addons, right? In such a case I think the systemPrincipal is correct, because addons are more or less treated like chrome code in that sense.

To sum it up, it's fairly hard for us to understand what the code is doing - if you think that the arguments provided for loadInfo are correct, then all good.

I hope you understand why we are slightly freaked out, but we have to change ~400 callsites and have to make sure to pass the right arguemnts everywhere.

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #10)
> > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> > > Thanks Erik for the speedy response, one thing I forgot to mention is that
> > > when a uri is coming from a webpage we should not use the systemPrincipal as
> > > the loadingPrincipal when calling NewChannel2.
> > > 
> > > Having that said, can you please take another look at
> > > /source/lib/sdk/net/url.js which has a function readURI - not sure what uri
> > > that is loading.
> > > 
> > > Sorry for not having provided that information initially.
> > 
> > so the sdk/net/url is used by the sdk/addon/bootstrap.js (which is the same
> > context as the change made in app-extension/bootstrap.js) and also
> > sdk/l10n/loader
> > 
> > https://github.com/mozilla/addon-sdk/
> > search?utf8=%E2%9C%93&q=readURI&type=Code
> > 
> > It could be used by the main.js of an add-on too, which afaict would be the
> > same as using it from the bootsrap.js context as far as this bug is
> > concerned.
> > 
> > Does this answer your question?
> 
> Erik, thanks for your answer. The problem is, that we are changing the API
> from using newChannel to newChannel2 all over the tree. We are not experts
> in all of the different parts of the tree and since the information passed
> in to the channel is security critical we are slightly scared. E.g.
> systemPrincipal bypasses all security checks. So once we move security
> checks into AsyncOpen() we wanna make sure that we have the correct info set
> on each channel.
> 
> Presumably, from what I can read for you answer, is that readURI is only
> used for addons, right? In such a case I think the systemPrincipal is
> correct, because addons are more or less treated like chrome code in that
> sense.

This seems correct to me.  ni'ing the sdk module owner for verification.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Sorry for the delay, looks good to me too.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4539d8d0c8a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f131685973

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4539d8d0c8a3
https://hg.mozilla.org/mozilla-central/rev/31f131685973

Comment 16

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d403ab95e9017938d5d0b1fbea98575fb0d4e929
Bug 1087723: Make JS callers of ios.newChannel call ios.newChannel2 in addon-sdk/ (r=erikvold)

https://github.com/mozilla/addon-sdk/commit/8e1f6e8c18d655529bbe61f60162c3eb08743295
Bug 1087723: Make JS callers of ios.newChannel call ios.newChannel2 in addon-sdk/ - tests (r=erikvold)

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Looks like this changes introduced test failures on sdk side:
https://tbpl.mozilla.org/?usetinderbox=1&tree=Jetpack&rev=29c30ab68b23

One issue was that `Services` were not declared / imported into a scope:
https://github.com/mozilla/addon-sdk/blob/master/test/test-xpcom.js#L140

After importing `Services` I do see this errors though that I'm not sure what are
they related to:

```
....................console.error: addon-sdk:
  Message: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIIOService.newChannelFromURI2]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack/addon-sdk/tests/test-xpcom.js :: testRegister :: line 171"  data: no]
  Stack:
    testRegister@resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack/addon-sdk/tests/test-xpcom.js:171:16
exports["test re-register"]@resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack/addon-sdk/tests/test-xpcom.js:196:2
@resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/sdk/test.js:75:12
start@resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/sdk/deprecated/unit-test.js:559:6
startMany/runNextTest/<@resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/sdk/deprecated/unit-test.js:522:10
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:22
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:6
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:36

..console.error: addon-sdk:
  Message: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIIOService.newChannelFromURI2]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack/addon-sdk/tests/test-xpcom.js :: testRegister :: line 171"  data: no]
  Stack:
    testRegister@resource://extensions.modules.5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack.commonjs.path/toolkit/loader.js -> resource://5f48e4a3-de31-418d-ab92-329901de6137-at-jetpack/addon-sdk/tests/test-xpcom.js:171:16
exports["test register"]@resource://extensions.modules.5f48e4a3-de31-4
```

This is a code that throws exceptions:
https://github.com/mozilla/addon-sdk/blob/master/test/test-xpcom.js#L170-L175

Christoph, Jonas

As far as I can see it comes from calling `ios.newChannel2`, any ideas what that
exception means or why is it thrown ?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

As discussed on IRC, since you are implementing nsIAboutModule, you have to change the function signature of newChannel from
- newChannel(aURI)
to
+ newChannel(aURI, aLoadInfo)
and pass the loadInfo along to the new channel.

That will fix it.

Comment 19

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Fix for the regressions

Comment 20

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0f2ba1873ba007f943c3ec5cb0f1cbec7eaf8bf5
Merge pull request #1851 from Gozala/bug/regression-in-xpcom-test@1087723

Bug 1087723 - Fix regression caused by use of new & outdated APIs togather. r=@mossop

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Did comment 18 fix the problem? If so, can you mark the bug as fixed again?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21)
> Did comment 18 fix the problem? If so, can you mark the bug as fixed again?

Apparently comment 18 fixed the problem - marking this bug as resolved.