Open Bug 158623 Opened 22 years ago Updated 2 years ago

Mozilla Using ShellExecute on Downloaded Files a Security Hazard

Categories

(Firefox :: File Handling, defect)

Product:
Firefox
Component:
File Handling
Platform:
x86
Windows XP
Type:
defect

Tracking

()

People

(Reporter: mozilla, Unassigned)

References

(Blocks 1 open bug)

Details

bug 116938 outlines a security hazard where Mozilla will ShellExecute a
downloaded file on Windows if the content-type of the file is such that Mozilla
believes that there is an application to open the file. The use of ShellExecute
presents a security risk (as outlined in bug 116938). Unless Mozilla changes its
file handling behavior, the only work around is to try to guess what extension
should be given to a particular content-type on the user's system and append
that extension to the filename when it is saved. This leads to a host of
problems described in bug 120327.

The RIGHT way to fix this is to never use ShellExecute on a file downloaded from
the network under any circumstances. Instead, the file should be downloaded, the
content-type looked up for an appropriate handler (if any), and then have the
file's location passed to the proper application as an argument (i.e.,
application.exe %1).
I agree that this is a security issue just waiting to bite us in various ways.  
If we can avoid the problem by not using ShellExecute() that would be great.  
Things that need to be addressed:

1)  Does the registry entry for an extension have the path to the app handling
    it?
2)  How does one get this path?
3)  Are there any reasons whatsoever to use ShellExecute() if #1 and #2 have
    reasonable answers?

Scott, you wrote this originally... any thoughts?
Severity: normal → major
Answers:

1) Yes and no. It's more complicated than that, but there's no need to go into
that. I will research a reliable Win32 API to use to find out what should be
used to open a particular file.

2) It will be from the Registry. The API from answer #1 should take care of it.
huh? the chief security risk is that a user saves a file "audio/wave" whose
extension was .exe and then double clicks on it using explorer. since explorer
hides extensions and the exe has a .wav icon the user then proceeds to run the
dangerous program.

it may be the case that shellexecute is problematic. but not correctly
persisting http content types is afaik in violation of http, and it's a
disservice to the user.

note the keyword correctly, we know that what we're currently doing is wrong.
but that doesn't mean that doing nothing is correct.
Timeless, this bug is about the "launch with helper" case.  The only way a user 
is going to get to a file in this case is if they go browsing in their %TEMP%.  
If they do that, they better know what they're doing (and _why_ would the icon 
be 'wav' if the (hidden) extension is 'exe').  "save as" issues should be taken 
to a completely separate bug, please.  Thank you.
A file named file.wav.exe will always have an exe icon.

No one is saying that not persisting HTTP content-types should be done. There is
no specification that says that a file with content-type X must be named in any
particular manner. Mozilla should just save the file with the name it has.

On Windows, the extension only matter with ShellExecute. If I name a picture
picture.exe and double click it, it will try to execute (but fail). If I open up
a graphics program and tell it to open picture.exe, it doesn't execute the file,
it just reads the data and displays the image.
Ok. What you want to do is use FindExecutableA in shell32.dll. This should
return the path to the executable for a given file extension. Why this is not
documented anywhere on Microsoft's site is beyond me. It appears to take the
following form

FindExecutableA(file, directory, lpresult)

So far, I can't tell you any more since there is a total lack of documentation,
but it's a starting point.
Severity: major → normal
If you are uneasy about using an undocumented (by Microsoft) function, you can
also do the following.

1. Lookup the file extension associated with the content-type in 
   "HKEY_CLASSES_ROOT\MIME\Database\Content Type".

   This is, of course, dependent on whether this database is present in all 
   versions of Windows 95 and up. I think the Mozilla code uses this already, so 
   it is probably ok.

2. Take the extension and look it up in the registry under 
   HKEY_CLASSES_ROOT\[.ext]. If present, this will give you a "name" for the file
   type (e.g., "ACDSee.GIF" in the case of a .gif on my system). If there is no 
   value present on this key, then there is no application configured to open 
   that file extension AFAIK. If this is the case, Mozilla should handle the file 
   as it would any ambiguous content-type.

3. You then look up HKEY_CLASSES_ROOT\ACDSee.GIF\Shell\open\command to see what 
   application should be used to open the file, and how it should be called.

   There is some ambiguity here because some applications would like to use DDE 
   to open files. In that case, there will be another key called
   HKEY_CLASSES_ROOT\ACDSee.GIF\Shell\Open\DDEExec (and subkeys) with the 
   information for using DDE to open the file.

I am assuming that whoever might fix this bug understands the Win32 registry
API's for querying keys. If not, I can provide that information.
2 ...  If there is no 
   value present on this key, then there is no application configured to open 
   that file extension AFAIK.
afaik this is wrong.

you need to check .ext\shell ...

3. You then look up HKEY_CLASSES_ROOT\ACDSee.GIF\Shell\open\command to see what 
   application should be used to open the file, and how it should be called.
no. you need to check ACDSee.Gif\Shell\<default> to find out the default command.
if there is no default command, then you fallback to open
not that on 2k+ iirc if open doesn't exist, windows fallsback to the
alphabetically first key in shell\

dde is of course the most fun part of the process. especially given that mozilla
can't even handle inbound dde correctly.

and when we're done reinventing shellexecute, would someone please help me
understand what we've gained? -- note that failure to abide by most of the rules
outlined above is an unacceptable bug (however it's probably ok to do pieces now
and pieces later).

also i think there are some other fun things like clsid handlers.

i'm sorry but i still don't understand why shellexecute is evil. i just spent a
long time reading bug 116938.  Also, can someone please explain how reinventing
shellexecute solves anything?

points i made to bz on irc:
.js
.vbs
.pl/.pls
.bat/.cmd
.pif/.lnk/.scf
.cpl
none of these are really raw executed, but they all can be dangerous.  can
someone please describe program flow showing how we'd decide not to execute the
above?
bz of course contributed .com, which i assert is equivalent to .exe and would
probably not be an issue if .exe is handled 'correctly'.
My WindowsXP has no keys or values that resemble .ext\shell. I can't recall ever
seeing that.

As far as other executable code, or scripts, there isn't much you can do about
that. Here we have a disagreement about what Mozilla's job is. I do not believe
that a browser's job is to think of every possible way that a user might screw
themselves and then try to stop it.

Out of curiosity, I wonder how many that want to make Mozilla behave by renaming
files on Windows, actually use Windows as their primary OS (the whole having to
reap what you sow thing).
I forgot to add that every single extension you listed does not have a
corresponding mime type on my Windows system (and I have applications that
handle all of them, including Perl 5), except for .js. All of these extensions
would fail at step 1 of my sequence, and thus, present no threat.
QA Contact: sairuh → bsharma
Bill Law (the guy who implemented the patch in bug 116938) uses it as his
primary OS.  So does the super-reviewer in that bug.  So does the guy who put in
the extension-changing stuff in the "save as" menuitems and "save link as", as
well as all the reviewers of that patch.

In fact, I think I'm the only person to touch this code in a very long time who
does _not_ use Windows as his primary OS.
Is there some reason that this isn't being worked on? AFAIK, it's a simple
matter of changing 

ShellExecute();

to

FindExecutableA();
ShellExecute(Application.exe %1);
It's not... The default viewing method may involve DDE.  It may be something
other than "open".  Etc, etc.  In short, simply finding "the app" and running it
is the wrong thing to do...
Ok. Let me ask you what NN4 did. NN4 had its own database of applications
associated with MIME types. Where did it get these values and how? NN4 worked
perfectly fine in this regard. Why can't Mozilla emulate this behavior?
n4 had two databases, one was the windows database, the other was a legacy
database which n1 (?) invented.

n4 didn't work perfectly, people managed to use it to assign helper apps to
.exe, which um... caused ... problems.  most changes made using n4 went into the
windows registry.

regmon from sysinternals.com will let you watch n4 (or any windows app that uses
the registry) so you can see what it does. the n4 behaviors were documented on
netscape's help site however i haven't had much luck using help.netscape.com in
the past few years.
As long as you handle application/octet-stream, the NN4 model worked fine. I
*never* had an issue with from 1997 - 2002 except for two unrelated things: 1)
sometimes the Save As named octet-stream file with an HTML extension, and 2) it
didn't always put quotes around paths with long file names so that the
associated programs wouldn't launch correctly. In terms of handling downloads it
worked just fine.

Let me ask this; were there any reported cases of security breaches due to NN4's
file handling? If not, why are we going to piss of thousands of Windows users to
avoid an issue that doesn't exist?
I posted a comment to bug #175436, and got a response that it is a dupe of Bug
#120327, and has a status FIXED. But the behavior I am experiencing using
Windows 98 SE and build 2002082611 for 1.1 Final does not match what is being
discussed here. My complaint is not just that the file name is being changed by
the Download Manager. That is part of my complaint, but the main complaint is
that the download fails with the message "Unable to open", and I can't get the
file at all, with any extension. At least if the extension is one I don't want,
I could rename the file, but if I can't download the file by any name there is a
problem.

On my system, files with extension .txt are associated with the UltraEdit
editor, which can open binaries for editing, but Mozilla is not invoking
UltraEdit to open an exe.txt file.

Also part of my complaint is that I am not given the choice of directory into
which to save the file. For security reasons, I don't download files to the C:
drive, but to a removable medium, and because it saves me having to archive what
is usually a temporary file, most often some software executable file that I
want to install.

Note that I have installed Mozilla on my E: drive, not on my C: drive, which I
reserve for things that absolutely cannot be installed anywhere else. I wonder
if that might be the cause of some other malfs, such as not being able to load
any Java applets, even though the plugin manager shows JDE 1.4.1 installed okay.
(Which I have posted in a comment to Bug #92782.)

Mozilla should work like Netscape 4.7: If one left-clicks on a link, if the MIME
type allows it to be opened in the browzer, then do so. If not, offer the option
of saving it to disk. If right-click, offer option of download to disk. If
choose download to disk, do so with no name change and DO NOT OPEN.

Changing the file extension to protect the user from inadvertant execution is
not a solution. If the user doesn't recognize the danger of downloading an
executable, then how is he going to know enough not to just rename the file
after it is saved? A warning is appropriate, but not a gatekeeper function.

One possible approach:

1)  When getting the mimeinfo, see what the handler is set to.
2)  If the handler is a program, just launch it ourselves
3)  If the handler is something else, use ::ShellExecute; make sure to munge the
    filename beforehand (as we do now) and to force-save if the file is a .exe or
    something after the filename has been munged (may want to unmunge when
    force-saving)

Thoughts?
Blocks: 249757
-> default owner and qa
Assignee: law → file-handling
QA Contact: bsharma → ian
Assignee: file-handling → nobody
QA Contact: ian → file-handling
Is this still an issue?  I see we're still using ShellExecute() (and variations) in a number of instances.
(In reply to Jerry Baker from comment #11)
> I forgot to add that every single extension you listed does not have a
> corresponding mime type on my Windows system (and I have applications that
> handle all of them, including Perl 5), except for .js. All of these
> extensions
> would fail at step 1 of my sequence, and thus, present no threat.

Then what's wrong with calling ShellExecute after your step 1 instead of reinventing ShellExecute? You disproved your threat yourself.
We are not using ShellExecute anymore to open downloaded files. We are using ShellExecuteEx instead. ShellExecuteEx has an option to open files with any class string ("ACDSee.GIF" in the comment 8 example).
(In reply to Masatoshi Kimura [:emk] from comment #22)
> Then what's wrong with calling ShellExecute after your step 1 instead of
> reinventing ShellExecute? You disproved your threat yourself.

The threat is that your proposal allows a malicious server to serve executable files to a Mozilla browser that will be silently executed under certain configurations.

> We are not using ShellExecute anymore to open downloaded files. We are using
> ShellExecuteEx instead. ShellExecuteEx has an option to open files with any
> class string ("ACDSee.GIF" in the comment 8 example).

ShellExecuteEx() is functionally equivalent to ShellExecute() in this case. It's roughly the *nix equivalent of setting the execute bit on a downloaded file and then running ./[filename] without any user interaction.
Product: Core → Firefox
Version: Trunk → unspecified
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.