From 03007d96fe27901c0b4f49a98e572bad984c3f53 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 19 Nov 2024 14:39:16 -0800 Subject: [PATCH] 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 --- Cargo.toml | 3 ++ src/lib.rs | 4 +-- src/util.rs | 16 ++++++++++- tests/namedtempfile.rs | 65 ++++++++++++++++-------------------------- 4 files changed, 44 insertions(+), 44 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0cb8e23da..c53c33475 100644 --- a/Cargo.toml +++ b/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"] } diff --git a/src/lib.rs b/src/lib.rs index 7fe44b180..221956e1d 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/util.rs b/src/util.rs index 2f6f381a5..6b625695f 100644 --- a/src/util.rs +++ b/src/util.rs @@ -32,7 +32,21 @@ pub fn create_helper( 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, diff --git a/tests/namedtempfile.rs b/tests/namedtempfile.rs index e0acfd315..7e2d9f28b 100644 --- a/tests/namedtempfile.rs +++ b/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::, _>>() + .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());