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