Mark all executables as coming from the Internet zone on Windows

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

({sec-want})

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In the Downloads API, we should mark executables saved to an NTFS file system as
coming from the Internet zone, regardless of the currently applied Security Zone
Policy for the download source.

More information in the firefox-dev thread:
https://mail.mozilla.org/pipermail/firefox-dev/2013-August/000848.html
Posted patch Proof of concept (obsolete) — Splinter Review
This is implemented on top of bug 853901, and is still missing the platform
conditionals, the executable check and the hidden preference to disable.

I tested this before editing the code a little, and it worked as expected.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Posted patch The patchSplinter Review
Attachment #795059 - Attachment is obsolete: true
Attachment #798603 - Flags: review?(enndeakin)
Comment on attachment 798603 [details] [diff] [review]
The patch

>       gDownloadPlatform.downloadDone(NetUtil.newURI(aDownload.source.url),
>                                      new FileUtils.File(aDownload.target.path),
>-                                     aDownload.contentType, aDownload.source.isPrivate);
>+                                     aDownload.contentType,
>+                                     aDownload.source.isPrivate);
>       this.downloadDoneCalled = true;
>-      return Promise.resolve();
>-    } catch(ex) {
>-      return Promise.reject(ex);
>-    }
>+    }.bind(this));
>   },


Do we need to report errors here if downloadDone fails?

>+#ifndef XP_WIN
>+      // Ask for confirmation if the file is executable, except on Windows where
>+      // the operating system will show the prompt based on the security zone.
>+      // We do this here, instead of letting the caller handle the prompt
>+      // separately in the user interface layer, for two reasons.  The first is
>+      // because of its security nature, so that add-ons cannot forget to do
>+      // this check.  The second is that the system-level security prompt would
>+      // be displayed at launch time in any case.
>       if (file.isExecutable() && !this.dontOpenFileAndFolder) {

What happens if the saveZoneInformation preferences is false? Neither this block of code will run, nor the downloadDone code.
(In reply to Neil Deakin from comment #3)
> Do we need to report errors here if downloadDone fails?

No, those result in the rejection of the function and subsequent propagation to the caller of the "start" method.

> What happens if the saveZoneInformation preferences is false? Neither this
> block of code will run, nor the downloadDone code.

Yes, as noted in the about:config preference comment, disabling it means that we get no executable prompts at all on Windows. I think this is in line with the expectations of users toggling that preference.
Attachment #798603 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/7d73a40c6ee9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Backed out in https://hg.mozilla.org/integration/fx-team/rev/ae42fd34f893 for causing lots of XPCShell failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=27601606&tree=Fx-Team
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: sec-want
Re-landed separately from bug 908240, as not related to the test failure:

https://hg.mozilla.org/integration/fx-team/rev/94e489c3dc98
https://hg.mozilla.org/mozilla-central/rev/94e489c3dc98
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 916126
Blocks: 916126
No longer depends on: 916126
Hi,

I am sorry to tell but your assumption that you have to mark a file downloaded from the internet so that it get scanned is wrong.

Yes, there is a ScanWithAntiVirus option. Per default, this option is disabled. Currently, only Microsoft's own scanner will turn this option on (programs like Symantec Anti-Virus are causing problem with Mozilla Firefox, when this option is enabled, please try it, you won't be able to download *any* file, because your downloaded file will be deleted immediately when download the download has finished on a system with Norton Antivirus 2013/2014 when ScanWithAntiVirus is set).

But, the security feature your are talking about is not relying on the zone information. The zone information are just another protection layer for explorer, to warn the user when she wants to execute a downloaded file. Nothing more.

See http://support.microsoft.com/kb/883260/en

So this "feature" is causing a regression on bug 760889. When the administrator has set

[HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Policies\Attachments]
"SaveZoneInformation"=dword:00000001

because he don't want zone information, you are now ignoring his wishes. 

Thanks :)
Hi Igor, as you've correctly noticed, we don't use the system-level configuration to determine whether to set zone information anymore. The reason is actually related to performance, code complexity, and the APIs provided by Windows, rather than the virus scanning itself (even if there is a correlation between them). Since the issue is complex, if you're curious feel free to check out the mailing list thread referenced in comment 0 for more details.

The system-level configuration that Windows uses includes the registry value you mentioned, as well as several other options. We think that we cannot easily interpret all the possible configuration changes made by system administrators in the registry to determine if we should set zone information or not.

Thus, we have implemented a simpler "about:config" preference to toggle this behavior:

"browser.download.saveZoneInformation"

If set to false, zone information is not saved anymore, covering this use case we know may exist.
Well, I am not sure if it is the right way to move away from system-level configuration. In a corporate environment it becomes hard to manage Firefox installations (maybe there are new tools I am not aware of). And Google Chrome adds more and more features like that. But this is another discussion.

Anyway, thanks for the explanation. That was more than I expected. As a desktop user I can live with the the new option.
This bug doesn't show up in the extended buglist linked in 26's release-notes.
Blocks: 952961
You need to log in before you can comment on or make changes to this bug.