Closed
Bug 116938
Opened 23 years ago
Closed 22 years ago
tries to save .exe file rather than play it
Categories
(Core Graveyard :: File Handling, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0
People
(Reporter: jamesrome, Assigned: law)
References
()
Details
(Keywords: privacy, Whiteboard: [adt1])
Attachments
(3 files, 4 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
mscott
:
superreview+
shaver
:
approval+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Over to Networking.
Assignee: asa → neeti
Component: Browser-General → Networking
QA Contact: doronr → benc
Comment 2•23 years ago
|
||
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•23 years ago
|
||
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?
Reporter | ||
Comment 4•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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?
Reporter | ||
Comment 7•23 years ago
|
||
The site changed the URLs so it no longer has that format.
Reporter | ||
Comment 8•23 years ago
|
||
Reporter | ||
Comment 9•23 years ago
|
||
Actually, that URL now asks me if I want to open it with Real Player, I say yes,
and nothing happens.
Updated•23 years ago
|
Attachment #69393 -
Attachment mime type: text/rdf → text/plain
Comment 10•23 years ago
|
||
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...
Reporter | ||
Comment 11•23 years ago
|
||
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•23 years ago
|
||
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.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 13•23 years ago
|
||
...meant to say changing severity, not priority. Sorry for the spam.
Comment 14•23 years ago
|
||
file handling... I think I know what's up here.
Assignee: neeti → law
Component: Networking → File Handling
QA Contact: benc → sairuh
Comment 15•23 years ago
|
||
Updated•23 years ago
|
Reporter | ||
Comment 16•23 years ago
|
||
I'm glad I finally uncovered a big security hole :-)
Comment 17•23 years ago
|
||
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.... :)
Summary: trys to save .exe file rather than play it → tries to save .exe file rather than play it
Assignee | ||
Comment 18•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Implement my suggestion #3. This needs a security check-over (Mitch?).
Attachment #69867 -
Attachment is obsolete: true
Comment 21•23 years ago
|
||
Updated•23 years ago
|
Keywords: mozilla0.9.9,
privacy
Comment 22•23 years ago
|
||
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).
Assignee | ||
Comment 23•23 years ago
|
||
>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•23 years ago
|
||
> 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.
Assignee | ||
Comment 25•23 years ago
|
||
>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•23 years ago
|
||
> 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?
Assignee | ||
Comment 27•23 years ago
|
||
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.
Reporter | ||
Comment 28•23 years ago
|
||
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•23 years ago
|
||
Nope. If there were changes that tried to fix this bug there would be reviews
and such in here....
Comment 30•23 years ago
|
||
*** Bug 127536 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 32•23 years ago
|
||
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).
Target Milestone: --- → mozilla1.0
Comment 33•23 years ago
|
||
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...
Attachment #70087 -
Attachment is obsolete: true
Attachment #70087 -
Flags: needs-work+
Updated•23 years ago
|
Attachment #70088 -
Attachment is obsolete: true
Attachment #70088 -
Flags: needs-work+
Assignee | ||
Comment 34•23 years ago
|
||
I'll take it and trust I can consult with you as needed; thanks.
Comment 35•23 years ago
|
||
Yeah. I'm certainly up for that. :)
Updated•23 years ago
|
Whiteboard: [adt1]
Reporter | ||
Comment 37•23 years ago
|
||
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
Assignee | ||
Comment 38•23 years ago
|
||
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?
Reporter | ||
Comment 39•23 years ago
|
||
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•23 years ago
|
||
*** Bug 136328 has been marked as a duplicate of this bug. ***
Comment 41•23 years ago
|
||
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•23 years ago
|
||
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)
Assignee | ||
Comment 43•23 years ago
|
||
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.
Whiteboard: [adt1] → [adt1] ETA 4/16
Assignee | ||
Comment 44•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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....
Assignee | ||
Comment 47•23 years ago
|
||
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.
Reporter | ||
Comment 48•23 years ago
|
||
Is this in a nightly build yet so I can test?
Comment 49•23 years ago
|
||
> 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.
Reporter | ||
Comment 50•23 years ago
|
||
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.
Reporter | ||
Comment 51•23 years ago
|
||
But
http://hurl.content.loudeye.com/scripts/hurl.exe?clipid=009610501050706550&cid=466568
launches windows media player and works.
Assignee | ||
Comment 52•23 years ago
|
||
>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•23 years ago
|
||
> 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•23 years ago
|
||
> 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"
Assignee | ||
Comment 55•23 years ago
|
||
>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.
Whiteboard: [adt1] ETA 4/17 → [adt1] ETA 4/18
Assignee | ||
Comment 56•23 years ago
|
||
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•23 years ago
|
||
> 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.
Assignee | ||
Comment 58•23 years ago
|
||
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.
Assignee | ||
Comment 59•23 years ago
|
||
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.
Attachment #79377 -
Attachment is obsolete: true
Comment 60•23 years ago
|
||
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. :)
Attachment #79672 -
Flags: review+
Assignee | ||
Comment 61•23 years ago
|
||
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•23 years ago
|
||
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!
Attachment #79672 -
Flags: superreview+
Comment 63•23 years ago
|
||
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 65•23 years ago
|
||
adding adt1.0.0+. Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.
Comment on attachment 79672 [details] [diff] [review]
final revision
a=shaver for 1.0 branch.
Attachment #79672 -
Flags: approval+
Comment 67•23 years ago
|
||
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•23 years ago
|
||
Bill or Samir, could you check this into the branch as soon as possible?
Updated•23 years ago
|
Whiteboard: [adt1] ETA 4/18 → [adt1] [ETA 4/26]
Updated•23 years ago
|
Priority: -- → P1
Assignee | ||
Comment 69•23 years ago
|
||
fix was checked in on branch last week
Keywords: fixed1.0.0
Whiteboard: [adt1] [ETA 4/26] → [adt1]
Comment 70•23 years ago
|
||
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.
Keywords: pp,
verified1.0.0
Assignee | ||
Comment 71•23 years ago
|
||
marking fixed
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 72•23 years ago
|
||
It works on the 4/28 build. The 4/29 and 4/30 builds do not ever start for me.
Comment 73•23 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 74•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
Updated•23 years ago
|
Keywords: verified1.0.0
Comment 77•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Can a single extension map to multiple MIME types on Windows?
Comment 81•23 years ago
|
||
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•23 years ago
|
||
should we just continue this current mac issue in bug 141330?
Assignee | ||
Comment 83•23 years ago
|
||
re-resolving as FIXED; the change has been undone on the Mac per bug 141330.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 84•23 years ago
|
||
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!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 85•23 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 86•23 years ago
|
||
Sairuh... This bug was reopened a few times. Is it still fixed on the branch?
Need bug Verified on Mac as well
Comment 87•23 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: verified1.0.0
Comment 88•22 years ago
|
||
*** Bug 144629 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 89•22 years ago
|
||
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•22 years ago
|
||
That's actually bug 120327.
Reporter | ||
Comment 91•22 years ago
|
||
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!
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 92•22 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED
Updated•22 years ago
|
Keywords: fixed1.0.0
Comment 94•22 years ago
|
||
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•22 years ago
|
||
> 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.
Reporter | ||
Comment 96•22 years ago
|
||
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.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•