1605308 - Do not use ShellExecuteByExplorer when we launch .exe files from Downloads folder

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P3)

Description

[Masatoshi Kimura [:emk]]

When we execute an installed app (such as Skype for Business) via file type associations, compatibility would be more important than security. But when we launch a random executable file from Downloads folder, is it really more desirable to load DLLs from Downloads folder silently than to fail execution? The purpose of PreferSystem32Images mitigation is to prevent such kind of attack.

Comment 2

[:Gijs (he/him)]

Is this really a dupe? It sounds like a request to treat launching downloaded executables differently, which I'm fairly sure we still don't currently do...

Comment 3

[Molly Howell (she/her) (no longer active)]

What happened here was that I misread this bug and didn't realize it was specific to launching downloads.

In any case though, Shell Integration isn't the right component for it

Comment 4

[Rachel Tublitz [:rachel]]

(In reply to :Gijs (he/him) from comment #2)

Is this really a dupe? It sounds like a request to treat launching downloaded executables differently, which I'm fairly sure we still don't currently do...

whoops - thanks. We were doing some triage and it looks like this one got misclassified. Removing the ni? from myself since it looks like Molly jumped in here (thanks Molly :)) and addressed it.

Comment 5

[:Gijs (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #0)

The purpose of PreferSystem32Images mitigation is to prevent such kind of attack.

Can you elaborate on what this means? What kind of attack are we aiming to prevent and how does this setting/mitigation help? If we break e.g. setup/installer files downloaded this way, that would still be bad...

Comment 6

[Masatoshi Kimura [:emk]]

Attackers trick users to download a malicious DLL. Then, if users download a vulnerable installer and execute it from our download panel, the installer will load the malicious DLL. If this mitigation is applied, the installer will load a legit DLL from system32.

Comment 7

[:Gijs (he/him)]

This will need XPCOM work to expose a way of doing this to nsIFile(Win), and then downloads work to call that API instead of .launch() - but only on executable files. :toshi or :emk, are either of you interested in working on this?

Edit: alternatively, launch could do the right thing internally...

Comment 8

[Toshihito Kikuchi [:toshi]]

We've already detected a downloaded file is executable or not in nsMIMEInfoWin::LaunchDefaultWithFile, and if so, the first attempt throws an exception. The exception was caught here, and we start the second attempt without mime info, which goes to nsLocalFile::Launch, hitting ShellExecuteByExplorer.

The easiest fix would be to update LaunchDefaultWithFile and we can launch a file with ShellExecute or CreateProcess there.

Comment 9

[Toshihito Kikuchi [:toshi]]

Actually I have a couple of plans to deal with ShellExecuteByExplorer. I described it in bug 1620335 just now. Let me associate this bug to it.

Comment 10

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1605308 - Do not use ShellExecuteByExplorer when a downloaded file is an executable. r=Gijs

This is one of the efforts to reduce usage of ShellExecuteByExplorer
(bug 1620335).

The purpose of using ShellExecuteByExplorer in the scenario to open a
downloaded file is to support applications which are not compatible with
the mitigation policies of our process. When a downloaded file is an
executable, however, we prefer security to compatibility and in particular
we want to prevent binary planting on a user's download directory.

The proposed fix is to go to ShellExecuteExW straight if the target
file to launch is an executable.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01cef3030838
Do not use ShellExecuteByExplorer when a downloaded file is an executable.  r=Gijs,froydnj

Comment 12

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=292916049&resultStatus=busted%2Cexception&revision=01cef303083885956bf7ef64eeda2d4e07a09c85

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=292916049&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/5be114c5f7a8008a42b60e9a95bc65dc6441da97

[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - make[4]: Entering directory '/builds/worker/workspace/obj-build/xpcom/io'
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang-cl -Xclang -std=c++17 -m32 -FonsLocalFileWin.obj -c -I/builds/worker/workspace/obj-build/dist/stl_wrappers -guard:cf -DNDEBUG=1 -DTRIMMED=1 -DUNICODE -D_UNICODE -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -D_SECURE_ATL -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DOS_WIN=1 -DWIN32 -D_WIN32 -D_WINDOWS -DWIN32_LEAN_AND_MEAN -DCOMPILER_MSVC -DWINAPI_NO_BUNDLED_LIBRARIES -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/xpcom/io -I/builds/worker/workspace/obj-build/xpcom/io -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/workspace/obj-build/xpcom -I/builds/worker/checkouts/gecko/xpcom/build -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -MD -FI /builds/worker/workspace/obj-build/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -Qunused-arguments -fcrash-diagnostics-dir=/builds/worker/artifacts -TP -Zc:sizedDealloc- -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -Gw -Wno-inline-new-delete -Wno-invalid-offsetof -Wno-microsoft-enum-value -Wno-microsoft-include -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -Wno-inconsistent-missing-override -Wno-implicit-exception-spec-mismatch -Wno-microsoft-exception-spec -Wno-unused-local-typedef -Wno-ignored-attributes -Wno-used-but-marked-unused -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -Oy- -Werror -Xclang -fexperimental-new-pass-manager -Xclang -MP -Xclang -dependency-file -Xclang .deps/nsLocalFileWin.obj.pp -Xclang -MT -Xclang nsLocalFileWin.obj /builds/worker/checkouts/gecko/xpcom/io/nsLocalFileWin.cpp
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - /builds/worker/checkouts/gecko/xpcom/io/nsLocalFileWin.cpp(2651,14): error: function declared 'stdcall' here was previously declared without calling convention
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - nsLocalFile::LookupExtensionIn(const char* const* aExtensionsArray,
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - ^
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - /builds/worker/checkouts/gecko/xpcom/io/nsLocalFileWin.h(87,12): note: previous declaration is here
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - nsresult LookupExtensionIn(const char* const* aExtensionsArray,
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - ^
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - 1 error generated.
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - /builds/worker/checkouts/gecko/config/rules.mk:745: recipe for target 'nsLocalFileWin.obj' failed
[task 2020-03-12T21:22:11.272Z] 21:22:11 ERROR - make[4]: *** [nsLocalFileWin.obj] Error 1
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - make[4]: Leaving directory '/builds/worker/workspace/obj-build/xpcom/io'
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - make[4]: *** Waiting for unfinished jobs....
[task 2020-03-12T21:22:11.272Z] 21:22:11 INFO - make[4]: Entering directory '/builds/worker/workspace/obj-build/xpcom/tests/gtest'

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7395dd853dc
Do not use ShellExecuteByExplorer when a downloaded file is an executable.  r=Gijs,froydnj

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/a7395dd853dc