proposal: path/filepath: addition of SecureJoin helper · Issue #20126 · golang/go · GitHub

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.

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

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

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.

@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, "/")
}

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

How would that work on windows?

Alex

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

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

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?

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

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).

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

But os.Readlink is broken on windows

Doesn't that imply that the Go stdlib is broken and should be fixed accordingly (as opposed to my implementation)? Though I would like to have some advice on how to write tests that work sanely between Windows and *nix (especially since filepath.Join cleans paths which is not favourable for testing the invariants of filepath.SecureJoin).

Doesn't that imply that the Go stdlib is broken and should be fixed accordingly

I am not sure what that implies.
I am just telling you that windows os.Readlink is broken in some situations. So you cannot assume your code will work as you expect it to work in these situations.

Alex

I am just telling you that windows os.Readlink is broken in some situations. So you cannot assume your code will work as you expect it to work in these situations.

I understand, I'm just not sure what I'm meant to do with that information... 😕 Are you saying I should fix os.Readlink on Windows first? Or re-implement it myself? Or should I just give up on trying to make it work on Windows because symlinks are fundamentally broken on that platform?

I understand, I'm just not sure what I'm meant to do with that information... Are you saying I should fix os.Readlink on Windows first? Or re-implement it myself?

I do not expect you to fix os.Readlink on Windows. I do not know how to fix it myself. Perhaps it is not possible to implement os.Readlink for some situations on Windows. I suspect os.Readlink was inspired by unix readlink - but Windows does not have similar API.

Perhaps you can implement filepath.SecureJoin without os.Readlink. Can you use os.Stat instead? os.Stat should work on windows.

Or should I just give up on trying to make it work on Windows because symlinks are fundamentally broken on that platform?

Sorry but I do not know. I still do not see any use for filepath.SecureJoin personally.
And do not worry about me - find other Windows users for your new filepath.SecureJoin API. Perhaps I will come around.

Alex

@cyphar, I think @alexbrainman's point is that we can't offer a portable function like SecureJoin that's implemented using pieces that aren't currently portable or fully implemented on all operating systems. And we're not interested in adding OS-specific stuff in the standard library. So if SecureJoint can't work everywhere consistently, it's a no go. That doesn't mean you have to fix ReadLink on Windows, though. It just means any inclusion in the standard library requires that the addition works portably everywhere in the same way.

we can't offer a portable function like SecureJoin that's implemented using pieces that aren't currently portable or fully implemented on all operating systems.

So, EvalSymlinks works on Windows (or I assume it does by the standards you've outlined) but implementing it requires having a concept of "evaluating a symlink". In particular that's all implemented with walkLink that handles some Windows quirks which (apparently) are not handled by os.Readlink. Two questions:

Would a solution to the os.Readlink problem possibly lie in some of the code used by filepath.EvalSymlinks?
I didn't use the existing internals of filepath because I didn't really want to copy a bunch of the code to another project (and felt it was nice that you could do it with just the public APIs) but if you prefer I can try to see whether walkLink and family can be used (which as above I assume work on Windows).

So if SecureJoin can't work everywhere consistently, it's a no go.

As I've mentioned in the thread before, I don't believe there is a fundamental reason why SecureJoin cannot work on Windows. I will admit that UNC path semantics are something I am not familiar with (I haven't used Windows in any real capacity for ~8 years) but I don't see why multiple drives or other quirks should cause fundamental issues here. I'm not entirely sure how testing should be done with regards to the Unix equivalent to / but that's a topic for discussion after the PoC is considered acceptable.

As an aside, I have ~95% test coverage on filepath-securejoin now on both OS X and Linux (Travis doesn't support Windows).

Actually I just checked and EvalSymlinks uses os.Readlink. If os.Readlink is seriously broken there are more fundamental issues here than this proposal IMO. There's clearly a problem somewhere I'm just trying to understand what is expected from me in order to move this proposal forward.

As an aside, I have ~95% test coverage on filepath-securejoin now on both OS X and Linux (Travis doesn't support Windows).

I am happy to test your code - I have Windows computers. Just ping me when you need help.

Actually I just checked and EvalSymlinks uses os.Readlink. If os.Readlink is seriously broken there are more fundamental issues here than this proposal IMO. There's clearly a problem somewhere I'm just trying to understand what is expected from me in order to move this proposal forward.

Yes EvalSymlinks is currently broken as os.Stat was broken before CL https://go-review.googlesource.com/#/c/41834/ was submitted. People complained about os.Stat, but no one created issues for EvalSymlinks. I am planing to fix EvalSymlinks in a similar way to os.Stat fix + GetFinalPathNameByHandle (as per https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/ suggestion). I am not sure when I will do that. Feel free to use EvalSymlinks if that achieves what you need.

Alex

It seems like the right next step is for the community to converge on a shared implementation, maybe @cyphar's. There doesn't seem to be a compelling reason for the standard library to provide this - it's a very niche kind of thing, and it's unclear to me that it's even possible to do 100% correctly at all from a library (as opposed to from inside the operating system kernel).

Any chance to move github.com/cyphar/filepath-securejoin to golang.org/x so as to incubate it officially?
After it gets matured it can be promoted to path/filepath.SecureJoin, and should be ported to other languages as well. (especially to Java: https://github.com/snyk/zip-slip-vulnerability )

@AkihiroSuda, golang.org/x isn't for incubating. It's fine at github.com/cyphar.

As an aside, @rsc is basically correct that doing scoping in a completely correct way is not really possible outside of the kernel. You can do scoping in a way where you can detect changes in the path after you've scoped it (which we don't do at the moment -- because it's very hard and only works on Linux, and also relies on some kernel bugs that really need to be fixed) -- so it could be done in a fail-safe way if an attacker wins in the race against you.

proposal: path/filepath: addition of SecureJoin helper · Issue #20126 · golang/go · GitHub

Repository navigation

proposal: path/filepath: addition of SecureJoin helper #20126

13 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

proposal: path/filepath: addition of SecureJoin helper · Issue #20126 · golang/go · GitHub

Repository navigation

proposal: path/filepath: addition of SecureJoin helper #20126

Description

Activity

diogomonica commented on Apr 26, 2017

bradfitz commented on Apr 26, 2017

ianlancetaylor commented on Apr 26, 2017

cyphar commented on Apr 26, 2017

alexbrainman commented on Apr 26, 2017

cyphar commented on Apr 27, 2017

alexbrainman commented on Apr 27, 2017

cyphar commented on Apr 27, 2017

jimmyfrasche commented on Apr 27, 2017

cyphar commented on Apr 28, 2017

jimmyfrasche commented on Apr 28, 2017

alexbrainman commented on Apr 30, 2017

13 remaining items

cyphar commented on May 16, 2017

alexbrainman commented on May 16, 2017

cyphar commented on May 16, 2017

alexbrainman commented on May 16, 2017

bradfitz commented on May 16, 2017

cyphar commented on May 16, 2017

cyphar commented on May 16, 2017

alexbrainman commented on May 17, 2017

rsc commented on Jun 12, 2017

AkihiroSuda commented on Jun 6, 2018

bradfitz commented on Jun 6, 2018

cyphar commented on Jun 7, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions