diff --git a/src/uu/sort/Cargo.toml b/src/uu/sort/Cargo.toml index 493781277b..28fc5771e9 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 a1962af4aa..f6c5ec68cc 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 078aa97f38..fbc5510a2f 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 88a6ef9a2b..b2d92f2dad 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"]