path/filepath: Join is susceptible to SlipZip

[rminnich]

Edit: moved from issue title to body for hyperlink: https://snyk.io/research/zip-slip-vulnerability

---

The u-root project has several programs that unarchive files -- cpio, tar, zip, etc.

A few weeks ago we were notified that we are vulnerable to something called ZipSlip. It's a simple enough problem: archives containing paths with .. can end up in unexpected places.

e.g., I am in /bin, and I unpack a cpio, I expect things to end up /bin, and they end up in /home/rminnich/bin/wildthing

This is an easy thing to fix: given a root, base, and a path, p: filepath.Join("/some/place", filepath.Join("/", p))
I can contain the files to base.

The bigger question: do people in general expect that for filepath.Join, things won't creep out of the first directory argument to other places? 3 different people wrote 3 different tools on u-root and missed this, I think because they assumed that everything stayed under path.

This seems like an accident waiting to happen, save that it already did.

Should filepath.Join be changed, or maybe we need a filepath.BaseJoin which guarantees things stay inside the base?

I can fix this for the 3 programs in question, but I'm wondering if it really belongs in the go runtime instead.

What version of Go are you using (go version)?

go version go1.14.3 darwin/amd64

Does this issue reproduce with the latest release?

Repros on every release since filepath.Join was written.

What operating system and processor architecture are you using (go env)?

Does not matter -- it's a filepath question

What did you do?

package main

import "fmt"
import "path/filepath"

func main() {
	fmt.Println(filepath.Join("/bin", "../../cat"))
	fmt.Println(filepath.Join("/bin", filepath.Join("/", "../../cat")))
}

What did you expect to see?

people don't expect to see files creep out of the target directory. Arguably it's a defect of the program, and a bad assumption on the part of the author(s), but still ...

What did you see instead?

We got a CVE ;-)

[OneOfOne]

Actually, I expected that from filepath.Join but not filepath.Clean.

A workaround would be filepath.Join("/bin", filepath.Clean("/"+"../../cat")).

[magical]

I, for one, expect path.Join and filepath.Join to follow their current semantics. .. is a perfectly valid path component and /bin/../../cat is a perfectly valid path for many applications (imagine implementing a shell, for instance).

Some applications might want different semantics. That's fine. They can implement their own version of Join, or use one from a third party library (are there any? surely there are). It looks like there was already a proposal (#20126) to add something like your BaseJoin function to the standard library, which was rejected.

It's possible the documentation could be improved. I'll note that filepath.Join has no examples of joining a path containing a .. component. It's probably worth adding one.

In any case, the Join functions have had their current semantics since the first Go release (as you point out) and there are surely applications that depend on those semantics, so i don't think it's feasible to change them now.

Side note: there are many ways to escape a directory; .. components are only one, symlinks are another. Do u-root's tools defend against malicious symlinks in archives?

[rminnich]

OneOfOne, your example is precisely the example as posted above. magical, your answer is appreciated, I had a feeling I'd get that reaction. We'll fix it in u-root. I'll leave this open to see if anyone else has a better idea.

[cagedmantis]

/cc @robpike @rsc @FiloSottile @katiehockman

[FiloSottile]

This comes up from time to time and indeed we should do something about it, although it's not clear what.

Last time it was someone surprised that Clean can return paths that start with ... I tried adding some docs in CL 227958.

(To be clear, we can't change the behavior of neither Join nor Clean, for compatibility reasons.)

[mdempsky]

Perhaps the path.Join and path/filepath.Join examples should be expanded to at least include an example or two of how ".." behaves?

[magical]

I can send a CL to add examples.

[gopherbot]

Change https://golang.org/cl/249177 mentions this issue: path,path/filepath: add Join examples with ".." components

[magical]

I submitted a couple examples. @rminnich does that help?

Anything left to do here?

[mdempsky]

The examples can be seen here:

https://tip.golang.org/pkg/path/#example_Join
https://tip.golang.org/pkg/path/filepath/#example_Join

[insomniacslk]

looking at this, since the CVE is still open and GitHub complains about using a vulnerable dependency in some other projects. Would https://github.com/cyphar/filepath-securejoin help here? By replacing every call to filepath.Join with a call to securejoin.SecureJoin? This was also discussed at #20126 (and rejected for inclusion in the standard library). While it's not a 100% safe solution without kernel support, it might be a significant step forward for u-root. Any thoughts?

[mpictor]

@magical The examples look good, though I wonder if there should be a note in the description as well. Join is one of those functions whose behavior seems simple enough that I'm not sure I'd bother expanding and reading through the example.

Perhaps something like

Note: attacker-controlled inputs could potentially result in operations being performed outside the expected directory, AKA ZipSlip. If the input is untrusted, check the result for unexpected occurrences of ../ or a leading /.