feat: re-seed from system randomness on collision

Re-seed thread-local RNG from system randomness if we run into a
temporary file-name collision. This should address the concerns about
using a predictable RNG without hurting performance in the common case
where nobody is trying to predict our filenames. I'm only re-seeding
once because if we _still_ fail to create a temporary file, the
collision was likely due to too many temporary files instead of an
attacker predicting our random temporary file names.

I've also reduced the number of tries from 2^31 to 2^16. If it takes
more than that to create a temporary file, something else is wrong.
Pausing for a long time is usually worse than just failing.

fixes #178

Author: Stebalien

Cargo.toml

@@ -22,6 +22,9 @@ fastrand = "2.1.1"
 # Not available in stdlib until 1.70, but we support 1.63 to support Debian stable.
 once_cell = { version = "1.19.0", default-features = false, features = ["std"] }
 
+[target.'cfg(any(unix, windows, target_os = "wasi"))'.dependencies]
+getrandom = { version = "0.2.15", default-features = false }
+
 [target.'cfg(any(unix, target_os = "wasi"))'.dependencies]
 rustix = { version = "0.38.39", features = ["fs"] }
 

src/lib.rs

@@ -29,7 +29,7 @@
 //! `tempfile` doesn't rely on file paths so this isn't an issue. However, `NamedTempFile` does
 //! rely on file paths for _some_ operations. See the security documentation on
 //! the `NamedTempFile` type for more information.
-//! 
+//!
 //! The OWASP Foundation provides a resource on vulnerabilities concerning insecure
 //! temporary files: https://owasp.org/www-community/vulnerabilities/Insecure_Temporary_File
 //!
@@ -150,7 +150,7 @@
 #[cfg(doctest)]
 doc_comment::doctest!("../README.md");
 
-const NUM_RETRIES: u32 = 1 << 31;
+const NUM_RETRIES: u32 = 65536;
 const NUM_RAND_CHARS: usize = 6;
 
 use std::ffi::OsStr;

src/util.rs

@@ -32,7 +32,21 @@ pub fn create_helper<R>(
         1
     };
 
-    for _ in 0..num_retries {
+    for i in 0..num_retries {
+        // If we fail to create the file the first time, re-seed from system randomness in case an
+        // attacker is predicting our randomness (fastrand is predictable). If re-seeding doesn't help, either:
+        //
+        // 1. We have lots of temporary files, possibly created by an attacker but not necessarily.
+        //    Re-seeding the randomness won't help here.
+        // 2. We're failing to create random files for some other reason. This shouldn't be the case
+        //    given that we're checking error kinds, but it could happen.
+        #[cfg(any(windows, unix, target_os = "redox", target_os = "wasi"))]
+        if i == 1 {
+            let mut seed = [0u8; 8];
+            if getrandom::getrandom(&mut seed).is_ok() {
+                fastrand::seed(u64::from_ne_bytes(seed));
+            }
+        }
         let path = base.join(tmpname(prefix, suffix, random_len));
         return match f(path) {
             Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists && num_retries > 1 => continue,

tests/namedtempfile.rs

@@ -421,49 +421,32 @@ fn test_make_uds() {
 #[cfg(unix)]
 #[test]
 fn test_make_uds_conflict() {
+    use std::io::ErrorKind;
     use std::os::unix::net::UnixListener;
-    use std::sync::atomic::{AtomicUsize, Ordering};
-    use std::sync::Arc;
-
-    // Check that retries happen correctly by racing N different threads.
-
-    const NTHREADS: usize = 20;
-
-    // The number of times our callback was called.
-    let tries = Arc::new(AtomicUsize::new(0));
-
-    let mut threads = Vec::with_capacity(NTHREADS);
-
-    for _ in 0..NTHREADS {
-        let tries = tries.clone();
-        threads.push(std::thread::spawn(move || {
-            // Ensure that every thread uses the same seed so we are guaranteed
-            // to retry. Note that fastrand seeds are thread-local.
-            fastrand::seed(42);
-
-            Builder::new()
-                .prefix("tmp")
-                .suffix(".sock")
-                .rand_bytes(12)
-                .make(|path| {
-                    tries.fetch_add(1, Ordering::Relaxed);
-                    UnixListener::bind(path)
-                })
-        }));
-    }
 
-    // Join all threads, but don't drop the temp file yet. Otherwise, we won't
-    // get a deterministic number of `tries`.
-    let sockets: Vec<_> = threads
-        .into_iter()
-        .map(|thread| thread.join().unwrap().unwrap())
-        .collect();
-
-    // Number of tries is exactly equal to (n*(n+1))/2.
-    assert_eq!(
-        tries.load(Ordering::Relaxed),
-        (NTHREADS * (NTHREADS + 1)) / 2
-    );
+    let sockets = std::iter::repeat_with(|| {
+        Builder::new()
+            .prefix("tmp")
+            .suffix(".sock")
+            .rand_bytes(1)
+            .make(|path| UnixListener::bind(path))
+    })
+    .take_while(|r| match r {
+        Ok(_) => true,
+        Err(e) if matches!(e.kind(), ErrorKind::AddrInUse | ErrorKind::AlreadyExists) => false,
+        Err(e) => panic!("unexpected error {e}"),
+    })
+    .collect::<Result<Vec<_>, _>>()
+    .unwrap();
+
+    // Number of sockets we can create. Depends on whether or not the filesystem is case sensitive.
+
+    #[cfg(target_os = "macos")]
+    const NUM_FILES: usize = 36;
+    #[cfg(not(target_os = "macos"))]
+    const NUM_FILES: usize = 62;
+
+    assert_eq!(sockets.len(), NUM_FILES);
 
     for socket in sockets {
         assert!(socket.path().exists());