AllowSymLinkAliasChecker not normalizing relative symlinks properly

[clauso]

We use the following code:

ResourceHandler fileResourceHandler = new ResourceHandler();
fileResourceHandler.setDirectoriesListed(false);
fileResourceHandler.setWelcomeFiles(new String[]{ "index.html" });
fileResourceHandler.setResourceBase("./resources/website");
fileResourceHandler.setEtags(true);

ContextHandler fileResourceContext = new ContextHandler();
fileResourceContext.setContextPath("/");
fileResourceContext.setAllowNullPathInfo(true);
fileResourceContext.setHandler(fileResourceHandler);

// Allow to follow symlinks
fileResourceContext.addAliasCheck(new AllowSymLinkAliasChecker());

Now we updated jetty9 to the from 9.2.2 to the latest release (9.3.9) and at the same time we moved to openjdk-8 (7 before). All of a sudden all symlinks do not work anymore. If I replace the symlink with the actual content, it works. If I replace the last line of code above with

fileResourceContext.addAliasCheck(new ContextHandler.ApproveAliases());

it works too (even though ApproveAliases seems too permissive for following symlinks).

So it seems like the AllowSymLinkAliasChecker does not work anymore. OS is Ubuntu xenial.

[joakime]

the use of setResourceBase() should either be a fully qualified path, or use the setBaseResource(Resource) option with a PathResource parameter you have created.

[clauso]

Replaced it with

String websiteResourcesPath = new File("./resources/website").getCanonicalPath();
fileResourceHandler.setResourceBase(websiteResourcesPath);

But that does not solve the problem.

[gregw]

Ignore issue #688. I created it from the email trail not realizing this was already an issue.

Greg Wilkins gregw@webtide.com CTO http://webtide.com

[gregw]

@clauso I can't reproduce your problem. In fact I have the opposite problem and that I can't get it to not allow the symlinks.

Note that currently ContextHandler is adding a AllowSymLinkAliasChecker by default if the file system separate character is '/'! I'm not sure why we started doing this and I'm no so keen on such conditional defaults.

Also, you should set the resourcebase on the context and not on the handler. The handler's resources are handled differently and the alias checkers of the context are not applied.

So currently I'm getting success with the code:

package org.eclipse.jetty.embedded;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.handler.AllowSymLinkAliasChecker;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.server.handler.ResourceHandler;

public class FileServer
{
    public static void main(String[] args) throws Exception
    {
        Server server = new Server(8080);

        ResourceHandler fileResourceHandler = new ResourceHandler();
        fileResourceHandler.setDirectoriesListed(true);
        fileResourceHandler.setWelcomeFiles(new String[]{ "index.html" });
        fileResourceHandler.setEtags(true);

        ContextHandler fileResourceContext = new ContextHandler();
        fileResourceContext.setContextPath("/");
        fileResourceContext.setAllowNullPathInfo(true);
        fileResourceContext.setHandler(fileResourceHandler);
        fileResourceContext.setResourceBase(".");

        fileResourceContext.clearAliasChecks();
        fileResourceContext.addAliasCheck(new AllowSymLinkAliasChecker());

        server.setHandler(fileResourceContext);
        server.start();
        server.join();
    }
}

[clauso]

Thanks for your help. Now things are getting very strange but we are closing in on the bug.

I tested with your code above and created the following subdirectories and files:

drwxrwxr-x 2 claus claus 4096 Jul  6 06:54 testdir
lrwxrwxrwx 1 claus claus    8 Jul  6 06:54 testdirlnk -> testdir/

and inside testdir I created

-rw-rw-r-- 1 claus claus  9 Jul  6 06:54 testfile.txt
lrwxrwxrwx 1 claus claus 12 Jul  6 06:54 testfilelnk.txt -> testfile.txt

Results are:

No.	Requested	Result	Evaluation
1	http://localhost:8080/testdir/	Dirlisting	OK-As-Expected
2	http://localhost:8080/testdirlnk/	Dirlisting	OK-As-Expected
3	http://localhost:8080/testdir/testfile.txt	File Content	OK-As-Expected
4	http://localhost:8080/testdir/testfilelnk.txt	File Content	OK-As-Expected
5	http://localhost:8080/testdirlnk/testfile.txt	404	Unexpected-Failure
6	http://localhost:8080/testdirlnk/testfilelnk.txt	File Content	OK-As-Expected - But unexpected regarding the last test case

Ok, test case 5 is a bug, as I would expect that if the linked dir is listed (case 2) I can also read a file in it that was accessible directly (case 3). Case 6 however is strange as it actually shows the content of the linked file in the linked dir, which is the expected behaviour but very inconsistent with case 5.

Case 5 was the case where I initially hit the bug.

[gregw]

I'm not seeing case 5 failing?? Can you run with debug on? Perhaps use the debugger and step through the AllowSymLinkAliasChecker and see why/where it is failing to approve the alias?

[clauso]

I am currently very short on time, I will try to look into this tonight (European time). Few questions:

• What jetty version are you using? (I am on 9.3.9)
• Are you on Linux? I am on Ubuntu xenial with a 4.4.0 kernel
• file system is ext4

BTW: On http://www.eclipse.org/jetty/index.html the "Downloads" Link in the quicklink box on the right does not point to the same page as the Download link in the navigation on the left. The download pages advertise different versions of jetty.

[gregw]

@clauso I'm testing on 9.3.11-SNAPSHOT on ubuntu 4.4.0-28-generic using ext4 and java version "1.8.0_77"

download pages are under repair

[gregw]

@WalkerWatch note the comment above about download links being different

[clauso]

OK, getting closer. Line 107 in AllowSymLinkAliasChecker says (On branch jetty-9.3.x, / 8a12915dacf03e039c470fa0e015096808edf9a1): https://github.com/eclipse/jetty.project/blob/7afc36064bdb74dceb2f98682e1970f8543cdef5/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java#L107-L112

if (pathResource.getAliasPath().equals(target))
{
    if (LOG.isDebugEnabled())
        LOG.debug("Allow path symlink {} --> {}",resource,target);
    return true;
}

This if-statement should be true. It's false however as target has an extra / added to the testdirlnk when it is resolved to testdir so the compared result is "[...]testdir//testfile.txt" which is compared to "[...]testdir/testfile.txt".

So the problem lies somewhere around these lines (starting at line 95) https://github.com/eclipse/jetty.project/blob/7afc36064bdb74dceb2f98682e1970f8543cdef5/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java#L95-L102

while (Files.exists(d) && Files.isSymbolicLink(d))
{
    Path link=Files.readSymbolicLink(d);    
    if (!link.isAbsolute())
        link=d.getParent().resolve(link);
    d=link;
    linked=true;
}

[clauso]

Actually the problem lies in line 92:

Path r=d.resolve(e);

where e contains testfile.txt and d is [...]testdir/. The resulting ris set to [...]testdir//testfile.txt. The question is: Is this a bug in nio or is this acceptable behavior and should jetty be able to deal with it? Or should the Path.equals(Object o) (line 107) method return true and accept that the double slash is still the same path?

[clauso]

I found this answer on stackoverflow: http://stackoverflow.com/a/29368388 Maybe a solution would be to replace line 107 with

if (java.nio.file.Files.isSameFile( pathResource.getAliasPath(), target)

But I am not really sure about what the purpose of AllowSymLinkAliasChecker is. What exactly is it checking and can you give an example case that should not pass the check? Therefore I can't tell if my proposed fix will work as expected.

Are there unit tests for this class?

[gregw]

@joakime Can you have a look at this, I think @clauso is onto something here

[joakime]

@clauso the problem seems to stem from how you are setting up your setBaseResource() - don't use java.io.File or .setResourceBase(String) as that leaves too many options open for interpretation of what that String means and how it should be processed.

We cannot use Files.isSameFile() as this is known problem on systems that have parent directory file permissions that don't allow the Java nio system to interrogate the FileSystem objects of those it does not have permission to.

I just checked in a testcase that replicates your example, showing that support for this alias checker is behaving, even in your example case.

See: https://github.com/eclipse/jetty.project/blob/jetty-9.3.x/jetty-server/src/test/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasCheckerTest.java

When this test executes, it creates a rootPath in the following directory ...

$ pwd
.../jetty.project/jetty-server/target/tests/test-AllowSymLinkAliasCheckerTest

$ tree
.
├── testdir
│   ├── testfilelnk.txt -> testfile.txt
│   └── testfile.txt
└── testdirlnk -> testdir

2 directories, 2 files

$ ls -la
drwxrwxr-x. 2 joakim joakim 4096 Jul  7 09:10 testdir
lrwxrwxrwx. 1 joakim joakim    7 Jul  7 09:10 testdirlnk -> testdir

$ ls -la testdirlnk/
lrwxrwxrwx. 1 joakim joakim   12 Jul  7 09:10 testfilelnk.txt -> testfile.txt
-rw-rw-r--. 1 joakim joakim   14 Jul  7 09:10 testfile.txt

[clauso]

Your test works here.

But, it cannot be transfered to @gregw example code, which I altered as follows:

/**
 * Test project for debugging
 * https://github.com/eclipse/jetty.project/issues/687
 */

package fileserver;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.handler.AllowSymLinkAliasChecker;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.server.handler.ResourceHandler;
import org.eclipse.jetty.util.resource.Resource;

public class FileServer
{
    public static void main(String[] args) throws Exception
    {
        Server server = new Server(8080);

        ResourceHandler fileResourceHandler = new ResourceHandler();
        fileResourceHandler.setDirectoriesListed(true);
        fileResourceHandler.setWelcomeFiles(new String[]{ "index.html" });
        fileResourceHandler.setEtags(true);

        ContextHandler fileResourceContext = new ContextHandler();
        fileResourceContext.setContextPath("/");
        fileResourceContext.setAllowNullPathInfo(true);
        fileResourceContext.setHandler(fileResourceHandler);

        fileResourceContext.setBaseResource(Resource.newResource("."));

        fileResourceContext.clearAliasChecks();
        fileResourceContext.addAliasCheck(new AllowSymLinkAliasChecker());

        server.setHandler(fileResourceContext);
        server.start();
        server.join();
    }
}

Now I use setBaseResource just as shown in the documentation. @gregw please note that in the doc the BaseResource is set on the Handler not the Context!

[joakime]

You are working with symlinks, you have to be precise in your Base Resource declaration.

Doing .setBaseResource(Resource.newResource(".")) is exactly equivalent to .setResourceBase(".") which is not precise enough.

Start with a fully qualified java.nio.file.Path, that is absolute, and canonical, then hand that to .setBaseResource(new PathResource(yourRootPath))

Don't rely on File.getCanonicalPath() as that doesn't do what you think it does (it performs filesystem queries to attempt to resolve to canonical, which does not always resolve mounts, symlinks, cross filesystem types, etc)

Use Path.toRealPath(), as that will do a much better job at working out the nuances of the Path.

We tried, in the past, to make the String form of .setResourceBase() automatically resolve to absolute, unwound any symlinks, and obtain a canonical path, but we had bugs filed against it, as many users wanted to work WITH the symlink, not the resolved link (allowing them to swap things in/out at runtime with new symlinks for example). So all we do now is test that the provided .setResoureBase() exists.

[joakime]

@WalkerWatch can you fix the documentation issue mentioned above by @clauso

[joakime]

@gregw in order to not mess things up in .setResourceBase(String) or Resource.newResource(String) again. How about this as a middle of the road warning approach.

If the provided String isn't a jar:// URL, or a ClassPath resource reference, it enters into the File system techniques (namely PathResource).

If the String isn't absolute, we resolve it with .toRealPath(), and toss a warning along the lines of ...

WARN: Resolving non-absolute '.' to '/home/user/path/to/mybase/' (To silence this warning, provide an absolute path to the base resource instead)

We can also go a step further, when we normalize the path (using Path.normalize()) we check the before/after to see if it changed, if it changed, then we toss a message.

WARN: Normalized path '/home/user/path/to/mybase' doesn't match provided path '/home/user/path/to/myproject/../mybase' (To silence this warning, provide a normalized path to the base resource instead)

We can also resolve the real path using Path.toRealPath() and if that path changes we can toss another warning.

WARN: Real path '/home/user/path/to/mybase' doesn't match provided path '/home/user/path/to/mybaselink' this will make all content in your base resource an alias of the real content (To silence this warning, provide the real path to your base resource instead)

That way we don't change the behavior for those wanting the symlink behavior, but also provide clear warnings for those that use .setResourceBase(String) in a lazy way understand the impact of their decisions.

These warnings will also catch bad declarations in Windows (for example).

Provided	Real
c:\jetty	C:\jetty
C:\jetty\	C:\jetty
C:\JETTY	C:\jetty
C:/jetty	C:\jetty
file://C:/jetty/	C:\jetty

[clauso]

@joakime Now I setup the BaseResource like this, but it does not make a difference:

Path currentDirPath = Paths.get(".").toRealPath(new LinkOption[]{});
fileResourceContext.setBaseResource(new PathResource(currentDirPath));

Can you give an example of how to set this up?

It also confuses me, that actually the links are all resolved well. Only the final comparison fails due to an extra slash. I am not sure if the setup of the BaseResource is actually the problem. See https://github.com/eclipse/jetty.project/issues/687#issuecomment-230983938 and https://github.com/eclipse/jetty.project/issues/687#issuecomment-230988052

[gregw]

No time to drill down into this for a few hours, but isn't it a bug in resolve to put a // into the resulting URI.

The convention we have used elsewhere is to always try to have a trailing / on directory resources, but regardless the resulting path should not have //. I'd like to see more info about why it does that and under which circumstances.

I'll have a bit more of a look later today.

[joakime]

Lets try this ...

I went ahead and created a minimal project here

https://github.com/joakime/resource-regression

To build / run do this ...

$ mvn clean install && ./makesample.sh && mvn exec:java

That will compile it, create the sample directories (just like you outlined above), and then execute the server against those directories.

In a separate terminal, run this to test that running server (note: this requires the curl command line tool) for behavior under the various request URIs you outlined.

$ ./testgets.sh
-- /testdir/ --
<HTML><HEAD><LINK HREF="jetty-dir.css" REL="stylesheet" TYPE="text/css"/><TITLE>Directory: /testdir/</TITLE></HEAD><BODY>
<H1>Directory: /testdir/</H1>
<TABLE BORDER=0>
<TR><TD><A HREF="/testdir/../">Parent Directory</A></TD><TD></TD><TD></TD></TR>

<TR><TD><A HREF="/testdir/testfile.txt">testfile.txt&nbsp;</A></TD><TD ALIGN=right>15 bytes&nbsp;</TD><TD>Jul 7, 2016 2:34:58 PM</TD></TR>
<TR><TD><A HREF="/testdir/testfilelnk.txt">testfilelnk.txt&nbsp;</A></TD><TD ALIGN=right>15 bytes&nbsp;</TD><TD>Jul 7, 2016 2:34:58 PM</TD></TR></TABLE>
</BODY></HTML>

-- /testdirlnk/ --
<HTML><HEAD><LINK HREF="jetty-dir.css" REL="stylesheet" TYPE="text/css"/><TITLE>Directory: /testdir/</TITLE></HEAD><BODY>
<H1>Directory: /testdir/</H1>
<TABLE BORDER=0>
<TR><TD><A HREF="/testdir/../">Parent Directory</A></TD><TD></TD><TD></TD></TR>

<TR><TD><A HREF="/testdir/testfile.txt">testfile.txt&nbsp;</A></TD><TD ALIGN=right>15 bytes&nbsp;</TD><TD>Jul 7, 2016 2:34:58 PM</TD></TR>
<TR><TD><A HREF="/testdir/testfilelnk.txt">testfilelnk.txt&nbsp;</A></TD><TD ALIGN=right>15 bytes&nbsp;</TD><TD>Jul 7, 2016 2:34:58 PM</TD></TR></TABLE>
</BODY></HTML>

-- /testdir/testfile.txt --
Hello TestFile

-- /testdir/testfilelnk.txt --
Hello TestFile

-- /testdirlnk/testfile.txt --
Hello TestFile

-- /testdirlnk/testfilelnk.txt --
Hello TestFile

If this fails for you, can you copy/paste the entire output from the server side?

Starting with from the line that says ...

[INFO] --- exec-maven-plugin:1.5.0:java (default-cli) @ resource-regression ---

... to the end

[clauso]

I found it! Clone https://github.com/clauso/resource-regression a fork of your repo @joakime and observe the subtle difference in creating one symlink. That is btw the way symlinks are usually created in bash with tab-completion. I think jetty should be able to serve those symlinks too?

[clauso]

As written in https://github.com/eclipse/jetty.project/issues/687#issuecomment-230672444 my dir structure looks like:

drwxrwxr-x 2 claus claus 4096 Jul  6 06:54 testdir
lrwxrwxrwx 1 claus claus    8 Jul  6 06:54 testdirlnk -> testdir/

Pay attention to the trailing slash in the second line. Links like this one are not handled nicely by jetty.

I tried to write a patch to the new unit test but failed to make java create a symlink with trailing slash. However I noticed that this

lrwxrwxrwx 1 claus claus    9 Jul 11 07:16 testdirlnk-failing -> ./testdir

also does not work as expected.

[joakime]

Interesting wrinkle in the process. We'll work with these use cases you discovered.

[joakime]

Symlinks with a prefix (testdirlnk -> ./testdir) is where the problem lies.

Symlinks with a suffix (testdirlink -> testdir/) has no effect on the original logic.

A 1 line fix has been committed, along with a host of new testcases.

[clauso]

@joakime Just reviewed your commit: The suffixed links seem to be normalized before creation, that is what I was saying in https://github.com/eclipse/jetty.project/issues/687#issuecomment-231644666 "I tried to write a patch to the new unit test but failed to make java create a symlink with trailing slash. " I.e. your unit test does not create the symlinks with trailing slashes and therefore the test does not test correctly. We will need to find a different way for creating the links for testing.

[clauso]

I tested this a little more and updated my fork of the Fileserver tests: https://github.com/clauso/resource-regression/commit/0496bedf0ad6083b3ac3314f0762a5d3e28d9961 . It seems like the prefixed and suffixed links now work. Only wrapped links (with prefix and suffix) are still failing (if I have not introduced some other stupid error). Can you confirm this @joakime ?

[joakime]

@clauso I went ahead and updated the resource-regression project, and then greatly simplified the logic in AllowSymLinkAliasChecker, please give it a whirl.

[clauso]

Thanks for your work on this @joakime . I am currently stuck in a very urgent project for a client so I have very little free time to look into it (and I somehow messed up my maven dependencies and will have to clean them up again). I will try to find some time soon to have a look, sorry for the delay.

[clauso]

I just tried it and it seems to work, great job! Thanks for the awesome support.