55406 - AppendRelativePath broken [FIX][REVIEW]

Status: RESOLVED FIXED

Description

[Johnny Stenback (:jst)]

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

[Brendan Eich [:brendan]]

Created attachment 16264 [details] [diff] [review]
proposed fix for Unix

Comment 2

[Brendan Eich [:brendan]]

Need r= and a= (from blizzard).  Cc'ing people.

/be

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

[Brendan Eich [:brendan]]

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

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 6

[Peter ``jag'' Annema]

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

[Brendan Eich [:brendan]]

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 8

[Brendan Eich [:brendan]]

Grumble.

/be

Comment 9

[Brendan Eich [:brendan]]

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

Comment 10

[Brendan Eich [:brendan]]

Cc'ing pete, who has been doing good work in nsLocalFileUnix.cpp lately -- mebbe
he'll take this one from me.

/be

Comment 11

[Pete Collins (MDG)]

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

Comment 12

[Brendan Eich [:brendan]]

Thanks!

/be

Comment 13

[Pete Collins (MDG)]

Created attachment 43717 [details] [diff] [review]
proposed fix

Comment 14

[Pete Collins (MDG)]

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;
}

Comment 15

[Pete Collins (MDG)]

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

[jag (Peter Annema)]

> // 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();

?

Comment 17

[Pete Collins (MDG)]

Created attachment 43739 [details] [diff] [review]
updated patch

Comment 18

[Pete Collins (MDG)]

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;
}

Comment 19

[Brendan Eich [:brendan]]

 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

[Brendan Eich [:brendan]]

Also, 'if (' is local style, not 'if('.

/be

Comment 21

[Pete Collins (MDG)]

Created attachment 43752 [details] [diff] [review]
updated patch as to Brendan's comments

Comment 22

[Brendan Eich [:brendan]]

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

Comment 23

[Pete Collins (MDG)]

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

Comment 24

[jag (Peter Annema)]

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

Comment 25

[Pete Collins (MDG)]

Created attachment 43774 [details] [diff] [review]
patch w/ check for NS_ERROR_OUT_OF_MEMORY

Comment 26

[Pete Collins (MDG)]

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

Comment 27

[Pete Collins (MDG)]

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

[jag (Peter Annema)]

Yeah, r=jag.

(Just wondering what's better, the explicit check against '\0', or the idiom
!*fragment)

Comment 29

[Brendan Eich [:brendan]]

sr=[e-mail], nice work -- but are we gonna move to strike this entire
method as redundant, in the nsIFile.idl cleanup?

/be

Comment 30

[Pete Collins (MDG)]

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

[Brendan Eich [:brendan]]

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

[jag (Peter Annema)]

Good points.

Comment 33

[Christopher Aillon (sabbatical, not receiving bugmail)]

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