53152 - filenames containing '..' or ending w/ '.' stop the directory enumerator (can affect imported IE favorites)

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Johnny Stenback (:jst)]

If you browse a local directory with mozilla and the directory happens to
contain a file with '..' (two or more dots) in it, the directory enumerator
stops enumerating the files in the directory. This same problem was fixed by
Brendan in xpcom/io/nsLocalFileUnix.cpp, but the windows version of that file
still has the problem. The problem is on line 755 in nsLocalFileWin.cpp (in
nsLocalFile::AppendRelativePath(const char *node)).

This is a serious problem that has very confusing symptoms, I realized this when
I tried to attach a file to a bug with mozilla and mozilla wouldn't show my diff
file in the filepicker. The reason for that was that I happened to have a file
named 'leaks...' in the same directory where I had the diff file.

Comment 1

[Johnny Stenback (:jst)]

Seems like dougt has been all over this file, reassigning to him (Ray, I hope
you don't mind), and also nominating for beta3...

Comment 2

[Doug Turner (:dougt)]

email from Dan Christensen <[e-mail]>

if (!node || (*node == '/') || (strstr(node, "..") != nsnull) || (strchr(node,
'/') != nsnull))
and what I suggested (with a slight correction) is:

if(!node || (strchr(node,'/') != nsnull) || (strcmp(node,"..") == 0))

Comparing the two versions, I have simply replaced the (*node == '/') 
and (strchr(node,'/') != nsnull) with a single (strchr(node,'/') and 
then replaced the (strstr(node,"..") != nsnull) with the 
(strcmp(node,"..") == 0). How could this break anything that is 
currently working? The only case that my version discards that the first 
doesn't is the case where node = "..". My version allows several cases 
that the first does not but they are all instances where ".." occurs 
within a valid filename, as a ".." must somehow be accompanied with a 
"/" to cause problems.

Comment 3

[Doug Turner (:dougt)]

Attachment: patch to fix nsLocalFileWin

Comment 4

[Doug Turner (:dougt)]

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

Comment 5

[Jesse Ruderman]

Windows 98 accepts "..." for "two directories up", at least at the command 
prompt and in the Run dialog.  Does the patch check for this case?  Do other 
versions of Windows behave differently?

Nominating for rtm, because this breaks importing IE favorites if the name of a 
favorite contains "..." or ends with ".".

Comment 6

[selmer (gone)]

marking [rtm need info] since you are working on this.  Please get r= and sr= 
and move to [rtm+] so the PDT can decide whether to approve this.  Seems 
unlikely to get approval unless you can describe why this would happen to a lot 
of regular users.

Comment 7

[djk]

  I feel this is a rather big issue for Windows users.  It's possible that
several Favorites have ".." somewhere in them.  It's even more likely that
a Favorite ends with a ".".
  This bug makes too many of *my* Favorites disappear, please fix it! :-)

Comment 8

[Jesse Ruderman]

I had about 7 favorites like that where I had not changed the name from the 
page's title, so I think lots of users will encounter this bug.  (OTOH, I have 
a whole lot of unsorted favorites, so I could be overestimating.)  Nominating 
for relnotertm in case it doesn't get fixed for rtm.

Comment 9

[Gervase Markham [:gerv]]

Side note: Windows 95 also, I've just discovered, accepts ... for 2 directories 
up.

Gerv

Comment 10

[Jason Eager]

This needs R= and SR= *NOW* if you're EVER going to get this in the branch.

Trying to get Waterson's attention.

Comment 11

[selmer (gone)]

Doug, is the patch you attached the correct patch?  Any testing done on it?
Have you tried to get reviewers?

Adding Jud to CC.  We need some immediate love on this!

Comment 12

[timeless]

Old patch.
scc: is the syntax reasonable?
would someone review this patch?

Comment 13

[timeless]

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

Comment 14

[Doug Turner (:dougt)]

No.  This is not the correct patch.  There needs to be more work on this fix
based on the comments that I got from IRC & via the first review.

Comment 15

[Phil Peterson]

PDT marking [rtm-]. Files with ".." in the name seem rare, and if we can't agree
on the patch, we're out of time.

Comment 16

[timeless]

Files containing .. are not rare. They are actually quite common.

I'm sorry the current patch isn't complete or correct. Thanks dougt for your 
prompt reply, i'm sorry if my pestering brought this into the killing radar.

Comment 17

[djk]

As jst originally mentioned in this bug:
Why not use a similar fix as was made in xpcom/io/nsLocalFileUnix.cpp?

That fix has been in the tree for a while (Sept 18), and hasn't been backed out.
 I'd think that a similar approach in the xpcom/io/nsLocalFileWin.cpp would be
acceptable, even this late in the game.

Comment 18

[djk]

It looks like OS/2 may suffer from the same problem.  The code in
nsLocalFile::AppendRelativePath in nsLocalFileOS2.cpp looks too similar to that
in nsLocalFileWin.cpp.

Comment 19

[Jason Eager]

Adding to the mozilla0.9 radar, hoping we'll get a patch for this in soon.

Comment 20

[Michael Percy (obsolete email address)]

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

Comment 21

[timeless]

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

Comment 22

[Doug Turner (:dougt)]

Attachment: Untested patch to allow dots in a relative path appending. Based on the unix version of nsLocalFile.

Comment 23

[djk]

Could we *please* have this reviewed and checked in?

What about the OS/2 version of this file (nsLocalFileOS2.cpp)?  It appears to
have the same faulty logic as the Windows version...

Comment 24

[Doug Turner (:dougt)]

brendan, can you review patch 22648.  It is basically your unix patch with 
slight mods.

Comment 25

[Brendan Eich [:brendan]]

Argh.  The patch will allow appending foo..bar, and forbid foo/../bar (Unix
pathname component separator used for clarity), but it won't stop
foo..bar/../baz, because it calls strstr(fragment, "..") once only.

Why are we proscribing .. components in relative pathnames, anyway?  I think we
all agreed that "security" was not a good reason.  Doug, how about a patch that
rips out .. checks altogether?

/be

Comment 26

[Doug Turner (:dougt)]

that would be kewl with me.  When creating nsIFile's, buyer beware.  Patch
coming shortly.

Comment 27

[Jesse Ruderman]

AppendRelativePath is called in quite a few places.  Ccing mstoltz to see if he 
knows whether the .. check is important.

Comment 28

[Brendan Eich [:brendan]]

On Unix, there's no safety in excluding ..'s without expanding symlinks.  And
since you can't expand symlinks, verify that the pathname doesn't go up above
some controlling directory, and then actually *use* the pathname, in one atomic
operation, there is no safety in checking symlinks and ..'s. The only safe
course is chroot(2).

So I think there can be no convincing case for excluding ..'s.  Prove me wrong,
anyone.

/be

Comment 29

[hin-tak.leung]

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

Comment 30

[Doug Turner (:dougt)]

Attachment: Patch as promised. Removes .. check on windows.

Comment 31

[Brian Haskin (Janzert)]

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

Comment 32

[Stephan Niemz]

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

Comment 33

[Stephan Niemz]

Adding IE to summary to help finding this bug.

Comment 34

[Aleksander Adamowski]

53152
Build ID: 2001021820
Platform: win32-talkback
OS: Windows 2000 Professional

Still happens with this build.

Do we have to turn off the .. check on Windows?
Wouldn't it be sufficient to check for occurance of ../ on windows?
And why does the directory enumerator have to stop enumerating the whole directory?
Can't it just skip the suspicious entry and advance to the next file?
(with my favorites, there is one that has a .. in its name, but Mozilla ignores
it and all other entries following it, including some subfolders.)

Comment 35

[Mike Jaques]

Alek: patch hasn't been checked in yet, so of course it still occurs, and 
exactly as you have pointed out.

Dougt:  What is the status on getting the patch r/sr/a by some of the people 
here on the Cc: list of this bug, and barring any major problems, getting the 
patch into the tree?  Would be nice :o)

Comment 36

[timeless]

+    if (!node || (*node == '/') || (strchr(node, '/') != nsnull))
=>
+    if (!node || *node == '/' || strchr(node, '/'))

Comment 37

[Jesse Ruderman]

timeless: what's so evil about adding parentheses to make code easier to read?

Comment 38

[Mike Shaver (:shaver emeritus)]

Why not just

  if (!node || strchr(node, '/'))

?

If you want an explicit check for the first character for some reason (I can't
think of why), then at least do:

  if (!node || *node == '/' || strchr(node+1, '/'))

Jesse: not all parentheses make things clearer, and certainly
parens+explicit-comparison-to-nsnull muddies the waters here.

I vote for

  if (!node || strchr(node. '/'))

myself: short and sweet.

Comment 39

[timeless]

r=timeless for shaver's suggestion. I agree 100%
i wasn't actually trying to understand what the code did before, all i could 
tell was that i couldn't understand it, it's a gag reflex.

Comment 40

[Doug Turner (:dougt)]

can someone put an sr= on this for shaver's last suggestion.  be?  

Comment 41

[Alec Flett]

sr=alecf

Comment 42

[Brendan Eich [:brendan]]

r/sr=brendan, what shaver said.

/be

Comment 43

[Aleksander Adamowski]

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

Comment 44

[ajbu]

note:
>     // Cannot start with a / or have .. or have / anywhere
The comment is no longer accurate, the 'or have ..' part can be deleted.

It is r/sr now, would be *very* nice if this was checked in :)

Comment 45

[Doug Turner (:dougt)]

fix checked in.  thanks all.

Comment 46

[timeless]

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

Comment 47

[Jacek Piskozub]

WFM on WindowsME (2001070204 build). I do not verify only because this bug can
depend on some OS differences with WinNT.

Marking mostfreq.