Status: VERIFIED FIXED [sg:fix] fixed-aviary1.0.1, fixed1.7.6 Core Graveyard Graveyard GFX: Win32 (show other bugs) Trunk x86 Windows ME -- normal with 3 votes (vote) --- Doug Turner (:dougt) Hixie (not reading bugmail) sg-ff101 sg-tb101 sg-moz176 Shortcut Show dependency tree / graph

Reported: 2004-11-25 07:16 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2009-01-22 10:17 PST (History)
16 users (show)
caillon: blocking1.7.6+
caillon: blocking‑aviary1.0.1+
QA Whiteboard:
Iteration: ---
Points: ---

Attachments
patch v.1 (708 bytes, patch)
2005-02-07 12:54 PST, Doug Turner (:dougt)
bryner: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.1+
dveditz: approval1.7.6+
dveditz: approval1.8b+
Details | Diff | Splinter Review

 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-11-25 07:16:29 PST 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...]. Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-11-25 07:36:14 PST I tested on WinMe. Daniel Veditz [:dveditz] 2004-12-01 08:55:58 PST 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. Darin Fisher 2004-12-01 10:24:40 PST > 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. Boris Zbarsky [:bz] 2004-12-01 10:39:18 PST 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? timeless 2004-12-01 15:08:30 PST 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... Boris Zbarsky [:bz] 2004-12-01 18:00:51 PST We certainly need to cc people familiar with file stuff on windows if they're not already cced. Brodie 2004-12-02 01:51:31 PST 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  neil@parkwaycc.co.uk 2004-12-02 04:56:45 PST I tried this on Windows 2000 and Windows 95. I downloaded test.pif twice on each platform. At no time was command.com overwritten. Brodie 2004-12-02 11:30:24 PST 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. Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-02 17:16:06 PST 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. Brodie 2004-12-03 04:06:28 PST 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. Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-03 07:32:38 PST 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. Brodie 2004-12-03 07:36:47 PST 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. Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-03 07:48:42 PST > 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. neil@parkwaycc.co.uk 2004-12-03 08:13:49 PST 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. neil@parkwaycc.co.uk 2004-12-03 08:25:29 PST I tried backing my change out but it doesn't seem to affect this bug. Brodie 2004-12-03 09:01:27 PST 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? Brodie 2004-12-10 08:18:33 PST 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? neil@parkwaycc.co.uk 2004-12-10 08:54:11 PST That was bug 251651, but I never got around to discussing the solution... Ere Maijala (slow) 2004-12-10 12:01:55 PST 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. Dean Tessman 2004-12-10 12:46:55 PST (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? neil@parkwaycc.co.uk 2004-12-10 16:37:03 PST (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? Dean Tessman 2004-12-10 18:10:27 PST 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. Ere Maijala (slow) 2004-12-11 00:02:21 PST I was testing save dialog, which is the culprit here, right? Open dialog might or Mozilla after getting the result might act differently. neil@parkwaycc.co.uk 2004-12-11 05:09:53 PST 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 Brodie 2004-12-13 02:24:27 PST 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. Doug Turner (:dougt) 2005-02-02 13:17:40 PST 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?  Brodie 2005-02-02 13:31:01 PST 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 Doug Turner (:dougt) 2005-02-02 13:43:08 PST 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?  Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-04 13:16:38 PST 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? Doug Turner (:dougt) 2005-02-04 17:17:36 PST 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. Doug Turner (:dougt) 2005-02-07 12:54:27 PST Created attachment 173667 [details] [diff] [review] 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. Daniel Veditz [:dveditz] 2005-02-07 13:15:04 PST Comment on attachment 173667 [details] [diff] [review] patch v.1 sr=dveditz Christian :Biesinger (don't email me, ping me on IRC) 2005-02-07 13:44:24 PST does this mean .lnk files pointing to directories no longer work in the dialog? Doug Turner (:dougt) 2005-02-07 15:20:34 PST 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. Brodie 2005-02-07 15:28:37 PST Doug: what are the results of the testcases from comment #26 with your patch? Doug Turner (:dougt) 2005-02-07 15:38:29 PST 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".  Brodie 2005-02-07 15:49:04 PST 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. Doug Turner (:dougt) 2005-02-07 16:05:07 PST 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.  Daniel Veditz [:dveditz] 2005-02-11 13:44:26 PST Comment on attachment 173667 [details] [diff] [review] patch v.1 a=dveditz for 1.7 and aviary1.0.1 branches Daniel Veditz [:dveditz] 2005-02-11 14:04:34 PST Comment on attachment 173667 [details] [diff] [review] patch v.1 a=dveditz for 1.8b too Doug Turner (:dougt) 2005-02-11 15:34:46 PST fixed on branches and trunk. sairuh (rarely reading bugmail) 2005-02-18 11:03:55 PST 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/ Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-02-18 22:55:34 PST Yes, looks good on latest builds. sairuh (rarely reading bugmail) 2005-02-22 11:46:41 PST thank you for checking! verifying per reporter. Jerry Baker 2005-03-12 10:03:03 PST This fix *DID* break the ability to browse shortcuts to other directories. See bug 283730. Jerry Baker 2005-03-19 08:18:45 PST 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? Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-03-19 08:36:01 PST 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. Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-03-19 08:43:44 PST I think that many users don't know the file is system file or not. Scott MacGregor 2005-03-25 11:33:05 PST I think this caused Bug #287717

 Note You need to log in before you can comment on or make changes to this bug.