AppendRelativePath broken [FIX][REVIEW]

RESOLVED FIXED in mozilla0.9.4

Status

()

P3
normal
RESOLVED FIXED
18 years ago
15 years ago

People

(Reporter: jst, Assigned: pete)

Tracking

Trunk
mozilla0.9.4
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

18 years ago
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.
Created attachment 16264 [details] [diff] [review]
proposed fix for Unix
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.)

Updated

18 years ago
Keywords: mozilla1.0
Target Milestone: --- → mozilla0.9

Comment 6

18 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...
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

Updated

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

Comment 11

18 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
Thanks!

/be
Assignee: brendan → petejc
Status: ASSIGNED → NEW
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 13

18 years ago
Created attachment 43717 [details] [diff] [review]
proposed fix
(Assignee)

Comment 14

18 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

18 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

18 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

18 years ago
Created attachment 43739 [details] [diff] [review]
updated patch
(Assignee)

Comment 18

18 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

18 years ago
Keywords: mozilla1.0 → patch, review
Summary: nsLocalFileUnix::AppendRelativePath() allows 'some/dir.../../../../etc/passwd' paths → AppendRelativePath broken [FIX]{REVIEW]
Target Milestone: mozilla0.9.5 → mozilla0.9.4
(Assignee)

Updated

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

Comment 21

18 years ago
Created attachment 43752 [details] [diff] [review]
updated patch as to Brendan's comments
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)

Updated

18 years ago
Blocks: 58481
(Assignee)

Comment 23

18 years ago
Created attachment 43759 [details] [diff] [review]
hrmm, like this?

Comment 24

18 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

18 years ago
Created attachment 43774 [details] [diff] [review]
patch w/ check for NS_ERROR_OUT_OF_MEMORY
(Assignee)

Comment 26

18 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)

Updated

18 years ago
No longer blocks: 58481
(Assignee)

Comment 27

18 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

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

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 30

18 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

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

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