mktemp templates & security

[dhardy]

Hiya, this isn't an 'issue' but I wanted to ask about the design / what use cases you aim to cover.

First, some "mktemp" functions allow a template filename to be passed, e.g. C's mkstemp and the shell command. Allowing this would be nice.

Second, there are several security issues to consider.

It may turn out better just to wrap C's mkstemp or mkstemps?

[samgiles]

Hey, good question, thanks for asking,

I created this quickly because I needed something easy to create, and then clean up temporary files and directories, and wanted to use Rust's lifetimes to manage the lifetime of the fs entity.

This isn't quite like libc's mktemp - so maybe the name is misleading. This simply creates a file, or directory, and then you'll have an available AsRef<Path>, which can be cleaned up from the filesystem when the bound variable's lifetime is complete.

The file is opened upon creation by the library, and upon opening, is truncated if it already exists (std::fs::File::create). Directories are also created. This definitely isn't perfect, and still suffers from a couple of potential things, plus there are a couple things I need to check with regard to the rand::Rng used to generate filenames (pretty certain we're good there using a cryptographically OK source of entropy, but I should check and confirm) - which I shall do tomorrow. But this is definitely better than mktemp(3) in terms of security - but definitely not perfect.

Of course, you/we can still use libc directly https://doc.rust-lang.org/libc/x86_64-unknown-linux-gnu/libc/index.html?search=mktemp, but I'd prefer a pure Rust implementation with as little unsafe code as possible. (While writing this I thought I might use Rust's "feature feature" to provide two implementations - one wrapping libc, if you want it, and one wrapping a pure Rust impl).

Templating is a nice idea - will also add it to the agenda.

[dhardy]

Whether you allow file-names to be specified via a template is really up to you.

As the CWE I linked points out, there is good reason for creating exclusively (failing if the file already exists). If the process is unprivileged this is less important, but best to get it right once than expect all users to check whether this is something that might concern them, in my opinion.

So I recommend:

• change create_file() to use OpenOptions::new().write(true).create_new(true).open(self)
• possibly make create_file() try again a number of times if file creation fails due to name conflict — may be a convenient way to partially avoid a denial-of-service attack, but not sure how useful otherwise
• possibly don't create directories recursively (not really sure if this is exploitable), however it would make sense to fail if the directory already exists

I don't personally see an issue linking to libc functions since libc is linked anyway and very well tested, but if you prefer not to it's not a big deal.

[pgerber]

I'd also recommend to set the proper permissions, the files are currently readable by anyone.

Something like this should do the trick: OpenOptions::new().mode(0o700).write(true).create_new(true).open(self)

[chris-morgan]

Bear in mind: OpenOptions.mode and mkstemp are not available on Windows.

Also exec should not be set on something other than very deliberately—a mode of 600 is preferable to 700.