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

x/exp/cmd/txtar: expansion of environment variables and writing outside of the current directory seems risky #46741

Open
mdempsky opened this issue Jun 14, 2021 · 6 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

The x/exp/cmd/txtar command does not limit the scope of files it might overwrite, which seems risky (e.g., see https://snyk.io/research/zip-slip-vulnerability). Additionally, the expansion of environment variables could make it even easier for attackers to construct malicious files:

$ go install golang.org/x/exp/cmd/txtar@latest
$ cat hmm.txt
-- $HOME/.ssh/not_authorized_keys --
hmm
$ txtar -x < hmm.txt
$ cat ~/.ssh/not_authorized_keys
hmm

(Being plain text gives a small advantage that it's a bit harder to hide the malicious path, but for sufficiently large txtar files, a user is unlikely to review all of the file paths.)

I'd recommend that x/exp/cmd/txtar default to only writing to files within the current directory. If any resolve outside of the current directory, txtar should exit with an error. (I think exiting without writing any files would be safest, but it seems fine to still write in-scope files if that happens to be easier with txtar's implementation.) A flag can be provided to allow expansion to arbitrary file paths.

I'd also recommend not expanding environment variables by default, instead requiring a flag for that functionality.

/cc @bcmills @rsc @FiloSottile

@mdempsky mdempsky added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jun 14, 2021
@mdempsky mdempsky added this to the Unplanned milestone Jun 14, 2021
@bcmills
Copy link
Contributor

bcmills commented Jun 14, 2021

A flag can be provided to allow expansion to arbitrary file paths.

Or — perhaps even better! — to provide an explicit list of allowed variables.

@mdempsky
Copy link
Member Author

So I see two related risks:

  1. txtar can write outside the current directory. This is a concern even without variable expansion; e.g., file paths starting with / or ../.
  2. txtar's behavior can be sensitive to environment variables. This makes the above issue riskier (e.g., easy to point to files in $HOME, rather than needing to guess where that might be), but I think it also has concerns of its own (e.g., create a file named $USER within a directory, and then rely on this file being processed by a glob later, revealing the user that ran txtar).

I think providing flexibility for an allow list of safe environment variables would be okay too. I just didn't want to request too many knobs or for too much complexity to be added. I just want that by default when I run txtar on untrusted input, that I don't need to worry about my SSH files being clobbered. :)

@bcmills
Copy link
Contributor

bcmills commented Jun 14, 2021

I just want that by default when I run txtar on untrusted input

To be clear: you should be careful about untrusted txtar inputs either way. txtar archives can include dotfiles (independent of variable expansion), and there are an awful lot of ways folks can inject malicious behavior via various dotfiles in the working directory (such as .git/config).

@jupj
Copy link
Contributor

jupj commented Dec 13, 2021

I sent two CLs for this:

  • allow only extraction within the current dir and add an --unsafe flag for extraction outside the current dir
  • add an --env flag for allowed specific environment variables to be expanded

These flags currently affect only the archive extraction, i.e. they have no effect when writing the archive.

I did experiment with applying the flags also when writing archives, but it seems to me that that would add more complexity than needed.

@gopherbot
Copy link

Change https://golang.org/cl/371274 mentions this issue: cmd/txtar: extract only in current dir by default

@gopherbot
Copy link

Change https://golang.org/cl/371275 mentions this issue: cmd/txtar: add flag for environment variables

gopherbot pushed a commit to golang/exp that referenced this issue Dec 16, 2021
Change the behavior of txtar so that files are extracted only in the
current directory, or a subdirectory of the current directory, by
default.

If the archive contains a path outside the current directory, txtar will
print an error message and quit with exit code 1. It will not extract
any files in this case. This also applies if an environment variable
used in the archive expands into a path outside the current directory.

Add flag -unsafe to remove the restriction and allow extracting files
outside the current dir.

Updates golang/go#46741

Change-Id: Ic12fb8286c5f2a930addd82dcfce196d4a04054c
Reviewed-on: https://go-review.googlesource.com/c/exp/+/371274
Trust: Cherry Mui <cherryyz@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants