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

@jimmyfrasche

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.

Yeah, you're right -- you've convinced me. And ultimately it's very trivial to take a full path and a "root path" and convert them into the two arguments for SecureJoin. I'll update this proposal.

@alexbrainman

But I will spent more time investigating your idea, if you point to the "windows version" of the code in Docker.

I linked it in the original section, but here it is again -- though the function is called FollowSymlinkInScope. Note though that @jimmyfrasche has convinced me to change the API to SecureJoin -- which is similar in spirit (effectively SecureJoin(a, b) is like EvalSymlinksInScope(Join(a, b), a)).

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.

I just checked and couldn't find tests in Docker's source code for the windows integration. The original authors of this extension of Docker's FollowSymlinkInScope are @jhowardmsft and @swernli -- maybe they can offer some more information about how symlink handling is meant to operate on Windows.

I know nothing about plan9, but I know it does not have symlinks.

In which case, SecureJoin would just be the same as Join (though I'm sure someone who is more familiar with Plan 9 could verify this). If Plan9's filesystem is a "pure" tree, then Join is always "secure" -- the purpose of SecureJoin is to not allow links in a DAG filesystem to reach a non-descendant of the root (and that's not possible unless the filesystem is a DAG).

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.

It's a bit hard to explain if you're not familiar with containers (or at least chroot). I tried to give some examples in the issue description, such as a file sharing server that has to open files in a user-controlled directory. Effectively what this API will allow is that a program which takes a path and wants to join it with a "root path" (to resolve a path which must be inside the "root path") to get a path that follows that requirement. Because of symlinks, it's a bit harder than just doing Join (though as you mention it is identical to doing Join on plan9).

I linked it in the original section, but here it is again -- though the function is called FollowSymlinkInScope

Thank you for the link, but none of that code is tested on windows - tests are specifically disabled for windows. That just goes to show how useful it for windows.

It's a bit hard to explain if you're not familiar with containers (or at least chroot).

Maybe it is a good sign that this does not belong in path/filepath?

Alex

but none of that code is tested on windows

There are no unit tests for Windows. The code itself is tested through integration tests (docker cp wouldn't work without the code I linked) but I understand that doesn't help you in understanding how it is meant to work on Windows (which is why I pinged the authors who will know more than me on the topic)

Maybe it is a good sign that this does not belong in path/filepath?

If EvalSymlinks belongs in path/filepath, I don't see why this shouldn't exist in path/filepath either (lack of unit tests in an implementation seems like a flimsy reason to me -- given that that code is used by every Docker installation in the world).

Let me give you an example with Windows paths (though note that I don't use Windows so I can't verify that this example is sane, it's just coming from my reading of this documentation):

> mkdir C:\someroot
> mklink /d ..\..\..\..\..\Windows\System32 C:\someroot\link
> mklink C:\link C:\someroot\linktolink
> mklink D:\some\file C:\someroot\file

I would anticipate that the following functions will return the following values (though I'm not entirely sure about the D:\some\file example and maybe one of the authors could clarify what is "sane" in this case):

SecureJoin("C:\someroot", "link") = "C:\someroot\Windows\System32"
SecureJoin("C:\someroot", "linktolink") = "C:\someroot\Windows\System32"
SecureJoin("C:\someroot", "file") = "C:\someroot\some\file" // or an error

If you wanted actual usecases, I've updated the original description, but here they are in case you can't see them:

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

I'm sure you'll agree those examples are not just Unix-specific.

mkdir C:\someroot
mklink /d ..........\Windows\System32 C:\someroot\link
mklink C:\link C:\someroot\linktolink
mklink D:\some\file C:\someroot\file

I think you got parameters wrong way around:

C:\>mklink
Creates a symbolic link.

MKLINK [[/D] | [/H] | [/J]] Link Target

        /D      Creates a directory symbolic link.  Default is a file
                symbolic link.
        /H      Creates a hard link instead of a symbolic link.
        /J      Creates a Directory Junction.
        Link    specifies the new symbolic link name.
        Target  specifies the path (relative or absolute) that the new link
                refers to.

SecureJoin("C:\someroot", "link") = "C:\someroot\Windows\System32"

I am sorry but how do you propose to implement SecureJoin?

And how is that useful? There is no C:\someroot\Windows directory. Where would you use the "C:\someroot\Windows\System32"?

Any container runtime or program that interacts with container root filesystems.

I know nothing about these. But I am just an average user.

Users of archive/tar where the archive might be malicious

Do you mean code that creates symlinks should be careful. Perhaps, yes. Isn't it up to the archive/tar user to consider all that?

BTW, only administrator can create symlinks on Windows. So it is not common occurance.

ftp servers or other file sharing servers that have to operate on user controlled directories

For this you should use filepath.EvalSymlinks, and then check the result is valid as per whatever rules the program has.

Alex

@alexbrainman

I am sorry but how do you propose to implement SecureJoin?

It has already been implemented and I've already linked to implementations. Please read them.

And how is that useful? There is no C:\someroot\Windows directory. Where would you use the "C:\someroot\Windows\System32"?

os.MkdirAll. This question is like asking "why does filepath.EvalSymlinks not error out if the path doesn't exist"?

I know nothing about these. But I am just an average user.

Okay, so then don't use this function. Problem solved. I've given several examples of security vulnerabilities that have occurred because a function like this was not part of the standard library and people look inside path/filepath and don't realise how symlinks can cause rootfs breakouts.

Do you mean code that creates symlinks should be careful. Perhaps, yes. Isn't it up to the archive/tar user to consider all that?

Not just code that creates symlinks. Code that interacts with paths that may contain symlinks must also be careful. But "being careful" is hard and should be part of the standard library. I don't understand why you appear to be against making the lives of Go developers easier.

I'm not suggesting that someone force Go users to use this utility, but the usefulness of such a function in the standard library is quite clear IMO...

For this you should use filepath.EvalSymlinks, and then check the result is valid as per whatever rules the program has.

That works if you want to disable effective use of symlinks, it doesn't work if you want symlinks to work in a way that a user might expect.

@cyphar you lost me. I can only make judgment about real problems. Your problem seems to be too general for my brain. Sorry.

Maybe others will be more helpful.

Alex

Before putting something into the standard library, can we start with a good shared implementation somewhere that everybody can use?

@bradfitz Yup, I've been working on it and I'll push my implementation to https://github.com/cyphar/filepath-securejoin in the coming days.

Still waiting for implementation, but this all seems way too complex.

https://github.com/cyphar/filepath-securejoin is the implementation, I haven't had time to write tests yet. I will add them in the coming days when I have a chance.

https://github.com/cyphar/filepath-securejoin is the implementation

I noticed you use os.Readlink to implement that. But os.Readlink is broken on windows - see, for example, #10935, #18555 and #19922 for details.

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:

1. Would a solution to the os.Readlink problem possibly lie in some of the code used by filepath.EvalSymlinks?

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