From 2c441958959e46fd03c7f2736806d75bebeebd30 Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Sat, 3 Jan 2015 21:49:01 +0000 Subject: [PATCH] Make temporary directory names non-deterministic. The previous scheme made it possible for another user/attacker to cause the temporary directory creation scheme to panic. All you needed to know was the pid of the process you wanted to target ('other_pid') and the suffix it was using (let's pretend it's 'sfx') and then code such as this would, in essence, DOS it: for i in range(0u, 1001) { let tp = &Path::new(format!("/tmp/rs-{}-{}-sfx", other_pid, i)); match fs::mkdir(tp, io::USER_RWX) { _ => () } } Since the scheme retried only 1000 times to create a temporary directory before dying, the next time the attacked process called TempDir::new("sfx") after that would typically cause a panic. Of course, you don't necessarily need an attacker to cause such a DOS: creating 1000 temporary directories without closing any of the previous would be enough to DOS yourself. This patch broadly follows the OpenBSD implementation of mkstemp. It uses the operating system's random number generator to produce random directory names that are impractical to guess (and, just in case someone manages to do that, it retries creating the directory for a long time before giving up; OpenBSD retries INT_MAX times, although 1<<31 seems enough to thwart even the most patient attacker). As a small additional change, this patch also makes the argument that TempDir::new takes a prefix rather than a suffix. This is because 1) it more closely matches what mkstemp and friends do 2) if you're going to have a deterministic part of a filename, you really want it at the beginning so that shell completion is useful. --- src/libstd/io/tempfile.rs | 74 +++++++++++++++++++++-------------- src/test/run-pass/tempfile.rs | 2 +- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/src/libstd/io/tempfile.rs b/src/libstd/io/tempfile.rs index 45e0dd4e8e5..394686be814 100644 --- a/src/libstd/io/tempfile.rs +++ b/src/libstd/io/tempfile.rs @@ -10,16 +10,18 @@ //! Temporary files and directories -use io::{fs, IoResult}; +use io::{fs, IoError, IoErrorKind, IoResult}; use io; -use libc; +use iter::{IteratorExt, range}; use ops::Drop; use option::Option; use option::Option::{None, Some}; use os; use path::{Path, GenericPath}; +use rand::{Rng, thread_rng}; use result::Result::{Ok, Err}; -use sync::atomic::{AtomicUint, ATOMIC_UINT_INIT, Ordering}; +use str::StrExt; +use string::String; /// A wrapper for a path to temporary directory implementing automatic /// scope-based deletion. @@ -31,7 +33,7 @@ use sync::atomic::{AtomicUint, ATOMIC_UINT_INIT, Ordering}; /// /// { /// // create a temporary directory -/// let tmpdir = match TempDir::new("mysuffix") { +/// let tmpdir = match TempDir::new("myprefix") { /// Ok(dir) => dir, /// Err(e) => panic!("couldn't create temporary directory: {}", e) /// }; @@ -46,7 +48,7 @@ use sync::atomic::{AtomicUint, ATOMIC_UINT_INIT, Ordering}; /// } /// { /// // create a temporary directory, this time using a custom path -/// let tmpdir = match TempDir::new_in(&Path::new("/tmp/best/custom/path"), "mysuffix") { +/// let tmpdir = match TempDir::new_in(&Path::new("/tmp/best/custom/path"), "myprefix") { /// Ok(dir) => dir, /// Err(e) => panic!("couldn't create temporary directory: {}", e) /// }; @@ -61,7 +63,7 @@ use sync::atomic::{AtomicUint, ATOMIC_UINT_INIT, Ordering}; /// } /// { /// // create a temporary directory -/// let tmpdir = match TempDir::new("mysuffix") { +/// let tmpdir = match TempDir::new("myprefix") { /// Ok(dir) => dir, /// Err(e) => panic!("couldn't create temporary directory: {}", e) /// }; @@ -78,47 +80,59 @@ pub struct TempDir { disarmed: bool } +// How many times should we (re)try finding an unused random name? It should be +// enough that an attacker will run out of luck before we run out of patience. +const NUM_RETRIES: u32 = 1 << 31; +// How many characters should we include in a random file name? It needs to +// be enough to dissuade an attacker from trying to preemptively create names +// of that length, but not so huge that we unnecessarily drain the random number +// generator of entropy. +const NUM_RAND_CHARS: uint = 12; + impl TempDir { /// Attempts to make a temporary directory inside of `tmpdir` whose name - /// will have the suffix `suffix`. The directory will be automatically + /// will have the prefix `prefix`. The directory will be automatically /// deleted once the returned wrapper is destroyed. /// /// If no directory can be created, `Err` is returned. - pub fn new_in(tmpdir: &Path, suffix: &str) -> IoResult { + pub fn new_in(tmpdir: &Path, prefix: &str) -> IoResult { if !tmpdir.is_absolute() { let abs_tmpdir = try!(os::make_absolute(tmpdir)); - return TempDir::new_in(&abs_tmpdir, suffix); + return TempDir::new_in(&abs_tmpdir, prefix); } - static CNT: AtomicUint = ATOMIC_UINT_INIT; - - let mut attempts = 0u; - loop { - let filename = - format!("rs-{}-{}-{}", - unsafe { libc::getpid() }, - CNT.fetch_add(1, Ordering::SeqCst), - suffix); - let p = tmpdir.join(filename); - match fs::mkdir(&p, io::USER_RWX) { - Err(error) => { - if attempts >= 1000 { - return Err(error) - } - attempts += 1; - } - Ok(()) => return Ok(TempDir { path: Some(p), disarmed: false }) + let mut rng = thread_rng(); + for _ in range(0, NUM_RETRIES) { + let suffix: String = rng.gen_ascii_chars().take(NUM_RAND_CHARS).collect(); + let leaf = if prefix.len() > 0 { + format!("{}.{}", prefix, suffix) + } else { + // If we're given an empty string for a prefix, then creating a + // directory starting with "." would lead to it being + // semi-invisible on some systems. + suffix + }; + let path = tmpdir.join(leaf); + match fs::mkdir(&path, io::USER_RWX) { + Ok(_) => return Ok(TempDir { path: Some(path), disarmed: false }), + Err(IoError{kind:IoErrorKind::PathAlreadyExists,..}) => (), + Err(e) => return Err(e) } } + + return Err(IoError{ + kind: IoErrorKind::PathAlreadyExists, + desc:"Exhausted", + detail: None}); } /// Attempts to make a temporary directory inside of `os::tmpdir()` whose - /// name will have the suffix `suffix`. The directory will be automatically + /// name will have the prefix `prefix`. The directory will be automatically /// deleted once the returned wrapper is destroyed. /// /// If no directory can be created, `Err` is returned. - pub fn new(suffix: &str) -> IoResult { - TempDir::new_in(&os::tmpdir(), suffix) + pub fn new(prefix: &str) -> IoResult { + TempDir::new_in(&os::tmpdir(), prefix) } /// Unwrap the wrapped `std::path::Path` from the `TempDir` wrapper. diff --git a/src/test/run-pass/tempfile.rs b/src/test/run-pass/tempfile.rs index 8fda8a95169..9ca5fc94ffc 100644 --- a/src/test/run-pass/tempfile.rs +++ b/src/test/run-pass/tempfile.rs @@ -29,7 +29,7 @@ fn test_tempdir() { let path = { let p = TempDir::new_in(&Path::new("."), "foobar").unwrap(); let p = p.path(); - assert!(p.as_vec().ends_with(b"foobar")); + assert!(p.as_str().unwrap().contains("foobar")); p.clone() }; assert!(!path.exists());