Akka-http: Directory Traversal Attack vulnerability in Akka HTTP's getFrom(Browseable)Directory directive

[roikonen]

I noticed that when running Akka HTTP server in Windows machine I am able to access all files of the server machine.

Example

With route:
pass { getFromBrowseableDirectory("/some/directory/i/only/want/to/share") }
or
pass { getFromDirectory("/some/directory/i/only/want/to/share") }

I can request for example:
http://localhost:8080/%5c..%5c..%5c..%5c..%5c..%5c..%5c..%5c..%5c..%5cwindows/win.ini
...and download the actual win.ini file that exist in C:/windows

In the case of getFromBrowseableDirectory I can also see all the files there is in the server machine.

That is Directory Traversal Attack vulnerability: https://www.owasp.org/index.php/Path_Traversal

[rbudzko]

FileAndResourceDirective check by tailrec if path try to escape base dir configured in routing by (head.indexOf('/') >= 0 || head == ".."). Backslash check is missing, but I don't think maintaining rules on server side will ever make it 100% safe.

[drewhk]

hm, shouldn't it check after parsing, using the JDK File API? It can canonicalize paths, but I am not an expert here.

[rbudzko]

Well, it will be more powerful than current solution, definitely. JDK *absolutePath seems to do a lot of magic (really magic 😨 ) to check it without calling OS.

[jypma]

In order to do this proper, we'd have to go all-in and check on symlinks etc. as well. Like apache does.

[jypma]

yes, maybe absolutePath will do the symlink dereffing as well?

[drewhk]

Nope, you need CanonicalPath. Absolute Path means that it is not relative to the current directory, but it does not mean that it does not have .. sequences in there. As far as I understood...

[drewhk]

A canonical pathname is both absolute and unique. The precise definition of canonical form is system-dependent. This method first converts this pathname to absolute form if necessary, as if by invoking the getAbsolutePath() method, and then maps it to its unique form in a system-dependent way. This typically involves removing redundant names such as "." and ".." from the pathname, resolving symbolic links (on UNIX platforms), and converting drive letters to a standard case (on Microsoft Windows platforms).

[rbudzko]

Yeah, also it calls native canonicalize0 which could be checked if implemented correctly or if making sys calls.

[jrudolph]

@rbudzko

FileAndResourceDirective check by tailrec if path try to escape base dir configured in routing by (head.indexOf('/') >= 0 || head == ".."). Backslash check is missing

Agreed, it could probably be fixed by adding a backslash check.

but I don't think maintaining rules on server side will ever make it 100% safe.

As long as OS path parsing contains so much logic, I agree, it is hard to make it completely safe. We should still try to be as conservative as possible, so I'd think adding additional hardening could make sense.

@drewhk

hm, shouldn't it check after parsing, using the JDK File API? It can canonicalize paths, but I am not an expert here.

The question is what the exact semantics of getBrowsableDirectories are or should be. I agree it would be safer if we would check that the chosen file is provably a file below the given root path. Though, as @jypma says:

In order to do this proper, we'd have to go all-in and check on symlinks etc. as well. Like apache does.

I.e. one has to define which kind of path traversals are going to be allowed and which not. .. is obviously not allowed but symlinks are often dangerous as well. It's probably somewhat easy to check that one canonical path is a non-symlinked descendant of another one. To allow symlinks (for which there probably are legitimate uses and which is also currently allowed) we would need to add extra checking logic.

[rbudzko]

It's probably somewhat easy to check that one canonical path is a non-symlinked descendant of another one.

@jrudolph How would you do that? I cannot think about any solution without dialing to a good friend OS. Out of curiosity - I've checked JVM behavior and it cannot do that using methods which does not call OS I think.

[2beaucoup]

We could e.g. check if (base + requested).canonical.startsWith(base.canonical), no?

[drewhk]

So what is wrong with canonicalPath? It does seem to resolve symlinks, etc. I think we should provide the _strictest_ semantics, if people need more performance, flexibility, they can write their own directives.

[drewhk]

alternative directive could be one that pre-traverses the directory explicitly listing resources and saving the list in some memory structure. Then only allow exact path matches from the structure. This will not pick up changes in the directory but this can be the safest of all and can be more easily audited.

[jrudolph]

@2beaucoup

We could e.g. check if requested.canonical.startsWith(base.canonical), no?

Yes, that would be the idea.

So what is wrong with canonicalPath? It does seem to resolve symlinks, etc. I think we should provide the strictest semantics, if people need more performance, flexibility, they can write their own directives.

I agree. We can certainly make the default stricter. Though, I'm pretty sure that people already rely on symlinks working for serving directories, so I'm a bit reluctant to remove that functionality completely. Also it seems the symlink security implications might not be as severe as it seems to be only a problem if the same server serves files from different user directories which might not be the most prominent use-case for akka-http driven servers (but of course you never know). So, I'm more concerned about the complexities it brings adding support for symlinks back in.

Would that work:

• add strict checking of canonical path to getFromDirectory and for directory listings in getFromBrowsableDirectory
• add a new parameter / configuration flag follow-symlinks that is set to false by default, and which when enabled will disable the above extra check
• harden the check in FileAndResourceDirectives.fileSystemPath to disallow accessing files with backslashes in their paths (which will be an actual restriction on Linux but one that we might want to still enforce)

[rbudzko]

We could e.g. check if (base + requested).canonical.startsWith(base.canonical), no?

mklink "C:\xyz" C:\Users passed as a C:\xyz to java.io is resolved as C:\xyz. That is why it imho is not resolved as canonical. On Linux box situation is different and canonical names are resolved, so ln -s ~/.ssh ~/xyz passed as a /home/user/xyz will be resolved as /home/user/.ssh.

Also, I'll check how is it possible that linux JVM knows it is .ssh. Is it acceptable that it calls OS for every file query?

[jrudolph]

Ugh... See here: https://bugs.openjdk.java.net/browse/JDK-8003887

[rbudzko]

Yeah, exactly. Wasn't aware it was a known bug. I think we are good since workaround has been given to all windows wanderers - toPath().toRealPath(boolean).

[jypma]

Well, these days nobody runs a server on Windows anymore anyways, right? _(ducks)_ :microphone:

[rbudzko]

Apart of half of the Asia, yes nobody 😉 .

[2beaucoup]

@jrudolph

add a new parameter / configuration flag follow-symlinks that is set to false by default, and which when enabled will disable the above extra check

+1. I'd limit that check to symlinks below by the specified base dir. Symlinks in the basedir should be always followed. The symlink check could be something like (basePath + unmatchedPath).absolute == (basePath + unmatchedPath).canonical.

harden the check in FileAndResourceDirectives.fileSystemPath to disallow accessing files with backslashes in their paths (which will be an actual restriction on Linux but one that we might want to still enforce)

Is it necessary to explicitly check for certain strings? Isn't (basePath + unmatchedPath).canonical.startsWith(base.canonical) enough to ensure that the target file lives inside the specified base dir?

[viktorklang]

are there no methods to check this without resorting to fragile string
matching?

Cheers,
√

On Sep 23, 2016 09:19, "2beaucoup" [email protected] wrote:

@jrudolph https://github.com/jrudolph

add a new parameter / configuration flag follow-symlinks that is set to
false by default, and which when enabled will disable the above extra check

+1. I'd limit that check to symlinks below by the specified base dir.
Symlinks in the basedir should be always followed. The symlink check could
be something like (basePath + unmatchedPath).absolute == (basePath +
unmatchedPath).canonical.

harden the check in FileAndResourceDirectives.fileSystemPath to disallow
accessing files with backslashes in their paths (which will be an actual
restriction on Linux but one that we might want to still enforce)

Is it necessary to explicitly check for certain strings? Isn't (basePath

• unmatchedPath).canonical.startsWith(base.canonical) enough to ensure
  that the target file lives inside the specified base dir?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/akka/akka-http/issues/346#issuecomment-249120526, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAqd78szQuOVw1dd12Kk438yjTxsdixks5qs32SgaJpZM4KDxV2
.

[drewhk]

I personally think that this will be full of wholes no matter what if we try to follow symlinks and don't go the canonicalPath approach.

Also, I still think there might be value in a version that pre-traverses explicitly the directory, saves the entries, and later on it only serves those exact entries. This does not pick up modifications, yes, but it is the safest I can think of.

[jypma]

I don't think one can do more than the suggested (basePath + unmatchedPath).canonical.startsWith(base.canonical).

The aforementioned windows mklink, just like bind mounts on linux, is not something that's typically traversed. I don't expect apache would traverse bind mounds either. So I would consider that out of scope for this directive.

However, since we're relying on JDK implementation, this _is_ something I'd prefer to see unit/integration tests cover on an actual Windows platform. Is there any hope of that?

[rbudzko]

Especially that behavior explained @drewhk can be enforced by default but configurable to old approach which is less safe.

[drewhk]

However, since we're relying on JDK implementation, this is something I'd prefer to see unit/integration tests cover on an actual Windows platform. Is there any hope of that?

I work from Windows but my involvement in Http is minimal, so I don't run the test suite frequently.

[jypma]

I work from Windows but my involvement in Http is minimal, so I don't run the test suite frequently.

I was angling more at running the test suite on a CI that's on a windows server. For stuff that touches on JDK internals. Eventually...

[2beaucoup]

are there no methods to check this without resorting to fragile string
matching?

WDYM? Fishing for special chars or the startsWith method? (I agree if you meant the former.)

[2beaucoup]

java.nio.file.Path.startsWith doesn't even harm any strings! :)

[jrudolph]

@2beaucoup

I'd limit that check to symlinks below by the specified base dir.

These would automatically work with the canonical path solution, right? But these are also the likely uninteresting symlinks. In more "interesting" cases, people could try to plug together a directory to serve by placing symlinks to files and folders outside of the served folder to prevent having to copy everything together.

The symlink check could be something like (basePath + unmatchedPath).absolute == (basePath + unmatchedPath).canonical.

Do we need that to support symlinks inside of the linked directory? Wouldn't the simple canonical solution still work?

@viktorklang

are there no methods to check this without resorting to fragile string
matching?

To check what? The problem is exactly that it is not clearly defined what kind of traversals (move a directory down, move a directory up, follow a symlink, traverse over FS boundaries / mount points etc) should be allowed, how such traversal operations map to actual OS operations, and how those operations are represented in paths.

@drewhk

Also, I still think there might be value in a version that pre-traverses explicitly the directory, saves the entries, and later on it only serves those exact entries. This does not pick up modifications, yes, but it is the safest I can think of.

Interesting idea. Not sure if it solves the problem, though, because we still need to define the allowed traversals and then match the target file with one from the pre-built list. How would you do the matching?

@jypma

The aforementioned windows mklink, just like bind mounts on linux, is not something that's typically traversed.

Traversed by what? It seems that mklink is more akin to ln -s than to bind mounts.

I don't expect apache would traverse bind mounds either.

I don't know, does it? The known security problem with symlinks is that unprivileged users can create symlinks into their own directory to inaccessible files/directories and then let apache follow the symlink and serve any content using apache's permissions which will usually have access to more than the unprivileged user. Mounting is usually restricted to privileged users so the problem might not be as severe.

@2beaucoup

java.nio.file.Path.startsWith doesn't even harm any strings! :)

Nice one if it works. Still need to check if the semantics match exactly.

[drewhk]

Not sure if it solves the problem, though, because we still need to define the allowed traversals and then match the target file with one from the pre-built list. How would you do the matching?

My assumption that a recursive traversal should not escape the parent directory. Following symlinks can be fine here, it is up to the user to put them there in the first place. I assume the behavior here would be exactly the same as if the symlinked directory was actually copied there.

As for matching, the point is to disallow any relative addressing (".." etc) and do an _exact_ match against the discovered paths during traversal. I.e. we canonicalize the paths ourselves, but do not resolve symlinks to their actual path just traverse them just like they were normal part of the directory.

So if I traversed a file that is "foo/bar/file.txt" relative to my current directory, then I cache its JDK Path in a hashmap keyed by "foo/bar/file.txt". A lookeup like "foo/bar/../bar/file.txt" would simply fail as it is not in this map.

[2beaucoup]

POC is here: #348

[jypma]

How about

1. Turn the incoming HTTP URI into a "canonical" one by resolving all .. in the middle of it (it won't be allowed to _start_ with any .., since that would always be illegal)
2. Do the additional restrictions, like not allowing \.
   3a. If follow symlinks is enabled, just read the file by concatenating the URI relative to the FS base
   3b. If follow symlinks is disabled, check the canonical path first, and then read the file

But that may be what @2beaucoup just whipped up :-) I'll give it a proper review on Monday.

[2beaucoup]

@jypma My PR doesn't look for specific strings but lets java.nio.file.Path's toRealPath/resolve/startsWith do the work.

[2beaucoup]

Looks like neither Path#toRealPath, Files#isSymbolicLink nor BasicFileAttributes#isSymbolicLink do consider symlinks on Windows. These always give the same result no matter if LinkOption.NOFOLLOW_LINKS is provided or not. Therefore it seems impossible to detect them (on Windows). :(

Could anybody confirm this plz? I'm testing on Wondows 10 BTW.

[rbudzko]

Ugh... See here: https://bugs.openjdk.java.net/browse/JDK-8003887

Some possible workaround are in comments of JDK bug.

[2beaucoup]

Some possible workaround are in comments of JDK bug.

toRealPath doesn't seem to work. See my comment above.

[jrudolph]

Hmm, this is a bit strange. The path API seems to have supported links in windows for some time now (search for WindowsLinkSupport in the JDK sources). It still might have some quirks, iiuc there's symbolic links and "junctions" in Windows which may have different behavior.

[jrudolph]

We might take an incremental approach:

• for now fix the issue at hand and leave the symbolic link issue as it is and comment ourselves out of it by adding comments to all the directives in question that the directive will follow links and that this might have security implications if users other than the user that runs the server is able to create links in the served directory
• actually try to fix it properly with more testing, similar to what you tried until now, @2beaucoup, by adding an extra setting to the config that control the actual traversal behavior

[jrudolph]

toRealPath doesn't seem to work. See my comment above.

FWIW it seems it neither resolves links on Linux:

Update: Forget that, it was sbt copying files around resolving links in the process...

[jrudolph]

Thanks a lot @roikonen for the report and @2beaucoup for the fix and all others for the discussion. The plan is to release the fix with the next akka version scheduled for tomorrow.

I created https://github.com/akka/akka-http/issues/365 to follow up with better control over following symlinks