x/exp/cmd/txtar: expansion of environment variables and writing outside of the current directory seems risky #46741
Comments
mdempsky
added
help wanted
NeedsFix
Or — perhaps even better! — to provide an explicit list of allowed variables. |
|
So I see two related risks:
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 |
To be clear: you should be careful about untrusted |
|
I sent two CLs for this:
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. |
|
Change https://golang.org/cl/371274 mentions this issue: |
|
Change https://golang.org/cl/371275 mentions this issue: |
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>
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>
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:
(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
The text was updated successfully, but these errors were encountered: