271732 - COMMAND.COM is overwritten by downloading the pif file

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041124
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041124

On Win9x, COMMAND.COM is overwritten by downloading the pif file.
The pif file is the shortcut of MS-DOS prompt.
(That is in Accessaries folder of Start menu of Win9x)

Reproducible: Always
Steps to Reproduce:
Server Side
1. Copy to any folder the shortcut of MS-DOS prompt.
2. Upload the shortcut to web server.

I'm uploaded the file, here.
http://www.d-toybox.com/mozilla/testpages/bug4103/test.pif

Client Side
1. Show Context Menu on the above link.
2. Select [Save link target as...]
3. Select file path.
Actual Results:  
Mozilla & Firefox will overwrite 'COMMAND.COM'.

Expected Results:  
Mozilla & Firefox should be save to selected path.

It is not reproduced by left click the link and saving.
It is reproduced only using [Save link target as...].

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I tested on WinMe.

Comment 2

[Daniel Veditz [:dveditz]]

Anyone know why save-as would be different from a unknown-type handler save?
Darin (and Johnny?) recently changed something here to fix a different bug where
one trusted the MIME type and the other went with the extension.

But this sounds more like we're cracking open the pif and treating it as a link.

Comment 3

[Darin Fisher]

> Anyone know why save-as would be different from a unknown-type handler save?

They use different code-paths for sure.  In the save-as case, we know that the
file is going to be saved (see contentAreaUtils.js), so we don't need to route
it through the URI loader, etc.  Instead, we do what we need to do to figure out
where to save it, and then we use nsIWebBrowserPersist to actually save it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Odd.  This works fine on Linux, and for save link as all we'd be doing is
grabbing the raw data and saving it in the persistence object, I would think...

Could it be that if the .pif already exists trying to open the relevant file
traverses through the pif on windows?

Comment 5

[timeless]

it would, pifs are treated like lnks, they're shortcuts. i'd like to cc two
people who've been playing w/ the lnk resolver lately if that's ok w/ people...

Comment 6

[Boris Zbarsky [:bzbarsky]]

We certainly need to cc people familiar with file stuff on windows if they're
not already cced.

Comment 7

[Brodie]

Unfortunately I don't have access to a Win9x box to test this at the moment. The
win32 nsILocalFile implementation explicitly resolves only *.lnk files as
shortcuts. (1)

Does the client step "3. Select file path" require selecting any particular file
path for download? i.e. the path to download to must be the path with
command.com in it, or the path with an existing test.pif file in it?


(1) 
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileWin.cpp#289
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileWin.cpp#525

Comment 8

[[e-mail]]

I tried this on Windows 2000 and Windows 95. I downloaded test.pif twice on each
platform. At no time was command.com overwritten.

Comment 9

[Brodie]

I've just tested on Win98 SE, Firefox 1.0. Downloaded test.pif using "Save link
as..." and left clicking (using the link in this bug report). Downloaded to c:\,
c:\windows and desktop, but I can't reproduce the problem.

Masayuki, would you please provide more detailed instructions for how to
reproduce this problem.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Realy?
I tested on WinMe. And original reporter in bugzilla-jp tested on WinMe too.
This problem is on WinMe Only??

Win 95   OK
Win 98   not tested
Win 98SE OK
Win ME   NG
Win 2k   OK

I don't have more detail.
Because on WinMe, I can reproduce with the steps of first comment.

Comment 11

[Brodie]

Just tested on a fresh install of WinME (English). No patches from Windows
Update had been installed. Download of test.pif using Firefox 1.0, both left
click and "save link as" were tested. 

I can't duplicate this problem. I can't see how this problem could occur. You
will need to provide more detailed steps to reproduce this. I wonder if it is
due to some way you have your WinME machine configured.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

We(bugizlla-jp) tested this bug.

Win95    not tested
Win98    OK
Win98 SE NG
WinMe    NG
Win2k    OK

Umm... more detail?

My WinMe system is on VMWare and without patchs.
That is installed c:\windows(default path) on virtual machine.
Mozilla & Firefox1.0 are installed each default install path.

Comment 13

[Brodie]

Both WinME and Win98SE for me are running under MS Virtual PC (similar to
VMware). I can't duplicate the problem with these instructions. 

From comment #11:
"You will need to provide more detailed steps to reproduce this."

Otherwise WORKSFORME.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Otherwise WORKSFORME.

wait a moment.
This problem is reproduced on the system of 3 people(includeing me).

http://www.d-toybox.com/mozilla/testpages/bug4103/test.pif
1. Move to cursor above link.
2. Click right button of the mouse.
3. Select [Save Link Target As...].
4. Select saving path(anywhere).
5. We can see the worning dialog of overwrite to command.com.

Comment 15

[[e-mail]]

I just realized that my build was patched to fix a similar bug. The patch to
widget/src/windows/nsFilePicker.cpp is this:
-    ofn.Flags = OFN_NOCHANGEDIR | OFN_SHAREAWARE | OFN_LONGNAMES |
OFN_OVERWRITEPROMPT | OFN_HIDEREADONLY | OFN_PATHMUSTEXIST;
+    ofn.Flags = OFN_NOCHANGEDIR | OFN_SHAREAWARE | OFN_LONGNAMES |
OFN_OVERWRITEPROMPT | OFN_HIDEREADONLY | OFN_PATHMUSTEXIST | OFN_NODEREFERENCELINKS;
The point of this extra flag is to stop Windows from downloading the target of a
.url file but it might well be applying here too.

Comment 16

[[e-mail]]

I tried backing my change out but it doesn't seem to affect this bug.

Comment 17

[Brodie]

Okay. I finally reproduced it. The steps to reproduce are the same as comment
#14 with the exception of step 4. The directory that you are downloading
test.pif to must already have a copy of test.pif in it.

The newly downloaded file is dereferencing the file refered to by the existing
pif file. Thus, if the existing pif file points to c:\windows\command.com, and
that file exists, then it will ask if you want to overwrite that file. If the
existing pif file points to "c:\foo\bar.txt" and that file doesn't exist, you'll
get a "The above file name is invalid" error.

Duplicated with Firefox 1.0 and Suite 1.8a5 on Windows ME. All versions are English.

I don't have a dev environment for Moz at the moment. Neil, would you try
reproducing with your unpatched source first, and then try the patch and see if
it fixes it?

Comment 18

[Brodie]

I just tested the change that Neil suggested and found that it does fix this bug
for me (latest nightly Mozilla + Windows 98 and Windows ME). 

Neil - when do you expect to check this change in for your other bug?

Comment 19

[[e-mail]]

That was bug 251651, but I never got around to discussing the solution...

Comment 20

[Ere Maijala (slow)]

The problem with OFN_NODEREFERENCELINKS is that it causes the save dialog to not
follow shortcuts (.lnk files) to other directories either. Is the same problem
present with a .lnk file? I don't know if there's a way to make the dialog not
dereference only links to files.

Comment 21

[:deanis74]

(In reply to comment #20)
> I don't know if there's a way to make the dialog not dereference only links to
> files.

Neither do I.  Would we be able to manually hook into the dialog and do the
resolution ourselves, based on whether we're opening or saving?

Comment 22

[[e-mail]]

(In reply to comment #20)
>The problem with OFN_NODEREFERENCELINKS is that it causes the save dialog to
>not follow shortcuts (.lnk files) to other directories either. Is the same
>problem present with a .lnk file? I don't know if there's a way to make the
>dialog not dereference only links to files.

Without OFN_NODEFERERENCELINKS:
If I go to open web location, choose file, click on Favorites\MSN then IE
downloads MSN to a temporary file whose location is returned to the open web
location dialog.
If I go to Shortcut to C:\ then the display changes to C:\

With OFN_NODEFERERENCELINKS:
If I choose Favorites\MSN then Favorites/MSN.url is returned to the open web
location dialog.
If I go to Shortcut to C:\ then the display changes to C:\

Or am I missing something?

Comment 23

[:deanis74]

Does it still work if you make it a shortcut to a directory under C:, say
C:\Temp?  Sorry for not checking myself, but I'm on a new machine and don't have
a dev environment set up on it yet.

Comment 24

[Ere Maijala (slow)]

I was testing save dialog, which is the culprit here, right? Open dialog might
or Mozilla after getting the result might act differently.

Comment 25

[[e-mail]]

Ah, it's in fact links to files that cause the problem.
If I open a link to index.htm I get an error.
If I save to a link to index.htm it overwrites index.htm.lnk

Comment 26

[Brodie]

My test results. See http://jellycan.com/etc/test.html for the source page I
used. The .lnk file links to c:\temp. The .pif file links to
c:\windows\command.com. The .url file links to http://jellycan.com/etc/test.html

Results
=======
lnk: 
  can .lnk files be followed to a different directory
exists: 
  what happens when a file of the same name exists in the destination
  directory. If follow, then we follow the link from the file. If
  replace then we replace the link file. 
FF1: Firefox 1.0
IE6: 6.0.2800.1106 (latest version from Windows update)

Note: If a file of the same name doesn't exist, then in all cases we download to
that directory. All saves were done using "save link as". All OS are updated to
date using Windows Update.

.pif
====
FF1, W2K: lnk: possible. exists: replace.
FF1, ME/98: lnk: possible. exists: follow.
IE6, W2K/ME: lnk: possible. exists: replace.
IE6, 98: lnk: possible. exists: follow.

.lnk
====
FF1, W2K/ME/98: lnk: possible. exists: follow.
IE6, W2K/ME: lnk: NOT possible. exists: replace.
IE6, 98: lnk: possible. exists: follow.

.url
====
FF1, W2K: lnk: possible. exists: follow ("test.url" not valid)
FF1, ME: lnk: possible. exists: follow ("http://link" not valid)
FF1, 98: lnk: possible. exists: replace.
IE6, W2K/ME/98: lnk: possible. exists: replace.


The differences between OS platforms is interesting. In the .pif and .lnk cases
IE has differences between ME and 98 that I didn't expect. This may infer that
there is a way to prevent the following of links on ME but not on 98. I can't
see any details of this in the open/save dialog documentation though.

Note: I don't see this bug as a major security risk. The user needs to download
a pif file into the same directory as an existing pif file. Trying to exploit
that is shooting arrows at the moon.

Comment 27

[Doug Turner (:dougt)]

Would it be correct to summarize this problem as:  If you download a shortcut
(.pif, .lnk, .url) file from a website into the same location, the second
download attempt may result in the target of the shortcut being destroyed?

Comment 28

[Brodie]

Yes.

Download and save to disk of a file with the same name as an existing
lnk/pif/url file will on some OS overwrite the target of the link instead of the
source file. Only the OS/browser combinations with "exists: follow" in my test
cases exhibit the problem.

.pif = ME/98
.lnk = All
.url = W2K/ME

Comment 29

[Doug Turner (:dougt)]

The bad guy basically has a site that changes the mime type to something that
causes the unknown content handler to kick in and ask the user to save the
content to the desktop.  This happens twice, and the bad guy can put any file
anywhere on the users machine.  Sounds bad.

I think losing the ablity to follow shortcuts in our dialogs is equally as bad.

The simpliest thing to do is to ensure that you can not replace a shortcut via
the filepicker.  This follows as you can't replace a folder.

Thoughts?

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

Drivers think we need this bug fixed for the branches.  Plussing.  The fix
suggested in comment 29 sounds reasonable.  Doug, any chance you can come up
with a patch?

Comment 31

[Doug Turner (:dougt)]

i think I have something that might work. I need a win me box for testing.  I
will see if I can get a box.  if you have one, please let me know.

Comment 32

[Doug Turner (:dougt)]

Attachment: patch v.1

On XP, GetSaveFileName never returns the dereferenced selected item.  However,
in SE, GetSaveFileName does return the dereferenced selected item. This patch
will pass OFN_NODEREFERENCELINKS whenever we ask the user to select a place to
download a file into.  

So, if the user ever downloads a shortcut, it will end up exactly where the
user intented it to be.  The drawback is that if the user wanted to download
anything and selected an shortcut via this dialog, the download will ask the
user if they want to replace the shortcut (no dereferencing happens).  In this
case, the user would have to double click the shortcut to open it in the
dialog.

I think that this would be the desired behavior of a typical save-as dialog on
windows.

This patch only changes the saving dialog; this patch does not change the open
dialog.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 173667 [details] [diff] [review]
patch v.1

sr=dveditz

Comment 34

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

does this mean .lnk files pointing to directories no longer work in the dialog?

Comment 35

[Doug Turner (:dougt)]

the short answer is .lnk files will continue to work.  without this patch and on
98ish(*) machines, if you select a shortcut (.lnk) file in this dialog, the
downloaded file will be placed in the directory which the shortcut points to. 
As you can see, this might now work if the shortcut is pointing at a file.  

You can still double-click on shortcuts to folders in the dialog.

Comment 36

[Brodie]

Doug: what are the results of the testcases from comment #26 with your patch?

Comment 37

[Doug Turner (:dougt)]

only tested 98 se.  I verified that I could reproduce the problem using FF1.0. 
Then I made this one change, and tested again.  With this patch, i was unable to
have command.com be overwritten.

One thing I did observe was that without this patch, the file that was
downloaded and displayed in the Download Manager was "Command.com.  Whereas on
XP, this file's name was "test.pif".  After this patch was applied, the file
name that was downloaded was "command.com".  

Comment 38

[Brodie]

I see that this fix would prevent the following of the link files, however we
should ensure that the patch doesn't have side-effects other than this, e.g. as
noted in comment #20. How about testing all 3 file types on different operating
systems? Through those test case results we can see that there are different
results depending on the OS for this bug. The proposed fix should be tested
similarly to prove that it does what is desired before being labelled as OK.

Comment 39

[Doug Turner (:dougt)]

I think that based on my observations, there is an inconsistency in the WIN32
api.  This patch does fix that.  

Comment #20 is somewhat unclear.  I think neil is talking about open (not
saving) files in IE or something.  

i need help testing all of these different os configurations.  

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 173667 [details] [diff] [review]
patch v.1

a=dveditz for 1.7 and aviary1.0.1 branches

Comment 41

[Daniel Veditz [:dveditz]]

Comment on attachment 173667 [details] [diff] [review]
patch v.1

a=dveditz for 1.8b too

Comment 42

[Doug Turner (:dougt)]

fixed on branches and trunk.

Comment 43

[sairuh (rarely reading bugmail)]

Hello Masayuki,

Does this look fixed to you using a recent 1.8 nightly build on WinME?

ftp://mozilla.isc.org/pub/mozilla.org/mozilla/nightly/2005-02-17-14-trunk/

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, looks good on latest builds.

Comment 45

[sairuh (rarely reading bugmail)]

thank you for checking! verifying per reporter.

Comment 46

[Jerry Baker]

This fix *DID* break the ability to browse shortcuts to other directories. See
bug 283730.

Comment 47

[Jerry Baker]

On reading Microsoft's documentation of the Windows Common Dialog, it is
supposed to behave the way it does in bug 271732. The common dialog only returns
the name of the file or directory that a user chooses. It is then up to the
application to do whatever with that information. If the user chooses a link,
then the target of the link is returned. I see nothing wrong here.

I did some testing. When you try to overwrite a link file with a downloaded
file, it *does* then overwrite the target of the link, ***BUT*** it prompts you.
It says, "XXXXX ALREADY EXISTS. DO YOU WANT TO REPLACE IT?" The XXX it displays
is the name and path of the actual file it is going to replace. Where's the problem?

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think some user does not know the extension. Because the default Windows
setting is that the extension is hidden. So, if the user download "command.pif",
the user cannot understand it is system file or the selected file.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that many users don't know the file is system file or not.

Comment 50

[Scott MacGregor]

I think this caused Bug #287717