Issue #6497 - Replace the Alias checkers with new implementation. (Jetty-10) #6668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
gregw
requested changes
Aug 30, 2021
| { | ||
| return String.format("%s@%x{checkSymlinkTargets=%s}", AllowedResourceAliasChecker.class.getSimpleName(), hashCode(), _checkSymlinkTargets); | ||
| } | ||
| } |
|
|
||
| /** | ||
| * <p>This will approve any alias to anything inside of the {@link ContextHandler}s resource base which | ||
| * is not protected by {@link ContextHandler#isProtectedTarget(String)}.</p> |
There was a problem hiding this comment.
Suggested change
| * is not protected by {@link ContextHandler#isProtectedTarget(String)}.</p> | |
| * is not protected by {@link #isProtectedTarget(Path, LinkOption[])}.</p> |
Comment on lines
+138
to
+139
| * <p>The resource path is protected if it is under one of the protected targets defined by | ||
| * {@link ContextHandler#isProtectedTarget(String)} in which case the alias should not be allowed. |
There was a problem hiding this comment.
Suggested change
| * <p>The resource path is protected if it is under one of the protected targets defined by | |
| * {@link ContextHandler#isProtectedTarget(String)} in which case the alias should not be allowed. | |
| * <p>The resource path is protected if it, or one of it's parents is in | |
| * {@link ContextHandler#getProtectedTargets()} when fetched from {@link #doStart()}.</p> | |
| * {@link ContextHandler#isProtectedTarget(String)} in which case the alias should not be allowed. |
Comment on lines
+161
to
+181
| for (Path protectedPath : _protectedPaths) | ||
| { | ||
| String protect; | ||
| if (Files.exists(protectedPath, linkOptions)) | ||
| protect = protectedPath.toRealPath(linkOptions).toString(); | ||
| else if (linkOptions == NO_FOLLOW_LINKS) | ||
| protect = protectedPath.normalize().toAbsolutePath().toString(); | ||
| else | ||
| protect = protectedPath.toFile().getCanonicalPath(); | ||
|
|
||
| // If the target path is protected then we will not allow it. | ||
| if (StringUtil.startsWithIgnoreCase(target, protect)) | ||
| { | ||
| if (target.length() == protect.length()) | ||
| return true; | ||
|
|
||
| // Check that the target prefix really is a path segment. | ||
| if (target.charAt(protect.length()) == File.separatorChar) | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remind me of why we can't walk the parent tree again?
| if (File.separatorChar == '/') | ||
| addAliasCheck(new AllowSymLinkAliasChecker()); | ||
| addAliasCheck(new SymlinkAllowedResourceAliasChecker(this)); |
There was a problem hiding this comment.
I'm wondering if we really still need this by default? I guesss too late to change in a dot release, but let's put a TODO in to remove if/when we go to 10.1
| @@ -0,0 +1 @@ | |||
| This file is inside a sibling dir to webroot. | |||
| @@ -0,0 +1 @@ | |||
| This is the web.xml file. | |||
| @@ -0,0 +1 @@ | |||
| This file is inside webroot/documents. | |||
| @@ -0,0 +1 @@ | |||
| This file is inside webroot. | |||
| <html> | ||
| <h1>hello world</h1> | ||
| <p>body of index.html</p> | ||
| </html> |
gregw
reviewed
Aug 30, 2021
| import static org.hamcrest.Matchers.is; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
|
||
| public class AliasCheckerSymlinkTest |
There was a problem hiding this comment.
Can this be combined with or replace AllowSymlinkAliasCheckerTest
|
Replaced with PR #6681. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #6497
Deprecate and replace the
SameFileAliasCheckerwith theAllowedResourceAliasCheckerwhich does some additional safety checks.This new AliasChecker is extended to also replace the
AllowSymLinkAliasCheckerwithSymlinkAllowedResourceAliasChecker.