Closed Bug 1469536 Opened 2 years ago Closed 2 years ago

Allow manual download of FTP files

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 + verified
firefox62 + verified

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

See bug 1404744 comment 39. We should probably uplift this to beta.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
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)
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)
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.
Attachment #8986147 - Attachment is obsolete: true
Attachment #8986511 - Flags: review?(ckerschb)
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+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74ec48ebf55
Allow the system principal to load FTP subresources. r=ckerschb
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?
No, I think this is fine. FTP is probably not a super important use case.
https://hg.mozilla.org/mozilla-central/rev/f74ec48ebf55
Status: ASSIGNED → RESOLVED
Closed: 2 years 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+
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+
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
(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.
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.
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.