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.

diff --git a/src/libstd/io/tempfile.rs b/src/libstd/io/tempfile.rs
index 45e0dd4e8e5dfd7fa5871871a59e21dd89c39f85..394686be814f24c593ce5c72080c6164c1fbc6be 100644 (file)
--- 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 @@
 ///
 /// {
 ///     // 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 @@
 /// }
 /// {
 ///     // 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 @@
 /// }
 /// {
 ///     // 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<TempDir> {
+    pub fn new_in(tmpdir: &Path, prefix: &str) -> IoResult<TempDir> {
         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> {
-        TempDir::new_in(&os::tmpdir(), suffix)
+    pub fn new(prefix: &str) -> IoResult<TempDir> {
+        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 8fda8a951693f2a67c20428a8f63adfec97babfb..9ca5fc94ffcb220f02a34c69b25c251a1b28904b 100644 (file)
--- 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());