proposal: path/filepath: addition of SecureJoin helper

[cyphar]

Currently the standard library does not provide any "simple" helper to avoid filepath.Join path traversals. As a result, I've had to include similar functions in several projects:

• Docker has several implementations that are pretty similar.

• runC / libcontainer has one which is used by several other projects.

• umoci.

The intention of this addition is to make it much easier for Go developers to do "the right thing" when it comes to path safety. Note that the above examples (intentionally) only operate lexically, but SecureJoin should operate on the actual filesystem in order to earn the "secure" label.

Docker already has an implementation of symlink evaluation within a root filesystem (called FollowSymlinkInScope), which safely resolves all symlink components of a path as though the process was in a chroot. The algorithm used by that code could be used to implement SecureJoin.

There are many Go programs that really need this sort of helper function:

• Any container runtime or program that interacts with container root filesystems. If a user controls the root filesystem, they can trick said runtime into evaluating files on the host unless the program implements some sort of rooted evaluation of a path.

• Users of archive/tar where the archive might be malicious need to be careful of cases where an archive entry's name contains a symbolic link controlled by the archive. Unless the archive's paths are securely joined with the extraction point of the archive, it's possible that such an archive will cause the archive/tar user to access files outside the extraction point of the archive.

• ftp servers or other file sharing servers that have to operate on user controlled directories. If a path contains user-controlled symlinks they may be able to access paths outside of the context that a file sharing server may expect.

• Web servers with user-defined paths have similar issues.

With helper methods like these in the standard library, recent issues like minio's path traversal vuln would hopefully happen less often. And with a name like SecureJoin one would hope that users will be incentivised to use this method (and may in fact become aware of the security issues of using filepath.Join with user-controlled path components).

One important thing to note is that symlink races will be out-of-scope for SecureJoin, simply because it's not possible to solve that problem within a standard library package.

If you'd like I could create a new project that contains just these two helper function implementations so they can be better reviewed for inclusion before I create a PR on gerrit.

[diogomonica]

I agree that a method like this would be useful/make it easier to do the right thing. +1

[bradfitz]

Regular reminder: https://golang.org/wiki/NoMeToo

[ianlancetaylor]

I'm sorry, I'm not clear on precisely what you are proposing. Can you write the docs and function signature of the function that you want? Thanks.

[cyphar]

@ianlancetaylor I linked to sample implementations, but sure here's the signatures and docstrings I would use. The actual name of the functions could be improved (I couldn't think of a nice name for CleanComponent because CleanPath doesn't make sense in path/filepath given that filepath.Clean already exists).

// CleanComponent lexically sanitises the given string so that it is a
// lexically safe path to use in filepath.Join and similar functions, such that
// Join(a, CleanComponent(b)) will always be a child path (or equal to) a.
func CleanComponent(component string) string

// EvalSymlinksInScope returns the given path name after the evaluation of any
// symbolic links in the scope of the given root path. Absolute symlinks will
// be resolved relative to the given root, and the returned string will always
// be within the provided root path. The given path must be within the root to
// begin with.
func EvalSymlinksInScope(path, root string) (string, error)

The implementation of EvalSymlinks would then become:

// EvalSymlinks is equivalent to EvalSymlinksInScope(path, "/"), with all
// symlinks being evaluated in the scope of the root of the filesystem tree.
func EvalSymlinks(path string) (string, error) {
        return EvalSymlinksInScope(path, "/")
}

[alexbrainman]

// EvalSymlinks is equivalent to EvalSymlinksInScope(path, "/"

How would that work on windows?

Alex

[cyphar]

@alexbrainman I wrote the docstring to explain it in terms that most programmers would understand (that EvalSymlinksInScope is a generalisation of the EvalSymlinks function) -- EvalSymlinksInScope(path, VolumeName(path)) would probably be more accurate but I don't use Windows. Note that there is a Windows implementation of EvalSymlinksInScope already (Docker uses it and I linked to it in the original issue description).

[alexbrainman]

@alexbrainman I wrote the docstring to explain it in terms that most programmers would understand (that EvalSymlinksInScope is a generalisation of the EvalSymlinks function)

Thank you for trying. I consider myself a programmer, but I fail to see how EvalSymlinksInScope would work.

// EvalSymlinksInScope returns the given path name after the evaluation of any
// symbolic links in the scope of the given root path.

What is "the scope of the given root path."?

... Absolute symlinks will
// be resolved relative to the given root,

That is not what Windows does. Why should EvalSymlinksInScope work differently to Windows?

but I don't use Windows

You are proposing new functions for path/filepath package, and the package "implements utility routines for manipulating filename paths in a way compatible with the target operating system-defined file paths". I expect your proposal to consider all operating systems that Go supports. Especially that you are trying to do "the right thing" when it comes to path safety.

Unless these functions are for Unix only? Is that your intention?

Alex

[cyphar]

@alexbrainman

but I fail to see how EvalSymlinksInScope would work. [...] What is "the scope of the given root path."?

The intention is to allow for a program to evaluate symlinks in a similar way to how the symlink would be evaluated if you preceded the call with Chroot(root). In other words, .. cannot be used to escape the provided root path and absolute symlinks are resolved relative to the provided root. Here is a few examples to show what I mean (I have linked the implementation and would ask that you at least skim it, though I will admit the one in Docker isn't exactly legible):

// Imagine these were preceded with the following shell commands:
//   $ mkdir -p /tmp/root/some/directories
//   $ ln -s /some/symlink /tmp/root/some/directories/symlink
//   $ ln -s ../../../../../../etc/passwd /tmp/root/some/symlink
//   $ ln -s /youlose /some/symlink
EvalSymlinksInScope("/tmp/root/some/directories/symlink", "/") // = "/youlose"
EvalSymlinksInScope("/tmp/root/some/directories/symlink", "/tmp") // = "/tmp/some/symlink"
EvalSymlinksInScope("/tmp/root/some/directories/symlink", "/tmp/root") // = "/tmp/root/etc/passwd"
EvalSymlinksInScope("/tmp/root/some/symlink", "/") // = "/etc/passwd"
EvalSymlinksInScope("/tmp/root/some/symlink", "/tmp") // = "/tmp/etc/passwd"
EvalSymlinksInScope("/tmp/root/some/symlink", "/tmp/root") // = "/tmp/root/etc/passwd"

The main things to notice is that it would mean that a program that wants to evaluate symlinks within a "root filesystem" 'context' they don't have to worry about escapes from the root filesystem (and also symlinks within that filesystem are "safe"). The Windows equivalents to the above I imagine operate in a similar way, but I'm not familiar enough with Windows symlinks to be sure.

That is not what Windows does. Why should EvalSymlinksInScope work differently to Windows?

I am not fully familiar with how symlinks work on Windows, can you enlighten me? The purpose of EvalSymlinksInScope is to allow for a program to compute what the "correct" evaluation of a symlink would be if you were in a chroot (using the Unix parlance). What would be sane behaviour for Windows in a similar context (though I don't think Windows has chroots, imagine if the directory was a root mountpoint)?

If we had to limit it to Unix I would be saddened (especially since Microsoft has worked on the implementation in Docker, which leads me to believe there is a sane equivalent to this behaviour on Windows).

but I don't use Windows

You are proposing new functions for path/filepath package

I'm aware that path/filepath is meant to make operating-system independent fs code easier to write (I use it quite extensively). The reason I said "I don't use Windows" is to illustrate that I am not entirely sure what the right semantics are, but some Microsoft devs have already implemented the Windows support into the Docker implementation -- so maybe reading their implementation would be a better start than asking me about an operating system I haven't used in several years? If we have to change the semantics, that's fine and I can ask the original developers for their opinion, it's just that asking me "how should this work on Windows" is a bit redundant given that I've already linked to an implementation that works on Windows.

I apologise for being sloppy and using the Unix equivalent in a hypothetical docstring, but that shouldn't be construed as "I don't understand that path/filepath is for making operating-system independent fs code easier to write".

[jimmyfrasche]

If I understand this correctly, the ultimate goal is to sanitize a user provided path in such a way that you can join it to an application provided path with the assurance that the joined path is always contained in the application path.

If that's the case, do all of these need to be exposed or would an API like the following suffice:

func SecureJoin(trustedRootPath, untrustedPath string) (string, error)

Do the other functions have uses that I'm not imagining?

[cyphar]

@jimmyfrasche [I had a comment earlier that detailed what the proposed functions uses are, but I've thought about it some more and below is my only real gripes with SecureJoin]

You're actually right that with SecureJoin you could remove most of the need for two separate functions. There's just a few concerns I had:

• If someone wants to do something like EvalSymlinksInScope with absolute paths, they'd have to use filepath.Rel the give the second argument and I'm not entirely sure whether that will have issues with symlink/.. semantics on non-plan9 systems.

• If a user wanted a purely lexical SecureJoin it's not entirely clear how to do it in a sane way -- the usecase I'm thinking of is when you want to store a "safe path" in a database and you don't have access to the filesystem it will be resolved relative to at that moment. Sure, you could just pass a trustedRootPath which has some prefix that you remove later (ensuring that there will be no symlink path components that will be expanded) -- but that seems inelegant to me.

Though aside from those two concerns, I'm open to the idea. Do you think my concerns are warranted?

Also I'm a bit saddened because EvalSymlinksInScope seems (to me) to be a nice extension of the already-existing EvalSymlinks (though we could make it a wrapper around SecureJoin("/" or Getwd(), path) or similar).

[jimmyfrasche]

SecureJoin is probably a bad name. It's just the first thing I thought of. I'm not married to it. I'm using it just to call it something.

I'm not opposed to EvalSymlinksInScope or CleanComponent. I just can't think of a concrete use case for either of them other than to implement a SecureJoin. Again, this could be paucity of imagination or lack of research on my part. I've never needed anything like this. I realize there could be a need that I am unaware of. I'm happy to be proven wrong because that would mean I learned something new.

Regardless, even if CleanComponent and EvalSymlinksInScope go in, I'd still want something like a SecureJoin to let people know "this is what you want 90% of the time: use this, this way, in these circumstances and then you wont have these problems". Even if it's relatively straightforward to construct a SecureJoin with those primitives, it doesn't mean everyone will do so correctly or even realize that it's what they want to do or that they need to use those specific primitives to do it safely. The easier it is to be secure and the more obvious it is how to be secure, the more programs will be secure.

If someone wants to do something like EvalSymlinksInScope with absolute paths,

That assumes there's a use for EvalSymlinksInScope outside of doing a SecureJoin. Where I'm getting lost with this is trying to Imagine a situation where you would want to EvalSymlinksInScope without the ultimate goal of doing a SecureJoin.

If a user wanted a purely lexical SecureJoin it's not entirely clear how to do it in a sane way

What does it mean to ensure there are no path traversals when you can't be sure there's no path traversals until you hit the file system and verify nothing resolves out of scope?

That question is half rhetorical and half ignorant. Unless I'm missing something, it seems like you either hit the file system and know it's actually safe or you do some lexical preprocessing so that it appears to be safe but you can't actually tell until you go to use it, at which point you need to use SecureJoin anyway.

In your example, wouldn't it be just as effective to store the raw path in the DB and do the SecureJoin against it when it's used? Even if you do preprocessing, wouldn't you still need to use SecureJoin after you pull it from the DB to make sure there are no symlink shenanigans? It seems like the preprocessing would just make the DB table look prettier at best and give a false sense of security at worst. (I spend a good part of my work week staring into databases, so I'm not opposed to making the data look prettier, mind you).

[alexbrainman]

The Windows equivalents to the above I imagine operate in a similar way, ...

You are obviously know more about this subject then me.

I am not fully familiar with how symlinks work on Windows, can you enlighten me?

I don't know much about the subject myself. I am learning on the job (as I fix problems in Go).

I am learning that there is no single symlink type. Instead there are "directory symbolic links", "file symbolic links" (CreateSymbolicLink) and "directory junctions" (there is no documented Windows API to create those). I am learning that there is no Windows API to read symlink target, like syscall.Readlink. I am learning that some symlinks are handled by Windows and its services transparently without any documentation what these are and how they work (see issue #18555 and #19922).

That is all I can think of. I hope it helps you, but I don't see how.

What would be sane behaviour for Windows in a similar context (though I don't think Windows has chroots, imagine if the directory was a root mountpoint)?

I have no idea what you are talking about. Sorry.

If we had to limit it to Unix I would be saddened

You forget that your new API have to work on plan9 too. I know nothing about plan9, but I know it does not have symlinks.

but some Microsoft devs have already implemented the Windows support into the Docker implementation -- so maybe reading their implementation would be a better start than asking me

I think it is up to you to sell your proposal to Go users, including Windows users. If you did not consider any OS, but Unix, then your proposal does not belong to path/filepath package.

But I will spent more time investigating your idea, if you point to the "windows version" of the code in Docker. I am also more interested to see some tests for these functions that test some Windows paths - that should help me understand what is proposed. Thank you.

Sorry if I am bit critical of your proposal, but I really cannot see how it could be useful for me personally. And I have been using Windows and Go for awhile. What about other Windows Go users.

Alex