Compare path elements when checking that a served file is below the base directory - #1218
Open
pjfanning wants to merge 1 commit into
Open
Compare path elements when checking that a served file is below the base directory#1218pjfanning wants to merge 1 commit into
pjfanning wants to merge 1 commit into
Conversation
…irectory 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
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.
Motivation
checkIsSafeDescendantinFileAndResourceDirectivesdecides whether a requested file is really below the served directory by comparing the two canonical paths as plain strings:A string prefix is not a path prefix. With
/var/wwwserved, the location/var/www-private/secretsatisfiesstartsWithwithout being contained in the served directory. The segment filter insafeJoinPathsdoes not catch this, since no path segment is suspicious — the path only moves out of the directory whengetCanonicalPathresolves a symbolic link that points at such a sibling. The file is then served.Modification
Compare the canonical paths element by element through
java.nio.file.Pathrather than as strings. The base directory itself still compares as contained, which the directory listing ingetFromBrowseableDirectoryrelies on (its root listing resolves to exactly the base path).Result
Only files that are genuinely below the served directory are served. A symbolic link into a sibling directory is rejected with the warning that was already there, whatever the sibling is called. Requests that were previously served are unaffected, and the existing symlink and traversal tests still pass.
Tests
sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSymlinkSpec"- pass; new test serves through a symlink pointing at a sibling directory named after the served one, and asserts the request is rejected. Verified that it fails without the change (the file is served,handledistrue).sbt http-tests/test- pass (1484 tests)sbt http/mimaReportBinaryIssues- passsbt http/scalafmt http-tests/Test/scalafmt- cleanReferences
None - tightens the containment check for file and resource directives