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

RESOLVED FIXED

Status

()

Core
DOM: Security
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: aus)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Reporter)

Comment 1

4 years ago
Created attachment 8554887 [details] [diff] [review]
js_5_dom_browser_element.patch

As requested in Comment 32 of Bug 1087728, splitting this one out into it's own bug.
Attachment #8554887 - Flags: review?(jonas)
Attachment #8554887 - Flags: review?(aus)
(Reporter)

Updated

4 years ago
Blocks: 1087728
Resubmitting my comments from bug 1087728 comment 26


From me:
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?


From Christoph:
(In reply to Jonas Sicking (:sicking))
> Comment on attachment 8546097 [details] [diff] [review] [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.


From me:
> 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.
I'm pretty sure that the loadingPrincipal and triggeringPrincipal that we use for "content-disposition: attachment" downloads are the same ones as we use for normal navigation. I.e. the triggeringPrincipal will be the principal of the page which linked the user to the url, and the loadingPrincipal will depend on where the load was targetted.

So at the very least we should dig out the principal of the page which triggered the download and use that as triggeringPrincipal.
Aus: is this something you could help with as it's a bit of a bigger change?
(Assignee)

Comment 5

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #4)
> Aus: is this something you could help with as it's a bit of a bigger change?

I'm not certain that changes are required yet for the usage that occurs in Gaia.

What we do in Gaia is that we actually use the 'download' method on the browser that is host to the content the user long presses the link from. From my understanding at the time, this meant that the document URI would be that of the content being displayed as the Browser itself uses yet another instance of the iframe (which in turn, is the one we *should* be calling 'download' on).

If it turns out this isn't what's going on, then something is wrong and we should fix that.

That being said, if we would also require 'download' on 'iframe' to enable you to download content for a host that is *not* currently displaying, we would absolutely need to add parameters to the 'download' method on the Browser Web API but we probably won't need Download Web API changes.

Drop by my desk if we should discuss in person. :)
(Assignee)

Comment 6

3 years ago
After discussing this with :sicking in person here's what we'll do --

One quick thing is we need to update how we get the referrer, it's not correct right now, and will be very easily done as part of this bug since we need to have the proper referrer anyway to fix this bug.

Second, we can't be using this._window.document as the document we pass into newChannelFromURI2, it isn't the document we're looking for. We need to pass null here instead. As for the principal, we will need to create a new one from the URI (which we can get from the inner iframe displaying the content), and the AppId of the inner iframe (need to ask bz how to do this).

I can take care of these changes and submit a new patch to this bug. It is to be noted that there will be simple Gaia changes required for this as well but they should be able to land whenever since the added parameters will be part of the 'options' object passed into the 'download' method which won't cause the method signature to change.

All patches and pull requests will be submitted to this bug. I will _not_ split the bug into gecko vs gaia changes.

:bz, it looks like we're mostly doing the right thing, but, we need to use the Script Security Manager with the URI of the referrer document as well as the AppId so we can call http://mxr.mozilla.org/mozilla-central/ident?i=getAppCodebasePrincipal and then use this principal to call createChannelFromURI2 as shown in the original patch attached to this bug. However, I am unsure how the AppId affects the security checks further down the line. Would it be possible to simply use getNoAppCodebasePrincipal with the referrer URI?
Assignee: mozilla → aus
Flags: needinfo?(bzbarsky)
Whiteboard: [systemsfe]
The appid/browserflag will (but doesn't currently) affect which cookies are used.

But that makes me realize where we have access to the appid/browserflag!

appid = this._frameLoader.loadContext.appId;
browserflag = this._frameLoader.loadContext.isInBrowserElement;
> As for the principal, we will need to create a new one from the URI (which we can get
> from the inner iframe displaying the content

Stop right there.  Which principal do you want, exactly, why do you think that creating it from a URI is the right idea, and why is the document.nodePrincipal of the iframe not what you want?
Flags: needinfo?(bzbarsky)
Because the document lives in a different process. This code runs in the parent process and the document which initiated the load runs in the child process.
OK.  So which URI do you plan to use?  If it's the URI of the document, are you sure it's not a data: URI or blob: URI or about:srcdoc or about:blank or any of the other edge cases I'm forgetting in which the URI of the document has nothing to do with its origin/principal?
More generally, it _really_ feels like we need a way to send principals across IPC somehow...  I'm not saying this bug needs to block on it, necessarily, but this problem keeps coming up.
What we *could* do is something like this:

* Whenever we send a message from the child process to the parent process about a mozbrowser* event
  needing to be fired, also send along the principal of the current page.
  This probably means that the following two functions need to take a principal and forward it:
  http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#22
  http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#36

* Forward that principal as the 4th argument when the child sends a message-manager message to the parent.
  The message manager will take care of properly serializing and deserializing the principal. It'll even
  do some security checks on the principal to make sure that the appid/browserflag is correct.

* On the parent side, when receiving the message and create an nsIDOMEvent, stick the principal in some
  internal member in the event. I'm not sure that we currently have a scriptable way to do this? Possibly
  if we create a new event interface that would be possible.

* Make the download() function take a URL to download, and an nsIDOMEvent. The download function would
  then grab the principal from member in the event and use that as triggering principal.


However this is an aweful lot of work. I'm not sure that it's worth it.
(In reply to Boris Zbarsky [:bz] from comment #10)
> OK.  So which URI do you plan to use?  If it's the URI of the document, are
> you sure it's not a data: URI or blob: URI or about:srcdoc or about:blank or
> any of the other edge cases I'm forgetting in which the URI of the document
> has nothing to do with its origin/principal?

I think we can do a check and only use the passed in URL to create a principal if the URL is a http(s) URL. Otherwise we can use a nullprincipal or the principal of the page containing the <iframe> or some such...
> Otherwise we can use a nullprincipal

I'd be ok with that, as long as it doesn't break web pages.  I'm not sure in what context we're actually using this principal, so can't comment on that part.

> or the principal of the page containing the <iframe>

This seems strictly wrong; for example if the iframe is sandboxed.
(Assignee)

Comment 15

3 years ago
Comment on attachment 8554887 [details] [diff] [review]
js_5_dom_browser_element.patch

I think we're all in agreement that more changes are required than this patch provides. I'll follow-up with a comment indicating our plan. Patch will be provided by me. I'll re-assign.
Attachment #8554887 - Flags: review?(aus) → review-
(Assignee)

Comment 16

3 years ago
* Add 'uri' of originating page in context menu events fired from mozbrowser.
* Use 'uri' from context menu event and pass it into the mozbrowser 'download' method.
* When 'uri' is provided to 'download' method, use it to get the principal.
* When 'uri' is provided to 'download' method, use it as the referrer.
* When 'uri' is *not* provided, use null principal.

Will have a patch for this ready sometime this week.
Sounds great. And if you get the uri using "document.nodePrincipal.URI || document.documentURI", then we should get the right URI in most situations.
(Reporter)

Comment 18

3 years ago
:aus, thanks for working on this. Just making sure, you voluntered to provide a patch for this, right?
Flags: needinfo?(aus)
(Assignee)

Comment 19

3 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> :aus, thanks for working on this. Just making sure, you voluntered to
> provide a patch for this, right?

I sure did. :) I actually have a patch that's ready now but I still have to do a round of manual testing on the device first before I post it.
Flags: needinfo?(aus)
(Reporter)

Comment 20

3 years ago
(In reply to Ghislain Aus Lacroix [:aus] from comment #19)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> > :aus, thanks for working on this. Just making sure, you voluntered to
> > provide a patch for this, right?
> 
> I sure did. :) I actually have a patch that's ready now but I still have to
> do a round of manual testing on the device first before I post it.

That's great news, thanks for the update.
(Assignee)

Comment 21

3 years ago
Created attachment 8566289 [details] [diff] [review]
Patch - v1 - Update newChannelFromURI JS callers
Attachment #8554887 - Attachment is obsolete: true
Attachment #8566289 - Flags: review?(kchen)
(Assignee)

Updated

3 years ago
Attachment #8566289 - Flags: review?(kchen) → review?(jonas)
Comment on attachment 8566289 [details] [diff] [review]
Patch - v1 - Update newChannelFromURI JS callers

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

Just some small changes needed.

::: dom/browser-element/BrowserElementParent.js
@@ +723,5 @@
> +      // This simply returns null if there is no principal available
> +      // for the requested uri. This is an acceptable fallback when
> +      // calling newChannelFromURI2.
> +      triggeringPrincipal = 
> +        Services.scriptSecurityManager.getNoAppCodebasePrincipal(referrer);

You can't use the 'NoApp' version here. You need to create a principal with the right appid/browserflag.

You should be able to get the frameloader for the <iframe mozbrowser> (it lives in the parent). From there you should be able to use

frameloader.loadContext.appId
frameloader.loadContext.isInBrowserElement

@@ +730,5 @@
> +    let channel = 
> +      Services.io.newChannelFromURI2(url,
> +                                     null,  // No document. 
> +                                     null,  // No loading principal
> +                                     triggeringPrincipal,

Use the principal above as both loadingPrincipal and triggeringPrincipal.
Attachment #8566289 - Flags: review?(jonas)
(Assignee)

Comment 23

3 years ago
Created attachment 8566850 [details] [diff] [review]
Patch - v2 - Update newChannelFromURI JS callers

Updated to address review comments.
Attachment #8566289 - Attachment is obsolete: true
Attachment #8566850 - Flags: review?(jonas)
Comment on attachment 8566850 [details] [diff] [review]
Patch - v2 - Update newChannelFromURI JS callers

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

Thanks!
Attachment #8566850 - Flags: review?(jonas) → review+
Comment on attachment 8566850 [details] [diff] [review]
Patch - v2 - Update newChannelFromURI JS callers

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

Thanks!

::: dom/browser-element/BrowserElementParent.js
@@ +732,5 @@
> +
> +    debug('Using principal? ' + !!principal);
> +
> +    let channel = 
> +      Services.io.newChannelFromURI2(url,

Oh, actually, small nit:

This can be done (somewhat) cleaner as

NetUtil.newChannel(
  { uri: url,
    loadingPrincipal: principal,
    securityFlags: Ci.nsILoadInfo.SEC_NORMAL,
    contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
  });

r=me either way.
(Assignee)

Comment 26

3 years ago
Commit (b2g-inbound): https://hg.mozilla.org/integration/b2g-inbound/rev/096a27bb1f8e

Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 28

3 years ago
Love me some cases of 'runs fine locally' but not elsewhere... Looking into it now. Thanks for the back-out :RyanVM.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 29

3 years ago
Ah, I think I know what's wrong. In some cases, documentURI can be a nsSimpleURI URI implementation. This implementation fails to serialize properly. I'll return the spec of the URI instead of fetching the whole URI. That will fix our problem and be simpler to use anyway.
(Assignee)

Comment 30

3 years ago
And that was exactly what was happening.

Commit (b2g-inbound): https://hg.mozilla.org/integration/b2g-inbound/rev/a394007d3e62

Tests were all green. Hooray!

Marking fixed once again.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.