Closed
Bug 55406
Opened 24 years ago
Closed 23 years ago
AppendRelativePath broken [FIX][REVIEW]
Categories
(Core :: XPCOM, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla0.9.4
People
(Reporter: jst, Assigned: pete)
References
Details
Attachments
(6 files)
1.18 KB,
patch
|
Details | Diff | Splinter Review | |
1.59 KB,
patch
|
Details | Diff | Splinter Review | |
1.77 KB,
patch
|
Details | Diff | Splinter Review | |
1.79 KB,
patch
|
Details | Diff | Splinter Review | |
1.78 KB,
patch
|
Details | Diff | Splinter Review | |
1.79 KB,
patch
|
Details | Diff | Splinter Review |
The check in nsLocalFileUnix::AppendRelativePath() doesn't recognize paths such as 'some/dir.../../../../etc/passwd' as invalid paths, it only checks to see if the first occurance of '..' in the path realy is the directory '..'. Same thing needs to happen in the Windows and possibly the Mac version of nsLocalFile.
Comment 1•24 years ago
|
||
Wait, why is that an invalid path? [shaver@wasabi dir]$ ls -l `pwd`/../../../../../../../../etc/passwd -rw-r--r-- 1 root root 946 Sep 5 13:29 /tmp/some/dir/../../../../../../../../etc/passwd Are we concerned that it's just using ``..'' to mean ``remove previous path component'', rather than checking that it's a real path component? ``/dir/file/../../etc/passwd'' should be invalid, as should ``/dir/does-not-exist/../../etc/passwd'', but I'm not sure those are the cases you're talking about.
Comment 4•24 years ago
|
||
Hey, I just fixed what was there, which comments claimed was intended to prevent .. components, but which instead just ruled out any relative pathname containing .. anywhere in it. Anyone know the reason for insisting on downward-only relative pathnames? /be
From nsILocalFile.idl: * @param relativeFilePath * relativeFilePath is a native relative path. For security reasons, * this cannot contain .. or cannot start with a directory separator For security reasons? I don't get it. What is untrusted code doing manipulating stuff via nsI{,Local}File? (And why can only nsILocalFile do a relative append, rather than all nsIFiles? I'm curious.)
Updated•24 years ago
|
Keywords: mozilla1.0
Target Milestone: --- → mozilla0.9
Comment 6•24 years ago
|
||
I don't get the security aspects of this either. With initWithPath, which can give you access to just about anything, why have such a restricted appendRelativePath? It just makes me have to do path parsing in the XP filepicker to successfully go to ../../foo, something which I think is better left to nsILocalFile's implementation, which knows about paths (because it can ask the OS to resolve the path), and could even have a security restriction check on the resolved path...
Comment 7•24 years ago
|
||
I don't buy this restriction either. Without chroot, who are we kidding? This isn't the layer to put security-driven pathname checking, anyway. I retract the patch (which got entangled with my patch to bug 57995). I'll come up with a fresh one that yanks all ".." check attempts. /be
Comment 9•23 years ago
|
||
The patch attached here is the wrong thing, we think. Without a new patch in hand, and with my laptop (where my working trees live) dying at the moment, I have no choice but to move this out. Someone less lame than me, feel free to steal this bug! /be
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Updated•23 years ago
|
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Comment 10•23 years ago
|
||
Cc'ing pete, who has been doing good work in nsLocalFileUnix.cpp lately -- mebbe he'll take this one from me. /be
Target Milestone: mozilla0.9.3 → mozilla0.9.5
Assignee | ||
Comment 11•23 years ago
|
||
Sure, asasign it to me Brendan, i have another appendRelativePath bug i wanty to fix for 0.9.4 perhaps i'll get to this one as well. Thanks --pete
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•23 years ago
|
||
Assignee | ||
Comment 14•23 years ago
|
||
Brendan, why can't we implement it like this? Works nicely for me . . . NS_IMETHODIMP nsLocalFile::AppendRelativePath(const char *fragment) { CHECK_mPath(); NS_ENSURE_ARG(fragment); if(strlen(fragment)==0) return NS_OK; // No leading '/' and no ".." component is allowed in the fragment. if (*fragment == '/') return NS_ERROR_FILE_UNRECOGNIZED_PATH; nsCAutoString newPath(mPath); if(fragment[0]!='/') newPath.Append('/'); newPath.Append(fragment); mPath.Adopt(nsCRT::strdup((const char *)newPath)); InvalidateCache(); return NS_OK; }
Assignee | ||
Comment 15•23 years ago
|
||
js> f=new JS_FS_File_Path('/usr/local/www/'); [xpconnect wrapped nsILocalFile @ 0x81b0e80] js> f.path; /usr/local/www js> f.appendRelativePath('../../../etc/passwd'); js> f.path; /usr/local/www/../../../etc/passwd js> f.exists(); true js>
Comment 16•23 years ago
|
||
> // No leading '/' and no ".." component is allowed in the fragment. > if (*fragment == '/') > return NS_ERROR_FILE_UNRECOGNIZED_PATH; > > nsCAutoString newPath(mPath); > if(fragment[0]!='/') > newPath.Append('/'); That |if| is completely unnecessary, given that you already bailed earlier if the first character actually was a '/'. > newPath.Append(fragment); > > mPath.Adopt(nsCRT::strdup((const char *)newPath)); > InvalidateCache(); So how about this instead: // No leading '/' is allowed in the fragment. if (*fragment == '/') return NS_ERROR_FILE_UNRECOGNIZED_PATH; mPath.Adopt(ToNewCString(mPath + NS_LITERAL_CSTRING("/") + nsDependentCString(fragment))); InvalidateCache(); ?
Assignee | ||
Comment 17•23 years ago
|
||
Assignee | ||
Comment 18•23 years ago
|
||
I like it, even less lines of code. ;-) I actually don't know these string interfaces all that well so that's why i couldn't be as efficient . . . NS_IMETHODIMP nsLocalFile::AppendRelativePath(const char *fragment) { CHECK_mPath(); NS_ENSURE_ARG(fragment); if(strlen(fragment)==0) return NS_OK; // No leading '/' if (*fragment == '/') return NS_ERROR_FILE_UNRECOGNIZED_PATH; mPath.Adopt(ToNewCString(mPath + NS_LITERAL_CSTRING("/") + nsDependentCString(fragment))); InvalidateCache(); return NS_OK; }
Assignee | ||
Updated•23 years ago
|
Summary: nsLocalFileUnix::AppendRelativePath() allows 'some/dir.../../../../etc/passwd' paths → AppendRelativePath broken [FIX]{REVIEW]
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Assignee | ||
Updated•23 years ago
|
Summary: AppendRelativePath broken [FIX]{REVIEW] → AppendRelativePath broken [FIX][REVIEW]
Comment 19•23 years ago
|
||
if(strlen(fragment)==0) return NS_OK; Prevailing style uses space on each side of == and other binary operators, but more important: don't call strlen(foo) just to compare to 0, when *foo == '\0' is faster and just as clear. /be
Comment 20•23 years ago
|
||
Also, 'if (' is local style, not 'if('. /be
Assignee | ||
Comment 21•23 years ago
|
||
Comment 22•23 years ago
|
||
How does one check for memory allocation failure (from Adopt, I guess)? Do that, return NS_ERROR_OUT_OF_MEMORY if a malloc failed, and I'll r/sr= this fix. /be
Assignee | ||
Comment 23•23 years ago
|
||
Comment 24•23 years ago
|
||
I guess that's one way to do it. The other way would be to temporarily store the result of ToNewCString in a char* and then adopt that after the test. I think I like this way better, though this will soon (need) to be written as: if (!mPath.get()) as part of the ongoing "remove implicit conversion to const CharT*" efforts, so you might as well start now :-)
Assignee | ||
Comment 25•23 years ago
|
||
Assignee | ||
Comment 26•23 years ago
|
||
I filed a general cleanup bug for nsLocalFileUnix.cpp if anyone is interested in tracking it. http://bugzilla.mozilla.org/show_bug.cgi?id=92569 --pete
Assignee | ||
Comment 27•23 years ago
|
||
Jag, do i have an r= on this one? I still want to check this in even though AppendRelativePath will probably go away so we can close out this bug. Thanks --pete
Comment 28•23 years ago
|
||
Yeah, r=jag. (Just wondering what's better, the explicit check against '\0', or the idiom !*fragment)
Comment 29•23 years ago
|
||
sr=brendan@mozilla.org, nice work -- but are we gonna move to strike this entire method as redundant, in the nsIFile.idl cleanup? /be
Assignee | ||
Updated•23 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 30•23 years ago
|
||
Checked in. Jag, in answer to `if (!*fragment)' i think it is a matter of: "six in one, half a dozen in the other" --pete
Comment 31•23 years ago
|
||
jag: notice how *fragment == '\0' matches the form of the very next if condition (*fragment == '/'). It's a little clearer, too, IMHO -- at a glance you know that fragment should be a char* or const char*. /be
Comment 32•23 years ago
|
||
Good points.
Comment 33•23 years ago
|
||
*** Bug 94935 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•