1087744 - Make JS callers of ios.newChannel call ios.newChannel2 in toolkit/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit.patch

This is the last one of the patches we have to review. Hopefully we pass the right arguments everywhere.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_tests.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Gavin, similar to what we did in Bug 1087726, we also need to find reviewers for this bug. Since you are already helping me in Bug 1087726 and since you are a toolkit peer, I was wondering if you can help me find reviewers for this patch as well?

At the moment I provide a separate patch for tests, but of course I can split up the patches they way you would like them and the way it makes sense for reviewers.

Comment 5

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

[Christoph Kerschbaumer [:ckerschb]]

Gavin, if you are too busy finding reviewers here, I could ask someone else - just let me know - thanks.

Comment 7

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

Gijs/Dolske, can you guys take these reviews (this bug and bug 1087726)?

Comment 8

[:Gijs (he/him)]

Comment on attachment 8542256 [details] [diff] [review]
js_15_toolkit_tests.patch

Did you do a try push already? :-)

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8542256 [details] [diff] [review]
> js_15_toolkit_tests.patch
> 
> Did you do a try push already? :-)

Not recently, but a while ago, why?
Of course I will do another one before I am going to land the patches on inbound!

Comment 10

[:Gijs (he/him)]

Comment on attachment 8542255 [details] [diff] [review]
js_15_toolkit.patch

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

I wonder if for a lot of these TYPE_DATAREQUEST wouldn't be better. It says it's an alias for TYPE_XMLHTTPREQUEST, which this obviously isn't, but they are often requests for data. Can you clarify?

I was planning to review the tests now, too, but it's been 1.5 hours and it's now 2.35am, so I think that'll have to happen after sleep. Sorry!

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +210,5 @@
>        }.bind(this)
>      };
>  
>      try {
> +      let channel = Services.io.newChannelFromURI2(NetUtil.newURI(aSpec),

If this was C++ I'd ask to assert that the scheme is resource:// or file:///, because AFAICT those are the only options here, but I'd like to be sure...

Then again, from a security perspective, at that point I guess we've already lost if code gets here, so there's probably little point and it's not related to this change per se.

::: toolkit/components/places/nsLivemarkService.js
@@ +673,5 @@
> +      let channel = NetUtil.newChannel2(this.feedURI.spec,
> +                                        null,
> +                                        null,
> +                                        null,      // aLoadingNode
> +                                        Services.scriptSecurityManager.getSystemPrincipal(),

Reading: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsLivemarkService.js#889

I would expect this to use the same URI-based principal. Is there any reason not to (would it break e.g. 30* redirects to other domains/principals)? Am I misunderstanding the point of the loading principal here?

@@ +676,5 @@
> +                                        null,      // aLoadingNode
> +                                        Services.scriptSecurityManager.getSystemPrincipal(),
> +                                        null,      // aTriggeringPrincipal
> +                                        Ci.nsILoadInfo.SEC_NORMAL,
> +                                        Ci.nsIContentPolicy.TYPE_OTHER).

Seeing as this is an XML doc, should this be TYPE_DOCUMENT, even if we're not going to load it in a browser?

::: toolkit/components/search/nsSearchService.js
@@ +1715,5 @@
> +                                                          null,      // aLoadingNode
> +                                                          Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                          null,      // aTriggeringPrincipal
> +                                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                                          Ci.nsIContentPolicy.TYPE_OTHER);

This is an image (see the logs above) so should be TYPE_IMAGE, I think?

::: toolkit/components/thumbnails/PageThumbsProtocol.js
@@ +82,5 @@
> +                                          null,      // aLoadingNode
> +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> +                                          null,      // aTriggeringPrincipal
> +                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                          Ci.nsIContentPolicy.TYPE_OTHER);

This is always an image (or file not found, I guess), so should probably also be TYPE_IMAGE

::: toolkit/content/contentAreaUtils.js
@@ +1162,5 @@
> +                                                 null,      // aLoadingNode
> +                                                 Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                 null,      // aTriggeringPrincipal
> +                                                 Ci.nsILoadInfo.SEC_NORMAL,
> +                                                 Ci.nsIContentPolicy.TYPE_OTHER);

This code is crazy confusing. However, after some archaeology, it seems the initial version was written by bsmedberg in bug 281847. Might want to get his stamp, because I don't claim to fully understand it, but I *think* that this is essentially guaranteed to go through external URI loading mechanisms before it goes back into a browser tab or any kind of "real" consumer (if it does), and so the current arguments seem fine. As bz said in 2005, "It might be worth filing a bug (cc me, darin, biesi) to talk about whether we should have some sort of better setup for this".

Quite.

::: toolkit/devtools/DevToolsUtils.js
@@ +481,5 @@
> +        null,      // aLoadingNode
> +        Services.scriptSecurityManager.getSystemPrincipal(),
> +        null,      // aTriggeringPrincipal
> +        Ci.nsILoadInfo.SEC_NORMAL,
> +        Ci.nsIContentPolicy.TYPE_OTHER);

The patch context here says "exports.fetch", and there is https://hg.mozilla.org/mozilla-central/annotate/a4a5d4fb5e2e/dom/base/nsIContentPolicy.idl#l158 . Are they different "fetch" things? If not, this should be TYPE_FETCH.

Either way, this file (and everything else in toolkit/devtools, same for the tests if there are any) should be reviewed by someone from devtools.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +162,5 @@
> +                                         null,      // aLoadingNode
> +                                         Services.scriptSecurityManager.getSystemPrincipal(),
> +                                         null,      // aTriggeringPrincipal
> +                                         Ci.nsILoadInfo.SEC_NORMAL,
> +                                         Ci.nsIContentPolicy.TYPE_OTHER);

Per the imgTools.decodeImageData(input) below, this should probably be TYPE_IMAGE. Still, this should get a devtools reviewer. :-)

::: toolkit/mozapps/extensions/internal/LightweightThemeImageOptimizer.jsm
@@ +127,5 @@
> +    null,      // aLoadingNode
> +    Services.scriptSecurityManager.getSystemPrincipal(),
> +    null,      // aTriggeringPrincipal
> +    Ci.nsILoadInfo.SEC_NORMAL,
> +    Ci.nsIContentPolicy.TYPE_OTHER);

Again, TYPE_IMAGE.

Nit: please reindent as something like:

this.foo.bar(
  baz,
  function quux(1, 2, 3) {
  },
  foopy,
  ...
);

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +1932,5 @@
> +                                                null,      // aLoadingNode
> +                                                Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                null,      // aTriggeringPrincipal
> +                                                Ci.nsILoadInfo.SEC_NORMAL,
> +                                                Ci.nsIContentPolicy.TYPE_OTHER);

This probably doesn't matter because all we're interested in is the resolved URI...

@@ +5391,5 @@
> +                                         null,      // aLoadingNode
> +                                         Services.scriptSecurityManager.getSystemPrincipal(),
> +                                         null,      // aTriggeringPrincipal
> +                                         Ci.nsILoadInfo.SEC_NORMAL,
> +                                         Ci.nsIContentPolicy.TYPE_OTHER);

Wonder if it's worth creating a TYPE_XPI...

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1368,5 @@
> +                                          null,      // aLoadingNode
> +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> +                                          null,      // aTriggeringPrincipal
> +                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                          Ci.nsIContentPolicy.TYPE_OTHER);

This more than anything seems like it'd fit TYPE_DATAREQUEST.

::: toolkit/webapps/NativeApp.jsm
@@ +449,5 @@
> +    let channel = NetUtil.newChannel2(aIconURI,
> +                                      null,
> +                                      null,
> +                                      null,      // aLoadingNode
> +                                      Services.scriptSecurityManager.getSystemPrincipal(),

I expect that this should be the principal for the web app's origin from this.webappJson . It's probably wise to ask :fabrice or Marco Castelluccio to review the change to this file.

@@ +452,5 @@
> +                                      null,      // aLoadingNode
> +                                      Services.scriptSecurityManager.getSystemPrincipal(),
> +                                      null,      // aTriggeringPrincipal
> +                                      Ci.nsILoadInfo.SEC_NORMAL,
> +                                      Ci.nsIContentPolicy.TYPE_OTHER);

aIconURI makes me expect that this should be TYPE_IMAGE.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8542256 [details] [diff] [review]
js_15_toolkit_tests.patch

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

::: toolkit/components/downloads/test/unit/test_cancel_download_files_removed.js
@@ +87,5 @@
>  
>    let set = DownloadListener.set = tests[currentTest];
>    currentTest++;
>  
> +  let channel = NetUtil.newChannel2("http://localhost:" +,

This survived tryserver? I'm so confused. Note "+," at the end.

Generally, might want an intermediate var for the url here, that'll solve any confusion about what parameters are going in here.

::: toolkit/components/downloads/test/unit/test_download_samename.js
@@ +110,5 @@
>    }
>    let set = DownloadListener.set = tests[currentTest];
>    currentTest++;
>  
> +  let channel = NetUtil.newChannel2("http://localhost:" +,

Same

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +1827,5 @@
>            };
>  
>            // Create a channel from the source, and listen to progress
>            // notifications.
> +          let channel = NetUtil.newChannel2(NetUtil.newURI(download.source.url),

This looks like it should be in the other patch.

I'd like :paolo to take a second look here, but considering this is a download, I'm a little worried that this is the system principal.

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +594,5 @@
> +    null,      // aLoadingNode
> +    Services.scriptSecurityManager.getSystemPrincipal(),
> +    null,      // aTriggeringPrincipal
> +    Ci.nsILoadInfo.SEC_NORMAL,
> +    Ci.nsIContentPolicy.TYPE_OTHER);

Here too, this should be indented to avoid confusion with the remaining lines in the block:

NetUtil.asyncFetch2(
  file,
  function(..) {
  },
  ...
);

::: toolkit/components/mediasniffer/test/unit/test_mediasniffer.js
@@ +70,5 @@
>  {
>    var ios = Components.classes["@mozilla.org/network/io-service;1"].
>                         getService(Ci.nsIIOService);
> +  var chan = ios.newChannel2("http://localhost:" +
> +                             httpserver.identity.primaryPort + url,

Probably still nice to stick this in a temp?

@@ +77,5 @@
> +                             null,      // aLoadingNode
> +                             Services.scriptSecurityManager.getSystemPrincipal(),
> +                             null,      // aTriggeringPrincipal
> +                             Ci.nsILoadInfo.SEC_NORMAL,
> +                             Ci.nsIContentPolicy.TYPE_OTHER);

Should this be TYPE_MEDIA ?

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +88,5 @@
>  let reference_fetch_file = function reference_fetch_file(path, test) {
>    test.info("Fetching file " + path);
>    let promise = Promise.defer();
>    let file = new FileUtils.File(path);
> +  NetUtil.asyncFetch2(file,

Probably good to get :Yoric to review the osfile bits.

::: toolkit/components/places/tests/browser/browser_favicon_setAndFetchFaviconForPage.js
@@ +53,5 @@
> +    null,      // aLoadingNode
> +    Services.scriptSecurityManager.getSystemPrincipal(),
> +    null,      // aTriggeringPrincipal
> +    Ci.nsILoadInfo.SEC_NORMAL,
> +    Ci.nsIContentPolicy.TYPE_OTHER);

getIconFile... probably should be TYPE_OTHER

I'd care about the principal here except it's a test, so it likely doesn't matter at all.

Also, indenting again. :-)

::: toolkit/components/places/tests/favicons/test_moz-anno_favicon_mime_type.js
@@ +62,5 @@
> +                                        null,      // aLoadingNode
> +                                        Services.scriptSecurityManager.getSystemPrincipal(),
> +                                        null,      // aTriggeringPrincipal
> +                                        Ci.nsILoadInfo.SEC_NORMAL,
> +                                        Ci.nsIContentPolicy.TYPE_OTHER)) {

TYPE_IMAGE here and below.

And, holy moly, this just got ugly.

Can you write a helper function and stick it in the head.js in this file? Just make it always request TYPE_IMAGE, and use typeof on the only parameter to decide whether you need ios.newChannelFromURI2 or newChannel2 - or normalize on the former and just convert the string to a URI before making the two calls below. :-)

::: toolkit/components/places/tests/network/test_history_redirects.js
@@ +61,5 @@
>  
>  function run_test() {
>    do_test_pending();
>  
> +  var chan = NetUtil.ioService.newChannelFromURI2(uri(PERMA_REDIR_URL),

Not sure what this is doing, ::mak should probably review this. He might have opinions on the favicon bits as well. :-)

::: toolkit/components/prompts/test/test_modal_prompts.html
@@ +1171,5 @@
> +                                        null,      // aLoadingNode
> +                                        SpecialPowers.Services.scriptSecurityManager.getSystemPrincipal(),
> +                                        null,      // aTriggeringPrincipal
> +                                        Ci.nsILoadInfo.SEC_NORMAL,
> +                                        Ci.nsIContentPolicy.TYPE_OTHER);

I'm assuming we don't prompt for auth except for documents (so not for just images or script or something) or that if we don't now, we might in the future, so for future-proofness, would it make sense to pass TYPE_DOCUMENT here?

::: toolkit/components/search/tests/xpcshell/test_serialize_file.js
@@ +47,5 @@
> +    null,      // aLoadingNode
> +    Services.scriptSecurityManager.getSystemPrincipal(),
> +    null,      // aTriggeringPrincipal
> +    Ci.nsILoadInfo.SEC_NORMAL,
> +    Ci.nsIContentPolicy.TYPE_OTHER);

Nit: more indenting

::: toolkit/mozapps/extensions/test/browser/browser_openDialog.js
@@ +28,5 @@
> +                                 null,      // aLoadingNode
> +                                 Services.scriptSecurityManager.getSystemPrincipal(),
> +                                 null,      // aTriggeringPrincipal
> +                                 Ci.nsILoadInfo.SEC_NORMAL,
> +                                 Ci.nsIContentPolicy.TYPE_OTHER);

This should be using whatever the current forwarding details are for newChannel, but shouldn't this also be implementing newChannel2 itself?

Comment 12

[:Gijs (he/him)]

So, for the next round of patches, could you please:
- split off the devtools bits (probably code + tests in one patch would work, could be a separate bug to keep things tidy) and request review from :jryans (who can probably forward as appropriate, I just picked him semi-randomly).
- split off the OS.File bits and get :yoric to review those
- split off the webapps bits and ask :fabrice or [:marco] for review
- split off the jsdownloads bits and ask :paolo for review
- split off the places bits and ask ::mak for review

... gosh, that makes me feel lazy. Hm. Well, with this many splits, I guess it makes sense to also split the work into separate bugs - all the components mentioned above have their own places in bugzilla, and then things can land independently as and when you get reviews in. Does that make sense or does it feel like too much make-work?

I'm happy to review the rest, although as noted earlier, it's probably wise to feedback? bsmedberg on the toolkit contentAreaUtils bit.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit.patch

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_tests.patch

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_components_jsdownloads.patch

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_components_osfile_tests.patch

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_components_places.patch

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_contentareautils.patch

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Gijs, first, thank you for looking so closely!

Here is what I've address in the patches:
* Fixed all the indendation nits (hopefully I got them all :-) )
* Split the patches into 3 bugs:
  a) This bug keeping everything within toolkit/ including toolkit/components (I think it doesn't make sense to have parts of components here and parts somewhere else)
  b) Bug 1124951, covering toolkit/devtools
  c) Bug 1124950, covering toolkit/webapps
* LiveBookmarks load data (potentially) by loading an XML document. I discussed with dvetitz and we concluded that TYPE_DATAREQEUST is what we should use for those cases. Probably for other cases as well within those patches (please tell me where).
* At this point we don't want to introduce new content types, so we should use content types we have available defined in nsIContentPolicy.idl.
* Incorporated suggestions regarding the loadingPrincipal as well as content types.

I think we are ready for the next round!

Comment 20

[:Gijs (he/him)]

Comment on attachment 8553423 [details] [diff] [review]
js_15_toolkit.patch

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

r+ with judicious use of TYPE_DATAREQUEST

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +661,5 @@
> +                                                  null,      // aLoadingNode
> +                                                  Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                  null,      // aTriggeringPrincipal
> +                                                  Ci.nsILoadInfo.SEC_NORMAL,
> +                                                  Ci.nsIContentPolicy.TYPE_OTHER);

Loading a local memory report from file... also TYPE_DATAREQUEST?

::: toolkit/components/search/nsSearchService.js
@@ +1391,5 @@
> +                                                    null,      // aLoadingNode
> +                                                    Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                    null,      // aTriggeringPrincipal
> +                                                    Ci.nsILoadInfo.SEC_NORMAL,
> +                                                    Ci.nsIContentPolicy.TYPE_OTHER);

This and the sync version below download a search engine spec (XML file). Could be TYPE_DATAREQUEST? I defer to you/dveditz.

@@ +3819,5 @@
> +                                                        null,      // aLoadingNode
> +                                                        Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                        null,      // aTriggeringPrincipal
> +                                                        Ci.nsILoadInfo.SEC_NORMAL,
> +                                                        Ci.nsIContentPolicy.TYPE_OTHER);

This seems to read list.txt from a chrome package, so based on comment #19, I think this should be TYPE_DATAREQUEST.

@@ +4885,5 @@
> +                                                        null,      // aLoadingNode
> +                                                        Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                        null,      // aTriggeringPrincipal
> +                                                        Ci.nsILoadInfo.SEC_NORMAL,
> +                                                        Ci.nsIContentPolicy.TYPE_OTHER).open();

This reads engine state from a local JSON file, so probably also TYPE_DATAREQUEST

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ +224,5 @@
> +                                                 null,      // aLoadingNode
> +                                                 Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                 null,      // aTriggeringPrincipal
> +                                                 Ci.nsILoadInfo.SEC_NORMAL,
> +                                                 Ci.nsIContentPolicy.TYPE_OTHER);

This is going to google safebrowsing to request data on a partial hash match, so should probably be TYPE_DATAREQUEST as well.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8553424 [details] [diff] [review]
js_15_toolkit_tests.patch

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

I'm really sorry for not being more clear about this, but can you include the relevant tests from this patch in the patches (or split them up into parallel patches) for the respective downloads, places and osfile reviewers? For similar reasons to the reasons regarding the code changes, I'm not sure I should be reviewing the osfile/downloads/places test changes here.

With the below addressed, consider the marked f+ as r=me for

test_mediasniffer.js | test_mediasniffer_ext.js | test_prompt.html | test_modal_prompts.html | test_serialize_file.js | test_ThirdPartyCookieProbe.js | browser_openDialog.js

Which I think is everything else.

::: toolkit/components/mediasniffer/test/unit/test_mediasniffer_ext.js
@@ +77,5 @@
> +                             null,      // aLoadingNode
> +                             Services.scriptSecurityManager.getSystemPrincipal(),
> +                             null,      // aTriggeringPrincipal
> +                             Ci.nsILoadInfo.SEC_NORMAL,
> +                             Ci.nsIContentPolicy.TYPE_OTHER);

This should be TYPE_MEDIA, too, I expect.

::: toolkit/components/passwordmgr/test/test_prompt.html
@@ +165,5 @@
> +                                         null,      // aLoadingNode
> +                                         Services.scriptSecurityManager.getSystemPrincipal(),
> +                                         null,      // aTriggeringPrincipal
> +                                         Ci.nsILoadInfo.SEC_NORMAL,
> +                                         Ci.nsIContentPolicy.TYPE_OTHER);

To be clear, you discussed/thought about changing these (and the one in test_modal_prompts.html - all for auth prompts) to TYPE_DOCUMENT (suggestion near the bottom of comment #11) ? I'm happy to defer to your opinion on whether that makes sense, even if these aren't actually loaded into a docshell etc. r+ either way, but want to make sure we've considered this.

::: toolkit/components/search/tests/xpcshell/test_serialize_file.js
@@ +49,5 @@
> +      null,      // aLoadingNode
> +      Services.scriptSecurityManager.getSystemPrincipal(),
> +      null,      // aTriggeringPrincipal
> +      Ci.nsILoadInfo.SEC_NORMAL,
> +      Ci.nsIContentPolicy.TYPE_OTHER);

As this is a search engine xml spec read from disk, it should probably also be TYPE_DATAREQUEST

::: toolkit/mozapps/extensions/test/browser/browser_openDialog.js
@@ +19,5 @@
>      uri.spec = aSpec;
>      return uri;
>    },
>  
>    newChannel: function CCP_newChannel(aURI) {

So this doesn't need to gain a newChannel2 implementation etc.? :-)

Comment 22

[Tanvi Vyas[:tanvi]]

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8542255 [details] [diff] [review]
> js_15_toolkit.patch
> 
> Review of attachment 8542255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/thumbnails/PageThumbsProtocol.js
> @@ +82,5 @@
> > +                                          null,      // aLoadingNode
> > +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> > +                                          null,      // aTriggeringPrincipal
> > +                                          Ci.nsILoadInfo.SEC_NORMAL,
> > +                                          Ci.nsIContentPolicy.TYPE_OTHER);
> 
If the uri is coming from a webpage, we should be using that webpage as the loadingNode/loadingPrincipal instead of system.  In this case, it appears that the thumbnail is coming from an webpage.  Anyway to get the right loadingPrincipal instead of using system?

Comment 23

[:Gijs (he/him)]

(In reply to Tanvi Vyas [:tanvi] from comment #22)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Comment on attachment 8542255 [details] [diff] [review]
> > js_15_toolkit.patch
> > 
> > Review of attachment 8542255 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/thumbnails/PageThumbsProtocol.js
> > @@ +82,5 @@
> > > +                                          null,      // aLoadingNode
> > > +                                          Services.scriptSecurityManager.getSystemPrincipal(),
> > > +                                          null,      // aTriggeringPrincipal
> > > +                                          Ci.nsILoadInfo.SEC_NORMAL,
> > > +                                          Ci.nsIContentPolicy.TYPE_OTHER);
> > 
> If the uri is coming from a webpage, we should be using that webpage as the
> loadingNode/loadingPrincipal instead of system.  In this case, it appears
> that the thumbnail is coming from an webpage.  Anyway to get the right
> loadingPrincipal instead of using system?

No, this is for thumbnails of the webpages. It doesn't fetch anything from the web, right above the bit that got quoted is:

   newChannel: function Proto_newChannel(aURI) {
     let {url} = parseURI(aURI);
     let file = PageThumbsStorage.getFilePathForURL(url);
     let fileuri = Services.io.newFileURI(new FileUtils.File(file));

fileuri is what gets fetched here. Using the system principal for that seems fine; the thumbnails would be used by about:newtab, the windows taskbar previews and panorama (tab groups) and so on.

Comment 24

[Tanvi Vyas[:tanvi]]

Comment on attachment 8553425 [details] [diff] [review]
js_15_toolkit_components_jsdownloads.patch

>diff --git a/toolkit/components/jsdownloads/src/DownloadCore.jsm b/toolkit/components/jsdownloads/src/DownloadCore.jsm
>--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
>+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
>           // Create a channel from the source, and listen to progress
>           // notifications.
>-          let channel = NetUtil.newChannel(NetUtil.newURI(download.source.url));
>+          let channel = NetUtil.newChannel2(NetUtil.newURI(download.source.url),
>+                                            null,
>+                                            null,
>+                                            null,      // aLoadingNode
>+                                            Services.scriptSecurityManager.getSystemPrincipal(),
>+                                            null,      // aTriggeringPrincipal
>+                                            Ci.nsILoadInfo.SEC_NORMAL,
>+                                            Ci.nsIContentPolicy.TYPE_OTHER);

Assuming the download request came from a webpage.  Can we use that webpage as the loadingPrincipal instead of system?  Paolo, do we have access to the loading uri?

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 8553427 [details] [diff] [review]
js_15_toolkit_components_places.patch

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

::: toolkit/components/places/nsLivemarkService.js
@@ +671,5 @@
>        let loadgroup = Cc["@mozilla.org/network/load-group;1"].
>                        createInstance(Ci.nsILoadGroup);
> +      let feedPrincipal =
> +        secMan.getSimpleCodebasePrincipal(this.feedURI);
> +      let channel = NetUtil.newChannel2(this.feedURI.spec,

how much safe is this? What does getSimpleCodebasePrincipal do?
The truth here is that we can't get a principal from a document or such, we just have a feed uri and we want to read its contents.
It's effectively loading data from the Web. do we need any additional safety?

Comment 26

[:Paolo Amadini]

Comment on attachment 8553425 [details] [diff] [review]
js_15_toolkit_components_jsdownloads.patch

(In reply to Tanvi Vyas [:tanvi] from comment #24)
> Assuming the download request came from a webpage.  Can we use that webpage
> as the loadingPrincipal instead of system?  Paolo, do we have access to the
> loading uri?

Downloads can be initiated in a number of different ways. This includes "Save Page", drive-by downloads started by redirects in conjunction with "Content-Disposition" headers, and finally downloads initiated by single clicks, usually with "Content-Disposition" headers but also, I believe, with the "download" attribute on the "a" element.

The code you're looking at is shared by all these cases. Once we get here, we already determined it's safe for the download to be in the system-global list of downloads. We'll keep the download around across sessions and the same code will be hit again in case the download is restarted. It's also relevant that single-click downloads reuse the original request and don't open a new channel unless they're restarted.

So a better question would be if we need additional checks when a download starts, in order to determine if we should add it to the global list in the first place or ignore the request altogether.

Do we have a wiki page or other document detailing the goal of the additional security checks we make when opening the channel, i.e. why we are doing this?

Note that downloads are scanned on completion, when relevant for additional security, using the Application Reputation infrastructure.

Comment 27

[:Paolo Amadini]

Comment on attachment 8553425 [details] [diff] [review]
js_15_toolkit_components_jsdownloads.patch

+          let channel = NetUtil.newChannel2(NetUtil.newURI(download.source.url),
+                                            null,
+                                            null,
+                                            null,      // aLoadingNode
+                                            Services.scriptSecurityManager.getSystemPrincipal(),
+                                            null,      // aTriggeringPrincipal
+                                            Ci.nsILoadInfo.SEC_NORMAL,
+                                            Ci.nsIContentPolicy.TYPE_OTHER);

I filed bug 1125618 to get a better API for this. The code here probably doesn't need any change if the compatibility/simplicity approach is taken there, although comment 26 still applies.

Comment 28

[Benjamin Smedberg]

Comment on attachment 8553428 [details] [diff] [review]
js_15_toolkit_contentareautils.patch

This is making explicit something about the "openURL" function which is that it implicitly trusts its context. Can you at least document that behavior, and if there's any concern file a followup audit bug to make sure that the callers are correct?

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Paolo Amadini from comment #26)
> Comment on attachment 8553425 [details] [diff] [review]
> js_15_toolkit_components_jsdownloads.patch
> 
> (In reply to Tanvi Vyas [:tanvi] from comment #24)
> > Assuming the download request came from a webpage.  Can we use that webpage
> > as the loadingPrincipal instead of system?  Paolo, do we have access to the
> > loading uri?
> 
> Downloads can be initiated in a number of different ways. This includes
> "Save Page", drive-by downloads started by redirects in conjunction with
> "Content-Disposition" headers, and finally downloads initiated by single
> clicks, usually with "Content-Disposition" headers but also, I believe, with
> the "download" attribute on the "a" element.
> 
> The code you're looking at is shared by all these cases. Once we get here,
> we already determined it's safe for the download to be in the system-global
> list of downloads. We'll keep the download around across sessions and the
> same code will be hit again in case the download is restarted. It's also
> relevant that single-click downloads reuse the original request and don't
> open a new channel unless they're restarted.
> 
> So a better question would be if we need additional checks when a download
> starts, in order to determine if we should add it to the global list in the
> first place or ignore the request altogether.
> 
> Do we have a wiki page or other document detailing the goal of the
> additional security checks we make when opening the channel, i.e. why we are
> doing this?
> 
> Note that downloads are scanned on completion, when relevant for additional
> security, using the Application Reputation infrastructure.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8553425 [details] [diff] [review]
js_15_toolkit_components_jsdownloads.patch

Marking the patch here is obsolete, since I filed bug 1125991 to handle jsdownloads.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8553427 [details] [diff] [review]
js_15_toolkit_components_places.patch

Marking this patch as obsolete since I filed bug 1126004 to handle components/places

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_tests.patch

Gijs, I split all the code including testcases into separate bugs, so (hopefully) only the ones that you agreed to review are left in this patch.

> ::: toolkit/components/passwordmgr/test/test_prompt.html
> @@ +165,5 @@
> > +                                         null,      // aLoadingNode
> > +                                         Services.scriptSecurityManager.getSystemPrincipal(),
> > +                                         null,      // aTriggeringPrincipal
> > +                                         Ci.nsILoadInfo.SEC_NORMAL,
> > +                                         Ci.nsIContentPolicy.TYPE_OTHER);
> 
> To be clear, you discussed/thought about changing these (and the one in
> test_modal_prompts.html - all for auth prompts) to TYPE_DOCUMENT (suggestion
> near the bottom of comment #11) ? I'm happy to defer to your opinion on
> whether that makes sense, even if these aren't actually loaded into a
> docshell etc. r+ either way, but want to make sure we've considered this.

Yes, we discussed this, but since these are testcases, I don't have a strong opinion about the contentType in this case. I think it's fine to stick with TYPE_OTHER.

> ::: toolkit/components/search/tests/xpcshell/test_serialize_file.js
> >    newChannel: function CCP_newChannel(aURI) {
> 
> So this doesn't need to gain a newChannel2 implementation etc.? :-)

I think it would be too much of an overhead to also convert those callsites. It's also in testcode, so I assume it's not worth changing those callers as well.

Comment 33

[:Gijs (he/him)]

Comment on attachment 8554790 [details] [diff] [review]
js_15_toolkit_tests.patch

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

You left two downloads tests in here; please ask paolo to review those. r=me on everything else. Thanks!

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_tests.patch

Moved the downloads tests into Bug 1125991 and asked Paolo for review.

Carrying over r+ from Gijs.

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8553426 [details] [diff] [review]
js_15_toolkit_components_osfile_tests.patch

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

r=me

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_contentareautils.patch

Benjamin, I filed bug 1129707 to investigate the callsites of openURL() and potentially change the signature of openURL().

As of now, Jonas, Tanvi and myself think it's ok to use the systemPrincipal.
As requested in your Comment 28, I added a comment on top of the openURL() to indicate that no content security checks are performed when calling this function.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a542293c8f59
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd00f92694a
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e75bce4db4
https://hg.mozilla.org/integration/mozilla-inbound/rev/91100de4f2ad

Comment 38

[Phil Ringnalda (:philor)]

Backed out 35e75bce4db4 in https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed8507adfcf - I could have just fixed the failure to import Services in test_mediasniffer_ext.js, but I figured you might like to actually run the test once, before relanding it, just to see if it actually does work.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a542293c8f59
https://hg.mozilla.org/mozilla-central/rev/3dd00f92694a
https://hg.mozilla.org/mozilla-central/rev/91100de4f2ad

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Phil Ringnalda (:philor) from comment #38)
> Backed out 35e75bce4db4 in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed8507adfcf - I
> could have just fixed the failure to import Services in
> test_mediasniffer_ext.js, but I figured you might like to actually run the
> test once, before relanding it, just to see if it actually does work.

Thanks Phil, and sorry about that, only ran the tests locally. Marking as 'Reopenend' to push the missing patch from that bug.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Pushing the rebased tests including the missing include:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/bf159aaf6f06

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf159aaf6f06