210588 - Cancelling download of .lnk file deletes target of .lnk file

Status: RESOLVED FIXED

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

Description

[Murray Fox]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6

When viewing a page with a hyperlink that refers to a Windows shorcut file (a
.lnk extension), clicking on the link brings up the "Opening xxx.lnk" prompt.
Clicking cancel here deletes the target file of the shortcut.

Reproducible: Always

Steps to Reproduce:
1. Create file test.txt on windows system
2. Create shortcut to test.txt
3. Make a simple html page with a link to the shortcut file (sample below):

<a href="Shortcut to test.txt.lnk">CLICK</a>

(note: the above assumes you have 'view file extensions for known file types'
enabled, otherwise the file might be just "Shorcut to test.lnk").

4. Open the html document, click the link. The 'Open Shortcut to test.txt.lnk'
appears.
5. Click Cancel. The file test.txt (ie, the target of the shortcut) is deleted.
Actual Results:  
The target of the shortcut is deleted. This works accross drives / network drives.

Expected Results:  
Left the file intact.

At a guess I'd say that Mozilla might be considering the original file a temp
copy that it has downloaded, and by cancelling it is removing that temp.

Comment 1

[noririty]

It occurs on Mozilla too.

Comment 2

[Jesse Ruderman]

-> file handling

Comment 3

[noririty]

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

Comment 4

[Jesse Ruderman]

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

Comment 5

[noririty]

.

Comment 6

[Jesse Ruderman]

Confirming based on number of dups and nominating for blocking1.5b.  Deleting
files is bad.

Comment 7

[Asa Dotzler [:asa]]

Ben, this looks nasty. Can you take a look at it?

Comment 8

[Asa Dotzler [:asa]]

my concern with this bug is that it seems like an attacker could pretty easily
delete files with known paths. Users who were tricked into clicking a link would
likely cancel out of the "opening" prompt making it a rather easy attack.

Comment 9

[Jesse Ruderman]

Can this be triggered by a .lnk loaded over http?  I can't test because I can't
repro on WinXP.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

I can't reproduce this in either Firebird or Seamonkey on WinXP, which is all I have

Comment 11

[Benjamin Smedberg]

I was playing with this, and was able to get mozilla to delete a local file from
a remote "lnk" file, win2k, latest trunk build. This is a major DOS attack
possibility. The same problem occurs in 1.4, so I think this should block 1.4.1
as well.

Comment 12

[Benjamin Smedberg]

reassigning to roc, per timeless

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd prefer if you reassign to someone with a Windows box...

Comment 14

[Jesse Ruderman]

Is this a regression?  I suspect it is, because this bug was first reported less
than 2 months ago and there are 2 dups.

Comment 15

[Asa Dotzler [:asa]]

who can own this? darin? dougt? 

Comment 16

[Darin Fisher]

-> me

Comment 17

[Darin Fisher]

hmm... the UNIX and OSX impls of nsIFile::remove will never unlink the target of
a symlink or alias.  the OS/2 code, which is very similar to the WIN32 code,
uses mWorkingPath instead of mResolvedPath.  in fact, i can't think of a single
reason why nsIFile::remove should ever remove the target of a "symlink" instead
of the "symlink" itself.

the WIN32 code has always used the resolved path.  however, the resolved path
may not be the true resolved path if the local file is constructed to not follow
links.  my guess is that this bug (if it is a regression) was introduced by some
change in the download/save code that caused the nsIFile to be constructed with
the follow links flag set to true.

based on this info, i think the best solution to this bug is to just fix
nsLocalFile::Remove to use the unresolved path.

Comment 18

[Darin Fisher]

Attachment: v1 patch

here's what i think we want to do for this bug.  i have not fully tested it,
however.  if anyone has time to test this out, please do :-)

Comment 19

[Darin Fisher]

ok, i confirmed that 1) i can repro the bug and 2) this patch fixes things.

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 129894 [details] [diff] [review]
v1 patch

Although, i think we should just be honoring the followLinks attribute of the
nsILocalFile, the other platforms do not ever recursively delete the target.  

Too bad we can't flip the logic around to only test for a symlink if the
nsLocalFile is a directory.  

no need to create the temp variable pathLen:

 const char* leaf = mWorkingPath.get() + mWorkingPath.Length() - 4

The assumption that the mWorkingPath has all non-terminals resolved is bogus. 
You can have a path such as:

foo/shortcut.lnk/bar/another.lnk

working path would be the above raw string.

Comment 21

[Darin Fisher]

Attachment: v1.1 patch

improved patch based on dougt's comments.

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 130141 [details] [diff] [review]
v1.1 patch

fine with the things i mentioned over aim.

Comment 23

[Darin Fisher]

Attachment: v1.2 patch

better patch.  previously, IsSymlink would only match terminal nodes ending
with exactly ".lnk".  this needs to be a case _in_sensitive check.  i went
through the rest of nsLocalFileWin.cpp and found other places where this was
not done.  IsShortcut() is the only one that caused me trouble.  i think i need
to compeletely rewrite that function to make it sane.  the current impl is
bogus, but i think we can live with the current impl for now.  it doesn't
impact this bug.

Comment 24

[Darin Fisher]

Comment on attachment 130144 [details] [diff] [review]
v1.2 patch

i think this is an important, low-risk fix for 1.5 beta and the 1.4 branch.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 130144 [details] [diff] [review]
v1.2 patch

a=asa (on behalf of drivers) for checkin to 1.5beta

Comment 26

[Darin Fisher]

fixed-on-trunk

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 130144 [details] [diff] [review]
v1.2 patch

a=asa (on behalf of drivers) for checkin to the 1.4 branch. Darin, can you get
this in quickly for us? We're trying to wrap up on 1.4.1 ASAP. Thanks.

Comment 28

[Darin Fisher]

fixed1.4.1