Allow manual download of FTP files

VERIFIED FIXED in Firefox 61

Status

()

defect
P3
normal
VERIFIED FIXED
10 months ago
10 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

({regression})

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61+ verified, firefox62+ verified)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 months ago
See bug 1404744 comment 39. We should probably uplift this to beta.
(Assignee)

Comment 1

10 months ago
(Assignee)

Updated

10 months ago
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
(Assignee)

Comment 2

10 months ago
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.
Attachment #8986147 - Flags: review?(ckerschb)
(Assignee)

Updated

10 months ago
(Assignee)

Updated

10 months ago
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?
Attachment #8986147 - Flags: review?(ckerschb)
(Assignee)

Comment 4

10 months ago
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.
Just a note that we're building RCs today.
(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!
Priority: -- → P3
Whiteboard: [domsecurity-active]
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 7 should than easily be uplift-able and we can re-evaluate what we do precisely about those cases in the next cycle.
(Assignee)

Updated

10 months ago
Attachment #8986147 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8986511 - Flags: review?(ckerschb)
(Assignee)

Comment 10

10 months ago
I verified that this also fixes Bug 1469891.
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?
Attachment #8986511 - Flags: review?(ckerschb) → review+

Comment 12

10 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74ec48ebf55
Allow the system principal to load FTP subresources. r=ckerschb
(Assignee)

Comment 13

10 months ago
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]: -
Attachment #8986511 - Flags: approval-mozilla-beta?
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.
Attachment #8986511 - Flags: approval-mozilla-beta? → approval-mozilla-release?
(Assignee)

Comment 15

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

Comment 16

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f74ec48ebf55
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: qe-verify+
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.
Attachment #8986511 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment 18

10 months ago
Verified as fixed on Nightly 62.0a1(20180621013659). FTP files are successfully downloaded, no error is displayed in browser console.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Comment 19

10 months ago
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
(Assignee)

Comment 21

10 months ago
(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.
(Assignee)

Comment 22

10 months ago
The type is TYPE_SAVEAS_DOWNLOAD, so this would actually be relatively easy to fix.
(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?
Please do any follow-up work in a new bug. This bug has already had a patch land and be uplifted.

Comment 25

10 months ago
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
You need to log in before you can comment on or make changes to this bug.