909022 - Mark all executables as coming from the Internet zone on Windows

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

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

Comment 1

[:Paolo Amadini]

Attachment: Proof of concept

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.

Comment 2

[:Paolo Amadini]

Attachment: The patch

Comment 3

[Neil Deakin]

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.

Comment 4

[:Paolo Amadini]

(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.

Comment 5

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/7d73a40c6ee9

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d73a40c6ee9

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 8

[:Paolo Amadini]

Re-landed separately from bug 908240, as not related to the test failure:

https://hg.mozilla.org/integration/fx-team/rev/94e489c3dc98

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/94e489c3dc98

Comment 10

[Igor]

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 :)

Comment 11

[:Paolo Amadini]

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.

Comment 12

[Igor]

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.

Comment 13

[Elbart]

This bug doesn't show up in the extended buglist linked in 26's release-notes.