Issue #6497 - Replace the Alias checkers with new implementation. (Jetty-9.4)

[lachlan-roberts]

Issue #6497

Deprecate and replace the SameFileAliasChecker with the AllowedResourceAliasChecker which does some additional safety checks.

This new AliasChecker is extended to also replace the AllowSymLinkAliasChecker with SymlinkAllowedResourceAliasChecker.

[lachlan-roberts]

This is a weird way to add the paths together, but doing path.resolve(...) does not work because the protectedTarget can start with "/".

[gregw]

why not use URIUtil.addPaths ?

[lachlan-roberts]

It has the signature (String, String) instead of (Path, String), and it returns a String instead of a Path.
And we need the Path so we can use the linkOptions to canonicalize it.

[sbordet]

It's really strange that we have a Path, we call toFile(), create a File and then back to toPath().
I'm surprised there is not a way to just use Path without using the File APIs.
If the File APIs are necessary, needs a comment.

[joakime]

The FileSystem.getPath(String first, String ... more) API supports this call structure.

[lachlan-roberts]

I don't like this code but I cant think of a better alternative. The call to toRealPath requires the file to exist otherwise it throws, but we must check this anyway, because /web-inf might not exist but /WEB-INF could exist and so the Files.exists call would return false.

[gregw]

I think we have made a rod for our backs by specifying the protected targets as lowercase /web-inf.
However, I very much doubt that anybody other than us adds any protected targets, so why don't we just fix this!
We can change our protected targets to use the correct cases and throw an ISE if anybody calls it with any other protected targets (or if the set a protected target that doesn't exist).

Then in doStart we can find the actual Paths of all the protected targets (that exist) and replace all this with the logic I prefer... walking up the parent links looking for any path that is sameFile to any of the protected targets

[lachlan-roberts]

I don't think its a good idea, we might not think anyone is calling that method, but that's just a guess.
If someone is using it and we change it, then it will expose directories that were previously protected.

Doesn't seem like something we want going into a minor release, especially for 9.4.

[lachlan-roberts]

Actually maybe we still wouldn't expose it because its still protected by ContextHander.isProtectedTarget which is case insensitive.

Still doesn't feel like a good idea. Maybe open another issue to consider changing protectedTargets to be case sensitive. We could still change the logic of this later on, but for now it seems like its doing the job of not allowing access to protected targets.

[gregw]

I think we can check if somebody sets a new protected target that does not exist, so I'm not that worried about breaking any usage of that.

The check that we want to make is that the resource is in the baseResource directory, but is not in any of the protected resource directories. We are trying to do that in a FS independent ways with case insensitive string-starts checks, but they are ultimately just hacks.

If we can precisely identify the Path of the base resource and the Paths of the protected resources. We can then walk the parent linkage of the resource and check that none of it's parents are sameFile as the protected resource and that ultimately one of the parents is sameFile as base resource - we then have an absolutely precise check that does exactly what we want.

[gregw]

Maybe we do this only in 10? ... but if we are adding new recommended checkers in 9 and deprecating the existing ones, we may as well add ones that we have 100% confidence in.

[lachlan-roberts]

@gregw can you re-review, I had to make some additional changes since last hangout, I have explained why in the comments.

[gregw]

I'm torn!
See my comments about why we shouldn't just fix the protected targets?

[gregw]

why not use URIUtil.addPaths ?

[gregw]

I think we have made a rod for our backs by specifying the protected targets as lowercase /web-inf.
However, I very much doubt that anybody other than us adds any protected targets, so why don't we just fix this!
We can change our protected targets to use the correct cases and throw an ISE if anybody calls it with any other protected targets (or if the set a protected target that doesn't exist).

Then in doStart we can find the actual Paths of all the protected targets (that exist) and replace all this with the logic I prefer... walking up the parent links looking for any path that is sameFile to any of the protected targets

[gregw]

@lachlan-roberts So I'm thinking that perhaps we do go forward with the string version for jetty-9, but for jetty-10/11 we use #6567 as a basis and then have a version that is based entirely on Paths
If so, I think we should work on the 10/11 version first, before committing this PR, to make sure any lessons learnt there can be echo'ed here to ensure a smooth transition.

[sbordet]

@lachlan-roberts still failing tests, can you make this PR pass the tests?

[sbordet]

It's really strange that we have a Path, we call toFile(), create a File and then back to toPath().
I'm surprised there is not a way to just use Path without using the File APIs.
If the File APIs are necessary, needs a comment.

[sbordet]

Why calling toAbsolutePath()? Isn't just a single /?
Is it because of Windows? If so, needs a comment.

[joakime]

This won't do what you expect on Windows.

If you are running from a Windows Share for example, take \\machine\sharename\jetty your code will result in a Path that is no longer in the share location.
Is that what you want?

The Path has a FileSystem object associated with it, you can ask for its root, or even for all roots that the FileSystem layer supports.

[sbordet]

Why is this check here? Should it not be in the SymlinkAllowedResourceAliasChecker subclass?
I mean that seems weird that this base class does a bit of symlink but not all... Can't all symlink handling and configuration fields be pushed to the subclass?

[lachlan-roberts]

This is a different check to what is done in the subclass, the subclass does not even use this logic, it sets _checkSymlinkTargets to false.

This is an optional check to ensure the target of the symlink is also an allowed resource.

[sbordet]

Why only add this for /?
Also, the class javadoc seems to be in contrast with what is configured here in code.

[joakime]

This is tricky.

Not all FileSystems on Linux or OSX support symlinks.
And some FileSystems on Windows support symlinks.

Basing this default off of File.separatorChar is not wholly accurate.

[lachlan-roberts]

This is pre-existing behaviour which we are maintaining to not break existing deployments.

[gregw]

This should be fine, because if a FS does not support symlinks, then this checker will never see a symlink and thus will be a noop

[sbordet]

Also, I'm not sure the for condition !p.equals(base) works, unless it is guaranteed that base is a normalized parent path of path: Path.of("/home/simon/..").equals("/home") => false.

[gregw]

@lachlan-roberts @sbordet @joakime I think the work being done in this PR is important, but I'm concerned that we are not converging on a solution that is easily understandable.

So I think it is worthwhile to take a step back a re-describe what is being attempted here, then discuss the possible algorithms to achieve that. There are two semantics we are trying to achieve here:

• AllowedResourceAliasChecker it checking that real file of the targeted resource is within the real file of the base resource and not within the real file directory of any protected resources. This will allow a user to support soft aliasing (eg case insensitive files and 8.3 names), but at the risk of bypassing some security constraints.
• SymlinkAllowedResourceAliasChecker is an extension of that will allow a real that is outside of base or a protected resource, but only if it is simlinked from a file that passes the AllowedResoureAliasChecker. It is for simlinks within a webapp that point outside of base or to a protected resource, which I don't think this is a big use-case. The original AllowSymLinkAliasChecker was added really to support symlinks to a jetty-base, webapps or webapp war/directory. This is a use-case that I think is much better dealt with by Fix #6565 Deploy Symlinked applications #6567 which has been merged to 10, but could be back ported to 9. It prevents the baseResource from itself being an alias and thus making the alias checkers fire on every request.

I think backporting #6567 to 9 is a bit big for a dot release, but it is probably better than continuing with the mostly broken AllowSymLinkAliasChecker, but then I think the algorithms below work without it anyway.. they will just be triggered less often.

My preferred algorithm for an AllowedResourceAliasChecker is that at doStart we determine the real path (following links) of the base resource and all protected resources (this is made a little difficult as we assume case insensitivity... see below). The to check if a aliased resource is OK we determine the real path (following links) of the resource and then:

1. if the resource is null or does not exist return false
2. if the resource is the sameFile as any protected resource return false
3. if the resource is the sameFile as the base resource return true
4. assign resource = resource.getParent loop back to 1.

I think this is a portable (somewhat expensive) check that will precisely determine if resource references a file within the real base resource that is not protected.

My preferred algorithm for SymlinkAllowedResourceAliasChecker is much the same, but we start the loop with the realpath (NOT following links) of the resource, so we check precisely that the link itself is in the base resource and not protected.

To achieve this, we will need to stop defining protected paths in a case insensitive way. We currently define "web-inf" and "meta-inf", which will not find the real protected targets on linux (hence this PR tries to get around it with case insensitive string checks). We can change this to "WEB-INF" and "META-INF", but if any users are use the API to add their own protected targets, then we will have a hard time telling they don't exist or are just case insensitive. I think we can work around this by: deprecating the public API and making it internal; failing to deploy if there are any user defined protected targets that do not exist in the base resource when we start.