From c5dbbcddd0940019623578910384d2eb26abc540 Mon Sep 17 00:00:00 2001 From: Etienne Champetier Date: Fri, 23 May 2025 09:35:45 -0400 Subject: [PATCH] dirext: improve permissions handling in atomic_* `atomic_replace_with` now takes the target permissions as an Option. Existing permissions copy now happen after flushing the buffers, so security related bits are not removed by the kernel. When using filesystems that do not support `O_TEMPFILE`, we now change the tempfile permissions to 0o000 as soon as possible. A malicious process still has a really short time to `open()` the tempfile, to prevent that for now you must the a restrictive umask. Signed-off-by: Etienne Champetier --- src/dirext.rs | 96 +++++++++++++++++++++++++++++------------------- tests/it/main.rs | 18 +++++++-- 2 files changed, 72 insertions(+), 42 deletions(-) diff --git a/src/dirext.rs b/src/dirext.rs index c4a00cc..f8b5d10 100644 --- a/src/dirext.rs +++ b/src/dirext.rs @@ -9,6 +9,8 @@ //! [`cap_std::fs::Dir`]: https://docs.rs/cap-std/latest/cap_std/fs/struct.Dir.html use cap_primitives::fs::FileType; +#[cfg(unix)] +use cap_primitives::fs::MetadataExt; use cap_std::fs::{Dir, File, Metadata}; use cap_tempfile::cap_std; use cap_tempfile::cap_std::fs::DirEntry; @@ -172,25 +174,26 @@ pub trait CapStdExtDirExt { /// require a higher level wrapper which queries the existing file and gathers such metadata /// before replacement. /// - /// # Example, including setting permissions + /// # Security /// - /// The closure may also perform other file operations beyond writing, such as changing - /// file permissions: + /// On filesystems that do not support `O_TMPFILE` (e.g. `vfat`), the TempFile is first created + /// with default permissions (mode 0o666 & ~umask) that can be readeable by everyone. + /// If this is a concern, you must set a more restrictive umask for your program. + /// + /// # Example /// /// ```rust /// # use std::io; /// # use std::io::Write; /// # use cap_tempfile::cap_std; + /// # use cap_std::fs::PermissionsExt; /// # fn main() -> io::Result<()> { /// # let somedir = cap_tempfile::tempdir(cap_std::ambient_authority())?; /// use cap_std_ext::prelude::*; /// let contents = b"hello world\n"; - /// somedir.atomic_replace_with("somefilename", |f| -> io::Result<_> { + /// let perms = cap_std::fs::Permissions::from_mode(0o600); + /// somedir.atomic_replace_with("somefilename", Some(perms), |f| -> io::Result<_> { /// f.write_all(contents)?; - /// f.flush()?; - /// use cap_std::fs::PermissionsExt; - /// let perms = cap_std::fs::Permissions::from_mode(0o600); - /// f.get_mut().as_file_mut().set_permissions(perms)?; /// Ok(()) /// }) /// # } @@ -200,6 +203,7 @@ pub trait CapStdExtDirExt { fn atomic_replace_with( &self, destname: impl AsRef, + perms: Option, f: F, ) -> std::result::Result where @@ -318,26 +322,27 @@ pub trait CapStdExtDirExtUtf8 { /// require a higher level wrapper which queries the existing file and gathers such metadata /// before replacement. /// - /// # Example, including setting permissions + /// # Security + /// + /// On filesystems that do not support `O_TMPFILE` (e.g. `vfat`), the TempFile is first created + /// with default permissions (mode 0o666 & ~umask) that can be readeable by everyone. + /// If this is a concern, you must set a more restrictive umask for your program. /// - /// The closure may also perform other file operations beyond writing, such as changing - /// file permissions: + /// # Example /// /// ```rust /// # use std::io; /// # use std::io::Write; /// # use cap_tempfile::cap_std; + /// # use cap_std::fs::PermissionsExt; /// # fn main() -> io::Result<()> { /// # let somedir = cap_tempfile::tempdir(cap_std::ambient_authority())?; /// # let somedir = cap_std::fs_utf8::Dir::from_cap_std((&*somedir).try_clone()?); /// use cap_std_ext::prelude::*; /// let contents = b"hello world\n"; - /// somedir.atomic_replace_with("somefilename", |f| -> io::Result<_> { + /// let perms = cap_std::fs::Permissions::from_mode(0o600); + /// somedir.atomic_replace_with("somefilename", Some(perms), |f| -> io::Result<_> { /// f.write_all(contents)?; - /// f.flush()?; - /// use cap_std::fs::PermissionsExt; - /// let perms = cap_std::fs::Permissions::from_mode(0o600); - /// f.get_mut().as_file_mut().set_permissions(perms)?; /// Ok(()) /// }) /// # } @@ -347,6 +352,7 @@ pub trait CapStdExtDirExtUtf8 { fn atomic_replace_with( &self, destname: impl AsRef, + perms: Option, f: F, ) -> std::result::Result where @@ -710,6 +716,7 @@ impl CapStdExtDirExt for Dir { fn atomic_replace_with( &self, destname: impl AsRef, + perms: Option, f: F, ) -> std::result::Result where @@ -718,23 +725,47 @@ impl CapStdExtDirExt for Dir { { let destname = destname.as_ref(); let (d, name) = subdir_of(self, destname)?; - let existing_metadata = d.symlink_metadata_optional(destname)?; - // If the target is already a file, then acquire its mode, which we will preserve by default. - // We don't follow symlinks here for replacement, and so we definitely don't want to pick up its mode. - let existing_perms = existing_metadata - .filter(|m| m.is_file()) - .map(|m| m.permissions()); + + // cap_tempfile doesn't support passing permissions on creation. + // https://github.com/bytecodealliance/cap-std/pull/390 let mut t = cap_tempfile::TempFile::new(&d)?; - // Apply the permissions, if we have them - if let Some(existing_perms) = existing_perms { - t.as_file_mut().set_permissions(existing_perms)?; + #[cfg(unix)] + let t_metadata = t.as_file().metadata()?; + #[cfg(unix)] + if t_metadata.nlink() > 0 { + let zeroperms = cap_std::fs::PermissionsExt::from_mode(0o000); + t.as_file().set_permissions(zeroperms)?; } + // We always operate in terms of buffered writes let mut bufw = std::io::BufWriter::new(t); // Call the provided closure to generate the file content let r = f(&mut bufw)?; // Flush the buffer, get the TempFile t = bufw.into_inner().map_err(From::from)?; + // Set the file permissions + // This must be done after flushing the application buffer else Linux clears the security related bits: + // https://github.com/torvalds/linux/blob/94305e83eccb3120c921cd3a015cd74731140bac/fs/inode.c#L2360 + if let Some(perms) = perms { + t.as_file().set_permissions(perms)?; + } else { + let existing_metadata = d.symlink_metadata_optional(destname)?; + // If the target is already a file, then acquire its mode, which we will preserve by default. + // We don't follow symlinks here for replacement, and so we definitely don't want to pick up its mode. + let existing_perms = existing_metadata + .filter(|m| m.is_file()) + .map(|m| m.permissions()); + if let Some(existing_perms) = existing_perms { + // Apply the existing permissions, if we have them + t.as_file().set_permissions(existing_perms)?; + } else { + // Else restore default permissions + #[cfg(unix)] + if t_metadata.nlink() > 0 { + t.as_file().set_permissions(t_metadata.permissions())?; + } + } + } // fsync the TempFile t.as_file().sync_all()?; // rename the TempFile @@ -745,7 +776,7 @@ impl CapStdExtDirExt for Dir { } fn atomic_write(&self, destname: impl AsRef, contents: impl AsRef<[u8]>) -> Result<()> { - self.atomic_replace_with(destname, |f| f.write_all(contents.as_ref())) + self.atomic_replace_with(destname, None, |f| f.write_all(contents.as_ref())) } fn atomic_write_with_perms( @@ -754,19 +785,8 @@ impl CapStdExtDirExt for Dir { contents: impl AsRef<[u8]>, perms: cap_std::fs::Permissions, ) -> Result<()> { - self.atomic_replace_with(destname, |f| -> io::Result<_> { - // If the user is overriding the permissions, let's make the default be - // writable by us but not readable by anyone else, in case it has - // secret data. - #[cfg(unix)] - { - use cap_std::fs::PermissionsExt; - let perms = cap_std::fs::Permissions::from_mode(0o600); - f.get_mut().as_file_mut().set_permissions(perms)?; - } + self.atomic_replace_with(destname, Some(perms), |f| -> io::Result<_> { f.write_all(contents.as_ref())?; - f.flush()?; - f.get_mut().as_file_mut().set_permissions(perms)?; Ok(()) }) } diff --git a/tests/it/main.rs b/tests/it/main.rs index fb65d82..6a6413d 100644 --- a/tests/it/main.rs +++ b/tests/it/main.rs @@ -164,13 +164,13 @@ fn default_mode(d: &Dir) -> Result { fn link_tempfile_with() -> Result<()> { let td = cap_tempfile::tempdir(cap_std::ambient_authority())?; let p = Path::new("foo"); - td.atomic_replace_with(p, |f| writeln!(f, "hello world")) + td.atomic_replace_with(p, None, |f| writeln!(f, "hello world")) .unwrap(); assert_eq!(td.read_to_string(p).unwrap().as_str(), "hello world\n"); let default_perms = default_mode(&td)?; assert_eq!(td.metadata(p)?.permissions(), default_perms); - td.atomic_replace_with(p, |f| writeln!(f, "atomic replacement")) + td.atomic_replace_with(p, None, |f| writeln!(f, "atomic replacement")) .unwrap(); assert_eq!( td.read_to_string(p).unwrap().as_str(), @@ -178,7 +178,7 @@ fn link_tempfile_with() -> Result<()> { ); let e = td - .atomic_replace_with(p, |f| { + .atomic_replace_with(p, None, |f| { writeln!(f, "should not be written")?; Err::<(), _>(std::io::Error::new(std::io::ErrorKind::Other, "oops")) }) @@ -238,6 +238,16 @@ fn link_tempfile_with() -> Result<()> { td.atomic_write_with_perms(p, "self-only file v3", Permissions::from_mode(0o640)) .unwrap(); assert_eq!(td.metadata(p).unwrap().permissions().mode() & 0o777, 0o640); + // Write a file with mode 000 + td.atomic_write_with_perms(p, "no permissions", Permissions::from_mode(0o000)) + .unwrap(); + let metadata = td.metadata(p).unwrap(); + assert_eq!(metadata.permissions().mode() & 0o777, 0o000); + assert_eq!(metadata.len(), 14); + // Replace a file with mode 000 + td.atomic_write_with_perms(p, "600", Permissions::from_mode(0o600)) + .unwrap(); + assert_eq!(td.metadata(p).unwrap().permissions().mode() & 0o777, 0o600); Ok(()) } @@ -245,7 +255,7 @@ fn link_tempfile_with() -> Result<()> { fn test_timestamps() -> Result<()> { let td = cap_tempfile::tempdir(cap_std::ambient_authority())?; let p = Path::new("foo"); - td.atomic_replace_with(p, |f| writeln!(f, "hello world")) + td.atomic_replace_with(p, None, |f| writeln!(f, "hello world")) .unwrap(); let ts0 = td.metadata(p)?.modified()?; // This test assumes at least second granularity on filesystem timestamps, and