Closed Bug 1087728 Opened 5 years ago Closed 5 years ago

Make JS callers of ios.newChannel call ios.newChannel2 in dom/

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(4 files, 5 obsolete files)

No description provided.
Blocks: 1087720
Duplicate of this bug: 1087727
Attached patch js_5_dom.patch (obsolete) — Splinter Review
Tanvi, same thing :-) We have to revisit all the callsites to make sure we pass the right arguments.
Attachment #8538863 - Flags: feedback?(tanvi)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment on attachment 8538863 [details] [diff] [review]
js_5_dom.patch

Fabrice, 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].

[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


Specifically, can you look over the below callsites and provide your thoughts on what the right principal should be?

/dom/apps/AppsUtils.jsm
This looks like its used to load a manifest file, in which case system seems fine. https://wiki.mozilla.org/B2G/DeveloperPhone

/dom/apps/OfflineCacheInstaller.jsm
Looks like we can pass in app.appId to readFile() and use 
let principal = Services.scriptSecurityManager.getAppCodebasePrincipal(
                     origin, appId, false);
We may be able to then do the same for dom/apps/AppUtils.jsm

/dom/apps/TrustedHostedAppsUtils.jsm
Again, use the appid to generate the principal.

/dom/apps/Webapps.jsm
We can use the principal from either the aOldApp ID or the aNewApp ID.  Which one do we use?

In _computeFileHash, if all we are doing is hashing a file, system is probably okay.  I also see no references to apps here.  Is system the right loadingPrincipal to pass into asyncFetch2()?
Flags: needinfo?(fabrice)
Fabrice, looks like we have a few more questions for you:

/dom/browser-element/BrowserElementParent.js
Can we use this._window to get the document and documents principal instead of using system? https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js#648

/dom/contacts/fallback/ContactDB.jsm 
Looks like this channel is used to upload the contacts file in b2g.  Is there an appid and associated principal for this, or should we just use system?

/dom/settings/SettingsDB.jsm 
Same question for SettingsDB as for ContactDB.

/dom/system/gonk/NetworkService.js
This looks like its trying to get network stats of some kind, in which case system is probably okay.  Please confirm and if possible provide a bit more background info.
(In reply to Tanvi Vyas [:tanvi] from comment #3)

> Specifically, can you look over the below callsites and provide your
> thoughts on what the right principal should be?
> 
> /dom/apps/AppsUtils.jsm
> This looks like its used to load a manifest file, in which case system seems
> fine. https://wiki.mozilla.org/B2G/DeveloperPhone

Yes.

> /dom/apps/OfflineCacheInstaller.jsm
> Looks like we can pass in app.appId to readFile() and use 
> let principal = Services.scriptSecurityManager.getAppCodebasePrincipal(
>                      origin, appId, false);
> We may be able to then do the same for dom/apps/AppUtils.jsm

Correct, you can pass app.localId and create a codebase principal. Does that matter when loading files from disk compared to using the system principal though?

> /dom/apps/TrustedHostedAppsUtils.jsm
> Again, use the appid to generate the principal.

Yes.

> /dom/apps/Webapps.jsm
> We can use the principal from either the aOldApp ID or the aNewApp ID. 
> Which one do we use?

Not sure which part of the code you're talking about... _getRequestChannel() sets the appId for nsILoadContext.

> In _computeFileHash, if all we are doing is hashing a file, system is
> probably okay.  I also see no references to apps here.  Is system the right
> loadingPrincipal to pass into asyncFetch2()?

Yes.
Flags: needinfo?(fabrice)
(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Fabrice, looks like we have a few more questions for you:
> 
> /dom/browser-element/BrowserElementParent.js
> Can we use this._window to get the document and documents principal instead
> of using system?
> https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementParent.js#648

Sounds right.

> /dom/contacts/fallback/ContactDB.jsm 
> Looks like this channel is used to upload the contacts file in b2g.  Is
> there an appid and associated principal for this, or should we just use
> system?

Just use system here. It's not tied to any app, because it's a way to add "default" contacts to the db.

> /dom/settings/SettingsDB.jsm 
> Same question for SettingsDB as for ContactDB.

And same answer!

> /dom/system/gonk/NetworkService.js
> This looks like its trying to get network stats of some kind, in which case
> system is probably okay.  Please confirm and if possible provide a bit more
> background info.

Yes, system is ok. Here also, this is chrome code not related to any app.
Attached patch js_5_dom.patch (obsolete) — Splinter Review
Hey Frabrice, thanks for all your answers. I updated the patch to create an appCodeBasePrincipal where needed. Can you please take a look and check if we pass the right arguments to create a channel? You can find more documentation about all the arguments here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#74

One question in particular, in dom/browser-element/BrowserElementParent.js we pass this._window.document as the loadingDocument.
Looking at the idl:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#127
aLoadingNode is defined as 'nsIDOMNode'. My question is, can we pass this._window.document or do we have to call something funky on it to make the document (which is an nsINode) be passed as an nsIDomNode? On the C++ side we usually call node->asDomNode(), but I haven't seen any usage of that in JS code when doing an dxr search. Do you happen to know?
Attachment #8538863 - Attachment is obsolete: true
Attachment #8538863 - Flags: feedback?(tanvi)
Attachment #8540817 - Flags: review?(fabrice)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)

> One question in particular, in dom/browser-element/BrowserElementParent.js
> we pass this._window.document as the loadingDocument.
> Looking at the idl:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> nsIIOService.idl#127
> aLoadingNode is defined as 'nsIDOMNode'. My question is, can we pass
> this._window.document or do we have to call something funky on it to make
> the document (which is an nsINode) be passed as an nsIDomNode? On the C++
> side we usually call node->asDomNode(), but I haven't seen any usage of that
> in JS code when doing an dxr search. Do you happen to know?

I would do a QueryInterface(nsIDOMNode) here.
(In reply to Fabrice Desré [:fabrice] from comment #5) 
> > /dom/apps/Webapps.jsm
> > We can use the principal from either the aOldApp ID or the aNewApp ID. 
> > Which one do we use?
> 
> Not sure which part of the code you're talking about... _getRequestChannel()
> sets the appId for nsILoadContext.
> 

In this part of the code, we have both an aOldApp and an aNewApp.  In the current patch we get the code base principal from aNewApp.  Is that correct, or should we get it from aOldApp?
(In reply to Fabrice Desré [:fabrice] from comment #8)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> 
> > One question in particular, in dom/browser-element/BrowserElementParent.js
> > we pass this._window.document as the loadingDocument.
> > Looking at the idl:
> > http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> > nsIIOService.idl#127
> > aLoadingNode is defined as 'nsIDOMNode'. My question is, can we pass
> > this._window.document or do we have to call something funky on it to make
> > the document (which is an nsINode) be passed as an nsIDomNode? On the C++
> > side we usually call node->asDomNode(), but I haven't seen any usage of that
> > in JS code when doing an dxr search. Do you happen to know?
> 
> I would do a QueryInterface(nsIDOMNode) here.

Chatted with some of the JS folks - we shouldn't worry about the conversion - magic magic magic :-)
We can pass the node as is and the conversion will happen automagically.
We should not use the system principal when fetching manifests and packaged apps. We should instead use the principal of the page that called mozApps.install()/installPackage(). I think we're already saving the origin/appid/browserflag of the page that called install/installPackage, so it should be possible to recreate that principal.
Hmm.. it might be a bit more complicated than that...

What cookie-jar are we using when fetching manifests, mini-manifests and packages right now? IIRC manifest fetching is done using the cookie-jar of the app. Mini-manifest and package fetching is done using the cookie jar of the page calling installPackage().

Is that correct?

I think we should ensure that the appid+browserflag of the loadingPrincipal matches the cookiejar that we want to use for loading. Eventually I'd like to get the cookiejar from the loadingPrincipal rather than from the loadcontext, so we should make sure that they match.

So if the cookie jars above are correct, then maybe we should do the following:

For manifests:
* LoadingPrincipal is the principal of the manifest itself. Including the appid+browserflag of the app.
* TriggeringPrincipal is the principal of the page calling mozApps.install().

For mini-manifests and packages:
* LoadingPrincipal is the principal of the page calling mozApps.installPackage().
* TriggeringPrincipal is the same as LoadingPrincipal (i.e. leave it null).

Technically we could make the loadingPrincipal and triggeringPrincipal of the package come from the mini-manifest. But that seems unnecessarily complicated given that we're likely going to phase out mini-manifests as we move to instant apps. So we might as well keep things simple for now.
Also, it might make sense to break out changes in this patch into separate bugs. I see changes to actual DOM APIs (Apps, Browser, Contacts, Settings, NetworkStats). For those we likely need to make sure to use a principal of a calling webpage rather than a system principal, so we should make sure to get the right one which means that we need a review from someone that knows the API.

Actually, for network-stats its probably the right thing to use the system principal since we're not loading a webpage-provided URL.

For the various test changes it's likely fine to simply use a system principal so there it might make sense to bundle it into a single bug.
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.
Attached patch js_5_a_dom_browser-element.patch (obsolete) — Splinter Review
Attachment #8540817 - Attachment is obsolete: true
Attachment #8540817 - Flags: review?(fabrice)
Attachment #8546097 - Flags: review?(fabrice)
Attachment #8546098 - Flags: review?(fabrice)
Attachment #8546099 - Flags: review?(fabrice)
Attachment #8546100 - Flags: review?(fabrice)
Attached patch js_5_e_dom_apps.patch (obsolete) — Splinter Review
Attachment #8546102 - Flags: review?(fabrice)
Attached patch js_5_f_dom_tests.patch (obsolete) — Splinter Review
Attachment #8546104 - Flags: review?(fabrice)
Fabrice, it seems this is a little more complicated than we initially thought (See comment 11, 12, 13). I split up the patches into smaller pieces. I flagged you for review on all of them, but I was hoping you can help me find the right reviewer within dom/ - whoever feels most confident about it. Probably the test cases are fine as they are (using systemPrincipal as the default), but for all other patches we have to make sure that we pass the right arguments to NewChannel. Since all of the arguments are security critical we should really make sure to pass the right arguments, otherwise we might end up bypassing security checks once we move all contentPolicy checks into AsyncOpen() of a channel.
Comment on attachment 8546104 [details] [diff] [review]
js_5_f_dom_tests.patch

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

::: dom/base/test/unit/test_cspreports.js
@@ +77,5 @@
> +                                     null,      // aLoadingNode
> +                                     Services.scriptSecurityManager.getSystemPrincipal(),
> +                                     null,      // aTriggeringPrincipal
> +                                     Ci.nsILoadInfo.SEC_NORMAL,
> +                                     Ci.nsIContentPolicy.TYPE_OTHER);

Note to myself: we could use TYPE_CSP_REPORT here.
Comment on attachment 8546104 [details] [diff] [review]
js_5_f_dom_tests.patch

Jonas, we are about to find the right reviewers for dom/. Fabrice basically is going to review all of the patches in this bug, but he would prefer if you could review the tests. Are you fine with that?
Attachment #8546104 - Flags: review?(fabrice) → review?(jonas)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> Comment on attachment 8546104 [details] [diff] [review]
> js_5_f_dom_tests.patch
> 
> Review of attachment 8546104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/test/unit/test_cspreports.js
> @@ +77,5 @@
> > +                                     null,      // aLoadingNode
> > +                                     Services.scriptSecurityManager.getSystemPrincipal(),
> > +                                     null,      // aTriggeringPrincipal
> > +                                     Ci.nsILoadInfo.SEC_NORMAL,
> > +                                     Ci.nsIContentPolicy.TYPE_OTHER);
> 
> Note to myself: we could use TYPE_CSP_REPORT here.

And use the document's principal (the document/principal the csp policy is attached to).
Comment on attachment 8546097 [details] [diff] [review]
js_5_a_dom_browser-element.patch

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

I'm not sure what the right thing to do is there. The api call is initiated by a content process, but we trigger the load in the parent. I'm pretty sure the parent knows the appId and browser flag of the loader, so maybe we should create a principal from that? Jonas, what do you think?
Attachment #8546097 - Flags: review?(fabrice) → review?(jonas)
Attachment #8546098 - Flags: review?(fabrice) → review+
Attachment #8546099 - Flags: review?(fabrice) → review+
Attachment #8546100 - Flags: review?(fabrice) → review+
Comment on attachment 8546097 [details] [diff] [review]
js_5_a_dom_browser-element.patch

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

Aus should review this since he implemented this API.

Aus, this is part of an effort to add more context information to network requests with the ultimate goal of moving various security checks and other policies into the network layer rather than having to have each callsite that creates a network request do its own security checks.

See documentation for the new arguments here: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.idl#69

Looking at the existing code, I'm surprised that we are using this._window.document.documentURIObject as referrer. Doesn't that mean that the referrer is the URI of the system app? That doesn't seem useful or even particularly good.

Christoph, this code is implementing the "save as" feature for FirefoxOS. Do you know what we are using as loadingPrincipal and triggeringPrincipal for "save as" in Firefox Desktop?
Attachment #8546097 - Flags: review?(jonas)
Attachment #8546097 - Flags: review?(aus)
Attachment #8546097 - Flags: feedback?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #26)
> Comment on attachment 8546097 [details] [diff] [review]
> js_5_a_dom_browser-element.patch
> Christoph, this code is implementing the "save as" feature for FirefoxOS. Do
> you know what we are using as loadingPrincipal and triggeringPrincipal for
> "save as" in Firefox Desktop?

Yes, currently we use the doc, see:
https://bugzilla.mozilla.org/attachment.cgi?id=8542282&action=diff#a/browser/base/content/nsContextMenu.js_sec2

I hope this is the browser equivalent of 'save as' in the browser, but I would guess so.
> Yes, currently we use the doc, see:
> https://bugzilla.mozilla.org/attachment.cgi?id=8542282&action=diff#a/browser/
> base/content/nsContextMenu.js_sec2

By "the doc" I think you mean "the web document that contained the URL that is downloaded".

The current patch uses the FirefoxOS equivalent of "the chrome document which contain the <browser> element which renders the web page". (More specifically, the page uses the document in the system app which contains the <iframe mozbrowser> element).

> I hope this is the browser equivalent of 'save as' in the browser, but I
> would guess so.

I was actually more talking about the situation when you click a link which triggers a "save as" dialog. I think this is a different piece of code.

But yeah, I think we should either way use the principal of the page that contain the URL as the loadingPrincipal and triggeringPrincipal.

That means that we need to send the principal of the page that trigger the download to the download API. Probably this means that we need to change the download API to take an additional parameter. Right now it takes the URL to be downloaded. Additionally it should take the URL of the page that triggered the download.

Aus: Do we currently have an event that is fired when a page inside the <iframe mozbrowser> trigger a download? And does the system app then manually use information in that event to call the download() function on the browser API? If so, we need to add additional information to that event which contains the URL of the page that triggered the event.

Christoph: I really wish that we used separate bugs for the API changes here. I.e. for the apps API and the download API. There's no way anyone is going to find these discussions if someone were to look at why the changes made here were made.
Comment on attachment 8546102 [details] [diff] [review]
js_5_e_dom_apps.patch

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

r=me with the change in TrustedHostedAppsUtils.jsm

::: dom/apps/TrustedHostedAppsUtils.jsm
@@ +235,5 @@
> +    let sRequestChannel = NetUtil.newChannel2(signatureURL,
> +                                              null,
> +                                              null,
> +                                              null,      // aLoadingNode
> +                                              Services.scriptSecurityManager.getSystemPrincipal(),

Please use the same principal as for mRequestChannel
Attachment #8546102 - Flags: review?(fabrice) → review+
Hi Fabrice,

Per Jonas' comment below, it sounds like the following patches shouldn't be using system principal:
* js_5_c_dom_settings.patch
* js_5_d_dom_contacts.patch
* js_5_e_dom_apps.patch (for this one, I'm not as sure.  Most of the time we find a principal, but there are 3 cases we use system)

(In reply to Jonas Sicking (:sicking) from comment #13)
> I see changes to actual DOM APIs (Apps, Browser, Contacts, Settings,
> NetworkStats). For those we likely need to make sure to use a principal of a
> calling webpage rather than a system principal, so we should make sure to
> get the right one which means that we need a review from someone that knows
> the API.
>
Flags: needinfo?(fabrice)
Comment on attachment 8546104 [details] [diff] [review]
js_5_f_dom_tests.patch

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

Clearing until this is broken out to a separate bug.
Attachment #8546104 - Flags: review?(jonas)
Comment on attachment 8546097 [details] [diff] [review]
js_5_a_dom_browser-element.patch

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

Clearing until this is broken out to a separate bug.
Attachment #8546097 - Flags: feedback?(jonas)
Depends on: 1126065
Comment on attachment 8546097 [details] [diff] [review]
js_5_a_dom_browser-element.patch

Marking this one as obsolete - filed Bug 1126065 to handle browser-elements.
Attachment #8546097 - Attachment is obsolete: true
Attachment #8546097 - Flags: review?(aus)
Depends on: 1126067
Comment on attachment 8546104 [details] [diff] [review]
js_5_f_dom_tests.patch

Marking this patch as obsolete and handle the test cases in bug 1126067.
Attachment #8546104 - Attachment is obsolete: true
(In reply to Tanvi Vyas [:tanvi] from comment #30)
> Hi Fabrice,
> 
> Per Jonas' comment below, it sounds like the following patches shouldn't be
> using system principal:
> * js_5_c_dom_settings.patch
> * js_5_d_dom_contacts.patch
> * js_5_e_dom_apps.patch (for this one, I'm not as sure.  Most of the time we
> find a principal, but there are 3 cases we use system)
> 
> (In reply to Jonas Sicking (:sicking) from comment #13)
> > I see changes to actual DOM APIs (Apps, Browser, Contacts, Settings,
> > NetworkStats). For those we likely need to make sure to use a principal of a
> > calling webpage rather than a system principal, so we should make sure to
> > get the right one which means that we need a review from someone that knows
> > the API.
> >

The changes to Contacts and Settings and in the code that loads default databases, it's not part of the DOM side.
The ones using a system principal in dom/apps are similar (file reading not tied to a window).

So I stand by my r+ on these.
Flags: needinfo?(fabrice)
Comment on attachment 8546100 [details] [diff] [review]
js_5_d_dom_contacts.patch

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

Yup, this looks fine to use system.
Attachment #8546100 - Flags: review+
Comment on attachment 8546099 [details] [diff] [review]
js_5_c_dom_settings.patch

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

Same here.
Attachment #8546099 - Flags: review+
Okay, Christoph is going to update the js_5_e_dom_apps.patch per comment 29 and then we are good to go.  Thanks for your help with this Fabrice!
Missed Comment 29 earlier; incorporated that change; carrying over r+ from fabrice.
Attachment #8546102 - Attachment is obsolete: true
Attachment #8559484 - Flags: review+
You need to log in before you can comment on or make changes to this bug.