1125991 - Make JS callers of ios.newChannel call ios.newChannel2 in toolkit/components/jsdownloads

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb}]

Before it gets too messy in bug Bug 1087744, lets split it up and use this bug for jsdownloads.

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_15_toolkit_components_jsdownloads_tests.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_15_toolkit_components_jsdownloads_tests.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_15_toolkit_components_jsdownloads.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Copying over Comment 26 from bug 1087744:

(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 5

[Christoph Kerschbaumer [:ckerschb}]

> > 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.

Probably, but we are not experts in this code, so I can't really tell at the moment.
 
> > 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?

Unfortunately we don't have a wiki (only the etherpad [1].

Simplified: the overall problem we have at the moment is that security checks for channels are done before a channel is even created by calling shouldLoad(), implemented within nsIContentSecurityPolicy.idl. Now, if a channel does not perform any security checks before creating a channel, then none are performed - which is obviously not that great.

The overall goal here is to attach a LoadInfo on all channels so we can reason about security throughout the lifetime of a channel. We are going to move security checks into AsyncOpen so that security checks are performed before the channel is openend. Attaching a loadInfo further allows us to perform security checks after redirects, becasue we are propagating the loadInfo to the redirected channel.

I hope that answers your question, if not, let me know!


[1] https://etherpad.mozilla.org/BetterNeckoSecurityHooks

Comment 6

[:Paolo Amadini]

Comment on attachment 8554758 [details] [diff] [review]
js_15_toolkit_components_jsdownloads_tests.patch

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

Changes looks fine, but can you please attach a patch with indentation corrected?

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +549,1 @@
>                                                       aInputStream.available());

nit: align

@@ +567,1 @@
>      });

incorrect indentation

Comment 7

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #6)
> @@ +567,1 @@
> >      });
> 
> incorrect indentation

Funny how the review tool didn't provide any context useful to understand where this is :-)

It's at the end of the patch.

Comment 8

[:Paolo Amadini]

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

Looks good. Per comment 5, there might be more work needed once more security checks are implemented, but we're not changing the current behavior so we can land this.

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_15_toolkit_components_jsdownloads_tests.patch

Fixed indentation nits.

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

Attachment: js_15_toolkit_components_downloads_tests.patch

As reqeusted by Gijs [1] I am also going to flag you for review on the download tests.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1087744#c33

Comment 11

[Tanvi Vyas[:tanvi]]

Jonas, can you take a look at the below comment from Paolo.  Are you okay with using system Principal for download requests - https://bugzilla.mozilla.org/attachment.cgi?id=8554760&action=diff#a/toolkit/components/jsdownloads/src/DownloadCore.jsm_sec1?  Sounds like the request has already gone through some vetting by the time it gets here.  If we do need to put the request through content policy / checkloaduri /etc, then we need to update all the places that call into this (which given Paolo's comment below sounds like a lot) and pass it along.  Can any of our policies / security checks prevent a user from downloading something?  Perhaps an addon generated content policy that is looking for malware?


(In reply to :Paolo Amadini from comment #26)
> 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.

Comment 12

[:Paolo Amadini]

Comment on attachment 8556023 [details] [diff] [review]
js_15_toolkit_components_downloads_tests.patch

These look good, don't need close scrutiny as they're going away soon.

Comment 13

[:Paolo Amadini]

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

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

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

nit: I've just noticed newChannel calls newURI for us already, you may remove it and just pass the URI string.

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

https://hg.mozilla.org/integration/mozilla-inbound/rev/719a742fc68f
https://hg.mozilla.org/integration/mozilla-inbound/rev/07970e4e2513
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed486b84db3


Also, clearing the needinfo for Jonas - in case we really want the change described in Comment 11 it needs to be a separate bug. Will bring that up in our next meeting on Wednesday!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/719a742fc68f
https://hg.mozilla.org/mozilla-central/rev/07970e4e2513
https://hg.mozilla.org/mozilla-central/rev/eed486b84db3

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/719a742fc68f
> https://hg.mozilla.org/integration/mozilla-inbound/rev/07970e4e2513
> https://hg.mozilla.org/integration/mozilla-inbound/rev/eed486b84db3
> 
> 
> Also, clearing the needinfo for Jonas - in case we really want the change
> described in Comment 11 it needs to be a separate bug. Will bring that up in
> our next meeting on Wednesday!

We missed this in our meeting today, so re-needinfo'ing Jonas to respond to comment 11.

Comment 17

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

(In reply to :Paolo Amadini from Bug 1087744 comment #26)
> 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.

Ideally we would grab the principal of the page which contains the download link and forward that principal to the code which calls newChannel here. Which means that we need to do that for all the codepaths above.

Then use that principal as both loadingPrincipal and triggeringPrincipal.

The result of that would be that we could eventually remove:

1. The other security checks that the download code does.
2. The code that sets the referrer.
3. The code that tracks and sets the private-browsing flag.

None of that could be removed quite yet though. But hopefully not too far into the future.

Comment 18

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Jonas Sicking (:sicking) from comment #17)
> (In reply to :Paolo Amadini from Bug 1087744 comment #26)
> > 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.
> 
> Ideally we would grab the principal of the page which contains the download
> link and forward that principal to the code which calls newChannel here.
> Which means that we need to do that for all the codepaths above.
> 
> Then use that principal as both loadingPrincipal and triggeringPrincipal.
> 
> The result of that would be that we could eventually remove:
> 
> 1. The other security checks that the download code does.
> 2. The code that sets the referrer.
> 3. The code that tracks and sets the private-browsing flag.
> 
> None of that could be removed quite yet though. But hopefully not too far
> into the future.

I agree, created
> Bug 1132784 - Pass the right Principal to LoadInfo for downloads
to follow up on this issue and closing this bug.