Skip to content

Commit

Permalink
feat: re-seed from system randomness on collision
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Stebalien committed Nov 19, 2024
1 parent 16209da commit 81fb52c
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 44 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ description = "A library for managing temporary files and directories."
[dependencies]
cfg-if = "1"
fastrand = "2.1.1"
getrandom = { version = "0.2.15", default-features = false }
# 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"] }

Expand Down
4 changes: 2 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
//!
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 19 additions & 1 deletion src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ fn tmpname(prefix: &OsStr, suffix: &OsStr, rand_len: usize) -> OsString {
buf
}

fn reseed() {
let mut seed = [0u8; 8];
if !getrandom::getrandom(&mut seed).is_ok() {
return;
}
fastrand::seed(u64::from_ne_bytes(seed));
}

pub fn create_helper<R>(
base: &Path,
prefix: &OsStr,
Expand All @@ -32,7 +40,17 @@ 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.
if i == 1 {
reseed()
}
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,
Expand Down
59 changes: 18 additions & 41 deletions tests/namedtempfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -421,49 +421,26 @@ 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(f) => 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 is 62 (26*2 + 10).
assert_eq!(sockets.len(), 62);

for socket in sockets {
assert!(socket.path().exists());
Expand Down

0 comments on commit 81fb52c

Please sign in to comment.