Use a pref to determine whether we auto-launch downloaded files

VERIFIED FIXED in Firefox 25

Status

()

Firefox for Android
General
VERIFIED FIXED
7 years ago
5 months ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

({sec-other})

Trunk
Firefox 26
sec-other
Points:
---
Dependency tree / graph
Bug Flags:
sec-review ?

Firefox Tracking Flags

(firefox23 wontfix, firefox24 wontfix, firefox25+ verified, firefox26+ verified, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 unaffected, fennec25+)

Details

(Whiteboard: [sg:nse mitigation][adv-main25-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Right now if Fennec finds a helper app for a downloadable file, it just automatically uses it. There's a little concern that could lead to a security problem if hackers find a vulnerability in, say a PDF reader. We should at the very least have a hidden pref to disable the feature.
(Assignee)

Updated

7 years ago
Blocks: 684449

Updated

7 years ago
Blocks: 573982
(Assignee)

Comment 1

7 years ago
Created attachment 563643 [details] [diff] [review]
Patch v1

This adds a pref defaulted to true in Fennec. If the pref is false, we will always download files. If its true we try to autoopen. Files we can't auto-open will get the "Tap to save" prompt.

I'm not really sure how to name this. The externalAppHandlerService has a pref "browser.helperApps.neverAsk.openFile" that I originally wanted to use, but its apparently a string or something? I didn't want to step on their toes to much, so I made my own "browser.helperAppsDialog.neverAsk.openFile". I can dig into theirs more if we want.
Assignee: nobody → wjohnston
Attachment #563643 - Flags: review?(mark.finkle)
It'd be nice to share prefs with desktop if possible, so addons and documentation can cover them both easily. Desktop, however, has a more complex set of different actions per type and that may be way too much for Fennec to handle.
Keywords: sec-review-needed
Whiteboard: [sg:nse mitigation]
sec review triage = needs full review
Whiteboard: [sg:nse mitigation] → [sg:nse mitigation][secr:curtisk]
Blocks: 711122
Whiteboard: [sg:nse mitigation][secr:curtisk] → [sg:nse mitigation][sec-assigned:curtisk:749368]
(Assignee)

Comment 4

6 years ago
mfinkle, this would still be nice to have and this code hasn't really changed in Native Fennec.
Flags: sec-review?(curtisk)
Attachment #563643 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Component: General → General
Product: Fennec → Firefox for Android
Target Milestone: --- → Future
(In reply to Wesley Johnston (:wesj) from comment #4)
> mfinkle, this would still be nice to have and this code hasn't really
> changed in Native Fennec.

We should touch base with UX. Adding Ian and Madhava.
Flags: needinfo?(ibarlow)
Whiteboard: [sg:nse mitigation][sec-assigned:curtisk:749368] → [sg:nse mitigation]
(Assignee)

Comment 6

6 years ago
Created attachment 690925 [details] [diff] [review]
Patch v2

Unbitrotted. Remove our old "Tap to download code" that's no longer used. Flipped the pref in webapps.
Attachment #563643 - Attachment is obsolete: true
Attachment #690925 - Flags: review?(mark.finkle)
Comment on attachment 690925 [details] [diff] [review]
Patch v2

Wes - How is "browser.helperApps.neverAsk.openFile" related to this action?
(Assignee)

Comment 8

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Wes - How is "browser.helperApps.neverAsk.openFile" related to this action?

I'm not sure what you're asking. The pref controls whether we open the file or not. Its actually hijacked from nsExternalHelperApps, but now that I look at it, that uses a string to handle different mime types. We can rename it.

Is that what you're asking?
(In reply to Wesley Johnston (:wesj) from comment #8)
> (In reply to Mark Finkle (:mfinkle) from comment #7)
> > Wes - How is "browser.helperApps.neverAsk.openFile" related to this action?
> 
> I'm not sure what you're asking. The pref controls whether we open the file
> or not. Its actually hijacked from nsExternalHelperApps, but now that I look
> at it, that uses a string to handle different mime types. We can rename it.
> 
> Is that what you're asking?

Kinda. Your patch uses "browser.helperAppsDialog.neverAsk.openFile" and I wondered if we could just use "browser.helperApps.neverAsk.openFile" unless it conflicted with some other action.
(Assignee)

Comment 10

6 years ago
Ahh. Its been so long since I wrote this I forgot I added that. 

"browser.helperApps.neverAsk.openFile" is actually a string pref containing a list of mimetypes to not ask about. We could use here instead if we want to single out one particular mimetype (application/vnd.android.package-archive for example) to block. The idea here was that we were worried about autolaunching basically anything, since it can be used to exploit vulnerabilities in other apps/malware.
Duplicate of this bug: 872480
I talked to Wes about a proposal:
1. never prompt for download
2. never auto-open
3. user can open via system notification ("download complete") or from download manager

Wes, Brad and I are fine with this proposal. We can look at whitelisting some filetypes in the future.

Wes is reviewing the code and preferences to see what needs to change.
Duplicate of this bug: 809445
tracking-fennec: --- → 25+
(Assignee)

Comment 14

5 years ago
Created attachment 804055 [details] [diff] [review]
Patch

There are two system prefs here that matter for what we're doing:

browser.helperApps.neverAsk.saveToDisk
browser.helperApps.neverAsk.openFiles

Both are a list of mimetypes that would not prompt for save/open. There is no global pref. This patch will flip everything to save by default, and we can use the openFiles pref if we want to special case some types. I put pdf in here as an example, but we don't have to check that in.

It also removes some code that likely doesn't work in Fennec.

Longer term we could offer a one time doorhanger or dialog for downloads?
Attachment #690925 - Attachment is obsolete: true
Attachment #690925 - Flags: review?(mark.finkle)
Attachment #804055 - Flags: review?(mark.finkle)
Comment on attachment 804055 [details] [diff] [review]
Patch

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+pref("browser.helperApps.neverAsk.openFile", "application/pdf");

Good to know we could do this, but let's remove for now

>diff --git a/mobile/android/components/HelperAppDialog.js b/mobile/android/components/HelperAppDialog.js

>   show: function hald_show(aLauncher, aContext, aReason) {

>+    aLauncher.MIMEInfo.preferredAction = Ci.nsIMIMEInfo.useSystemDefault;
>+    aLauncher.saveToDisk(null, false);

Add a comment here that basically says we are always saving to a file
Attachment #804055 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 17

5 years ago
Comment on attachment 804055 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): XUL Fennec
User impact if declined: Potential security issue.
Testing completed (on m-c, etc.):  Landed on mc today. Tested locally.
Risk to taking this patch (and alternatives if risky): Low risk patch. Simplifies this code quite a bit. Most of it was likely already failing, but we don't hit it enough to see.
String or IDL/UUID changes made by this patch: None.
Attachment #804055 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/57249b332b69
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox26: --- → fixed
Resolution: --- → FIXED
Target Milestone: Future → Firefox 26

Updated

5 years ago
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox25: --- → +
tracking-firefox26: --- → +

Updated

5 years ago
Attachment #804055 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
relnote-firefox: --- → ?
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/f950cd9cd5d5
status-firefox25: affected → fixed
status-firefox23: affected → wontfix
status-firefox24: affected → wontfix

Updated

5 years ago
Keywords: verifyme
Mobile only, will need to be added to Beta 25 notes.
Flags: needinfo?(akeybl)
Verified fixed Firefox 26 Nightly / 25 Aurora.
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
status-firefox26: fixed → verified

Updated

5 years ago
Keywords: verifyme
Duplicate of this bug: 916583
I'm actually not sure whether or not we want/need to note this and bring it up again.
Flags: needinfo?(akeybl)
status-firefox-esr17: --- → unaffected
status-firefox-esr24: --- → unaffected
Whiteboard: [sg:nse mitigation] → [sg:nse mitigation][adv-main25-]
status-b2g18: --- → unaffected
relnote-firefox: ? → ---
this shipped without completing the blocking bug for sec-review (just found as we were cleaning out sec-review cruft)
Group: core-security
Flags: sec-review?(curtisk) → sec-review?
You need to log in before you can comment on or make changes to this bug.