From 3cfb815b941ecb90e72d4e4c7b8a93e5fffb09f9 Mon Sep 17 00:00:00 2001 From: Johannes Rudolph Date: Fri, 30 Sep 2016 11:01:36 +0200 Subject: [PATCH] Directory traversal fix (backport to akka-http 2.4.x) (#21584) * =htp #346 add test to check for potential directory traversal path patterns (cherry picked from commit cb7ba25ece893311a520410b31ab55765d6261c4) * =htp add test checking getFromDirectory is able to serve files with percent-encodings (cherry picked from commit 1d18d9e189fed1d3b5b776bee7ba86472c0d71ec) * =htp #346 add test for path traversal attempts in listDirectoryContents (cherry picked from commit 7027eaf1457d7a93309609476da9232d6f612f18) * =htp test if getFromDirectory follows symbolic links (cherry picked from commit f1b379f9b9022cab21c6a00c135dca4c6bac327f) * =htp #346 prevent path traversal attempts in getFromDirectory / listDirectoryContents (cherry picked from commit dac15938dd563f890aa12c67ec696126267bad42) --- .../src/test/resources/dirWithLink/linked-dir | 1 + .../test/resources/sübdir/sample späce.PDF | 1 + .../FileAndResourceDirectivesSpec.scala | 99 +++++++++++++++++++ .../FileAndResourceDirectives.scala | 59 ++++++++--- 4 files changed, 149 insertions(+), 11 deletions(-) create mode 120000 akka-http-tests/src/test/resources/dirWithLink/linked-dir create mode 100644 akka-http-tests/src/test/resources/sübdir/sample späce.PDF diff --git a/akka-http-tests/src/test/resources/dirWithLink/linked-dir b/akka-http-tests/src/test/resources/dirWithLink/linked-dir new file mode 120000 index 0000000000..1ac44d7bd4 --- /dev/null +++ b/akka-http-tests/src/test/resources/dirWithLink/linked-dir @@ -0,0 +1 @@ +../subDirectory \ No newline at end of file diff --git a/akka-http-tests/src/test/resources/sübdir/sample späce.PDF b/akka-http-tests/src/test/resources/sübdir/sample späce.PDF new file mode 100644 index 0000000000..fdfab78488 --- /dev/null +++ b/akka-http-tests/src/test/resources/sübdir/sample späce.PDF @@ -0,0 +1 @@ +This is PDF \ No newline at end of file diff --git a/akka-http-tests/src/test/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala b/akka-http-tests/src/test/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala index 3e8b3039a7..a59aab9c26 100644 --- a/akka-http-tests/src/test/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala +++ b/akka-http-tests/src/test/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala @@ -20,12 +20,18 @@ import akka.http.scaladsl.model._ import akka.http.scaladsl.model.headers._ import akka.http.impl.util._ import akka.http.scaladsl.TestUtils.writeAllText +import akka.http.scaladsl.model.Uri.Path class FileAndResourceDirectivesSpec extends RoutingSpec with Inspectors with Inside { // operations touch files, can be randomly hit by slowness implicit val routeTestTimeout = RouteTestTimeout(3.seconds) + // need to serve from the src directory, when sbt copies the resource directory over to the + // target directory it will resolve symlinks in the process + val testRoot = new File("akka-http-tests/src/test/resources") + require(testRoot.exists(), s"testRoot was not found at ${testRoot.getAbsolutePath}") + override def testConfigSource = "akka.http.routing.range-coalescing-threshold = 1" "getFromFile" should { @@ -124,6 +130,69 @@ class FileAndResourceDirectivesSpec extends RoutingSpec with Inspectors with Ins } } + "getFromDirectory" should { + def _getFromDirectory(directory: String) = getFromDirectory(new File(testRoot, directory).getCanonicalPath) + + "reject non-GET requests" in { + Put() ~> _getFromDirectory("someDir") ~> check { handled shouldEqual false } + } + "reject requests to non-existing files" in { + Get("nonExistentFile") ~> _getFromDirectory("subDirectory") ~> check { handled shouldEqual false } + } + "reject requests to directories" in { + Get("sub") ~> _getFromDirectory("someDir") ~> check { handled shouldEqual false } + } + "reject path traversal attempts" in { + def route(uri: String) = + mapRequestContext(_.withUnmatchedPath(Path("/" + uri))) { _getFromDirectory("someDir/sub") } + + Get() ~> route("file.html") ~> check { handled shouldEqual true } + + def shouldReject(prefix: String) = + Get() ~> route(prefix + "fileA.txt") ~> check { handled shouldEqual false } + shouldReject("../") // resolved + shouldReject("%5c../") + shouldReject("%2e%2e%2f") + shouldReject("%2e%2e/") // resolved + shouldReject("..%2f") + shouldReject("%2e%2e%5c") + shouldReject("%2e%2e\\") + shouldReject("..\\") + shouldReject("\\") + shouldReject("%5c") + shouldReject("..%5c") + shouldReject("..%255c") + shouldReject("..%c0%af") + shouldReject("..%c1%9c") + } + "return the file content with the MediaType matching the file extension" in { + Get("fileA.txt") ~> _getFromDirectory("someDir") ~> check { + mediaType shouldEqual `text/plain` + charsetOption shouldEqual Some(HttpCharsets.`UTF-8`) + responseAs[String] shouldEqual "123" + val lastModified = new File(testRoot, "someDir/fileA.txt").lastModified() + headers should contain(`Last-Modified`(DateTime(lastModified))) + } + } + "return the file content with the MediaType matching the file extension (unicode chars in filename)" in { + Get("sample%20sp%c3%a4ce.PDF") ~> _getFromDirectory("sübdir") ~> check { + mediaType shouldEqual `application/pdf` + charsetOption shouldEqual None + responseAs[String] shouldEqual "This is PDF" + val lastModified = new File(testRoot, "sübdir/sample späce.PDF").lastModified() + headers should contain(`Last-Modified`(DateTime(lastModified))) + } + } + "not follow symbolic links to find a file" in { + Get("linked-dir/empty.pdf") ~> _getFromDirectory("dirWithLink") ~> check { + handled shouldBe false + /* TODO: resurrect following links under an option + responseAs[String] shouldEqual "123" + mediaType shouldEqual `application/pdf`*/ + } + } + } + "getFromResource" should { "reject non-GET requests" in { Put() ~> getFromResource("some") ~> check { handled shouldEqual false } @@ -399,6 +468,36 @@ class FileAndResourceDirectivesSpec extends RoutingSpec with Inspectors with Ins "reject requests to file resources" in { Get() ~> listDirectoryContents(base + "subDirectory/empty.pdf") ~> check { handled shouldEqual false } } + "reject path traversal attempts" in { + def _listDirectoryContents(directory: String) = listDirectoryContents(new File(testRoot, directory).getCanonicalPath) + def route(uri: String) = + mapRequestContext(_.withUnmatchedPath(Path("/" + uri)).mapRequest(_.copy(uri = "/" + uri))) { + _listDirectoryContents("someDir/sub") + } + + Get() ~> route("") ~> check { + handled shouldEqual true + } + + def shouldReject(prefix: String) = + Get() ~> route(prefix) ~> check { + handled shouldEqual false + } + shouldReject("../") // resolved + shouldReject("%5c../") + shouldReject("%2e%2e%2f") + shouldReject("%2e%2e/") // resolved + shouldReject("..%2f") + shouldReject("%2e%2e%5c") + shouldReject("%2e%2e\\") + shouldReject("..\\") + shouldReject("\\") + shouldReject("%5c") + shouldReject("..%5c") + shouldReject("..%255c") + shouldReject("..%c0%af") + shouldReject("..%c1%9c") + } } def prep(s: String) = s.stripMarginWithNewline("\n") diff --git a/akka-http/src/main/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectives.scala b/akka-http/src/main/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectives.scala index e5f4fe4655..538072502e 100644 --- a/akka-http/src/main/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectives.scala +++ b/akka-http/src/main/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectives.scala @@ -127,17 +127,15 @@ trait FileAndResourceDirectives { * * @group fileandresource */ - def getFromDirectory(directoryName: String)(implicit resolver: ContentTypeResolver): Route = { - val base = withTrailingSlash(directoryName) - extractUnmatchedPath { path ⇒ + def getFromDirectory(directoryName: String)(implicit resolver: ContentTypeResolver): Route = + extractUnmatchedPath { unmatchedPath ⇒ extractLog { log ⇒ - fileSystemPath(base, path, log) match { + safeDirectoryChildPath(withTrailingSlash(directoryName), unmatchedPath, log) match { case "" ⇒ reject case fileName ⇒ getFromFile(fileName) } } } - } /** * Completes GET requests with a unified listing of the contents of all given directories. @@ -151,11 +149,13 @@ trait FileAndResourceDirectives { val path = ctx.unmatchedPath val fullPath = ctx.request.uri.path.toString val matchedLength = fullPath.lastIndexOf(path.toString) - require(matchedLength >= 0) + require(matchedLength >= 0, s"Unmatched path '$path' wasn't a suffix of full path '$fullPath'. " + + "This usually means that ctx.unmatchedPath was manipulated inconsistently " + + "with ctx.request.uri.path") val pathPrefix = fullPath.substring(0, matchedLength) - val pathString = withTrailingSlash(fileSystemPath("/", path, ctx.log, '/')) + val pathString = withTrailingSlash(safeJoinPaths("/", path, ctx.log, '/')) val dirs = directories flatMap { dir ⇒ - fileSystemPath(withTrailingSlash(dir), path, ctx.log) match { + safeDirectoryChildPath(withTrailingSlash(dir), path, ctx.log) match { case "" ⇒ None case fileName ⇒ val file = new File(fileName) @@ -199,7 +199,7 @@ trait FileAndResourceDirectives { extractUnmatchedPath { path ⇒ extractLog { log ⇒ - fileSystemPath(base, path, log, separator = '/') match { + safeJoinPaths(base, path, log, separator = '/') match { case "" ⇒ reject case resourceName ⇒ getFromResource(resourceName, resolver(resourceName), classLoader) } @@ -217,14 +217,33 @@ object FileAndResourceDirectives extends FileAndResourceDirectives { BasicDirectives.extractSettings private def withTrailingSlash(path: String): String = if (path endsWith "/") path else path + '/' - private def fileSystemPath(base: String, path: Uri.Path, log: LoggingAdapter, separator: Char = File.separatorChar): String = { + + /** + * Given a base directory and a (Uri) path, returns a path to a location contained in the base directory, + * while checking that no path traversal is possible. Path traversal is prevented by two individual measures: + * - A path segment must not be ".." and must not contain slashes or backslashes that may carry special meaning in + * file-system paths. This logic is intentionally a bit conservative as it might also prevent legitimate access + * 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. + */ + private def safeDirectoryChildPath(basePath: String, path: Uri.Path, log: LoggingAdapter, separator: Char = File.separatorChar): String = + safeJoinPaths(basePath, path, log, separator) match { + case "" ⇒ "" + case path ⇒ checkIsSafeDescendant(basePath, path, log) + } + + private def safeJoinPaths(base: String, path: Uri.Path, log: LoggingAdapter, separator: Char = File.separatorChar): String = { import java.lang.StringBuilder @tailrec def rec(p: Uri.Path, result: StringBuilder = new StringBuilder(base)): String = p match { case Uri.Path.Empty ⇒ result.toString case Uri.Path.Slash(tail) ⇒ rec(tail, result.append(separator)) case Uri.Path.Segment(head, tail) ⇒ - if (head.indexOf('/') >= 0 || head == "..") { + if (head.indexOf('/') >= 0 || head.indexOf('\\') >= 0 || head == "..") { log.warning("File-system path for base [{}] and Uri.Path [{}] contains suspicious path segment [{}], " + "GET access was disallowed", base, path, head) "" @@ -233,6 +252,24 @@ object FileAndResourceDirectives extends FileAndResourceDirectives { rec(if (path.startsWithSlash) path.tail else path) } + /** + * Check against directory traversal attempts by making sure that the final is a "true child" + * of the given base directory. + * + * Returns "" if the finalPath is suspicious and the canonical path otherwise. + */ + 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)) { + log.warning(s"[$finalFile] points to a location that is not part of [$baseFile]. This might be a directory " + + "traversal attempt.") + "" + } else canonicalFinalPath + } + object ResourceFile { def apply(url: URL): Option[ResourceFile] = url.getProtocol match { case "file" ⇒