raboof/pekko
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages Wiki Activity Actions 10

Directory traversal fix (backport to akka-http 2.4.x) (#21584)

Browse source 
* =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)
This commit is contained in:
Johannes Rudolph 2016-09-30 11:01:36 +02:00 • committed by Konrad Malawski
parent c49ef96f11
commit 3cfb815b94
4 changed files with 149 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

1
akka-http-tests/src/test/resources/dirWithLink/linked-dir Symbolic link
View file

@ -0,0 +1 @@
../subDirectory

1
akka-http-tests/src/test/resources/sübdir/sample späce.PDF Normal file
View file

@ -0,0 +1 @@
This is PDF

99
akka-http-tests/src/test/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectivesSpec.scala
View file

@ -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")

59
akka-http/src/main/scala/akka/http/scaladsl/server/directives/FileAndResourceDirectives.scala
View file

@ -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" ⇒