Closed Bug 55406 Opened 25 years ago Closed 24 years ago

AppendRelativePath broken [FIX][REVIEW]

Categories

(Core :: XPCOM, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla0.9.4

People

(Reporter: jst, Assigned: pete)

References

Details

Attachments

(6 files)

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.
Need r= and a= (from blizzard). Cc'ing people. /be
Status: NEW → ASSIGNED
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.
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.)
Keywords: mozilla1.0
Target Milestone: --- → mozilla0.9
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...
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
Grumble. /be
Target Milestone: mozilla0.9 → mozilla0.9.1
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
Target Milestone: mozilla0.9.2 → mozilla0.9.3
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
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
Thanks! /be
Assignee: brendan → petejc
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch proposed fixSplinter Review
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; }
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>
> // 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(); ?
Attached patch updated patchSplinter Review
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; }
Keywords: mozilla1.0patch, review
Summary: nsLocalFileUnix::AppendRelativePath() allows 'some/dir.../../../../etc/passwd' paths → AppendRelativePath broken [FIX]{REVIEW]
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Summary: AppendRelativePath broken [FIX]{REVIEW] → AppendRelativePath broken [FIX][REVIEW]
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
Also, 'if (' is local style, not 'if('. /be
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
Blocks: 58481
Attached patch hrmm, like this?Splinter Review
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 :-)
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
No longer blocks: 58481
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
Yeah, r=jag. (Just wondering what's better, the explicit check against '\0', or the idiom !*fragment)
sr=brendan@mozilla.org, nice work -- but are we gonna move to strike this entire method as redundant, in the nsIFile.idl cleanup? /be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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
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
Good points.
*** 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.

Attachment

General

Created:
Updated:
Size: