From 7c61f67d612082829231d96d82f48b84399d9eb3 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 13 Sep 2026 18:23:33 +0200 Subject: [PATCH] sort: create temporary files private to the owner When the input does not fit in memory, sort spills sorted chunks to files under TMPDIR. The temporary directory was created by tempfile's TempDir (0777 & ~umask) and the chunks by File::create (0666 & ~umask), so under the usual 022 umask they came out 0755 and 0644: every local user could read the input being sorted, in sorted pieces, for as long as the sort ran. GNU sort (9.11) creates its temporaries 0600 whatever the umask. Create the directory 0700, and the chunks through the existing uucore::safe_copy::create_dest_restrictive, which opens 0600 with O_NOFOLLOW and O_CLOEXEC. Nothing should exist at a chunk path yet, so a symlink there is hostile rather than something to write through. The files are restricted as well as the directory so that a directory whose mode is later relaxed does not expose the data. The output-is-input path needs the same care: it took a temporary file from next_file and then overwrote it with fs::copy, which carries the source's permission bits across and put a 0644 output file's mode back on the copy. Write through the descriptor next_file already opened. safe_copy uses rustix::fs but the safe-copy feature did not declare it; it built only because every current user also enables uucore's "fs". Declare it so sort is not the second one relying on that. --- src/uu/sort/Cargo.toml | 1 + src/uu/sort/src/merge.rs | 43 +++++++++++++++++++-- src/uu/sort/src/tmp_dir.rs | 76 +++++++++++++++++++++++++++++++++----- src/uucore/Cargo.toml | 2 +- 4 files changed, 108 insertions(+), 14 deletions(-) diff --git a/src/uu/sort/Cargo.toml b/src/uu/sort/Cargo.toml index 493781277b8..28fc5771e9e 100644 --- a/src/uu/sort/Cargo.toml +++ b/src/uu/sort/Cargo.toml @@ -35,6 +35,7 @@ thiserror = { workspace = true } uucore = { workspace = true, features = [ "fs", "parser-size", + "safe-copy", "version-cmp", "i18n-decimal", "i18n-collator", diff --git a/src/uu/sort/src/merge.rs b/src/uu/sort/src/merge.rs index a1962af4aa6..f6c5ec68ccf 100644 --- a/src/uu/sort/src/merge.rs +++ b/src/uu/sort/src/merge.rs @@ -16,7 +16,7 @@ use std::{ collections::BinaryHeap, ffi::{OsStr, OsString}, fs::{self, File}, - io::{BufWriter, Read, Write}, + io::{self, BufWriter, Read, Write}, iter, path::{Path, PathBuf}, process::{Child, ChildStdin, ChildStdout, Command, Stdio}, @@ -51,8 +51,13 @@ fn replace_output_file_in_input_files( if let Some(copy) = © { *file = copy.clone().into_os_string(); } else { - let (_file, copy_path) = tmp_dir.next_file()?; - fs::copy(&output_path, ©_path) + // Write through the descriptor `next_file` just opened rather + // than `fs::copy`, which would put the source's permission bits + // on the temporary file and undo the 0600 it was created with. + let (mut copy_file, copy_path) = tmp_dir.next_file()?; + let mut source = File::open(&output_path) + .map_err(|error| SortError::OpenTmpFileFailed { error })?; + io::copy(&mut source, &mut copy_file) .map_err(|error| SortError::OpenTmpFileFailed { error })?; *file = copy_path.clone().into_os_string(); copy = Some(copy_path); @@ -387,7 +392,7 @@ impl FileMerger<'_> { &mut self, writer: &mut impl Write, settings: &GlobalSettings, - ) -> std::io::Result { + ) -> io::Result { if let Some(file) = self.heap.peek() { let prev = self.prev.replace(PreviousLine { chunk: file.current_chunk.clone(), @@ -639,3 +644,33 @@ impl MergeInput for PlainMergeInput { &mut self.inner } } + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + /// When the output file is also an input it is copied to a temporary file + /// first. That copy must stay private to the owner: `fs::copy` would carry the + /// source's 0644 over and undo what `next_file` created the file with. + #[test] + fn output_copy_keeps_the_tmp_file_private() { + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("out"); + fs::write(&out, b"a\n").unwrap(); + fs::set_permissions(&out, fs::Permissions::from_mode(0o644)).unwrap(); + + let mut tmp_dir = TmpDirWrapper::new(dir.path().to_owned()); + let mut files = vec![out.clone().into_os_string()]; + replace_output_file_in_input_files(&mut files, Some(out.as_os_str()), &mut tmp_dir) + .unwrap(); + + let copy = Path::new(&files[0]); + assert_ne!(copy, out, "the input was not replaced by a copy"); + assert_eq!( + fs::metadata(copy).unwrap().permissions().mode() & 0o777, + 0o600 + ); + assert_eq!(fs::read(copy).unwrap(), b"a\n"); + } +} diff --git a/src/uu/sort/src/tmp_dir.rs b/src/uu/sort/src/tmp_dir.rs index 078aa97f38c..fbc5510a2f0 100644 --- a/src/uu/sort/src/tmp_dir.rs +++ b/src/uu/sort/src/tmp_dir.rs @@ -13,6 +13,9 @@ use std::{ sync::{Arc, LazyLock, Mutex}, }; +#[cfg(unix)] +use std::{fs::Permissions, os::unix::fs::PermissionsExt}; + use tempfile::TempDir; use uucore::error::UResult; #[cfg(not(any(target_os = "redox", target_os = "wasi")))] @@ -112,14 +115,18 @@ impl TmpDirWrapper { fn init_tmp_dir(&mut self) -> UResult<()> { assert!(self.temp_dir.is_none()); assert_eq!(self.size, 0); - self.temp_dir = Some( - tempfile::Builder::new() - .prefix("uutils_sort") - .tempdir_in(&self.parent_path) - .map_err(|_| SortError::TmpFileCreationFailed { - path: self.parent_path.clone(), - })?, - ); + // The chunks hold the whole input, so keep them out of reach of other + // local users instead of leaving the mode to the umask. GNU sort (9.11) + // creates its temporaries 0600 whatever the umask. + let mut builder = tempfile::Builder::new(); + builder.prefix("uutils_sort"); + #[cfg(unix)] + builder.permissions(Permissions::from_mode(0o700)); + self.temp_dir = Some(builder.tempdir_in(&self.parent_path).map_err(|_| { + SortError::TmpFileCreationFailed { + path: self.parent_path.clone(), + } + })?); let path = self.temp_dir.as_ref().unwrap().path().to_owned(); let state = HANDLER_STATE.clone(); @@ -145,8 +152,16 @@ impl TmpDirWrapper { let file_name = self.size.to_string(); self.size += 1; let path = self.temp_dir.as_ref().unwrap().path().join(file_name); + // Only the owner may read a chunk. `nofollow` because nothing should + // exist at the path yet: a symlink there is hostile, not something to + // write through. + #[cfg(unix)] + let file = uucore::safe_copy::create_dest_restrictive(&path, true); + #[cfg(not(unix))] + let file = File::create(&path); + Ok(( - File::create(&path).map_err(|error| SortError::OpenTmpFileFailed { error })?, + file.map_err(|error| SortError::OpenTmpFileFailed { error })?, path, )) } @@ -195,3 +210,46 @@ fn remove_tmp_dir(path: &Path) -> std::io::Result<()> { } std::fs::remove_dir(path) } + +#[cfg(all(test, unix))] +mod tests { + use super::TmpDirWrapper; + use std::os::unix::fs::PermissionsExt; + + fn mode(path: &std::path::Path) -> u32 { + std::fs::metadata(path).unwrap().permissions().mode() & 0o777 + } + + /// Restores the process umask on drop, so a panic in the test cannot leak the + /// value into the rest of the binary. + struct UmaskGuard(libc::mode_t); + + impl UmaskGuard { + fn set(mask: libc::mode_t) -> Self { + // SAFETY: umask(2) has no failure mode; it returns the previous value. + Self(unsafe { libc::umask(mask) }) + } + } + + impl Drop for UmaskGuard { + fn drop(&mut self) { + unsafe { libc::umask(self.0) }; + } + } + + #[test] + fn tmp_files_are_private_regardless_of_umask() { + // Pin a permissive umask: under 0077 the umask alone would produce 0700 and + // 0600, so the assertions would hold for a broken implementation too. The + // guard restores it, and the only other test here that creates files sets + // the modes it cares about explicitly. + let _umask = UmaskGuard::set(0o022); + + let parent = tempfile::tempdir().unwrap(); + let mut wrapper = TmpDirWrapper::new(parent.path().to_owned()); + let (_file, path) = wrapper.next_file().unwrap(); + + assert_eq!(mode(path.parent().unwrap()), 0o700); + assert_eq!(mode(&path), 0o600); + } +} diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index 88a6ef9a2ba..b2d92f2dadd 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -183,7 +183,7 @@ proc-info = ["tty", "walkdir"] quoting-style = ["i18n-common"] ranges = [] ringbuffer = [] -safe-copy = [] +safe-copy = ["rustix/fs"] safe-traversal = ["libc", "nix/fs", "nix/dir", "nix/user"] selinux = ["dep:selinux"] smack = ["xattr"]