Issue #11271 - fix use of AliasCheckers with CombinedResource

[lachlan-roberts]

Issue #11271

Add support for CombinedResource in AllowedResourceAliasChecker and SymlinkAllowedResourceAliasChecker.

Also added more testing into AliasCheckerSymlinkTest.

[gregw]

We could have isSameFile(Path) as a method on Resource, then only CombinedResource would need the iteration.

[gregw]

@lachlan-roberts bump

[lachlan-roberts]

@gregw with the change in behaviour to SymlinkAllowedResourceAliasChecker, now testRelativeRedirect and testResourceRedirect are failing from ResourceHandlerTest.

They are expecting a request for /context/dir/index.html/ will redirect to /context/dir/index.html, but now the alias is rejected (because there is no symlink) and we get 404.

Are we ok with this behaviour change? if so I will just make these tests use the AllowedResourceAliasChecker.

[gregw]

@lachlan-roberts there is a bit to unpack here.

Firstly the redirect from /context/dir/index.html/ to /context/dir/index.html is kind of to avoid the alias in the first place. @lorban Could (should?) the redirection of resources like this happen before we check for aliases?

Otherwise, I don't like behaviour changes.... but I think in this case it is really REALLY strange that by adding something to allows symlinks we are also allowing arbitrary aliasing within the docroot, possibly bypassing security checks etc. To me that was wrong behaviour in the first place, so I'm kind of OK with changing it.

What is the additive behaviour of alias checkers? Does it just need to be approved by one? In which case the fix for anybody that complains is to add both symlinks and allowed alias checkers (does that fix the failing test?)

However, I am still curious as to why there is a behaviour change? What was the previous checker doing that says /context/dir/index.html/ is an OK alias, but that /context/dir%2Findex.html is not? What operation was not decoding the %2F before?

[lorban]

@gregw AFAICT alias checking should take precedence over everything else. This is how they were designed, and it's what I think makes the most sense.

So it feels like this (fortunately small) change of behavior is actually fixing something that used to be incorrect.

[lachlan-roberts]

What is the additive behaviour of alias checkers? Does it just need to be approved by one?

@gregw yes it just needs to be approved by one, so adding AllowedResourceAliasChecker will cause it to pass.

However, I am still curious as to why there is a behaviour change? What was the previous checker doing that says /context/dir/index.html/ is an OK alias, but that /context/dir%2Findex.html is not? What operation was not decoding the %2F before?

In the /dir%2Findex.html case, previously we would throw because the file dir%2Findex.html did not exist. Now we are passing the test as it matches to an alias of dir/index.html which does exist, but there is no symlink in the path so the SymlinkAllowedResourceAliasChecker still rejects it. So this one still contains some behaviour change.

The /context/dir/index.html/ case it is an alias for the index.html file, which if approved ResourceService will see its a file ending with / and will redirect to /context/dir/index.html.

[gregw]

We should not allow dir%2Findex.html just because we allow symlinks. This is exactly the kind of security constraint bypassing alias that the alias mechanism was implemented to protect against.

[lachlan-roberts]

We should not allow dir%2Findex.html just because we allow symlinks. This is exactly the kind of security constraint bypassing alias that the alias mechanism was implemented to protect against.

Then we are going to have to rethink the design of our alias checkers. We never re-verify against the security constraints, only the protected targets. We even say in the javadoc:

Aliases approved by this may still be able to bypass SecurityConstraints, 
so this class would need to be extended to enforce any additional 
security constraints that are required.

We only base this on whether it is a file inside the base resource which is not a protected target. And for the symlink checker once we hit a symlink file which is an allowed resource we approve, regardless of what comes after that symlink.

[gregw]

@lachlan why do we need a big rethink? If somebody adds the allowed file resource checker, then any alias is ok so long as it is in the docroot and not protected.

If they add them symlink checker it should just allow Sym links and not arbitrary other aliases. The name says it!

[lachlan-roberts]

If they add them symlink checker it should just allow Sym links and not arbitrary other aliases. The name says it!

Well the name says SymlinkAllowedResourceAliasChecker not SymlinkAliasChecker and it extends AllowedResourceAliasChecker. And the javadoc says:

will allow symlinks alias to arbitrary targets, so long as the symlink file itself is an allowed resource

So right now if there is a symlink file which is an "allowed resource" as defined by the AllowedResourceAliasChecker then it will be approved.

From what I understand you want the symlink alias checker to approve aliases if they are an alias only because of symlinks and nothing else. But to do this we would need to separate it from the logic of the AllowedResourceAliasChecker and find some way of determining why a given resource is an alias. Right now we only know if it is an allowed resource but we don't know exactly why it's an alias, we just know that path.toRealPath() resolved to something different than path.

[gregw]

@lachlan-roberts I don't understand why you want to combine allowed resources with symlink checking? If you want both then add both be alias checkers.

What's wrong with the implementation as we last reviewed it together. Just remove that last conditional return true after checking for symlinks and it is ok.

[lachlan-roberts]

@gregw I'm not trying to combine them they already are. This is just how it is currently works. If you want to separate them then we're going to have to make some bigger changes to SymlinkAllowedResourceAliasChecker.

What's wrong with the implementation as we last reviewed it together. Just remove that last conditional return true after checking for symlinks and it is ok.

This is already removed.

That check was saying if it didn't contain a symlink we could approve it anyway if its an allowed resource. But now we're talking about the case where it does contain a symlink.

[lachlan-roberts]

failing due to unrelated flaky tests

[lachlan-roberts]

After re-running tests it is getting to an actual failure.
It is breaking AliasCheckerMultipleResourceBasesTest.

It is failing after the changes we made to SymlinkAllowedResourceAliasChecker, because the baseResource is the symlink.

[lachlan-roberts]

@gregw @joakime this is ready for review

[joakime]

@gregw bump

[gregw]

Any idea why it is not passing CI?

[gregw]

@lachlan-roberts bump

[lachlan-roberts]

@gregw the failures were flaky tests