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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser.patch

Tanvi, same here, we have to make sure we pass the right arguments!

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser.patch

Benjamin, 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 browser/ 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 3

[Benjamin Smedberg]

Comment on attachment 8540849 [details] [diff] [review]
js_3_browser.patch

You'd better ask a browser peer ;-)

It's likely that this should be broken up by functional area, but I don't know exactly how that would be, so I'm going to redirect this to Gavin to either review directly or suggest a way to split the patch apart more usefully.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> You'd better ask a browser peer ;-)

I thought you are one :-)
https://wiki.mozilla.org/Modules/All#Firefox
 
> It's likely that this should be broken up by functional area, but I don't
> know exactly how that would be, so I'm going to redirect this to Gavin to
> either review directly or suggest a way to split the patch apart more
> usefully.

Thanks for acting so quickly, I am totally happy to split the patch up and flag individual reviewers.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8540849 [details] [diff] [review]
js_3_browser.patch

This patch should be split into at least four:
- all of the devtools/ changes (might be worth splitting test changes and non-test changes here too). These can go in another devtools bug.
- the PDF.js changes (separate Firefox::PDF Reader bug, will need to be upstreamed to https://github.com/mozilla/pdf.js)
- test changes only (browser_*, test_*), fine to leave in this bug
- the rest

I can help find reviewers for each of these once that is done.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser_tests.patch

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Gavin, thanks for acting so quickly - hopefully I splited all the patches the way you requested it, but I think so:

This Bug: browser and browser-tests
Bug 1115193: devtools and devtools-tests
Bug 1115197: changes in pdfjs

I would like to take advantage of your offer in Comment 5 - could you please suggest reviewers?

Comment 9

[Tanvi Vyas[:tanvi]]

Comment on attachment 8541002 [details] [diff] [review]
js_3_browser.patch

I reviewed part of this patch and made some notes that I will paste here for the reviewer to look at (once it's determined who that is)...

* browser/components/feeds/FeedConverter.js
There is a newchannel function.  This might be for the channel that gets the actual feed.  Not sure what principal and content type this should have.  Can feeds ever be embedded in webpages?  Or do they only show up in chrome UI?

* browser/components/feeds/FeedWriter.js
What is the preview document?  
Looks like we have an "aWindow".  We also have the channel associated with that window (current document).  That channel should have a loadInfo object that we could just pass to the new channel.  But does that make sense for this context?  Also, looks like this newChannel is only created to do a comaprison, and then thrown away.  So maybe it doesn't matter what we use for the loadInfo.

* browser/components/migration/ChromeProfileMigrator.js
Channel used to copy the bookmarks file.  system principal is fine.

* browser/components/places/PlacesProtocolHandler.js
What is the places protocol handler used for?  It looks like this sets system for every channel created, which is not good.  Is there another patch that updates this newChannel and its callsites to pass in loadInfo? (my guess is probably not).  Depending on what this protocol handler is for, we may not need this (i.e. maybe always using system is okay).

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8541002 [details] [diff] [review]
js_3_browser.patch

I have to admit I'm still a little confused by how these additional properties on the channel are used in practice, which makes it difficult to suggest which values should be used. I suspect this is likely to be true of most of your other reviewers as well. What kind of decisions are made based on the channel's principal?

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>+    var channel = ioService.newChannelFromURI2(makeURI(linkURL),
>+                                               doc,

This channel gets used to download content when you choose "Save X As..." in a context menu. "doc" is the content document of the element that you right clicked on (a link, an image, etc.). I'm not sure that it makes sense to pass it in here as aLoadingNode, because it is not "where the result of this request will be used" (the result of this request will be saved on disk).

>+                                               Ci.nsIContentPolicy.TYPE_OTHER);

This seems somewhat contrary to https://hg.mozilla.org/mozilla-central/annotate/a4a5d4fb5e2e/dom/base/nsIContentPolicy.idl#l36, but I think the existing options there don't really cater to this particular "persist arbitrary content to disk" case.

>diff --git a/browser/components/feeds/FeedConverter.js b/browser/components/feeds/FeedConverter.js
>diff --git a/browser/components/feeds/FeedWriter.js b/browser/components/feeds/FeedWriter.js

Neil or someone who understands how this code works should probably review these changes. I don't think using the system principal is correct. Can you split this into its own bug and ask Neil (neil@httl) for review?

>diff --git a/browser/extensions/shumway/content/ShumwayStreamConverter.jsm b/browser/extensions/shumway/content/ShumwayStreamConverter.jsm

We likely don't want the systemprincipal here, and I think TYPE_OTHER is wrong too. Probably best handled in a separate bug, request review from [e-mail].

>diff --git a/browser/components/places/PlacesProtocolHandler.js b/browser/components/places/PlacesProtocolHandler.js

I'm actually not sure what this is used for anymore, it seems maybe busted? NI Marco to see what he thinks.

>diff --git a/browser/metro/components/SessionStore.js b/browser/metro/components/SessionStore.js

You can omit the changes to this file, it is effectively dead code.

The rest of the changes look OK, though I still would appreciate some extra information about what implications this has.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8541003 [details] [diff] [review]
js_3_browser_tests.patch

Given that these are only used in tests, I suppose this doesn't matter too much. Is using getSystemPrincipal() for these introducing a behavior change of some sort? What principal do newChannel()-created channels currently get by default?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #10)
> Comment on attachment 8541002 [details] [diff] [review]
> js_3_browser.patch
> 
> I have to admit I'm still a little confused by how these additional
> properties on the channel are used in practice, which makes it difficult to
> suggest which values should be used. I suspect this is likely to be true of
> most of your other reviewers as well. What kind of decisions are made based
> on the channel's principal?

Let me try to clarify why we are adding a loadInfo to every channel and what we use the information within that loadInfo:
Channels know very little about who initiated the request for the data they are fetching, hence we added a LoadInfo object that hangs off every channel and is frozen at channel creation time. This LoadInfo object allows us to reason about security throughout the lifetime of a channel. (Who initated the load? What contentType?, etc.) At the moment we use the  loadInfo information for MixedContentBlocker and ContentSecurityPolicy redirects. Further, the loadInfo->TriggeringPrincipal() [which is the same as the loadingPrincipal in most cases) is going to replace the current 'owner' of a channel eventually.

Once we have a loadInfo on every channel we are going to move security checks (currently handled by SecurityPolicies before the channel is created (basically shouldLoad()) into AsyncOpen. So in other words, currently one can bypass all security checks for a channel by not calling ShouldLoad. Once we move security checks into AsyncOpen, securityChecks can not be bypassed anymore.

To answer your question, using SystemPrincipal as the channel's principal will shortcut securityChecks (allowing the load) within asyncOpen once we have moved security checks into AsyncOpen().

What I've seen from you review, you already have a good feeling of what we should use - thanks for looking so closely.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #11)
> Comment on attachment 8541003 [details] [diff] [review]
> js_3_browser_tests.patch
> 
> Given that these are only used in tests, I suppose this doesn't matter too
> much. Is using getSystemPrincipal() for these introducing a behavior change
> of some sort?

Using default values (basically what we use) for testcases does *not* change any behavior of the test. As long as the SEC_NORMAL is used (and not any of SEC_SANDBOXED or SEC_INHTERIT_PRINCIPAL) the loadInfo arguments are not even used at the moment.

> What principal do newChannel()-created channels currently get
> by default?

With the loadInfo we are going to introduce the concept of having a loadingPrincipal attached to teach channel. Didn't exist before.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser.patch

(In reply to :Gavin Sharp [email: [e-mail]] from comment #10)
> >diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
> 
> >+    var channel = ioService.newChannelFromURI2(makeURI(linkURL),
> >+                                               doc,
> 
> This channel gets used to download content when you choose "Save X As..." in
> a context menu. "doc" is the content document of the element that you right
> clicked on (a link, an image, etc.). I'm not sure that it makes sense to
> pass it in here as aLoadingNode, because it is not "where the result of this
> request will be used" (the result of this request will be saved on disk).

Hm, not completely sure myself. Probably we should keep the doc anyway. I'll ask Jonas and Tanvi what they think about that case.
 
> >+                                               Ci.nsIContentPolicy.TYPE_OTHER);
> 
> This seems somewhat contrary to
> https://hg.mozilla.org/mozilla-central/annotate/a4a5d4fb5e2e/dom/base/
> nsIContentPolicy.idl#l36, but I think the existing options there don't
> really cater to this particular "persist arbitrary content to disk" case.

I agree, TYPE_OTHER is not a perfect fit, but still the best we got.

> >diff --git a/browser/components/feeds/FeedConverter.js b/browser/components/feeds/FeedConverter.js
> >diff --git a/browser/components/feeds/FeedWriter.js b/browser/components/feeds/FeedWriter.js
> 
> Neil or someone who understands how this code works should probably review
> these changes. I don't think using the system principal is correct. Can you
> split this into its own bug and ask Neil (neil@httl) for review?

I filed bug 1116278, but couldn't find Neil, it's not Neil Deakin, is it? https://wiki.mozilla.org/Modules/All#Firefox

> 
> >diff --git a/browser/extensions/shumway/content/ShumwayStreamConverter.jsm b/browser/extensions/shumway/content/ShumwayStreamConverter.jsm
> 
> We likely don't want the systemprincipal here, and I think TYPE_OTHER is
> wrong too. Probably best handled in a separate bug, request review from
> [e-mail].

Filed Bug 1116281 and flagged Yuri.
 
> >diff --git a/browser/components/places/PlacesProtocolHandler.js b/browser/components/places/PlacesProtocolHandler.js
> 
> I'm actually not sure what this is used for anymore, it seems maybe busted?
> NI Marco to see what he thinks.

Let's wait till Marco responds.

> >diff --git a/browser/metro/components/SessionStore.js b/browser/metro/components/SessionStore.js
> You can omit the changes to this file, it is effectively dead code.

Reverted changes for SessionStore.js

Comment 15

[Marco Bonardo [:mak]]

PlacesProtocolHandler.js is dead code, it was added in bug 529597 to make place: queries viewable in-content, that project was never completed. bug 737046 exists to remove it, I will convert it to a mentored bug.
Its scope was to read data from the bookmarks/history database and present it to the user in an in-content ui.
While we wait for the removal, I think the system principal should be ok.
The type should be TYPE_DOCUMENT though.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> At the moment we use the  loadInfo
> information for MixedContentBlocker and ContentSecurityPolicy redirects.
> Further, the loadInfo->TriggeringPrincipal() [which is the same as the
> loadingPrincipal in most cases) is going to replace the current 'owner' of a
> channel eventually.

OK - this is a little scary then. We need to carefully audit those getSystemPrincipal users.

The context menu saving code should not be able to cause a link to e.g. about:config to be saved, and WindowsPreviewPerTab.jsm should also not be able to download arbitrary content specified by the page.

Comment 17

[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 18

[Tanvi Vyas[:tanvi]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #15)
> PlacesProtocolHandler.js is dead code, it was added in bug 529597 to make
> place: queries viewable in-content, that project was never completed. bug
> 737046 exists to remove it, I will convert it to a mentored bug.
> Its scope was to read data from the bookmarks/history database and present
> it to the user in an in-content ui.
> While we wait for the removal, I think the system principal should be ok.
> The type should be TYPE_DOCUMENT though.

If it's dead code that's not being called anywhere, can we use null principal and TYPE_OTHER instead?

Comment 19

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

(In reply to :Gavin Sharp [email: [e-mail]] from comment #16)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> > At the moment we use the  loadInfo
> > information for MixedContentBlocker and ContentSecurityPolicy redirects.
> > Further, the loadInfo->TriggeringPrincipal() [which is the same as the
> > loadingPrincipal in most cases) is going to replace the current 'owner' of a
> > channel eventually.
> 
> OK - this is a little scary then. We need to carefully audit those
> getSystemPrincipal users.

Yup, that would be great. Note that the way network security works it's the responsibility of the creator of a channel to do all security checks. So the fact that you don't have any security checks there now already means that there aren't any security checks.

> The context menu saving code should not be able to cause a link to e.g.
> about:config to be saved, and WindowsPreviewPerTab.jsm should also not be
> able to download arbitrary content specified by the page.

Cool. That is probably not protected against right now. This is exactly the type of things that we were hoping to catch with these changes.

Comment 20

[Marco Bonardo [:mak]]

(In reply to Tanvi Vyas [:tanvi] from comment #18)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #15)
> > PlacesProtocolHandler.js is dead code
> If it's dead code that's not being called anywhere, can we use null
> principal and TYPE_OTHER instead?

The code can be called, just that it doesn't return anything useful. Surely I don't care if you want to use mores strict values, it's fine.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Gavin, thanks for your help - can you also suggest reviewers for the patches on this bug, (maybe :mak)?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I meant to ni? Gijs here too...

Comment 23

[:Gijs (he/him)]

Comment on attachment 8541003 [details] [diff] [review]
js_3_browser_tests.patch

I can do an initial pass the way I did for the toolkit patches tomorrow. It does seem like quite a bit of it has been covered in the bug already though...

Comment 24

[:Gijs (he/him)]

Comment on attachment 8541003 [details] [diff] [review]
js_3_browser_tests.patch

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

r=me with the get_user_media case addressed, and assuming gfritzsche is happy about us being safe in the plugin case.

::: browser/base/content/test/general/browser_devices_get_user_media_about_urls.js
@@ +155,5 @@
> +                                       null,      // aLoadingNode
> +                                       Services.scriptSecurityManager.getSystemPrincipal(),
> +                                       null,      // aTriggeringPrincipal
> +                                       Ci.nsILoadInfo.SEC_NORMAL,
> +                                       Ci.nsIContentPolicy.TYPE_OTHER);

This points to a .html which presumably is at least meant to be loaded at some point (not sure if the test does that, but there we are), so should probably be TYPE_DOCUMENT.

::: browser/base/content/test/plugins/browser_pluginplaypreview.js
@@ +106,5 @@
> +                                          null,      // aLoadingNode
> +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> +                                          null,      // aTriggeringPrincipal
> +                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                          Ci.nsIContentPolicy.TYPE_OTHER);

So this should really in theory be using the stuff from the page. In practice, it's just a test, and I guess that in the real case this is done by a plugin, and so it doesn't actually matter. feedback? gfritzsche to doublecheck my assumptions here

::: browser/components/feeds/test/unit/test_355473.js
@@ +26,5 @@
> +                                            null,      // aLoadingNode
> +                                            Services.scriptSecurityManager.getSystemPrincipal(),
> +                                            null,      // aTriggeringPrincipal
> +                                            Ci.nsILoadInfo.SEC_NORMAL,
> +                                            Ci.nsIContentPolicy.TYPE_OTHER);

Mhm. Reading bug 355473, I'm pretty sure the passed principal stuff here shouldn't matter in the case of this test.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8542282 [details] [diff] [review]
js_3_browser.patch

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

So... r=me for the ChromeProfileMigrator.js, that looks straightforward (feel free to land that if you want to get it out of the way).

PlacesProtocolHandler.js is dead.
AboutRedirector.js is also effectively dead (see bug 1039866).

I don't understand the last part of the WindowsPreviewPerTab.jsm change, so I've asked a question there.

The nsContextMenu.js hunk I think is currently actively wrong. I'm guessing it needs to be a null doc and a system loading principal, although I could be wrong.  Please see the comment (you said earlier that you'd ask Tanvi, maybe they want to weigh in :-) ).

(also, apologies for the delay in getting to this - I had errands to run today and they took longer than anticipated, so it's now later than I thought it would be...)

::: browser/base/content/nsContextMenu.js
@@ +1264,5 @@
> +                                               doc,
> +                                               null, // aLoadingPrincipal
> +                                               null, // aTriggeringPrincipal
> +                                               Ci.nsILoadInfo.SEC_NORMAL,
> +                                               Ci.nsIContentPolicy.TYPE_OTHER);

I can but share gavin's reservations about |doc| here. In particular, I expect that currently, if I have a page on http://www.nice.com and it links to http://www.evil.com/someexecutable.exe, I can right click and save as... just fine - despite the domain names, there's no reason to not allow that.

I would expect (and I could be wrong here) the doc-derived principal here to be the one for nice.com, and I vaguely hope that it loading content from evil.com would be stopped... so this doesn't seem right to me.

::: browser/components/places/PlacesProtocolHandler.js
@@ +29,5 @@
>      return uri;
>    },
>  
>    newChannel: function PPH_newChannel(aUri) {
> +    let chan = NetUtil.newChannel2(URL,

This file is now dead, per bug 737046

::: browser/metro/components/AboutRedirector.js
@@ +75,5 @@
>                getService(Ci.nsIIOService);
>  
> +    var newURI = ios.newURI(moduleInfo.uri, null, null);
> +
> +    var channel = ios.newChannelFromURIWithLoadInfo(newURI, aLoadInfo);

I'm assuming this is just as dead as the sessionstore.js code. OTOH, this seems so trivial, that sure, rs=me.

::: browser/modules/WindowsPreviewPerTab.jsm
@@ +79,5 @@
> +                                         null,      // aLoadingNode
> +                                         Services.scriptSecurityManager.getSystemPrincipal(),
> +                                         null,      // aTriggeringPrincipal
> +                                         Ci.nsILoadInfo.SEC_NORMAL,
> +                                         Ci.nsIContentPolicy.TYPE_IMAGE);

So gavin said:

> WindowsPreviewPerTab.jsm should also not be
> able to download arbitrary content specified by the page.

This doesn't protect against this. However, following callers, as best I can tell this is only called from getFaviconAsImage, which only ever asks for the default favicon, or the page's favicon. I'm assuming this is already safe, because the tabbrowser's icons also just src="" an http:// URI. I'm assuming this isn't any different (even if there's more machinery behind the thing), and webpages are apparently perfectly allowed to do e.g.:

data:text/html;charset=utf-8,<!DOCTYPE HTML PUBLIC "-%2F%2FW3C%2F%2FDTD HTML 4.0%2F%2FEN">%0D%0A<html lang%3D"en">%0D%0A<head>%0D%0A<link rel%3D"icon" href%3D"https%3A%2F%2Fwww.google.co.uk%2Fimages%2Fgoogle_favicon_128.png">%0D%0A<%2Fhtml>%0D%0A

So I'm assuming this is OK.

@@ +86,5 @@
>      channel.setPrivate(privateMode);
>    } catch (e) {
>      // Ignore channels which do not support nsIPrivateBrowsingChannel
>    }
> +  NetUtil.asyncFetch2(channel, function(inputStream, resultCode) {

Not sure I get it. Why does this need to use asyncFetch2, without adding any of the additional context parameters? If the signatures are the same, can't we just update NetUtil to do the right thing here, or does that break the world or something? :-)

Comment 26

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #25)
> I'm assuming this is just as dead as the sessionstore.js code. OTOH, this
> seems so trivial, that sure, rs=me.

I forgot to remove this comment per IRC discussion. In the interest of clarity, no, let's just not update metro anymore so that we can remove it efficiently.

Comment 27

[Tanvi Vyas[:tanvi]]

(In reply to :Gijs Kruitbosch from comment #25)
> Comment on attachment 8542282 [details] [diff] [review]
> js_3_browser.patch
>
> ::: browser/base/content/nsContextMenu.js
> @@ +1264,5 @@
> > +                                               doc,
> > +                                               null, // aLoadingPrincipal
> > +                                               null, // aTriggeringPrincipal
> > +                                               Ci.nsILoadInfo.SEC_NORMAL,
> > +                                               Ci.nsIContentPolicy.TYPE_OTHER);
> 
> I can but share gavin's reservations about |doc| here. In particular, I
> expect that currently, if I have a page on http://www.nice.com and it links
> to http://www.evil.com/someexecutable.exe, I can right click and save as...
> just fine - despite the domain names, there's no reason to not allow that.
> 
> I would expect (and I could be wrong here) the doc-derived principal here to
> be the one for nice.com, and I vaguely hope that it loading content from
> evil.com would be stopped... so this doesn't seem right to me.
> 

Okay, I see what you are saying.  Currently, no security checks are done when trying to "save link as".  So you can save http://www.evil.com even if it's from http://www.nice.com.  When we move security checks to asyncOpen(), that will change here https://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1293.  If we put in systemPrincipal, we essentially bypass these checks.  That seems like the right thing to do here, because there are less malicious situations where we do want to be able to save link as (http://a.com has a link to http://b.com that the user "saves link as").  However, the url we are saving is coming from a webpage, so per Jonas, we shouldn't use system here.  needinfo'ing him to see what he thinks.

Comment 28

[Tanvi Vyas[:tanvi]]

Christoph - for browser/components/places/PlacesProtocolHandler.js, either don't change the callsite or use a nullprincipal and TYPE_OTHER.

Comment 29

[Tanvi Vyas[:tanvi]]

(In reply to :Gijs Kruitbosch from comment #25)
> Comment on attachment 8542282 [details] [diff] [review]
> js_3_browser.patch
>
> ::: browser/modules/WindowsPreviewPerTab.jsm
> @@ +79,5 @@
> > +                                         null,      // aLoadingNode
> > +                                         Services.scriptSecurityManager.getSystemPrincipal(),
> > +                                         null,      // aTriggeringPrincipal
> > +                                         Ci.nsILoadInfo.SEC_NORMAL,
> > +                                         Ci.nsIContentPolicy.TYPE_IMAGE);
> 
> So gavin said:
> 
> > WindowsPreviewPerTab.jsm should also not be
> > able to download arbitrary content specified by the page.
> 
> This doesn't protect against this. However, following callers, as best I can
> tell this is only called from getFaviconAsImage, which only ever asks for
> the default favicon, or the page's favicon.
The only callers so far.  We could end up with more callers that do end up downloading arbitrary images specified by the webpage.

> I'm assuming this is already
> safe, because the tabbrowser's icons also just src="" an http:// URI. I'm
> assuming this isn't any different (even if there's more machinery behind the
> thing), and webpages are apparently perfectly allowed to do e.g.:
>
If it's an https:// page that includes an http:// image, content policies (which are moving to asyncOpen) would want to know that the loadingPrincipal is https and handle the mixed content favicon accordingly.  If we use system, we'd bypass this check.
 
> data:text/html;charset=utf-8,<!DOCTYPE HTML PUBLIC "-%2F%2FW3C%2F%2FDTD HTML
> 4.0%2F%2FEN">%0D%0A<html lang%3D"en">%0D%0A<head>%0D%0A<link rel%3D"icon"
> href%3D"https%3A%2F%2Fwww.google.co.uk%2Fimages%2Fgoogle_favicon_128.
> png">%0D%0A<%2Fhtml>%0D%0A
> 
> So I'm assuming this is OK.
> 

Can we pass the document's principal from callers into getFaviconAsImage, then pass it into imageFromURI and use that instead of system?

Comment 30

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

> ::: browser/base/content/nsContextMenu.js
> @@ +1264,5 @@
> > +                                               doc,
> > +                                               null, // aLoadingPrincipal
> > +                                               null, // aTriggeringPrincipal
> > +                                               Ci.nsILoadInfo.SEC_NORMAL,
> > +                                               Ci.nsIContentPolicy.TYPE_OTHER);
> 
> I can but share gavin's reservations about |doc| here. In particular, I
> expect that currently, if I have a page on http://www.nice.com and it links
> to http://www.evil.com/someexecutable.exe, I can right click and save as...
> just fine - despite the domain names, there's no reason to not allow that.
> 
> I would expect (and I could be wrong here) the doc-derived principal here to
> be the one for nice.com, and I vaguely hope that it loading content from
> evil.com would be stopped... so this doesn't seem right to me.

No, the above is not correct.

You are right that the principal would be for nice.com. However even if the URL is for evil.com, we won't block the load. If we did, then it would mean that nice.com couldn't load images or scripts from a CDN.

That said, we could definitely use a system principal as the loadingPrincipal, and doc.nodePrincipal as triggeringPrincipal. I think that would be ok too here.

Comment 31

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

Though I always feel iffy to use the system principal as either the loadingPrincipal or triggeringPrincipal when the URL is coming from a webpage. So I think using the document's principal as both loadingPrincipal and triggeringPrincipal is the right thing to do. At least for now.

Comment 32

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

Favicons should absolutely use the document's principal as both loadingPrincipal and triggeringPrincipal. A favicon is really very similar to an <img>. It's just that the rendering is done outside the page.

So we should pass in the document as the loadingNode and null for both principals. That way necko will grab both the loadingPrincipal and triggeringPrincipal off the document.

Comment 33

[:Gijs (he/him)]

(In reply to Tanvi Vyas [:tanvi] from comment #29)
> (In reply to :Gijs Kruitbosch from comment #25)
> > ::: browser/modules/WindowsPreviewPerTab.jsm
> 
> Can we pass the document's principal from callers into getFaviconAsImage,
> then pass it into imageFromURI and use that instead of system?

This (or Jonas' suggestion of passing the document for loadingNode and null for both principal arguments) sounds fine to me. Sorry for not being strict enough.

(In reply to Jonas Sicking (:sicking) from comment #30)
> > ::: browser/base/content/nsContextMenu.js
> You are right that the principal would be for nice.com. However even if the
> URL is for evil.com, we won't block the load. If we did, then it would mean
> that nice.com couldn't load images or scripts from a CDN.

They (images/scripts) have different loading types, though. I hope that we will block TYPE_XMLHTTPREQUEST, for instance. This is TYPE_OTHER. What are the plans for that? I had assumed we'd be the least permissive for it, but maybe that assumption is wrong.

(In reply to Jonas Sicking (:sicking) from comment #32)
> Favicons should absolutely use the document's principal as both
> loadingPrincipal and triggeringPrincipal. A favicon is really very similar
> to an <img>. It's just that the rendering is done outside the page.

Sorry for getting this wrong, too... :-\
How should/could we ensure this happens for the xul <image>s used in the tabs, which are in the chrome document and therefore probably currently get the system principal? Should we have a bug on file to do whatever magic is necessary there? (custom forwarding protocol or something? are there easier solutions?)

Comment 34

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

(In reply to :Gijs Kruitbosch from comment #33)
> (In reply to Jonas Sicking (:sicking) from comment #30)
> > > ::: browser/base/content/nsContextMenu.js
> > You are right that the principal would be for nice.com. However even if the
> > URL is for evil.com, we won't block the load. If we did, then it would mean
> > that nice.com couldn't load images or scripts from a CDN.
> 
> They (images/scripts) have different loading types, though. I hope that we
> will block TYPE_XMLHTTPREQUEST, for instance. This is TYPE_OTHER. What are
> the plans for that? I had assumed we'd be the least permissive for it, but
> maybe that assumption is wrong.

I don't think we've really made a decision on this. However my thinking was that we don't use the load type to determine security policy. Instead we use the securityflags argument to specify flags like "same origin only", "cross origin", "use CORS" or "use CORS with cookies".

For things like TYPE_IMAGE the policy actually varies with what attributes are set on the <img> element.

> (In reply to Jonas Sicking (:sicking) from comment #32)
> > Favicons should absolutely use the document's principal as both
> > loadingPrincipal and triggeringPrincipal. A favicon is really very similar
> > to an <img>. It's just that the rendering is done outside the page.
> 
> Sorry for getting this wrong, too... :-\
> How should/could we ensure this happens for the xul <image>s used in the
> tabs, which are in the chrome document and therefore probably currently get
> the system principal? Should we have a bug on file to do whatever magic is
> necessary there? (custom forwarding protocol or something? are there easier
> solutions?)

That's a really good question. We probably need some ability for xul <image>s to "pretend" they are part of another document. Somehow. Definitely file a separate bug for this.

Comment 35

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gijs Kruitbosch from comment #24)
> ::: browser/base/content/test/plugins/browser_pluginplaypreview.js
> @@ +106,5 @@
> > +                                          null,      // aLoadingNode
> > +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> > +                                          null,      // aTriggeringPrincipal
> > +                                          Ci.nsILoadInfo.SEC_NORMAL,
> > +                                          Ci.nsIContentPolicy.TYPE_OTHER);
> 
> So this should really in theory be using the stuff from the page. In
> practice, it's just a test, and I guess that in the real case this is done
> by a plugin, and so it doesn't actually matter. feedback? gfritzsche to
> doublecheck my assumptions here

Yes, that seems fine.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser.patch

Hi Gijs,

let me summarize all the decisions of that bug and see if we got everything correct:

* browser/base/content/nsContextMenu.js
Using the 'doc' as the loadingPrincipal (TriggeringPrincipal) as per Comment 31.

* browser/components/migration/ChromeProfileMigrator.js
Correct and done as per Comment 25.

* browser/metro/components/AboutRedirector.js
Correct and done as per Comment 25.

* browser/modules/WindowsPreviewPerTab.jsm
Passing the document to getFaviconAsImage() as discussed in Comment 33.
I am not sure how to query the document in this context, is this.win.win.document the right way of doing this?

Comment 37

[:Gijs (he/him)]

Comment on attachment 8558213 [details] [diff] [review]
js_3_browser.patch

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

r- because of WindowsPreviewPerTab, but see also the metro note below.

::: browser/metro/components/AboutRedirector.js
@@ +75,5 @@
>                getService(Ci.nsIIOService);
>  
> +    var newURI = ios.newURI(moduleInfo.uri, null, null);
> +
> +    var channel = ios.newChannelFromURIWithLoadInfo(newURI, aLoadInfo);

We shouldn't be fixing metro anymore, as per comment 26 (sorry that it gets confusing with several patches + review rounds in a bug). So you can rm the hunk for this file from the patch.

::: browser/modules/WindowsPreviewPerTab.jsm
@@ +501,5 @@
>      let preview = AeroPeek.taskbar.createTaskbarTabPreview(docShell, controller);
>      preview.visible = AeroPeek.enabled;
>      preview.active = this.tabbrowser.selectedTab == controller.tab;
>      // Grab the default favicon
> +    getFaviconAsImage(this.win.win.document, null, PrivateBrowsingUtils.isWindowPrivate(this.win), function (img) {

No, this.win.win is wrong, I'm pretty sure. I don't know why you see this.win.win as even being available (did you test this?) - but this.win itself is a chrome window, not the content window.

You basically want controller.linkedBrowser.contentWindow.document, AFAICT.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser.patch

Gijs,

thanks for you comments.
* I removed the changes to AboutRedirectors.js even though I am not sure why we shouldn't land those changes.

* The other question; I am not an expert in this code, can you please tell me how to query the document in the 2 cases of getFaviconAsImage() within WindowsPreviewPerTab.jsm?

Comment 39

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #27)
> > ::: browser/base/content/nsContextMenu.js
> Okay, I see what you are saying.  Currently, no security checks are done
> when trying to "save link as".  So you can save http://www.evil.com even if
> it's from http://www.nice.com.  When we move security checks to asyncOpen(),
> that will change here
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> nsContextMenu.js#1293.  If we put in systemPrincipal, we essentially bypass
> these checks.  That seems like the right thing to do here, because there are
> less malicious situations where we do want to be able to save link as
> (http://a.com has a link to http://b.com that the user "saves link as"). 
> However, the url we are saving is coming from a webpage, so per Jonas, we
> shouldn't use system here.  needinfo'ing him to see what he thinks.

We've decided to use the doc here instead of systemPrincipal.  When we move security checks, we should be careful to test this scenario (save link as on a link who's origin differs from the top level document's origin and make sure that the html is actually saved to disk).

Comment 40

[Tanvi Vyas[:tanvi]]

Gijs, can you file a bug for this:


(In reply to Jonas Sicking (:sicking) from comment #34)
> > Sorry for getting this wrong, too... :-\
> > How should/could we ensure this happens for the xul <image>s used in the
> > tabs, which are in the chrome document and therefore probably currently get
> > the system principal? Should we have a bug on file to do whatever magic is
> > necessary there? (custom forwarding protocol or something? are there easier
> > solutions?)
> 
> That's a really good question. We probably need some ability for xul
> <image>s to "pretend" they are part of another document. Somehow. Definitely
> file a separate bug for this.

Comment 41

[:Gijs (he/him)]

(In reply to Tanvi Vyas [:tanvi] from comment #40)
> Gijs, can you file a bug for this:

I did at the time, it's bug 1125627. :-)

I'll reply in more detail to comment #38 tomorrow morning (it's 1.20am here, just came back from London).

Comment 42

[:Gijs (he/him)]

Comment on attachment 8559390 [details] [diff] [review]
js_3_browser.patch

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

w/ the fix below, r=me

::: browser/modules/WindowsPreviewPerTab.jsm
@@ +596,5 @@
>    //// Browser progress listener
>    onLinkIconAvailable: function (aBrowser, aIconURL) {
>      let self = this;
> +    getFaviconAsImage(
> +      [???].document, // which document in this case?

aBrowser.contentWindow.document

should work here.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser.patch

Gijs, thanks for all your help and feedback.

Carrying over r+.

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93c1130d0d47
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aaca5080baf

Comment 45

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/93c1130d0d47
https://hg.mozilla.org/mozilla-central/rev/8aaca5080baf

Comment 46

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I believe this has caused bug 1130816.