68279 - Better security warning about running (opening) downloaded code

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Peter Lubczynski]

I think the current warning for download code is not information the user 
enough information that they may be executing a harmfull application such as a 
virus [that has stuck my home computer and made others recently]. 

1) It would be at least nice to put up a second warning like IE does (in the 
screenshot below) and fill in the "Open" empty text box with some more 
information. The current "lauch" button doesn't do anything and the application 
luaching promptly after downloading.

2) It would be even better if after the fist few bytes of an executable 
application came through, we could read the deployment info (like Author, 
Title, version, etc... in Windows) and fill in that info on the download box. 
Something other browser don't do. 

3) The coolest feature, by far,would be to have an XPCOM componenet (although 
many virii arne't XP) which basically did a virus scan upon download and 
execute. It could be pluggable by other vendors but perhaps Mozilla could use 
an open source one. Netscape, however, could probably stike a deal with a huge 
vendor. An auto-update of definitions would also be nice, but I'm dreaming ;)

4) Finnaly, for those of us who are wild and simply trust anything that comes 
through, have check boxes for disabling the warning and/or virus scan. 

I think Internet Virii will become more common as apps move to the network. 
With built-in support, it's something we can taunt our competitors with.

Comment 1

[Peter Lubczynski]

Sorry for all the typos, that was the 5th of typing the same bug due to a 
crash :(.

Comment 2

[Tim Powell]

Reporter, were you planning to add a screenshot?

Comment 3

[Jesse Ruderman]

cc mstoltz

Comment 4

[Peter Lubczynski]

Screen shot comming comparing our warning vs. IE's on running downloaded code. 
Sorry, my computer at home is horked.

Comment 5

[Peter Lubczynski]

Attachment: screenshot comparing IE's security warning to our absent one

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Does Mozilla/NS actually run an executable after downloading it? I wasn't aware
that it would be automatically run. If not, then this isn't that much of a
problem, I think.

Comment 7

[Peter Lubczynski]

At least on Win32, if you choose the "Open" radio button and click OK, it will 
run the application right away, with no warnings. The only warning you get is 
that the MIME type is of type application/octet-stream in that previous 
dialog, what you see in the screenshot. IMO, I don't think many people would 
understand what that means. It doesn't even say WHAT application (filename) it 
will run.

Comment 8

[Peter Lubczynski]

Attachment: screenshot of 4.x's warning

Comment 9

[Phil Peterson]

> if you choose the "Open" radio button and click OK, it will run the
> application

I think this is the wrong thing. We should prevent users from running downloaded
code directly from the browser, at least by default, and maybe altogether. 4.x
prevents it altogether.

N.B. there are interesting platform variations in what "code" means. For
example, on Windows, .COM and .PIF files have been used for viruses, preying on
stupid apps which only look for .EXE. Let's not be a stupid app.

> screenshot of 4.x's warning

Right, but that's only for non-executable files. 4.x will not run executables
directly from the browser.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

I agree. We shouldn't be running downloaded executables automatically under any
circumstances. The user should have to go and double-click on the executable.
That way it's out of our hands.

Comment 11

[Tony Jordan]

changing severity to enhancement as per the summary

Comment 12

[Mitchell Stoltz (not reading bugmail)]

No, this is more than an enhancement. It's at least "major." We should not be
running downloaded executables automatically without so much as a warning.

Comment 13

[Peter Lubczynski]

-->Security

Comment 14

[Mitchell Stoltz (not reading bugmail)]

->0.9.2; I think we're generally doing the right thing but I will re-examine
when I have time.

Comment 15

[Phil Peterson]

> I think we're generally doing the right thing

Are you sure? With the new Navigator download stuff, the default action for EXE
files appears to be to run them without warning. At least that's how it seems
downloading n6setup.exe for daily builds. cc'ing [e-mail]. I think we
may need to look at this for 0.9.1

Comment 16

[Bill Law]

Adding mscott to cc: list.  Scott, we have two holes to plug (it looks like):

1. Detect when an nsIMIMEInfo from the OS will launch the file directly.
2. Don't do ShellExecute for executables from nsLocalFile::Launch in
nsLocalFileWin.cpp.

Comment 17

[Peter Lubczynski]

I like this feature of being able to "open" a file like for installing apps. I 
think it just needs a better warning for dangerous files like EXE, COM, BAT, 
PIF, VBS, JS, etc...

Comment 18

[Mitchell Stoltz (not reading bugmail)]

After my last post, dveditz showed me the new version of the dialog. I think
we're really asking for trouble. The average user has no idea what the MIME type
means, or that the "default action" (which is not explained further) is to
execute the downloaded file. We must not execute any downloaded code that can
potentially do dangerous things (as opposed to JavaScript, for example, which is
protected by our security manager). At least, we must present a dialog before
executing downloaded code that warns the user of the risk they're taking. This
dialog should not have a "don't warn me again" button.

This exactly the behavior that gets Microsoft in trouble. We really need to fix
this, or we're going to read about it in C-net. Is law the owner of this dialog?
Reassigning. Please fix this soon.

Comment 19

[Bill Law]

Strongly encourage us to do this ASAP.  I think we can detect this on Win32 via
SHGetFileInfo to determine whether the file will be executed directly as a
result of the ShellExecute.  I don't know about Mac or whether there's even a
problem on Unix.

Do we need to do any checking in nsILocalFile::Launch?  We can detect that case
in the progress dialog code when the "Launch" button is pressed but that won't
help other code that might call the nsILocalFile method someday.  Do we care
about that?

Comment 20

[selmer (gone)]

Shouldn't the priority of this bug also be set to something like P1?

Comment 21

[Viswanath Ramachandran]

nav+pdt triage: this is a must have. for m0.9.1, we need to make it so that 
users CANNOT automatically execute from the helper app dialog. They must be 
forced to download and save and then goto the OS to execute the file. There shd 
be no way for them to select to execute it. 

for rtm, we can think about allowing them to execute it, but there has to be a 
warning dialog that they are downloading code, the dangers etc. 

since that is a UI change, ccing german to think more about the spec etc. 

Comment 22

[Peter Lubczynski]

Attachment: screenshot of new, even more confusing dialog that doesn't warn

Comment 23

[Bill Law]

The downloaded file is executed from one of two points:

1. When the user chooses "Use default action" from the helper app dialog (as 
illustrated in the screen shot Peter recently attached to this bug).

2. When the user presses "Launch..." on the progress dialog (after download is 
complete).

Those are essentially independent code paths, although the underlying code 
overlaps considerably.

First, we need a means of detecting that the file will be executed directly.  
The way to do that, I think, is via nsIFile::IsExecutable.  That function, as 
currently implemented, does pretty much what we want.  The Win32 implementation 
is deficient, however, in that it simply checks the file extension for ".exe" 
or ".bat".  I think that code should be rewritten to use SHGetFileInfo, which 
seems to be based on the same logic that ShellExecute will use (we use 
ShellExecute to launch the file).

In general, this bug would be fixed by calling nsIFile::IsExecutable prior to 
invoking the code that launches the file, and, if executable, displaying an 
appropriate alert.

In case 1, that's not too hard.  The progress dialog can readily do the check 
and show the dialog.

Case 2. is a little more complicated.  The logic that is used to determine what 
application will be launched is embedded in the guts of 
nsOSExternalAppService::LaunchAppWithTempFile.  That method is implemented 
differently for each platform and is not called until the download completes 
(which may be long after the dialog has been dismissed).

I think we really need to put the code that shows the alert in the dialog, 
rather than the backend uriloader/exthandler code.  If for no other reason, to 
enable embedders to replace that dialog with their own.

There are two hurdles to detecting that a downloaded file will be executed 
directly from the dialog.  The first is that the logic is different for each 
platform.  The second is that it may be difficult to determine whether a file 
is directly executable prior to it being completely resident on disk.  The 
helper app dialog is dismissed while the file is in the process of being 
downloaded; in the case of a large file (or slow connection or server), the 
file may not be on disk till much later.

The first hurdle may be overcome by relying on the current logic of the various 
implementations of LaunchAppWithTempFile.  Unix never will launch the file 
directly, Mac doesn't appear to ever do that (although I'm not 100% sure of 
that).  Win32 and OS/2 only do it if the "preferredAction" (as defined by the 
nsIMIMEInfo object) is "useSystemDefault" *or* 
the "preferredApplicationHandler" is null (*and*, if the file is executable, of 
course).

So I think this logic will work XP (applied only when "use default action" is
selected):
  if ( mLauncher.MIMEInfo.preferredAction == useSystemDefault ||
       ( mLauncher.MIMEInfo.preferredAction == useHelperApp &&
         mLauncher.MIMEInfo.prefferedApplicationHandler == null ) ) {
    // Check if file is executable.
    if ( tempFile.IsExecutable() ) {
      // Show alert.
      if ( ok ) {
        // Go ahead (and close dialog).
      } else {
        // Leave dialog up (so they can choose "save to disk" or
        // pick a different application.
      }
    }
  }

As for the second hurdle, I'm going to try ignoring the problem for the moment 
and hope that SHGetFileInfo works on the temporary file currently being 
downloaded.  If that fails, then I'll look into moving the logic into the 
progress dialog that will be displayed.

I'll post a patch to implement this early this afternoon.

Please comment if you can contribute any insights on this problem.

Comment 24

[Bill Law]

Vishy offers a simpler alternative: Rather than alert the user and ask for 
confirmation when we are about to execute a downloaded file directly, we should 
detect this before giving them the option to execute it and prohibit those 
options.

Specifically, in case 1 (the progress dialog), we simply disable 
the "Launch..." button if the file is executable.  In the second case, we offer 
only "save to disk" when the file is executable (and in fact "choose" that for 
the user by going straight to the file picker; that's what happens in 4.x, BTW).

The only problem with that is that we are still possibly exposed to the file 
executable-ness not being available at the appropriate time.  If the helper app 
dialog can't determine that the file is executable, then it can't know that the 
other options won't be available.  One solution to that is to have fallback 
code in nsLocalFileWin's implementation of IsExecutable() that makes an 
intelligent guess (based on file extension) if SHGetFileInfo fails.

From an implementation perspective, this solution requires essentially the same 
changes to the same modules.  This option would simply eliminate the need for 
an additional confirmation dialog.  I will continue the work I'm doing (almost 
done, except for changes to the helper app dialog) and try to settle the issue 
of whether SHGetFileInfo will work if the file is not completely downloaded.

Mitch, do you have a preference for what approach to take, from a security 
perspective?  Disabling options for executable files is certainly the more 
conservative approach so we would guess that you'd prefer we do that.

Comment 25

[Bill Law]

Attachment: Patch that shows "security alert" before executing downloaded file directly

Comment 26

[Bill Law]

I just attached a patch that causes a "security warning" dialog to appear 
before executing a downloaded file.

Note that the changes to nsHelperAppDlg.js also include fixes for another bug 
that I already had in my local version (for bug 79231).

I will now work on modifications that will simply disable the option of 
launching a downloaded file and will attach that shortly. 

Comment 27

[Bill Law]

Attachment: Simpler fix to disable execution of downloaded files

Comment 28

[Bill Law]

Simpler fix to completely disable direct execution of downloaded files.

I do see one potential problem with that patch.  In the case of a very small 
executable file (16K), I get an assertion when I dismiss the file picker dialog 
that appears when I click on a link to an executable.  This comes from some 
back-end uriloader code that is attempting to ensure a "script context."  
Things work OK if I simply "Ignore" when I hit this assertion but something 
seems to be amiss.

I don't understand the uriloader code that throws the assertion, unfortunately.

Comment 29

[Peter Lubczynski]

What about XPI packages? Will they be able to just "open" or will one have to 
click on the icon in the OS.

I think disabling running completely is a poor solution. Not only will novice 
users not be able to find the location of where they saved their file, they have 
to dig through the OS to click on it. And then, instead of being helpful and 
warning that this file was downloaded and could potentially do damage as IE 
does, we ASSUME the user knows what harm they could possibly be doing and force 
them to find and execute the file. I'm specificaly thinking of AOL users or 
users trying to install plugins.

And then, what about .vbs, .js, .htc, .bat, .pif, .com, etc....and others. Are 
we going to rely on a Microsoft Windows API to demermine what is safe to run? 
Are we putting ourselves at the mercy of similar security flaws in IE?

Comment 30

[Mitchell Stoltz (not reading bugmail)]

I agree with Peter's second point; there are things besides executables which we
also don't want to run without warning.

As for whether to present a warning dialog or disabling running completely, I
think the latter is a better policy. That's the policy used in 4.x - let the
user find it on their desktop or wherever and double-click it. Having to "dig
through the OS" is better protection than a warning dialog, which many users
will simply ignore. In general, there are lots better ways of educating users
than warning dialogs.

I think the main point is making sure that when a user runs a downloaded
exeutable, that's a deliberate act. Running automatically with no warning is the
least deliberate form. Adding a warning is a little more deliberate, but a
warning does not ensure the user really understands the consequences of clicking
OK, or that they will even read it. Having to double-click on an icon on the
desktop is well understood by users. Users understand that this means they are
running an application which is separate from the browser. This is a deliberate
act, and users already understand its consequences, without us having to explain
it to them in a dialog that many won't read anyway.

Comment 31

[Bill Law]

> there are things besides executables which we
> also don't want to run without warning.

Can you be more explicit?

Some things are directly executable (.com, .exe).

Some things are executable slightly less directly (.bat, .pif).

Some things are indirectly executable (.vbs, .pl).

Some things are even less directly executable (.doc).

Some things we just don't know about.

So where do we draw the line and what's the best code to check for such files?  
SHGetFileInfo works for .exe/.bat/.cmd/.com but not for .pif.  Should we check 
for extensions only?  What about using PATHEXT (which controls cmd.exe, but not 
ShellExecute, I don't think).

I don't think there's a test for things further down the list.

Comment 32

[Bill Law]

Attachment: Final patch

Comment 33

[Bill Law]

I talked to Mitch and came to the agreement that we'll block execution 
of "executables."  We defined executables (on Win32) to mean any files with 
extension .exe, .bat, .com, .pif, .cmd, .js, .vbs, or .wsf.

I re-did the code in nsLocalFileWin.cpp to properly detect these extensions (it 
was previously getting false positives for names like "foo.exe.bar" and false 
negatives for names like "FOO.EXE").

Now I need review and super-review.  Adding dougt to cc: list because he should 
approve the change to nsLocalFileWin.

Comment 34

[Mitchell Stoltz (not reading bugmail)]

The change looks good to me. r=mstoltz, FWIW.

Comment 35

[timeless]

what about .pl or .pls? and perhaps python and ... [well generically, any 
script being handled by cscript.exe or wscript.exe]

other bad ones: .scf and .lnk

i think it's better to ban by handler than by extension. if explorer, 
rundll*, *script or cmd are going to handle the object then ban it.

Comment 36

[Bill Law]

.lnk files are definitely on the black-list.

What's up with .scf?  Those are handled by explorer.exe but can they do any harm?

I don't think we will worry about .pl because that's not supported on Windows
out-of-the-box and users bear *some* responsibility.

On my system, .pls is "RealPlayer.MP3PL.6" so I don't think a blanket ban is
called for.

The problem with checking the handler vs. extension is that "parsing" the
handler can be troublesome and some extensions don't even have one (like
.lnk->lnkfile).

Comment 37

[Bill Law]

I found a list of "dangerous" file extensions at microsoft.com:

File extension  File type
---------------------------------------------------
.ade            Microsoft Access project extension
.adp            Microsoft Access project
.asx            Windows Media Audio / Video
.bas            Microsoft Visual Basic class module
.bat            Batch file
.chm            Compiled HTML Help file
.cmd            Microsoft Windows NT Command script
.com            Microsoft MS-DOS program
.cpl            Control Panel extension
.crt            Security certificate
.exe            Program
.hlp            Help file
.hta            HTML program
.inf            Setup Information
.ins            Internet Naming Service
.isp            Internet Communication settings
.js             JScript file
.jse            Jscript Encoded Script file
.lnk            Shortcut
.mdb            Microsoft Access program
.mde            Microsoft Access MDE database
.msc            Microsoft Common Console document
.msi            Microsoft Windows Installer package
.msp            Microsoft Windows Installer patch
.mst            Microsoft Windows Installer transform; Microsoft Visual Test
source file
.pcd            Photo CD image; Microsoft Visual compiled script
.pif            Shortcut to MS-DOS program
.prf            Microsoft Outlook profile settings
.reg            Registration entries
.scf            Windows Explorer command
.scr            Screen saver
.sct            Windows Script Component
.shb            Shell Scrap object
.shs            Shell Scrap object
.url            Internet shortcut
.vb             VBScript file
.vbe            VBScript Encoded script file
.vbs            VBScript file
.wsc            Windows Script Component
.wsf            Windows Script file
.wsh            Windows Script Host Settings file

.scf is on the list.

I notice that .pl isn't, for example.  I suppose MS only cares if it's one of
*their* scripts.

Should we check for *all* these?

I think that might be a problem in terms of twisting "isExecutable" to coincide
with this list (.inf files wouldn't seem to qualify).  Maybe we need a new
"isDangerous" method in nsIFile?

Comment 38

[Peter Lubczynski]

Wow...what a long list. Does that include Windows XP too? I imagine they have 
some new goodies in there.

Comment 39

[timeless]

.pl is of course perl and .pls is the corresponding perl script (requires 
activestate activeperl w/ activescripting -- or an impersonation thereof).

.inf files if set to install are dangerous, however the default action _should_ 
be open (w/ notepad ...)

Should we add .eml (email) to the list? I wonder if an infected email document 
opened by outlook [express] could do damage ...

Ah yes, shell scraps... although i don't remember shb.. -- oh yes i do...
.shb            --Shell Scrap object-- "Shortcut into a document" not exactly 
the same thing but just as dangerous =)

while i'm adding activestate extensions [perl] .t, .pm ... [python] .py, .pyc, 
.pyo, .pys, .pyw

I doubt winxp added many if any file types.

[x] I don't care that it's truly executable
 [x] Let me run it anyways
  [x] I won't care the next time either
   [x] I'm either brilliant or too stupid to care
    [ ] I keep seperate backups for ten months and will restore as needed.

Oddly, ms didn't mention .class or .jar both of which are executables.  Wow 
long list =) does anyone want to suggest .sh since some mozilla users build 
mozilla and might ... nah

Comment 40

[Bill Law]

OK, I'm going to try to get a super-review for the patch as it currently stands 
(with the addition of ".lnk" and ".reg").

These others will have to wait for further analysis.

Comment 41

[timeless]

true executables (must block): .bat .cmd .com .cpl .exe .scr
script objects (high risk): .sct .wsc .wsf .wsh 
well known virus containers (high risk): .shb .shs 
shortcuts (medium-high risk): .lnk .pif .scf 
script applications (medium risk): .jse .vb .vbe .hta
scripts (shouldn't work in average case): .js .vbs
registry object (os should warn): .reg 
help apps (low risk): .chm .hlp 

please check for everything that's medium-high, high and must-block.

Although once the list expands beyond 4 things I suspect a regexp might be 
faster/cheaper and more scalable...

Comment 42

[Scott MacGregor]

*sigh* I'm very sad about losing the ability to click the launch App button in
the progress dialog. I love being able to install my next daily build that way. =(. 

Bill, your change to nsLocalFile::Win and to the helper app JS looks great.
Thanks for driving this.

sr=mscott

Comment 43

[Bill Law]

Thanks, Scott.  I agree about losing the ability to run downloaded .exe's, but
the guideline was better safe than sorry, for now.  We can look at permitting
this with appropriate warning post mozilla0.9.1.

timeless, thanks for being so diligent about enumerating these things.  I'm not
sure I concur with your list, though.  I think we should block extensions that:
1. Are defined to Windows by default (or usually).
2. Are by default (or, again, usually) "executed" either directly or by some
standard Windows component.
3. Can do damage.

Now one might argue that .doc falls under these rules :-).  Most of the ones on
your list qualify (I tested a few; .sct was the only one that I found wouldn't
qualify).  But, I'd like to do a more rigourous investigation before accepting them.

I'm anxious to get the code checked in so I'm going with what I've got currently.

It would be helpful if we could get a comprehensive list of extensions, and also
the output of "ASSOC .ext" (and then the output of *that* pumped through FTYPE).
 That would establish criteria 1. and 2., I think.

Comment 44

[Arthur]

Wouldn't a preferences setting be nice? Then "High-Security:
I-run-nothing-at-all" could be the default setting and the user could chose to
be able to run apps directly (with or without warning).

Comment 45

[timeless]

assoc/ftype are nt commands afaik. doc isn't viral unless you get word or 
perhaps works.  since neither of these are part of windows [yet] they don't 
count =). [yes wordpad can try to open doc files, but it won't execute 
viruses... or macros ...]

win98's shell.inf has the basics. if you ever fry your system associations, you 
can [assuming you did't fry inf's install ...] right click it and choose 
install.

a cursory look in w2k didn't reveal the equivalent file :-(

Comment 46

[Bill Law]

Hey, I was just joking about the .doc files.

I'll try to cobble together a home-grown assoc.exe and ftype.exe so we can
easily examine Windows default settings on Win98, etc.  Maybe these utilities do
more than simply dump the registry contents on Win2k, but at least that's a start.

Comment 47

[Bill Law]

Attachment: assoc.cpp

Comment 48

[Bill Law]

Attachment: assoc.exe

Comment 49

[Bill Law]

Ignore that; it doesn't seem to work right on Win98.  I'll figure out why 
tomorrow.

Comment 50

[Bill Law]

Resolving fixed.  Let's do the fine-tunig via other (mozilla0.9.2) bugs, please.

Comment 51

[ckritzer (gone)]

Peter, does this bug appear to be fixed to you?

Comment 52

[Peter Lubczynski]

Not really. There still isn't any security warning. Moot point anyway because  
one of my favorite features of directly running downloaded application is gone 
:-(
Will this stay like this for good?


As for the status of this bug, I'm not quite sure what to do bug I think the 
best thing would be to mark it CLOSED and reference another bug if more work 
will be done. Please feel free to reopen if you feel this bug is better for 
tracking this issue.

Comment 53

[Jacek Piskozub]

Bug 82584 is about the .doc files being opened as default if MSOffice is installed. 

BTW, it is funny Microsoft does not list them as dangerous as Word viruses are
every month on the top 10 lists - like http://www.uk.sophos.com/virusinfo/topten/

Comment 54

[Daniel Bratell]

CLOSED? I didn't think we used that status. According to the bugzilla help for
VERIFIED (which this bug never was): 
"Bugs remain in this state until the product they were reported against actually
ships, at which point they become CLOSED."

Comment 55

[Jacek Piskozub]

You are certainly right! Reopening, and let someone decide what to  do with this
baby.

Personally I do not think this is fixed (no warning and not all dangerous file
types covered)

Comment 56

[ckritzer (gone)]

Okay, so this bug has gone from:

1) Point: we need a better security alert for when stuff happens

to

2) [D]evolve: that stuff should never happen in the first place

I'm okay with that if stuff is getting fixed (which I'm sure it is/has been).  
Peter seems a bit :-( about one of his fav features going bye-bye (who wouldn't 
be sad?), and it seems that this bug has raised an issue about alerts.  I know 
the general feeling about seems to be (at least for the engineers/qa folk I've 
talked to) that 'less is more'.  Peter, if you'd like to open a separate bug 
about security alerts to cover that issue a bit more, that's fine with me (I'll 
most likely be the qa person dealing with the bug). 

In the meantime, I'll consider the point/focus of this bug to be "we shouldn't 
automatically launch certain filetypes" and test it as such.

Comment 57

[Mitchell Stoltz (not reading bugmail)]

Chris,
   I agree, that has become the point of this bug. The fix that was checked in
arose from a conversation between me and Bill Law. We decided that, at least for
0.9.1, we should not automatically launch any file types we know to be
dangerous, with the understanding that we can't enumerate every dangerous file
type, as there are some we may not know about and some we want to leave off, for
the sake of not crippling functionality too much.

I try to avoid warnings where possible. Please see my comments above about why
requiring the user to go to the desktop and double-click the downloaded file
makes for better understanding of the inherent risks.

Comment 58

[Jacek Piskozub]

If you decide to completely stop .doc files from running (regardless of Windows
settings), bug 82584 would be made a dup of this one. What stops me is the
shakey status of this one (once already "fixed").

Comment 59

[Viswanath Ramachandran]

lets close this bug out and get it verified. If there are specific issues, lets 
open thim in separate new bugs to track them. 

Comment 60

[Jacek Piskozub]

Veryfying. 

Leaving a short info on the new bugs on this page would be nice, saving us the
time to look for them. Thanks.