Skip to content

std::env::temp_dir should return env var XDG_RUNTIME_DIR under Linux if declared #39081

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

Closed
sanmai-NL opened this issue Jan 15, 2017 · 16 comments
Closed
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@sanmai-NL
Copy link

Currently, Rust programs under Unix will use $TMPDIR as root temporary directory. This normally means /tmp/.

This is not optimally secure with most modern Linux distros. $TMPDIR is not guaranteed to be private.

Instead, I propose using the value of $XDG_RUNTIME_DIR first, since that is most likely set.

Meta

rustc --version --verbose:

rustc 1.16.0-nightly (1a2ed98d3 2017-01-13)
binary: rustc
commit-hash: 1a2ed98d344b6cbddc57db8841b42f935877e08d
commit-date: 2017-01-13
host: x86_64-unknown-linux-gnu
release: 1.16.0-nightly
LLVM version: 3.9
@sanmai-NL sanmai-NL changed the title std::env::temp_dir should return env var XDG_RUNTIME_DIR under Linux if it exists std::env::temp_dir should return env var XDG_RUNTIME_DIR under Linux if declared Jan 15, 2017
@Stebalien
Copy link
Contributor

Stebalien commented Jan 15, 2017

In theory, I'm all with you (however, IMO, it should be $XDG_RUNTIME_DIR/tmp); I've actually spent way too much time thinking about this. However, there are several big problems with this proposal:

  1. Nobody else does it this way. The standard is to use $TMPDIR and fallback on /tmp and most users will expect that.
  2. This would be a breaking change (the current behavior is documented).
  3. XDG_RUNTIME_DIR has a 0700 mode (by spec). Programs may expect that other programs running as other users (e.g., less privileged users) will be able to read temporary files put in the directory returned by temp_dir.

The real current best solution is to set TMPDIR on your system (and get Linux distributions to agree on how to do this correctly). Eventually, we might even be able to get the systemd pam service to do this.


Also, FYI, issues like this usually go in the RFC repo (https://github.com/rust-lang/rfcs/issues).

@est31
Copy link
Member

est31 commented Jan 16, 2017

Also, note that XDG is only for the GNU/Linux desktop, and not for the server.

@sanmai-NL
Copy link
Author

sanmai-NL commented Jan 16, 2017

@Stebalien:
So the current best solution would be ‘altering $TMPDIR as a user’*.

I do think issue 1 and 2 are actually a single issue. As you point out, the programmer (‘user’) may expect the behavior as documented now. This behavior would indeed change, but only superficially, since the new behavior may very well match old behavior. That is the case when a user chooses to apply the current best solution you proposed*. To rephrase, in case the user sets up $TMPDIR in the (sensible) way you propose*, software making assumptions about the nature of std::env::temp_dir() may fail on issue 3 just as well. This implies that the old behavior gave insufficient guarantees anyway. It's surely worth a look, though, what's done in other standard libraries. That IMO shouldn't, however, be the sole motivation to not make changes, otherwise everyone will stay doing the same given better alternatives. Issue 3 is, I think also an extension of issues and 1 and 2. The only guarantee that temp_dir() now gives regarding the TMPDIR env var is that it will use it if it's a filesystem path. Just for the sake of argument, one can reason in another way about this. If * is a sensible action to take (i.e. is not expected to lead to failure in practice) by an experienced Rust programmer (in his user role) like you, and you have already thought the ‘temporary directory issue’ through, then wouldn't it be good to refine temp_dir()s behavior to the same effect? (BTW, if $TMPDIR is set to a non-directory path, that's accepted as well currently but it shouldn't I think.)

Perhaps there should be temp_dir_private() in addition to temp_dir(), as a cross-platform way to explicitly store temporary data privately. This allows programmers to express intent better.

My thinking was that the original proposal isn't a language change but a bug fix/improvement to the standard library, so not an RFC candidate. Perhaps I'm mistaken, if so, can someone point to some rules regarding this I overlooked?

@sanmai-NL
Copy link
Author

sanmai-NL commented Jan 16, 2017

@est31:
That's what the original XDG (containing ‘Desktop’) name would suggest, but looking it up I can only find evidence to the contrary ... The XDG Base Directory Specification (XDGBDS) has been brought up as relevant to typical headless host software such as tmux as well, and that wasn't contested. Also see the Debian handbook and Wiki. These sources do not tie the XDGBDS to graphical hosts, whereas the handbook does make that distinction elsewhere (‘Graphical desktops usually (...)’).

Also note that I propose that $XDG_RUNTIME_DIR shall be preferred if declared, not exclusively depended on.

@Stebalien
Copy link
Contributor

I do think issue 1 and 2 are actually a single issue.

One and two really are different issues. In 1, I'm referring to the fact that Rust, as a programming language, really shouldn't be dictating stuff like this. Changes like this should start at a distro level and go through some sort of standardization process like a Freedesktop spec. In 2, I'm referring to Rust's stability guarantees.

This behavior would indeed change, but only superficially, since the new behavior may very well match old behavior.

I disagree but that doesn't really matter. It's still a large breaking change by Rust's standards.

To rephrase, in case the user sets up $TMPDIR in the (sensible) way you propose*, software making assumptions about the nature of std::env::temp_dir() may fail on issue 3 just as well.

Currently, Rust is behaving correctly according to the POSIX spec (as far as I can tell). Also, I should have noted in my comment that I set the mode of my TMPDIR to 0755 for precisely this reason.


Really, at the end of the day, I think this issue is much bigger than Rust and should be solved by distros first.

@est31
Copy link
Member

est31 commented Jan 16, 2017

It's still a large breaking change by Rust's standards.

Its not. Rust reserves itself the right to introduce breakage where there is a bug, and ignoring distro standards I think counts as a bug.

@Stebalien
Copy link
Contributor

@est31 True but, in this case, Rust is following distro standards to the T (this is an improvement, not a bugfix). The article linked in the first post nicely outlines when you should use what directory (the numbered list at the end of the article).

@brson brson added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-needs-decision Issue: In need of a decision. labels Jan 18, 2017
@sfackler
Copy link
Member

Do any other languages have an equivalent function that looks at XDG_RUNTIME_DIR?

@sanmai-NL
Copy link
Author

sanmai-NL commented Jan 18, 2017

On the standards

What other libraries of equal status do
CPython 3.6
C++ Boost 1.63 filesystem

Considering all the evidence I just listed,

  1. I do agree with @Stebalien that Linux distros based on systemd should redefine $TMPDIR to equal $XDG_RUNTIME_DIR.
  2. I retract my proposal to make temp_dir() prefer $XDG_RUNTIME_DIR over $TMPDIR.
  3. Still, I propose that temp_dir() be fixed to check $TMPDIR is a path resolving to a directory.
  4. Also, I maintain that temporary files created by applications are best private by default. The files are supposed to be deleted when out of scope or when some filesystem garbage collection runs. The fact that the temporary directory names are being named so as to prevent name collision attacks confirms $TMPDIR is a target of attacks. All of those properties are very different from a shared ‘scratch’ namespace to pass files around reliably.

Does temp_dir() not use the mkdtemp() extension to the C Standard Library) on Unix on purpose?

@Stebalien
Copy link
Contributor

I do agree with @Stebalien that Linux distros based on systemd should redefine $TMPDIR to equal $XDG_RUNTIME_DIR.

Just for accuracy, I didn't suggest that. Linux distros need to agree on a standard secure temporary file location but I don't think $XDG_RUNTIME_DIR is necessarily the right place (for scratch files).

Does temp_dir() does not use the mkdtemp() extension to the C Standard Library) on Unix on purpose?

temp_dir just tells you the correct temporary directory to use on the current platform. If you want to create temporary directories/files, use the tempdir or tempfile crate.

@sanmai-NL
Copy link
Author

You didn't state that, sorry.
The tempdir crate uses temp_dir(), which was the start of my journey to this issue. So indeed, my question should instead be addressed to the maintainer of tempdir.

@timotree3
Copy link
Contributor

Triage: no change. temp_dir() still outputs the value of $TMPDIR even if $XDG_RUNTIME_DIR is set or $TMPDIR is not a directory.

@steveklabnik
Copy link
Member

Triage: no change

crate::env::var_os("TMPDIR").map(PathBuf::from).unwrap_or_else(|| {

@joshtriplett
Copy link
Member

I agree that XDG_RUNTIME_DIR isn't the right directory, both because it doesn't match the general policy of respecting TMPDIR, and because the runtime directory is a guaranteed ramdisk that may not be suitable for all kinds of temporary files. It's most commonly used for things like sockets. It's also not guaranteed to be set, even on modern distributions; for instance, it often won't be set for system daemons.

I do think there's an improvement to make here, though: the documentation for std::env::temp_dir should note that the temporary directory is often shared and not a secure storage location, and discuss the issues with shared temporary directories. It should mention, in particular, that creating a temporary file with a fixed name is not a secure practice (leading to "insecure temporary file" vulnerabilities), and that any temporary files or directories must be created securely with a randomly generated name component. In support of this, the example code shouldn't create a file with a fixed name.

#81219

@est31
Copy link
Member

est31 commented Jan 20, 2021

You make good points @joshtriplett , I retract what I said earlier.

@m-ou-se m-ou-se removed the I-needs-decision Issue: In need of a decision. label Feb 17, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Feb 17, 2021

Reading back this thread, it seems that everyone came to the conclusion that temp_dir should not return XDG_RUNTIME_DIR, so I'm closing this issue.

However, some other issues were brought up:

1: We're using TMPDIR without a check:

  • Still, I propose that temp_dir() be fixed to check $TMPDIR is a path resolving to a directory.

2: A problem with temporary files in a shared folder:

  • Also, I maintain that temporary files created by applications are best private by default. The files are supposed to be deleted when out of scope or when some filesystem garbage collection runs. The fact that the temporary directory names are being named so as to prevent name collision attacks confirms $TMPDIR is a target of attacks. All of those properties are very different from a shared ‘scratch’ namespace to pass files around reliably.

3: A documentation issue:

I do think there's an improvement to make here, though: the documentation for std::env::temp_dir should note that the temporary directory is often shared and not a secure storage location, and discuss the issues with shared temporary directories. It should mention, in particular, that creating a temporary file with a fixed name is not a secure practice (leading to "insecure temporary file" vulnerabilities), and that any temporary files or directories must be created securely with a randomly generated name component. In support of this, the example code shouldn't create a file with a fixed name.

Issue 1 requires a separate discussion about what the right behaviour would be. @sanmai-NL: Feel free to open a new issue or PR for this, to start that discussion.

Issue 2 is a general problem with temp folders, and temp_dir might not be the right place to solve it. Crates like tempdir provide a solution here. The possibility of the standard library solving this could probably use some discussion on the internals forum first.

Issue 3 is already addressed by #81219.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants