233169 - It's impossible to set helper applications for executables

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Manuel Reimer]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.6) Gecko/20040113

Mozilla doesn't launch helper applications if they are defined for executable
files in the latest builds (it was possible in older versions without problems).
I use an helper application to prevent an not very experienced user from
executing (and saving!) applications by setting an helper application for
executable files that simply displays a dialog "you are not allowed to do this"
adds the file to a logfile and kills it. Then I've setted "editable" to "false"
for this "dangerous" mime-types in mimeTypes.rdf. This successfully protected
this user from executing any malware so far. No this "save" dialog reduces the
security in this scenario since this user is now able to save the file to
somewhere and open it.

Another useful application area for helper applications for executable files
would be an script that automatically passes the file to an virus scanner and
then decides if this file gets saved to the download-directory or if the file
gets deleted.

I've found out that this is caused by an patch for fixing the following bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=68279

I completely agree that it's needed to block the automatich Execution of
executables from mozilla (disabling "Use default action for this type of file")
but I don't agree that it's needed to make it impossible to set helper
applications for them. If you don't want to make it possible to set helper
applications for executables per default then I think there should be at least
an hidden setting to make it possible to enable this via "user.js".

Reproducible: Always
Steps to Reproduce:
1. Set an new helper application for "application/octet-stream" (for example
open with "notepad.exe")
2. Add an .exe-file of your choice to an compose-mail window and choose "Send
Later" from the file menu
2. Go to your "unsend messages", open the unsend mail and doubleclick the
attachment.
Actual Results:  
I get the "save file" dialog even if I've setted an helper application for this
mime-type.

Expected Results:  
Mozilla should have executed the helper application.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The problem is that we need to differentiate between the "we have a helper app"
case and "the OS has a helper app" case....  This needs to be done _very_
carefully.  In the "the OS has a helper app" case, ShellExecute() is used to
launch the file, and for an executable that would be disastrous....

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

application/octet-stream is totally not specific to .exe files. in fact it only
means "this is some binary data".

Are you even sure that your exe file uses application/octet-stream as its type?
What type does the helper app dialog show?

Comment 3

[Boris Zbarsky [:bzbarsky]]

There is no helper app dialog if isExecutable() is true, biesi -- we just put up
a filepicker.

Comment 4

[Manuel Reimer]

(In reply to comment #2)
> application/octet-stream is totally not specific to .exe files. in fact it only
> means "this is some binary data".

Yes that's true. I've created a little program which "filters" the
application/octet-stream by file extension. So the file extension "pps" gets
automatically opened with "Powerpoint Viewer", "pdf" gets forwarded to "Acrobat
Reader" and all the dangerous extensions cause an "You are not allowed to do
this". See my homepage (german) for details:

http://www.mreimer.de.vu/mozilla/tut-attachfilter.php

This forces this not experienced user ("Silliest Possible User") to not open
dangerous files.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

so something like this

bz was right, of course :)

win/nsOSHelperAppService.cpp makes sure that we don't ShellExecute .exe files,
so this should be safe.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 140678 [details] [diff] [review]
patch

I can attach a -w version of the patch, if you prefer (the code inside the new
if is just reindented, unchanged)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140678 [details] [diff] [review]
patch

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>+    if (action != nsIMIMEInfo::useHelperApp) {

I would be happier if this also checked the actual helper app set on the MIME
info.  Or if the LaunchAppWithTempFile impls _didn't_ check it.  Compare the
win and beos versions (which check) with the others (which don't).  As things
stand, a malformed nsIMIMEInfo can lead to a security hole with this patch.

r=bzbarsky with that change.

> win/nsOSHelperAppService.cpp makes sure that we don't ShellExecute .exe files,

Well, it's better if the user never sees an error message that sounds like a
.exe file was almost executed... perhaps we need to reword this error message?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

only skip the isExecutable check if action is useHelperApp and
preferedApplication is nonnull.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 140753 [details] [diff] [review]
patch v2

>Compare the
win and beos versions (which check) with the others (which don't).  As things
>stand, a malformed nsIMIMEInfo can lead to a security hole with this patch.

remember that this is all ifdef XP_WIN code, so how non-win helperappservices
behave isn't really relevant.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140753 [details] [diff] [review]
patch v2

r=bzbarsky, and I don't trust code to stay ifdefed... I also don't trust people
not to blindly copy/paste.  ;)

Comment 11

[Darin Fisher]

Comment on attachment 140753 [details] [diff] [review]
patch v2

>+        PRBool isExecutable;
>+        rv = fileToTest->IsExecutable(&isExecutable);
>+        if ((NS_SUCCEEDED(rv) && isExecutable) ||
>+            NS_FAILED(rv)) {  // Paranoia is good

nit: no need to check |rv| twice.  do this instead:

	  if (NS_FAILED(rv) || isExecutable)


sr=darin

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed