1469536 - Allow manual download of FTP files

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Tom S [:evilpie]]

See bug 1404744 comment 39. We should probably uplift this to beta.

Comment 1

[Tom S [:evilpie]]

Attachment: Allow manual download of FTP files. r?

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8986147 [details] [diff] [review]
Allow manual download of FTP files. r?

I wonder why this case doesn't use TYPE_SAVEAS_DOWNLOAD? The nsContentMenu code seems to use it anyway. A previous version of my original patch in bug 1404744 just allowed the system-principal, which would have worked here.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8986147 [details] [diff] [review]
Allow manual download of FTP files. r?

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

I would prefer if we can find the root cause of the problem. Where does the download originate? Ideally we should update that code with the right contentPolicyType (TYPE_SAVEAS_DOWNLOAD) and whitelist that together with TYPE_DOCUMENT.

Sounds good?

Comment 4

[Tom S [:evilpie]]

I will look into why this download doesn't have the right type, but I am somewhat skeptical that this is going to be something we want to uplift.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Just a note that we're building RCs today.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> I will look into why this download doesn't have the right type, but I am
> somewhat skeptical that this is going to be something we want to uplift.

Let's see if we can figure out where we generate that channel. If it's just the wrong content-type in the loadinfo, then this would be something I feel comfortable to uplift.

Thanks for looking into this problem!

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8986147 [details] [diff] [review]
Allow manual download of FTP files. r?

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

::: dom/security/nsContentSecurityManager.cpp
@@ +202,5 @@
>  
> +  // We want to allow manual downloading of FTP files. This is the closest
> +  // approximation of this action. The restriciton to the system principal
> +  // should make this safe.
> +  if (nsContentUtils::IsSystemPrincipal(triggeringPrincipal) &&

As discussed over IRC, I suppose we could whitelist load triggered by the systemPrincipal but not worrying about the TYPE_OTHER case as well. This should also fix the problem described in Bug 1469891.

Basically

if (nsContentUtils::IsSystemPrincipal(triggeringPrincipal) {
  return NS_OK;
}

and move that higher up in that function.

Agreed?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment 7 should than easily be uplift-able and we can re-evaluate what we do precisely about those cases in the next cycle.

Comment 9

[Tom S [:evilpie]]

Attachment: Allow the system principal to load FTP subresources. r?

Comment 10

[Tom S [:evilpie]]

I verified that this also fixes Bug 1469891.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8986511 [details] [diff] [review]
Allow the system principal to load FTP subresources. r?

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

Thanks Tom, r=me!

Can you file the uplift request as well?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74ec48ebf55
Allow the system principal to load FTP subresources. r=ckerschb

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8986511 [details] [diff] [review]
Allow the system principal to load FTP subresources. r?

Approval Request Comment
[Feature/Bug causing the regression]: bug 1404744
[User impact if declined]: Saving images and looking at pdf hosted on FTP servers is broken
[Is this code covered by automated tests?]: No :(
[Has the fix been verified in Nightly?]: Not yet. Manual local build only
[Needs manual test from QE? If yes, steps to reproduce]: Try downloading images from ftp://, by opening that image in a tab and right clicking "Save Image As". Open pdf file from ftp://, it should show up in the browser.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This allows more stuff that was allowed before.
[String changes made/needed]: -

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8986511 [details] [diff] [review]
Allow the system principal to load FTP subresources. r?

Fx61 is already in the RC stage. At this point, I'm not inclined to respin the RCs just for this issue, but I'd take it as a ride-along for the next respin or dot release. If you feel more strongly that this needs more urgent attention, please say so.

Comment 15

[Tom S [:evilpie]]

No, I think this is fine. FTP is probably not a super important use case.

Comment 16

[Cosmin Sabou [:CosminS]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8986511 [details] [diff] [review]
Allow the system principal to load FTP subresources. r?

We're going to spin another RC build. Taking this as a ride-along for it.

Comment 18

[Gyula Palko[:gpalko]]

Verified as fixed on Nightly 62.0a1(20180621013659). FTP files are successfully downloaded, no error is displayed in browser console.

Comment 19

[Xu Zhen]

If there is a ftp://xxx link on a http page, right click on that link and choose "save link as" still be blocked.
You can try this ftp://ftp.mirror.nl/pub/Museum/hyperwrt/icons/back.gif

Comment 20

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/5afdaceeb96a

Comment 21

[Tom S [:evilpie]]

(In reply to Xu Zhen from comment #19)
> If there is a ftp://xxx link on a http page, right click on that link and
> choose "save link as" still be blocked.
> You can try this ftp://ftp.mirror.nl/pub/Museum/hyperwrt/icons/back.gif

Thanks for the report. I am not sure this is important enough to fix. I will at least check if this has some simple nsContentPolicyType that we could match.

Comment 22

[Tom S [:evilpie]]

The type is TYPE_SAVEAS_DOWNLOAD, so this would actually be relatively easy to fix.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tom Schuster [:evilpie] from comment #22)
> The type is TYPE_SAVEAS_DOWNLOAD, so this would actually be relatively easy
> to fix.

Yeah, I guess we already discussed that option. We should do, if type == TYPE_DOC or type == TYPE_SAVEAS_DOWNLOAD we return from our function and let it load.

Tom could you provide that patch and then potentially uplift both fixes at once?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please do any follow-up work in a new bug. This bug has already had a patch land and be uplifted.

Comment 25

[Gyula Palko[:gpalko]]

Verified as fixed on Beta 61.0(20180621125625). FTP files are successfully downloaded, no error is displayed in browser console. Environments covered: Win10 x64, Ubuntu 16.04, MacOS 10.12