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

io/ioutil: reject path separators in TempDir, TempFile pattern #33920

Open
snyff opened this issue Aug 28, 2019 · 10 comments

Comments

@snyff
Copy link

@snyff snyff commented Aug 28, 2019

In

name := filepath.Join(dir, prefix+nextRandom()+suffix)
The prefix and suffix extracted from the variable pattern are used in filepath.Join. Since there is no filtering in place, this could lead to directory traversal vulnerabilities.

For example, the following value for pattern can create an unexpected behaviour:

ioutil.TempFile("/tmp", path.Base("../../somewhere/else.*.suffix"))

A less-surprising behaviour would be to call path.Base:

name := filepath.Join(dir, path.Base(prefix+nextRandom()+suffix))

@gopherbot gopherbot added this to the Proposal milestone Aug 28, 2019
@gopherbot gopherbot added the Proposal label Aug 28, 2019
@ianlancetaylor ianlancetaylor changed the title proposal: ioutil.TempFile: pattern should prevent directory traversal proposal: io/ioutil: TempFile: pattern should prevent directory traversal Sep 3, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 3, 2019

Are there any examples of real Go code that does not pass a string constant as the second argument to ioutil.TempFile?

@snyff

This comment has been minimized.

Copy link
Author

@snyff snyff commented Sep 3, 2019

I did a quick Github search for it and couldn't find it in the first few pages. But that's only publicly available code

Regardless, the current behaviour is unexpected and this small change can avoid people shooting themself in the foot.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 3, 2019

Well, it's a tradeoff. If there are programs intentionally using directory traversal, we break them. If there are programs accidentally using it, we fix them. If we don't know of any actual cases that would be fixed, the conservative approach is to leave well enough alone.

@snyff

This comment has been minimized.

Copy link
Author

@snyff snyff commented Sep 3, 2019

I couldn't find any publicly available code using the directory traversal on purpose. My guess is that people would put the traversal in the first argument if they need to...

@FiloSottile FiloSottile added the Security label Sep 4, 2019
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Sep 4, 2019

If we can't find examples of it being used either intentionally or unintentionally, in doubt I'd make it behave safely, which presumably is not going to break much, but might prevent security issues in the future.

Also, pattern is not even documented to accept path separators, and they definitely make more send in dir.

@snyff

This comment has been minimized.

Copy link
Author

@snyff snyff commented Nov 19, 2019

@ianlancetaylor what do you think?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 19, 2019

The comment by @FiloSottile sounds reasonable to me.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

name := filepath.Join(dir, path.Base(prefix+nextRandom()+suffix))

Please don't do this. What if suffix has a slash in it? Then the randomness is removed entirely.
If we're going to do anything at all, we should simply make TempFile return an error when pattern contains a file system path separator.

@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

We shouldn't make this problematic case (which we think does not happen ever in real programs) silently succeed with an unexpected meaning. But assuming we simply return an error instead, this seems like a likely accept based on the discussion. Everyone agrees with doing something and no one thinks it matters for real programs.

Leaving open for a week for final comments.

@rsc rsc changed the title proposal: io/ioutil: TempFile: pattern should prevent directory traversal proposal: io/ioutil: reject path separators in TempDir, TempFile pattern Nov 27, 2019
@rsc rsc moved this from Incoming to Likely Accept in Proposals Dec 4, 2019
@rsc rsc changed the title proposal: io/ioutil: reject path separators in TempDir, TempFile pattern io/ioutil: reject path separators in TempDir, TempFile pattern Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2019

No change in consensus, so accepting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Likely Accept
5 participants
You can’t perform that action at this time.