Jetty 12 - Review and fix Alias Checking

[lachlan-roberts]

Jetty 12 alias checking needs to be reviewed and fixed.

AliasCheck is an interface defined in jetty-core but is only used in the ee10 servlet ContextHandler and is not used in the core ContextHandler, which also has no way to define protected targets.

[joakime]

@lachlan-roberts the jetty-12.0.x branch build was red/broken as a result of your last merge (commit 19e2e7babc0b2d22253123cf237051b5b0046af9)

I went ahead and reverted a small hack you had in your last merge. See commit e7e570f4bbab151729644dde580305f79ed6d2db

This brought the build back to green.

[lorban]

While you're a it, I think there is a bug in ee9 ContextHandler.checkAlias: if the resource is an alias but there are no alias checks then false is returned while I think true should be returned.

[gregw]

@lachlan-roberts can we add a test for that.

[lachlan-roberts]

While you're a it, I think there is a bug in ee9 ContextHandler.checkAlias: if the resource is an alias but there are no alias checks then false is returned while I think true should be returned.

This is not a bug, we do not allow aliases unless there is an alias check in place to specifically allow them. We have even deprecated ApproveAliases (which always returns true) and warn if it is used because using it would introduce security issues if you used it without a very good reason.

[lorban]

Something doesn't feel right, he's why I believe ContextHandler.checkAlias is buggy in its current form: if one creates a ee9 DefaultServlet to serve the contents of a JAR file then it's not possible to serve the directory listings of any subfolder of the jar because jar:file:///some/file.jar!/foo/ is considered an alias of jar:file:///some/file.jar!/foo. Maybe @joakime has some insights to share about this?

I've created the https://github.com/eclipse/jetty.project/commits/jetty-12.0.x-enable-ee9-demo-embedded branch to illustrate the problem I found.

I added this test that configures the ee9 DefaultServlet against a jar file's contents as it base resource: https://github.com/eclipse/jetty.project/blob/jetty-12.0.x-enable-ee9-demo-embedded/jetty-ee9/jetty-ee9-demos/jetty-ee9-demo-embedded/src/test/java/org/eclipse/jetty/ee9/demos/JarServerTest.java

If you run this test, you should get testDirListingRootFolder to pass and testDirListingSubDir0 to fail, unless you uncomment the following: https://github.com/eclipse/jetty.project/blob/jetty-12.0.x-enable-ee9-demo-embedded/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java#L1437-L1439

I'm not saying ContextHandler.checkAlias is the problem, but that it may be. If we decide the problem is somewhere else I suppose a new issue should be created then.

[lachlan-roberts]

@lorban currently the way to allow aliases like this is to add the AllowedResourceAliasChecker.

I tried this with this test and it passes if the AllowedResourceAliasChecker is added to the context created in JarServer.

context.addAliasCheck(new AllowedResourceAliasChecker(context));

[lorban]

Adding the AllowedResourceAliasChecker isn't needed if the resource is backed by a file:/// URI, but it is if the resource is backed by a jar:file:/// URI.

This is surprising, inconsistent and a change of behavior from 11.0.x.

[joakime]

I'll have to dig into it, but I think this can be explained by FileSystem behaviors.

In a JAR, the entry is case sensitive.

That entry is known to be one of two things, a directory or a file, nothing else (no symlinks, alternate names, system files, relative path references, etc).

So if you have an entry "foo" in a Jar, it cannot exist as more than 1 thing. It's going to be a File or a Directory, never existing more than once in the archive with the same name but different things.

Since it's case-sensitive, that also means you cannot use Windows file-system like tricks and say have a file at Foo and a directory at foo in the same Jar and have a conflict if accessing it with foo (you'll only get the directory).

Also, since we are dealing with a FileSystem built on a URI jar:file:/path/to/foo.jar!/inside/, I would expect that the normal URI rules should be applied. A directory reference always ends with /, which it seems to be enforcing.

[joakime]

I opened PR #8436 to attempt to tackle the discovery that @lachlan-roberts made about jar:file: and directory paths.

It appears that the Path.toURI() on a directory within jar:file: FileSystem returns without a trailing slash. So in that PR I skip the URI diff check and the symlink check (symlinks don't exist in this FileSystem) if it's a jar:file: based URI. But I need more unit tests. Can you suggest a few more?

[gregw]

@joakime @lachlan-roberts @lorban just a note to re-enforce the principles on which aliase checking is intended to work.

By default we should NOT server aliased resources. i.e. we will only serve a resource when requested by its canonical path/name.

We support exceptions to this rule with alias checkers, but historically we have messed up with these. The mistake we have made is that alias checkers have said "this alias is OK because it is a symlink", but that is in error because a resource might be an alias for multiple reasons... it may be a symlink AND inside a protected resource.

Lachlan has started re-orienting our alias checkers so that rather than saying "this type of alias is OK", they instead say "it is OK to serve this resource". This can be easy to check the resource is in within the baseResource and not a protected target, but is more difficult to check if it bypasses any servlet security constraints. We have more work to do here.

Regarding the example above of jar:file:///some/file.jar!/foo/ vs jar:file:///some/file.jar!/foo, we have to strive to be consistent with how we handle trailing / (and also the /// after file:) so that we do not needlessly generate aliases.

In this case, I would think we should never try to serve jar:file:///some/file.jar!/foo as it should be a redirection to jar:file:///some/file.jar!/foo/. I would then not expect jar:file:///some/file.jar!/foo/ to be seen as an alias, so we should perhaps detune the alias detecting code to better handle trailing slash.

[joakime]

Regarding the example above of jar:file:///some/file.jar!/foo/ vs jar:file:///some/file.jar!/foo, we have to strive to be consistent with how we handle trailing / (and also the /// after file:) so that we do not needlessly generate aliases.

Btw, there's a new URIUtil.correctFileURI(URI) that will correct the bad file:/path/ to file:///path/ URIs, even if they exist as jar:file:/path/foo.jar it will correct the file:/path portion to result in jar:file:///path/foo.jar.

The most common source of these awkward file:/path URIs seems to be from the URLClassLoader. For that we have a Stream<URI> URIUtil.streamOf(URLClassLoader) to address that concern.

[lorban]

@gregw thanks for the explanation / reminder about what aliases and alias checking are about. That summary of yours is definitely worth a place somewhere in the javadoc.

[lachlan-roberts]

PR #8449 has been merged so I think this can be closed.

As a summary, the same AliasCheck class is now used for jetty-core, ee10 and ee9. Multiple AliasChecks can be added to a org.eclipse.jetty.server.handler.ContextHandler, and ContextHandler.checkAlias() is used to go through all the AliasCheck of the context.

Jetty-core and ee10 use the core ResourceService which now uses the ContextHandler as an AliasCheck, while ee9 calls checkAlias from the same places it does in jetty 10 & 11.