ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jaikiran Pai <jai.forums2...@gmail.com>
Subject Re: FileUtils.normalize/isLeadingPath have bitten me
Date Thu, 28 Jun 2018 14:48:58 GMT
I don't have enough background on the expectations of the normalize 
method, so can't really say how it should behave. Plus, it gets used in 
relatively larger number of places as compared to isLeadingPath, so not 
too sure how it might impact other places if we do change its existing 
semantics.

However, looking at the FileUtils#isLeadingPath(...) implementation, I 
wonder why it even uses normalize. Given that the goal of that API (as 
stated in the javadoc) is to figure out if one path leads the other, to 
me that translates to being a check to see whether the "leading" param 
to that method is a parent of the "path". I suppose that can be 
implemented by using the java.io.File#getCanonicalFile() call on each of 
the params and then doing a iterative getParent() call on the canonical 
File returned for the "path" and seeing if it ends up leading to the 
canonical File returned for "leading". The call to 
java.io.File#getCanonicalFile() should take care of the ".", ".." and 
even symbolic link reference resolutions (which I guess is a good 
thing?). Do you think this has merits? Or is the expectation of the 
isLeadingPath API solely based on the names of that passed files rather 
than their actual resolved location on the filesystem? I haven't yet 
tried it out myself to have a more clearer understanding of how it will 
end up behaving in the context that we use this API.

-Jaikiran


On 28/06/18 7:17 PM, Stefan Bodewig wrote:
> Hi all
>
> while looking into https://bz.apache.org/bugzilla/show_bug.cgi?id=62502
> I realized that I had a false expectation of what normalize would do in
> a certain edge case.
>
> If you feed into it a path with more "../" segments than can be
> travelled up, like
>
> FileUtils.normalize("/tmp/dest/../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../Temp/evil.txt")
>
> it will realize it would go outside of the file system root and returns
> a new File instance with the original path. I'm not sure what I had
> expected (an exception maybe) but this is now what I had assumed, my
> fault.
>
> Then isLeadingPath calls normalize for both its arguments and ends up
> seeing "/tmp/dest/" is a prefix of
> "/tmp/dest/../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../Temp/evil.txt"
> and unzip allows the file to escape.
>
> It seems to depend on the OS what it makes of the path, on Linux I
> receive an exception but apparently Windows just swallows the excess ../
> segments.
>
> I'm not 100% sure how to fix this properly.
>
> Is normalize doing the right thing? If so, we need to fix isLeadingPath
> for example by simply always returning false if either normalized path
> contains "../" segments (because that means the path cannot exist on
> disk at all).
>
> Or should the behavior of normalize change? This unit test snippet
>
>          assertEquals("will not go outside FS root (but will not throw an exception either)",
>                  new File(localize("/1/../../b")),
>                  FILE_UTILS.normalize(localize("/1/../../b")));
>
> makes me think we better leave it as is as it seems to be by design (and
> I just have forgotten about it).
>
> In either case, once we agree on the fix I propose to cut new releases
> immediately.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


Mime
View raw message