116938 - tries to save .exe file rather than play it

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[James Rome]

Links such as
http://www.content.loudeye.com/scripts/hurlPNM.exe?/~ttt-466568/0334758_0102_07_0002.ra
appear on the http://www.bmgmusicservice.com pages to allow customers to play
audio clips.

Mozilla trys to save the URL as hurlPNM.exe rather than to execute it on the
server and run real player.

Comment 1

[Oliver Klee]

Over to Networking.

Comment 2

[Andy Lyttle]

I'm getting "Content-Type: audio/x-pn-realaudio" in the HTTP header, so
apparently Mozilla is ignoring the MIME type in favor of the file extension?

Comment 3

[Boris Zbarsky [:bzbarsky]]

We only do that if we can't find a handler for that mime type...  Does 
setting up a helper in helper apps for audio/x-pn-realaudio help?

Also, the server is doing something odd.  I can get the data from it with telnet 
but not with wget.  Can someone open a command prompt, set NSPR_LOG_MODULES to 
"nsHttp:5" then start mozilla and record the log output to a file while opening 
the above URL, then attach the resulting log to this bug?

Comment 4

[James Rome]

ordinary Real URLs work just fine for me. Such as
http://sunsite.utk.edu:8080/ramgen/encoder/wuot.rm

It asks me if I want to open it with Real Player.

Comment 5

[Chris Veenboer]

Win2K SP2 buildID 2002020206

Clicking on the link tries to save the file (hurlPNm.exe),
adding a file type in the Helper Applications/file extensions with values:
RA
audio/x-pn-realaudio, open with realplayer, clicking on the link opens dialog
asking me to choose between opening with [realplayer] or saving, (works so far),
however, it still says it's doing something with the .exe file, and I don't have
realplayer installed. Setting the mimetype made moz open the correct dialog however.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  This worksforme when I have realplayer defined in helper apps.

I bet what happens here is the following:

1)  We try to find what to do with audio/x-pn-realaudio files.  Nothing is found
    in prefs or the system setup
2)  We look at the extension in the url.  It's ".exe".  We look for something to
    do with an exe file.  The only option is "save".

So the problem is in the lookup in step #1 failing.  James, could you attach
your mimeTypes.rdf file to this bug if you're still seeing the problem?

Comment 7

[James Rome]

The site changed the URLs so it no longer has that format.

Comment 8

[James Rome]

Attachment: mimetypes.rdf file as requested

Comment 9

[James Rome]

Actually, that URL now asks me if I want to open it with Real Player, I say yes,
and nothing happens.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  No mentions of "audio/x-pn-realaudio" in the file... James, what build are
you using?  I get prompted to run realplayer and then it runs and the music plays...

Comment 11

[James Rome]

When I try the URL now, hurlPNM-4.exe gets put in /tmp, even though I said open
with real player. No .rm file is put there.

Comment 12

[Ere Maijala (slow)]

Whow, this is ugly. I have RealPlayer installed and working ok as a helper app.
I've set it to always open with RP without confirming. What Mozilla (build
20020214, Win2K) really did was to save the ra file as hurlPNM.exe to the temp
folder and EXECUTE it (causing the Win16 subsystem to crash as it apparently was
not a valid exe). If I interpreted the result corrently, this is clearly a huge
security hole allowing to masquerade an (possibly malicious) executable as .ra
file. Confirming and raising priority.

Comment 13

[Ere Maijala (slow)]

...meant to say changing severity, not priority. Sorry for the spam.

Comment 14

[Boris Zbarsky [:bzbarsky]]

file handling... I think I know what's up here.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 16

[James Rome]

I'm glad I finally uncovered a big security hole :-)

Comment 17

[Boris Zbarsky [:bzbarsky]]

I should note that the fact that any file with a .exe extension is treated as an
executable in these circumstances by the system even though Mozilla _explicitly_
runs the realplayer app on that file is odd beyond belief....  :)

Comment 18

[Bill Law]

I'm afraid I still don't understand exactly what's going on here.  Boris, can 
you spell it out a little more explicitly, please?  Where exactly is the 
security hole?

It strikes me that the fix here is too extreme.  I'm not sure why we shouldn't 
be respecting the content-disposition header, or the actual name of the file on 
the server, just because the user has set up their helper-app pref entry 
incorrectly.

Sorry if I'm being dense.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Bill, the problem here is that we copy the file to the mSuggestedFileName before
running the helper on it (a somewhat recent change, right?).

So here is the problematic sequence of events:

1)  The user does not set up a helper for audio/x-pn-realaudio, but there is a
    registry entry for it.
2)  At some point the user selects "never ask me for this type" on an
    audio/x-pn-realaudio file (a perfectly reasonable thing to do).
3)  A server sends data as type audio/x-pn-realaudio, with a content-disposition
    filename of "foo.exe" (or the filename in the URL is foo.exe, as here, in
    which case the server is not really being malicious....)
4)  OnStartRequest never opens the helper app dialog (since we have "never ask"
    set), hence we never do the isExecutable() check from the helper app dialog.
    Thus we proceed to LaunchWithApplication() and try to launch the file via
    the system shell.  And since we do this after copying to mSuggestedFilename
    it ends up being run as an executable.

Needless to say, a malicious server could use Content-Disposition and sending
executable data as some random (eg audio/x-pn-realaudio) type to silently run
code on the user's machine.

I agree that my fix is a little extreme... Other options would be:

1)  Add an isExecutable() check in the "never ask" branch of OnStartRequest,
    just like we have in the helper app dialog.  For that matter, we could do
    the check in OnStartRequest before the "do we ask?" check, and thus avoid it
    completely in the helper app dialog.
2)  Only munge the filename if we're planning to "Open with application", not if
    we plan to "save as"
3)  Do #1 and do the extension-munging for a filename we get from the URL as per
    my existing patch.

Of these three, #3 is the only one that will really solve the original issue in
this bug.  What happened to James initially was that he did not have "never ask"
checked and isExecutable() tested true in the helper app dialog.  So we forced
to "Save As" as a security measure.

Going to attach a patch attempting to do #3.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v1.1

Implement my suggestion #3.  This needs a security check-over (Mitch?).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v1.1 as diff -uw for ease of review

Comment 22

[Boris Zbarsky [:bzbarsky]]

As a note, the fix I just attached will _still_ sometimes munge the filename if
said filename comes from a url.  It won't do it often, imo, and it'll do it in a
manner consistent with "save link as" (except slightly more accurately, since it
already has the real type the server sends).

Comment 23

[Bill Law]

>the problem here is that we copy the file to the mSuggestedFileName before
>running the helper on it (a somewhat recent change, right?).

There was a change to copy/rename the file after the download was complete, yes.
 I think we always set the extension, however.  But that may not have been
subject to content-dispostion header influence.  Also, I'm not sure exactly what
precedence was given to the url's extension vs. the helper app entry's.  Note
that it is this uncertainty that makes me cautious about leaping too quickly on
some new fix.

Anyway, thank you for explaining the problem.  I was uncertain whether we would
ever do a ShellExecute against a file for which there was a helper app entry. 
Perhaps the hole is in the case where there's a pseudo-helper app entry, as is
the case when the user says "don't ask me" for a given mime type.

Is that the only hole?  If they say "don't ask me" in a *real* helper app entry,
then that entry will have to be either save-to-disk, or, specify a real
application (which we will launch).

If that is the thing that needs fixing, then that could be handled at the point
where we manufacture the "pseudo" helper app entry from the don't-ask-me pref
list (by checking for IsExecutable()).

Anyway, we'll see how closely your latest patch matches that speculation.

Oh, one other thing.  I am worried about the case where the user creates a
helper app entry for some random type, like application/octet-stream.  If this
entry has no extension list, then what happens if there's a .zip file with type
application/octet-stream?  I thought your previous patch would preclude
suggesting .zip as a file extension when the user saved, and I think that would
be wrong.  I think we should consider this case when examining the other solutions.

OK, one *more* thing:  I'd prefer to see the "policy" implemented at as high a
level as possible (higher meaning further from the guts of the uriloader).  That
way, different mozilla-based browsers could institute different policies more
easily.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> makes me cautious about leaping too quickly on some new fix

Agreed.  Let's do this right once and for all.

> Is that the only hole?

I _think_ so, but I don't claim to understand the Windows nsOSHelperAppHandler
that well...

> If this entry has no extension list

In that case mTempFileExtension is empty, and so we just keep the "original"
filename.... The issues start if the user has a helper app setting for
application/octet-stream listing "foo" as an extension. In that case we would
append ".foo" to the ".zip" file.  I'm not sure how acceptable that would be.

So the two things we're trying to fix are:

1)  "Don't ask me" helpers gotten from the system never get an isExecutable()
    check
2)  A url ending in .exe will preclude running helpers on content coming from
    that url because we use the URL filename as the helper filename.  Thus the
    user just gets a save dialog.

Comment 25

[Bill Law]

>I _think_ so, but I don't claim to understand the Windows nsOSHelperAppHandler
>that well...

Makes my head spin, too (although I'm usually puzzling over it at 2:00am so
maybe it's just me).

>The issues start if the user has a helper app setting for
>application/octet-stream listing "foo" as an extension. In that case we would
>append ".foo" to the ".zip" file.  I'm not sure how acceptable that would be.

Not very, I suspect.  The thing is that this twiddling of the extension is just
a means to an end (the end being not doing ShellExecute on an .exe file).  We
can block that by other means, I think.

>1)  "Don't ask me" helpers gotten from the system never get an isExecutable()
>    check

Right.  And there's really two sides of this:
a. We need to make sure that executable files don't get ShellExecute-d.
b. We need to make sure that non-executable files get an extension based on the
helper app settings (but only if they will be ShellExecute-d).  If we don't do
this, then it is unlikely that the system will launch the proper helper app.

>2)  A url ending in .exe will preclude running helpers on content coming from
>    that url because we use the URL filename as the helper filename.  Thus the
>    user just gets a save dialog.

Unless they say "don't ask me," right?  Because in that case, we go straight
down the "launch with application" path and if there's really a helper app, then
we run it against the temp file (and it doesn't matter what the temp file is named).

It seems like the problems occur only when we run ShellExecute.  But that might
be a rash assumption.

But if it were the case, then maybe we could just do the IsExecutable() check at
the point where we're about to do the ShellExecute.  Provided we can deal with
the situation properly at that point in time, then that might cover all the
IsExecutable() issues.

BTW, we always do ShellExecute via the "Launch" button on the progress dialog,
so that needs an IsExecutable check, regardless.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> twiddling of the extension is just a means to an end (the end being not doing
> ShellExecute on an .exe file)

Actually, no.  The end is, in the orifinal incarnation of this bug, to be able
to listen to realaudio content served from a url which ends in ".exe".

Agreed on points a. and b. in your comment.  :)

Moving the IsExecutable() check to right before ShellExecute sounds like a very
good idea.  In fact, if we want to be nice to embeddors we could add a bool
argument to nsPIExternalAppLauncher::launchAppWithTempFile that could be set to
true to bypass the security check (if embeddors want to, eg, enable running of
executables directly).  Would that work?

Comment 27

[Bill Law]

Making the security check an option on the nsPIHelperAppLauncher method is a
good enhancement.

So how do we handle the situation when about the ShellExecute the .exe file?  We
need to make sure an appropriate error response filters back to a point where we
can give the user the "save to disk" option.

Comment 28

[James Rome]

Have any fixes for this been put into the nightly builds? Bug 127322 I filed on
the 2/22 build which now forgets mozillacompose.xul and tries to download it.
Hence you can't send mail except for the first time.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Nope.  If there were changes that tried to fix this bug there would be reviews
and such in here.... 

Comment 30

[Boris Zbarsky [:bzbarsky]]

*** Bug 127536 has been marked as a duplicate of this bug. ***

Comment 31

[Bill Law]

Nominating, due to security ramifications.

Comment 32

[Bill Law]

Setting target milestone.

Boris, are you willing to be assigned bugs such as this that you're working 
on?  Otherwise, I'm going to have to go look for some victim on which to 
offload some of my bugs (or they will get pushed out).

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 70087 [details] [diff] [review]
Patch v1.1

I'm not actively working on this one right now...  For one
thing, I have no windows system to test on, which makes
changes to windows-only code hard to develop/debug...

I can try to do it if there is no one else, but it may take me some time...

Comment 34

[Bill Law]

I'll take it and trust I can consult with you as needed; thanks.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Yeah.  I'm certainly up for that.  :)

Comment 36

[Peter Trudelle]

nsbeta1+ per ADT triage team

Comment 37

[James Rome]

I still can't get Real links to play. Columbia House has:
<!--Switch Row Colors Note : See if we can move to method based variable so we
can remove one copy-->
<tr>
<td bgcolor="#cccccc" width="302">Frederic Chopin (1810-49)</td>

<!-- CHECKING FOR WINDOWS MEDIA -->
<td bgcolor="#cccccc" align="center"><a
href="http://www.columbiahouse.com/m2/rams/asfhier/35/01/350116_01_00_2.asx"
onMouseDown="CMPageView(this)" ><img
src="//a248.e.akamai.net/f/1596/1167/1d/images.columbiahouse.com/ch/images/se2/ch_music/master/listen.gif"
width="22" height="12" alt="Frederic Chopin (1810-49)" border="0"></a></td>
<!-- CHECKING FOR REAL AUDIO -->
<td bgcolor="#cccccc" align="center"><a
href="http://www.columbiahouse.com/m2/rams/rm5hier/35/01/350116_01_00_5.ram"
onMouseDown="CMPageView(this)"  ><img
src="//a248.e.akamai.net/f/1596/1167/1d/images.columbiahouse.com/ch/images/se2/ch_music/master/listen.gif"
width="22" height="12" alt="Frederic Chopin (1810-49)" border="0"></a></td>
<!-- CHECKING FOR REAL AUDIO G2 -->
<td bgcolor="#cccccc" align="center"><a
href="http://www.columbiahouse.com/m2/rams/rmhier/35/01/350116_01_00.ram"
onMouseDown="CMPageView(this)" ><img
src="//a248.e.akamai.net/f/1596/1167/1d/images.columbiahouse.com/ch/images/se2/ch_music/master/listen.gif"
width="22" height="12" alt="Frederic Chopin (1810-49)" border="0"></a></td>
</tr>

The .ram links pop up a window. I select "open with real player," and then real
comes up with an invalid metafile message, trying to play 
file://C:/TEMP/350116_01_04_5.ram

Comment 38

[Bill Law]

James,

Your last report seems to be a separate problem.  It looks like we're 
downloading the content with the correct extension (.ram) and launching real-
player, but the data is bad.  That could be a problem on the server, or 
something else.

Can you save one of those .ram files (using "save link as") and try opening 
realplayer on it directly?

Comment 39

[James Rome]

Hmmm. The "file" contains
rstp://nyra2.columbiahouse.com/g2rm/35/01/350116/350116_01_00.RM
and if I put the rstp URL in a mozilla URL I get "r is not a registered protocol"

If I put it into RealPlayer, I get "A general error has occurred"

Comment 40

[Joonas]

*** Bug 136328 has been marked as a duplicate of this bug. ***

Comment 41

[Vadim Berezniker]

Regarding the bug that was just duped to this one,

I found a windows machine to test on. I installed realone on it.
Tried my test URL (http://www.kryptolus.com/t.exe) and the executable was
automatically executed.

Comment 42

[Rainer Bielefeld]

So it is,
(http://www.kryptolus.com/t.exe)
first opens RealPlayer and than is executed automatically with
Mozilla/5.0 (Windows; U; Win98; de-AT; rv:0.9.9+) Gecko/20020403(03)

Comment 43

[Bill Law]

I'm going to extract a subset of the big patch I've got for bug 86640 to plug 
this security hole.  That will be a very small-scale and low-risk patch.

Comment 44

[Bill Law]

Attachment: revised patch

This patch has two parts:

1. Ensure mSuggestedFileName always has mTempFileExtension

This is accomplished by adding a new utility method to nsExternalAppHandler
called EnsureSuggestedFileNameExtension.  This function compares the file
extension in mSuggestedFileName to that in mTempFileExtension.	The latter is
always tied to the content type (see code in ::DoContent).  If that extension
differs from what's in mSuggestedFileName, then it is tacked onto the end of
mSuggestedFileName.

This function is called from two places: SetupTempFile where mSuggestedFileName
is initialized, and ExtractSuggestedFileFromChannel where it is reset based on
the Content-Disposition response header.

Thus, http://www.kryptolus.com/t.exe produces "t.exe.ram" as suggested file
name (and the name of the file passed to ::ShellExecute).  Something like
"http://www.foobar.com/foobar.ram" where the site does something evil like
return Content-Disposition: attachment; filename="foobar.exe", will produce a
suggested file name of "foobar.exe.ram".

2) win/nsOSExternalAppService.cpp is changed so that it opens temp files
indirectly via nsLocalFileWin::Launch, after checking that the file is not
executable (if executable, it returns an error).  Item 1) above makes it so
this code can't be reached, but I put the check in just to be safe (there may
be some deviant path through nsExternalHelperAppService that could get you
here).	It also buys a bit of functionality in that there is code in
nsLocalFileWin::Launch that will display the Win32 application-picker if there
is no application currently associated with the file extension.

Samir or Boris, can you review, please?  Who's up for super-review?

Comment 45

[Scott MacGregor]

Comment on attachment 79377 [details] [diff] [review]
revised patch

I noticed that the original ShellExecute call in nsOSExternalAppService passes
"open" as the 2nd argument. 

nsILocalFileWin::Launch passes NULL for this argument. I seem to remember it
being a bug when we weren't passing "open" in the external
nsOSExternalAppService. Are we opening ourselves up to a bug by changing that
behavior?

The code looks great to me. Couple testing comments:

1) Can you try opening some mail attachments the require helper apps like MS
Word, Real Audio and Acrobat.

2) Can you try opening some content that uses a content disposition header for
getting the file name. Just wanted to make sure we still behave correctly when
determining the file name in this scenario.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 79377 [details] [diff] [review]
revised patch

This will make our suggested filename suck in the "save" case.... Could we
possibly call EnsureSuggestedFileNameExtension right before the MoveFile/Launch
combination?  At that point we know for sure we're launching it....

Comment 47

[Bill Law]

re: comment 45

I don't know about the NULL vs. "open" on ::ShellExecute.  I'll investigate
that.  I'm not sure why it would need to be different here versus when opened
via the Launch button on the progress dialog or dialog manager (the other place
where nsLocalFileWin::Launch is called).  It may be that "open" is *different*
than NULL (opens versus "do the default"), but I'm not sure we should do
different things, so maybe nsLocalFileWin::Launch should be fixed.

I've tested the Content-Disposition case.  We still respect the filename
specified there, and modify it if its extension is bogus.

re: comment 46

Suck?  How so?  We suggest with an extension that makes sense (meaning: if the
user saves with the name we suggest, they get a file whose extension that
actually indicates the type of content in the file).
In the canonical case (t.exe served up as audio/x-pn-realaudio), I don't think
we want to encourase the user to save that as a file with a .exe extension.

Comment 48

[James Rome]

Is this in a nightly build yet so I can test?

Comment 49

[Boris Zbarsky [:bzbarsky]]

> Suck?  How so?

For example it'll append ".txt" to everything that's text/plain and does not
have a ".txt" extension (this is pretty gratuitous).  The contentAreaUtils.js
code already has that problem.....

For another example, it'll append extensions to the server-selected name on Mac
and Linux and other OSes where the extension is not what determines the file
type.

My suggestion would be to:

1)  Always twiddle the extension in the "execute" case
2)  Only twiddle the extension in the "save" case if
  a)  We're on Win32 or OS/2
  or
  b)  We're not, and the type is not text/plain and the filename came from the
      URL, not the content-disposition header

I'd be happy with just doing 1) and 2a) for now...  (and I'll probably do 2b at
some point, but that's not a security issue, whereas 1 and 2a are).

> Is this in a nightly build yet so I can test?

No.  There'll be a comment about it getting checked in before it is.

Comment 50

[James Rome]

With the 41603 build
http://www.content.loudeye.com/scripts/hurlPNM.exe?~ttt-466568/0337466_0107_07_0002.ra
now opens a dos box, runs something, but never gets the link to real player.

Comment 51

[James Rome]

But
http://hurl.content.loudeye.com/scripts/hurl.exe?clipid=009610501050706550&cid=466568
launches windows media player and works.

Comment 52

[Bill Law]

>For example it'll append ".txt" to everything that's text/plain and does not
>have a ".txt" extension (this is pretty gratuitous).  The contentAreaUtils.js
>code already has that problem.....

Well, everything that's text/plain will just open in the browser :-).

But your point is still valid, to some extent.  Given something like
"http://www.music.com/MyFavoriteSong" maybe saving it as "MyFavoriteSong" is
better than "MyFavoriteSong.ram" (on Mac and Linux).  I can't imagine a Mac or
Linux user who would want an audio/x-pn-realaudio file to be named "t.exe,"
however.

This is why I hate working on this code (and at least I'm getting paid to do it :-).

>1)  Always twiddle the extension in the "execute" case

By "execute" I presume you mean "open with application" (from the helper app
dialog, versus save-to-disk).

But if, as you say, "the extension is not what determines the file type," then
why does it matter what extension we put on the file (on Mac and Linux and other
such OSes) in that case?

Also, is it really the case that on Linux that the extension does not determine
the filetype?  There's no MIME type in the file system, so after it's saved, the
extension is the only clue the system has.  Don't the desktops use the extension
to determine which application to open (when you double-click the file on the
desktop)?  I would think we would need to help the user exploit that (by
offering to put the "proper" extension on the file when we offer to save it).

Comment 53

[Boris Zbarsky [:bzbarsky]]

> But if, as you say, "the extension is not what determines the file type," then
> why does it matter what extension we put on the file 

Because there are unfortunately many applications (staroffice, Mozilla, etc)
that care about the extension.  So in the "open with application" case it's
better to be safe and add the extension.

> There's no MIME type in the file system, so after it's saved, the
> extension is the only clue the system has.

Well, nicely behaved things use the 'file' utility, which looks at magic numbers
in the file (and current distributions have magic-number-to-mime mapping files).
 But yes, desktops do tend to key off extensions....  Which is why I suggest
that we _do_ add the extension if it came from the URL (since the URL can have
nothing to do with the file type).  I suggest we keep content-disposition
filenames on Linux/Mac when saving, however (you're right that text/plain is not
an issue in this code).

So my revised summary is:

1)  On Windows and OS/2 always fix up the extension
2)  Otherwise, fix it up if
     a) We're going to "open with application"
     or
     b) The filename came from the URL.

Does that make sense?

> I can't imagine a Mac or Linux user who would want an audio/x-pn-realaudio
> file to be named "t.exe," however.

True.  That offers an alternative, which is to fix up the extension on those
OSes in the save case only if there is already an extension on the file....  I
think the other method is better, but that's just me.  I'd be fine with either one.

> This is why I hate working on this code

agreed.  :)

Comment 54

[Boris Zbarsky [:bzbarsky]]

> Which is why I suggest that we _do_ add the extension if it came from the URL

I meant "Which is why I suggest that we _do_ add the extension if the filename
came from the URL"

Comment 55

[Bill Law]

>Because there are unfortunately many applications (staroffice, Mozilla, etc)
>that care about the extension.  So in the "open with application" case it's
>better to be safe and add the extension.

Seems odd that these "many applications" would suddenly *stop* caring after the 
file is saved.

But I'll concede you this one since Linux users are easy enough to please, 
regardless.

Now what about MacOS X?  That seems real file-extension sensitive so maybe we 
need to try to set the extension properly there, too (although i don't know if 
the Internet Config code on that platform even supplies us with file extension 
hints).

I'll work on code that implements your strategy and post it here tomorrow.

Comment 56

[Bill Law]

BTW, I tried opening a mail attachment and it worked fine.  I'm not sure how I 
can construct a mail attachment where the mime type and file extension are at 
odds, though.  Are there scenarios like that which I should try, and if so, how?

Comment 57

[Boris Zbarsky [:bzbarsky]]

> Now what about MacOS X?

Frankly, I don't know.  I do recall that it keys off extension, though, so we
may want to make that do the same thing as OS/2 and Windows...

> I'm not sure how I can construct a mail attachment where the mime type and file
> extension are at odds, though.

Just sent you a PDF attached as application/pdf with a ".doc" extension on the
filename.

Comment 58

[Bill Law]

Attachment: screen shot of helper app dialog when opening mail attachment

This is what you get when opening an attachment of type application/pdf when it
is named *.doc.  Note that I had to remove my Acrobat plugin to get that
(otherwise, it opened in the mail window).

Note the file name on the dialog title bar.  That's mSuggestedFileName, which
has the .pdf extension in this case.

Comment 59

[Bill Law]

Attachment: final revision

I started to implement the Boris Strategy.  But it really complicates the code.
 Instead of one code path, we have about 20 (save vs. open, filename from url
vs. from content disposition header, then different logic for every platform).

We (I, after consultation with some other respected engineers who were around)
decided that it wasn't worth it.

1. On Mac classic, it doesn't much matter because everybody is going to MacOS X
anyway.

2. When saving a file, one very likely still wants the extension set right
since that influences what happens when the file is opened on the system
desktop.

3. For every case where the extension we arrive at is worse than the one from
the URL (or content disposition header), there will probably be a thousand
where it is better (i.e., in the vast majority of cases where they are even
different, the default will be .cgi or .dll).

So this patch implements the same basic strategy.  I've made two concessions by
changing the name of the function to make it more generic
("EnsureSuggestedFileName" instead of "EnsureSuggestedFileNameExtension") and
made the function virtual so it will be easier for platforms to override the
default behavior.

Will you sign off on this, Boris?  We can revisit the issue again when I try to
land the fixes for bug 86640.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 79672 [details] [diff] [review]
final revision

r=bzbarsky

Yeah, I can see that the other version got complicated...  I'm fine with
getting this in for
now (that gives us about the same behavior as "save page/link as", actually).

I'll likely end up revisiting this in the context of fixing the pure "save as"
bugs on
the extension-adding behavior... at that point, we'll likely want to make this
function scriptable so all the icky filename logic can
live in one place....

But for now let's just fix the security bug.  :)

Comment 61

[Bill Law]

Need sr= from Scott.

I could find no documentation that would lead me to believe there is any 
difference between specifying NULL vs. "open" on the ::ShellExecute.  I haven't 
seen any bugs relating to the Launch button on the progress dialog, either.  So 
I don't think there's a problem there and if there is, we need to fix it over 
in nsLocalFileWin::Launch.

Comment 62

[Scott MacGregor]

Comment on attachment 79672 [details] [diff] [review]
final revision

Ok, thanks for looking into the "open" stuff Bill. 

sr=mscott

This patch will also fix our compatibility with:
www.mlb.com when you try to listen to a live baseball game. The temp file isn't
getting the right extension so real one isn't launching. Your patch fixes that.
Thanks!

Comment 63

[bugzilla]

This is *still* not fixed in RC1 -- this bug is a huge security hole which I
would consider a must-fix for any milestone, and certainly a release candidate.
Please fix this in RC2!!

Comment 64

[scottputterman]

adding adt1.0.0 nomination

Comment 65

[scottputterman]

adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.

Comment 66

[Mike Shaver (:shaver emeritus)]

Comment on attachment 79672 [details] [diff] [review]
final revision

a=shaver for 1.0 branch.

Comment 67

[Malcolm Rowe]

Bill,

FWIW, there is a difference between NULL and "open" on ::ShellExecute.  
Specifically, NULL will pick the 'default' action.

From http://msdn.microsoft.com/library/default.asp?url=/library/en-
us/shellcc/platform/Shell/reference/functions/shellexecute.asp

[when using NULL as the parameter]
For systems prior to Microsoft® Windows® 2000, the default verb is used if it 
is valid and available in the registry. If not, the "open" verb is used. 

For Windows 2000 and later systems, the default verb is used if available. If 
not, the "open" verb is used. If neither verb is available, the system uses the 
first verb listed in the registry.

We should probably use NULL in preference to "open".

Comment 68

[scottputterman]

Bill or Samir, could you check this into the branch as soon as possible?

Comment 69

[Bill Law]

fix was checked in on branch last week

Comment 70

[sairuh (rarely reading bugmail)]

verified fixed using 2002.04.29.08-1.0.0 (branch) commercial bits on win2k.
tested using the URL orginally reported.

adding verified1.0.0 keyword.

Comment 71

[Bill Law]

marking fixed

Comment 72

[James Rome]

It works on the 4/28 build. The 4/29 and 4/30 builds do not ever start for me.

Comment 73

[Steve Dagley]

Reopening.  The 'fix' for this bug is unacceptable in the Mac.  If you want to
address a Win32 specific problem please fix it in a manner that doesn't screw
the Mac.

Comment 74

[Bill Law]

So why is naming a RealAudio media file with extension ".ram" rather 
than ".exe" so unnacceptable to "the Mac?"

Did you read the discussion of this issue above?  Do you have counter-arguments?

As it happens, we debated this fix with more than one Mac user and they didn't 
feel particularly screwed by it.  So if we're just tallying votes, you lose, 2 
to 1.  We need more constructive input.

Comment 75

[Steve Dagley]

Upwards of 99% of the files Mac users download will be of the type
application/octet-stream.  This 'fix' seesm to be the root cause of all these
files now sporting a '.binary' extension on top of whatever extension they
already had.  The Mac file system APIs currently in use limit the file names to
31 characters and you just truncated the allowable name length to 24 characters
to accomodate the '.binary' extension.  How is that considered acceptable, or
useful?  Change the name however you like on other platforms but leave them
alone on the Mac is what I'll give you as my constructive input.

Comment 76

[Mike Pinkerton (not reading bugmail)]

make that two on dagley's side. i find this patch aggrivating on macos (and
yeah, i did read the bug).

will i now find myself ending up with files on my desktop like "foo.mpeg.mpg"
because our comparisons are lax with adding the suffix? When we get a particular
mimetype, that should dictate the type/creator that the file gets saved as on
mac, not the extension. I don't want you messing with my extensions. I don't
want you to add .txt to the end of all my text files.

Mac users are fairly picky about such things. I see no need to "correct" the
extension on macOS, and doing so only gives a bad user experience.

Comment 77

[Samir Gehani]

To add to the mix, what about Mac OS X?  I believe associations of a file to an
app are made based on the extension on Mac OS X.  Emperical observation:
changing a file named ``foo'' to ``foo.txt'' and then double-clicking it results
in the file being opened with TextEdit.  Should we deal with files on Mac OS X
in the same fashion we deal with files downloaded on Unices/Windows giving
importance to the ``most relevant'' MIME type <-> extension mapping?

Comment 78

[Steve Dagley]

It's not so simple as you might think.  Mac OS X is in a state of flux as it
supports both the classic Mac type/creator system and file anme extensions.  How
things will settle out isn't clear yet but for now the type/creator system has
precedence over the extension system.  Where you see the latter coming into play
is when an app doesn't provide any type/creator info.

Comment 79

[Kathleen :Brade]

comment 77 is not relevant

What I don't understand about this bug is we can't compare the "desired
extension" in a more general fashion (get its mimetype and see if the current
extension is also of the same mimetype).  Ideally the code would be architected
to maintain the mimetype rather than trying to maintain a desired extension.  Or
am I missing something?

Comment 80

[Boris Zbarsky [:bzbarsky]]

Can a single extension map to multiple MIME types on Windows?

Comment 81

[Peter Trudelle]

Could someone please open up a separate, Mac-specific bug for this?  I'd rather
not morph it to cover the side effect.

Comment 82

[sairuh (rarely reading bugmail)]

should we just continue this current mac issue in bug 141330?

Comment 83

[Bill Law]

re-resolving as FIXED; the change has been undone on the Mac per bug 141330.

Comment 84

[James Rome]

There is still a problem. I tried to download an rpm (redhat) file from windows
http://www.dtek.chalmers.se/groups/dvd/srpm/libdvdcss-0.0.3.ogle3-1.src.rpm
and it opened in sone sort of real player screen!

Comment 85

[Boris Zbarsky [:bzbarsky]]

That's a problem with the server, which sends the audio/x-pn-realaudio-plugin
MIME type for that file.  The only correct thing to do there is to pass that
content off to RealPlayer (and NS4 does exactly the same thing).  Re-resolving.

Comment 86

[Michael Dunn]

Sairuh... This bug was reopened a few times. Is it still fixed on the branch? 
Need bug Verified on Mac as well

Comment 87

[sairuh (rarely reading bugmail)]

yes, this looks good on the branch: vrfy'd fixed using 2002.05.06.0x-1.0.0 comm
bits on linux rh7.2, win2k and mac 9.1.

double-checked this on the trunk as well using somewhat older builds (which
should still have the fix):

win2k, 2002.04.30.10-trunk
mac 10.1.4, 2002.05.02.03-trunk
linux rh7.2, 2002.04.30.10-trunk

Comment 88

[Pete Boyd]

*** Bug 144629 has been marked as a duplicate of this bug. ***

Comment 89

[James Rome]

I think this fix may have broken the mail attachments. I got sent a registry key
file (.key) to unlock software as an e-mail attachment. But when I saved it, it
stuck on an extra .exe extension, thus preventing the file from launching properly.

Comment 90

[Boris Zbarsky [:bzbarsky]]

That's actually bug 120327.

Comment 91

[James Rome]

I think this still needs to be reopened.

I got an e-mail from Suse for security patches that had 
   i386 Intel Platform:

    SuSE-8.0
    ftp://ftp.suse.com/pub/suse/i386/update/8.0/n2/apache-1.3.23-120.i386.patch.rpm
      58752b3a35523263428c325b340c9ae8
    ftp://ftp.suse.com/pub/suse/i386/update/8.0/n2/apache-1.3.23-120.i386.rpm
      b52837fe3f8512155ae93f7462526841

When I clicked on the ftp link to download it,guess what happened? It launched
the RealPlayer plugin in my mail window and tried to play the file.
1) The real plugin should not be launched in the mail window (There was another
bug about the pdf plugin being launched there which got fixed).
2) It should check to be sure it is a real file and not a rpm patch!

Comment 92

[Boris Zbarsky [:bzbarsky]]

That's not this bug.  Item #1 is a general bug about plugins in mail (already
filed).  Item #2, there is no really good way of doing that (it's a matter of
whether we trust our content-sniffing or the system's extension-to-type mappings
more, and since out content-sniffing is fairly primitive and out of date by
definition we go with the system mappings first).  In either case, the changes
would be completely unrelated to the original bug reported here.

Reclosing this one since the original problem is fixed.

Comment 93

[sairuh (rarely reading bugmail)]

re-vrfy'ing...

Comment 94

[Jerry Baker]

As noted in another bug, this fix is the wrong one. Mozilla should never
ShellExecute a file downloaded from the network...EVER. That is the bug here.
Mozilla should only offer to pass the file's location on to an application
registered to handle the type of data that the server said it was (content-type).

Comment 95

[Boris Zbarsky [:bzbarsky]]

> Mozilla should only offer to pass the file's location on to an application
> registered to handle the type of data that the server said it was (content-
> type).

There is no such concept ("application registered to handle a content-type") on 
Windows as far as I have been determined.  If there is, could you let us know 
how it can be accessed via the Win32 API?  That would be very useful.

Does the registry entry for an extension contain the exact path of the thing 
that will launch it?  Is this format constant across Win32 versions?

Please file a _separate_ bug on this issue, Jerry.  Cc me on it.  I'd very much 
like to see this fixed, but discussing it in 2 different bugs that are 
primarily concerned with other things is a sure way to get it _not_ to happen.

Comment 96

[James Rome]

But, Mozilla should only run things listed in the helper applications list, but
offer launch a wizard to add this new mime type and player to the list.