This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/pekko-http.git
The following commit(s) were added to refs/heads/main by this push:
new 2bb837f55 Compare path elements when checking that a served file is
below the base directory (#1218)
2bb837f55 is described below
commit 2bb837f5543cc35ddbe61d51fdb64deade1398f9
Author: PJ Fanning <[email protected]>
AuthorDate: Tue Sep 1 11:54:59 2026 +0100
Compare path elements when checking that a served file is below the base
directory (#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
---
.../directives/FileAndResourceDirectivesSpec.scala | 12 ++++++++++
.../FileAndResourceDirectivesSymlinkSpec.scala | 23 +++++++++++++++++++
.../directives/FileAndResourceDirectives.scala | 26 +++++++++++++++++-----
3 files changed, 56 insertions(+), 5 deletions(-)
diff --git
a/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala
b/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala
index de3f1e70f..92b13af23 100644
---
a/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala
+++
b/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala
@@ -195,6 +195,18 @@ class FileAndResourceDirectivesSpec extends RoutingSpec
with Inspectors with Ins
shouldReject("..%c0%af", warnings = 0)
shouldReject("..%c1%9c", warnings = 0)
}
+ "reject requests whose path can never name a file" in {
+ // a percent-encoded NUL decodes into the path segment, but no
file-system path may contain it, so
+ // canonicalization throws; that must surface as a rejection, not as an
error escaping to the exception handler
+ def route(uri: String) =
+ mapRequestContext(_.withUnmatchedPath(Path("/" + uri))) {
_getFromDirectory("someDir") }
+
+ EventFilter.warning(pattern = ".* points to a location that is not part
of .*", occurrences = 1).intercept {
+ Get() ~> route("na%00ive.txt") ~> check {
+ handled shouldEqual false
+ }
+ }
+ }
"return the file content with the MediaType matching the file extension"
in {
Get("fileA.txt") ~> _getFromDirectory("someDir") ~> check {
mediaType shouldEqual `text/plain`
diff --git
a/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSymlinkSpec.scala
b/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSymlinkSpec.scala
index 42f9324bf..9fa202a96 100644
---
a/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSymlinkSpec.scala
+++
b/http-tests/src/test/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectivesSymlinkSpec.scala
@@ -41,9 +41,21 @@ class FileAndResourceDirectivesSymlinkSpec extends
RoutingSpec
Paths.get(dirWithLink.getAbsolutePath, "linked-dir"),
new File(testRoot, "subDirectory").toPath.toAbsolutePath)
+ // a sibling of the served directory whose name has the name of the served
directory as a prefix
+ val siblingDir = new File(tempDir.toFile, "dirWithLink-private")
+ siblingDir.mkdir()
+ val siblingFile = new File(siblingDir, "secret.txt")
+ Files.createFile(siblingFile.toPath)
+ val siblingSymlink = Files.createSymbolicLink(
+ Paths.get(dirWithLink.getAbsolutePath, "linked-sibling"),
+ siblingDir.toPath.toAbsolutePath)
+
override def afterAll(): Unit = {
super.afterAll()
Files.deleteIfExists(symlink)
+ Files.deleteIfExists(siblingSymlink)
+ Files.deleteIfExists(siblingFile.toPath)
+ Files.deleteIfExists(siblingDir.toPath)
Files.deleteIfExists(dirWithLink.toPath)
Files.deleteIfExists(tempDir)
}
@@ -69,5 +81,16 @@ class FileAndResourceDirectivesSymlinkSpec extends
RoutingSpec
}
}
}
+
+ "not follow symbolic links into a sibling directory whose name starts with
the served directory" in {
+ Files.isSymbolicLink(siblingSymlink) shouldBe true
+ // the canonical location of the file is
`<tmp>/dirWithLink-private/secret.txt`, which has the canonical
+ // path of the served directory, `<tmp>/dirWithLink`, as a string prefix
+ EventFilter.warning(pattern = ".* points to a location that is not part
of .*", occurrences = 1).intercept {
+ Get("linked-sibling/secret.txt") ~> _getFromDirectory() ~> check {
+ handled shouldBe false
+ }
+ }
+ }
}
}
diff --git
a/http/src/main/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectives.scala
b/http/src/main/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectives.scala
index 9d8882e2b..3692011db 100644
---
a/http/src/main/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectives.scala
+++
b/http/src/main/scala/org/apache/pekko/http/scaladsl/server/directives/FileAndResourceDirectives.scala
@@ -16,6 +16,7 @@ package directives
import java.io.{ File, FileNotFoundException, IOException, InputStream }
import java.net.{ JarURLConnection, URL, URLConnection }
+import java.nio.file.{ InvalidPathException, Paths }
import scala.annotation.tailrec
import scala.jdk.CollectionConverters._
@@ -235,9 +236,12 @@ object FileAndResourceDirectives extends
FileAndResourceDirectives {
* to files containing one of those characters on a file-system that
allows those characters in file names
* (e.g. backslash on posix).
* - Resulting paths are checked to be "contained" in the base directory.
"Contained" means that the canonical location
- * of the file (according to File.getCanonicalPath) has the canonical
version of the basePath as a prefix. The exact
- * semantics depend on the implementation of `File.getCanonicalPath` that
may or may not resolve symbolic links and
- * similar structures depending on the OS and the JDK implementation of
file system accesses.
+ * of the file (according to File.getCanonicalPath) has the canonical
version of the basePath as a path-element
+ * prefix, compared element by element rather than as strings, so that a
sibling directory whose name merely starts
+ * with the base path's name is not treated as contained. The exact
semantics depend on the implementation of
+ * `File.getCanonicalPath` that may or may not resolve symbolic links and
similar structures depending on the OS and
+ * the JDK implementation of file system accesses; on Windows in
particular it does not resolve NTFS symbolic links
+ * and junctions, so a link out of the base directory is not detected
there.
*/
private def safeDirectoryChildPath(basePath: String, path: Uri.Path, log:
LoggingAdapter,
separator: Char = File.separatorChar): String =
@@ -271,9 +275,21 @@ object FileAndResourceDirectives extends
FileAndResourceDirectives {
private def checkIsSafeDescendant(basePath: String, finalPath: String, log:
LoggingAdapter): String = {
val baseFile = new File(basePath)
val finalFile = new File(finalPath)
- val canonicalFinalPath = finalFile.getCanonicalPath
- if (!canonicalFinalPath.startsWith(baseFile.getCanonicalPath)) {
+ // compared element by element instead of as plain strings:
`/var/www-private/secret` has the canonical path of
+ // `/var/www` as a string prefix without being contained in that
directory, which canonicalization can produce
+ // for a symbolic link that points at a sibling directory
+ val canonicalFinalPath =
+ try {
+ val canonical = finalFile.getCanonicalPath
+ if
(Paths.get(canonical).startsWith(Paths.get(baseFile.getCanonicalPath)))
canonical else ""
+ } catch {
+ // a segment can contain characters that no file-system path may hold
(NUL on all platforms, '<' and
+ // similar on Windows): getCanonicalPath and Paths.get throw on those,
and such a file cannot exist anyway
+ case _: InvalidPathException | _: IOException => ""
+ }
+
+ if (canonicalFinalPath.isEmpty) {
log.warning("[{}] points to a location that is not part of [{}]. This
might be a directory traversal attempt.",
finalFile, baseFile)
""
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
