Closed Bug 690252 Opened 9 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 + verified
firefox26 + verified
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected
fennec 25+ ---

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Keywords: sec-other, Whiteboard: [sg:nse mitigation][adv-main25-])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 573982
Attached patch Patch v1 (obsolete) — Splinter Review
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.
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]
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)
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]
Attached patch Patch v2 (obsolete) — Splinter Review
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?
(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.
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+
Attached patch PatchSplinter Review
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: Future → Firefox 26
Attachment #804055 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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)
Whiteboard: [sg:nse mitigation] → [sg:nse mitigation][adv-main25-]
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.