Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

path/filepath: Join is susceptible to SlipZip #40373

Open
rminnich opened this issue Jul 23, 2020 · 12 comments
Open

path/filepath: Join is susceptible to SlipZip #40373

rminnich opened this issue Jul 23, 2020 · 12 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@rminnich
Copy link
Contributor

rminnich commented Jul 23, 2020 •
edited by ALTree

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 ;-)
@mdlayher mdlayher changed the title filepath.Join is susceptible to SlipZip (https://snyk.io/research/zip-slip-vulnerability) path/filepath: Join is susceptible to SlipZip Jul 23, 2020
@OneOfOne
Copy link
Contributor

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

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

@magical
Copy link
Contributor

magical commented Jul 24, 2020

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
Copy link
Contributor Author

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 cagedmantis added this to the Backlog milestone Jul 27, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 27, 2020
@cagedmantis
Copy link
Contributor

/cc @robpike @rsc @FiloSottile @katiehockman

@FiloSottile
Copy link
Contributor

FiloSottile commented Jul 27, 2020 •
edited

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
Copy link
Member

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

@magical
Copy link
Contributor

magical commented Aug 13, 2020

I can send a CL to add examples.

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Aug 19, 2020
@magical @robpike
path,path/filepath: add Join examples with ".." components
98a0071 
People sometimes expect Join to trim .. components from its arguments
before joining, and are surprised that it doesn't. This is bad if they
were relying on that assumed behaviour to prevent directory traversal
attacks.

While a careful reading of the documentation for Join and Clean
might dispel this notion, it is not obvious at first glance.

Add a case to the examples to nudge people in the right direction.

Updates #40373

Change-Id: Ib5792c12ba1000811a0c0eb77048196d0b26da60
Reviewed-on: https://go-review.googlesource.com/c/go/+/249177
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@magical
Copy link
Contributor

magical commented Aug 19, 2020

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

Anything left to do here?

@mdempsky
Copy link
Member

The examples can be seen here:

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

@insomniacslk
Copy link

insomniacslk commented Jul 30, 2021 •
edited

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
Copy link

mpictor commented Sep 23, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
9 participants
@mdempsky @rminnich @cagedmantis @magical @mpictor @OneOfOne @FiloSottile @insomniacslk @gopherbot