Compare path elements when checking that a served file is below the base directory (#1218) - #1269
Open
pjfanning wants to merge 1 commit into
Open
Compare path elements when checking that a served file is below the base directory (#1218)#1269pjfanning wants to merge 1 commit into
pjfanning wants to merge 1 commit into
Conversation
…ase directory (apache#1218) * compare path elements when checking that a file is below the served directory Motivation: `checkIsSafeDescendant` compared the canonical location of the requested file with the canonical path of the served directory as a plain string prefix. A path such as `/var/www-private/secret` has `/var/www` as a string prefix without being contained in it, so a symbolic link inside the served directory that resolves to such a sibling directory passed the check and the file was served. The segment filter in `safeJoinPaths` does not catch this, because no path segment is suspicious; only canonicalization moves the location out of the directory. Modification: Compare the two canonical paths element by element via `java.nio.file.Path` instead of as strings. That keeps the base directory itself accepted, which the directory listing of `getFromBrowseableDirectory` relies on. Result: Only files that are really below the served directory are served, and a symbolic link to a sibling directory is rejected with the existing warning regardless of how the sibling is named. Tests: - sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSymlinkSpec" - pass, 1 new test that serves a symlink to a sibling directory named after the served one; it fails without the change - sbt http-tests/test - pass - sbt http/mimaReportBinaryIssues - pass - sbt http/scalafmt http-tests/Test/scalafmt - clean References: None - tightens the containment check for file and resource directives * reject paths that cannot exist instead of erroring, document the element-wise containment Motivation: Review of the containment change found that Paths.get can throw an unchecked InvalidPathException where the previous String.startsWith never threw: on Windows a decoded segment such as `a<b` passes safeJoinPaths and File.getCanonicalPath but not the stricter WindowsPathParser, turning a previously clean rejection into a 500 that skips the traversal warning and breaks rejection-based route chaining. A NUL byte in a segment (`%00`) had the same effect on every platform even before this PR, via the IOException that File.getCanonicalPath throws for it. The scaladoc above safeDirectoryChildPath also still described the string-prefix containment semantics that the previous commit removed as insecure, and the symlink spec wrote file content that no assertion reads. Modification: Catch InvalidPathException and IOException in checkIsSafeDescendant and treat such paths as not contained, producing the existing warning and rejection; no file with such a name can exist, so nothing servable is lost. Restate the scaladoc containment contract as element-wise comparison and note that Windows canonicalization does not resolve NTFS symbolic links or junctions. Create the sibling fixture file empty in the symlink spec. Rebased onto main. Result: A request segment that no file-system path may contain is rejected with the traversal warning instead of escaping to the exception handler as a 500, on all platforms; the documented contract matches the implementation. Tests: - sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSymlinkSpec" - pass (52 tests); new test rejects a %00 segment and asserts the traversal warning; verified it fails with the fix stashed (unhandled IOException) - sbt http/mimaReportBinaryIssues - pass - native scalafmt run on the three changed files - clean References: None - hardens the containment check follow-up from review
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
cherry pick 2bb837f #1218