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

RESOLVED FIXED

Status

()

P1
major
RESOLVED FIXED
19 years ago
18 years ago

People

(Reporter: jst, Assigned: dougt)

Tracking

({relnote})

Trunk
x86
Windows NT
relnote
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-] relnote-user)

Attachments

(3 attachments)

(Reporter)

Description

19 years ago
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.
(Reporter)

Comment 1

19 years ago
Seems like dougt has been all over this file, reassigning to him (Ray, I hope
you don't mind), and also nominating for beta3...
Assignee: rayw → dougt
Keywords: correctness, nsbeta3
Priority: P3 → P1
(Assignee)

Comment 2

19 years ago
email from Dan Christensen <daniel.l.christensen@uwrf.edu>

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.
(Assignee)

Comment 4

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

Comment 5

19 years ago
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 ".".
Keywords: rtm

Updated

19 years ago
Whiteboard: [rtm need info]

Comment 6

19 years ago
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

19 years ago
  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

19 years ago
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.
Keywords: relnote, relnoteRTM
Side note: Windows 95 also, I've just discovered, accepts ... for 2 directories 
up.

Gerv
Whiteboard: [rtm need info] → [rtm need info] relnote-user

Updated

19 years ago
Summary: filenames containing '..' stop the directory enumerator... → Favorites containing ".." in them cause all other favorites to not be displayed! filenames containing '..' stop the directory enumerator...

Comment 10

19 years ago
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

19 years ago
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!

Updated

19 years ago
Summary: Favorites containing ".." in them cause all other favorites to not be displayed! filenames containing '..' stop the directory enumerator... → filenames containing '..' stop the directory enumerator (can affect imported favorites)

Comment 12

19 years ago
Old patch.
scc: is the syntax reasonable?
would someone review this patch?
Keywords: approval, patch, review

Comment 13

19 years ago
*** Bug 57071 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 14

19 years ago
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

19 years ago
PDT marking [rtm-]. Files with ".." in the name seem rare, and if we can't agree
on the patch, we're out of time.
Whiteboard: [rtm need info] relnote-user → [rtm-] relnote-user

Comment 16

19 years ago
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

19 years ago
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

19 years ago
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

19 years ago
Adding to the mozilla0.9 radar, hoping we'll get a patch for this in soon.
Keywords: mozilla0.9
*** Bug 62156 has been marked as a duplicate of this bug. ***

Comment 21

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

Comment 23

18 years ago
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...
(Assignee)

Comment 24

18 years ago
brendan, can you review patch 22648.  It is basically your unix patch with 
slight mods.
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
(Assignee)

Comment 26

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

Comment 27

18 years ago
AppendRelativePath is called in quite a few places.  Ccing mstoltz to see if he 
knows whether the .. check is important.
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

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

Updated

18 years ago
Summary: filenames containing '..' stop the directory enumerator (can affect imported favorites) → filenames containing '..' or ending w/ '.' stop the directory enumerator (can affect imported favorites)
*** Bug 69239 has been marked as a duplicate of this bug. ***

Comment 32

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

Comment 33

18 years ago
Adding IE to summary to help finding this bug.
Summary: filenames containing '..' or ending w/ '.' stop the directory enumerator (can affect imported favorites) → filenames containing '..' or ending w/ '.' stop the directory enumerator (can affect imported IE favorites)
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

18 years ago
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

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

Comment 37

18 years ago
timeless: what's so evil about adding parentheses to make code easier to read?
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

18 years ago
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.
(Assignee)

Comment 40

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

Comment 41

18 years ago
sr=alecf
r/sr=brendan, what shaver said.

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

Comment 44

18 years ago
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 :)
(Assignee)

Comment 45

18 years ago
fix checked in.  thanks all.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 46

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

Comment 47

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

Marking mostfreq.
Keywords: mostfreq
You need to log in before you can comment on or make changes to this bug.