From c50907da2fae139e83a3e00f97067cec61768cef Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 4 Feb 2024 09:33:19 +0100 Subject: [PATCH 1/5] feat: Add `Builder::permissions()` method. With it it's possible to change the default mode of tempfiles on unix systems. The example also doesn't hide the fact that it is currently only useful on Unix, but as the ecosystem matures, thanks to the usage of `std::fs::Permissions` it should be able grow into more platforms over time. --- src/lib.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 4b6371d49..9b1b99e38 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -197,6 +197,7 @@ pub struct Builder<'a, 'b> { prefix: &'a OsStr, suffix: &'b OsStr, append: bool, + permissions: Option, } impl<'a, 'b> Default for Builder<'a, 'b> { @@ -206,6 +207,7 @@ impl<'a, 'b> Default for Builder<'a, 'b> { prefix: OsStr::new(".tmp"), suffix: OsStr::new(""), append: false, + permissions: None, } } } @@ -399,6 +401,63 @@ impl<'a, 'b> Builder<'a, 'b> { self } + /// The permissions to create the tempfile with. + /// This allows to them differ from the default mode of `0o600` on Unix. + /// + /// # Security + /// + /// By default, the permissions of tempfiles on unix are set for it to be + /// readable and writable by the owner only, yielding the greatest amount + /// of security. + /// As this method allows to widen the permissions, security would be + /// reduced in such cases. + /// + /// # Platform Notes + /// ## Unix + /// + /// The actual permission bits set on the tempfile will be affected by the + /// `umask` applied by the underlying `open` syscall. + /// + /// ## Windows and others + /// + /// This setting is ignored. + /// + /// # Limitations + /// + /// Permissions for directories aren't currently set even though it would + /// be possible on Unix systems. + /// + /// # Examples + /// + /// ``` + /// # use std::io; + /// # fn main() { + /// # if let Err(_) = run() { + /// # ::std::process::exit(1); + /// # } + /// # } + /// # fn run() -> Result<(), io::Error> { + /// # use tempfile::Builder; + /// #[cfg(unix)] + /// { + /// use std::os::unix::fs::PermissionsExt; + /// let all_read_write = std::fs::Permissions::from_mode(0o666); + /// let tempfile = Builder::new().permissions(all_read_write).tempfile()?; + /// let actual_permissions = tempfile.path().metadata()?.permissions(); + /// assert_ne!( + /// actual_permissions.mode() & !0o170000, + /// 0o600, + /// "we get broader permissions than the default despite umask" + /// ); + /// } + /// # Ok(()) + /// # } + /// ``` + pub fn permissions(&mut self, permissions: std::fs::Permissions) -> &mut Self { + self.permissions = Some(permissions); + self + } + /// Create the named temporary file. /// /// # Security From 334eb145dff004049a2460824b3260e2a250dd58 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 4 Feb 2024 09:38:27 +0100 Subject: [PATCH 2/5] Use `permissions` field of `Builder` on Unix to affect file mode The implementation favors the least invasive solution even though its forced to pull platform dependent code up. The alternative would have been to alter the method signatures of `create_named()` on all platforms. --- src/file/imp/unix.rs | 16 +++++++--------- src/lib.rs | 11 ++++++++++- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/file/imp/unix.rs b/src/file/imp/unix.rs index 1f179276f..350f9e843 100644 --- a/src/file/imp/unix.rs +++ b/src/file/imp/unix.rs @@ -4,7 +4,7 @@ use std::fs::{self, File, OpenOptions}; use std::io; cfg_if::cfg_if! { if #[cfg(not(target_os = "wasi"))] { - use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + use std::os::unix::fs::MetadataExt; } else { #[cfg(feature = "nightly")] use std::os::wasi::fs::MetadataExt; @@ -20,14 +20,11 @@ use { }; pub fn create_named(path: &Path, open_options: &mut OpenOptions) -> io::Result { - open_options.read(true).write(true).create_new(true); - - #[cfg(not(target_os = "wasi"))] - { - open_options.mode(0o600); - } - - open_options.open(path) + open_options + .read(true) + .write(true) + .create_new(true) + .open(path) } fn create_unlinked(path: &Path) -> io::Result { @@ -50,6 +47,7 @@ fn create_unlinked(path: &Path) -> io::Result { #[cfg(target_os = "linux")] pub fn create(dir: &Path) -> io::Result { use rustix::{fs::OFlags, io::Errno}; + use std::os::unix::fs::OpenOptionsExt; OpenOptions::new() .read(true) .write(true) diff --git a/src/lib.rs b/src/lib.rs index 9b1b99e38..1402f16f3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -532,7 +532,16 @@ impl<'a, 'b> Builder<'a, 'b> { self.prefix, self.suffix, self.random_len, - |path| file::create_named(path, OpenOptions::new().append(self.append)), + |path| { + let mut open_options = OpenOptions::new(); + open_options.append(self.append); + #[cfg(all(unix, not(target_os = "wasi")))] + { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + open_options.mode(self.permissions.as_ref().map(|p| p.mode()).unwrap_or(0o600)); + } + file::create_named(path, &mut open_options) + }, ) } From 5ffc70636dcfd0b2e5b10cacd15efa291402cfca Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 4 Feb 2024 10:10:20 +0100 Subject: [PATCH 3/5] Respect module boundaries and make all platforms permissions aware. This way, permissions can be passed and handled by platform specific code, which avoid having to use platform dependent code in the top-level. --- src/file/imp/other.rs | 6 +++++- src/file/imp/unix.rs | 22 +++++++++++++++------- src/file/imp/windows.rs | 6 +++++- src/file/mod.rs | 3 ++- src/lib.rs | 13 +++++-------- 5 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/file/imp/other.rs b/src/file/imp/other.rs index 8721d2da6..bba36712a 100644 --- a/src/file/imp/other.rs +++ b/src/file/imp/other.rs @@ -9,7 +9,11 @@ fn not_supported() -> io::Result { )) } -pub fn create_named(_path: &Path, _open_options: &mut OpenOptions) -> io::Result { +pub fn create_named( + _path: &Path, + _open_options: &mut OpenOptions, + _permissions: Option<&std::fs::Permissions>, +) -> io::Result { not_supported() } diff --git a/src/file/imp/unix.rs b/src/file/imp/unix.rs index 350f9e843..666996a63 100644 --- a/src/file/imp/unix.rs +++ b/src/file/imp/unix.rs @@ -19,12 +19,20 @@ use { std::fs::hard_link, }; -pub fn create_named(path: &Path, open_options: &mut OpenOptions) -> io::Result { - open_options - .read(true) - .write(true) - .create_new(true) - .open(path) +pub fn create_named( + path: &Path, + open_options: &mut OpenOptions, + #[cfg_attr(target_os = "wasi", allow(unused))] permissions: Option<&std::fs::Permissions>, +) -> io::Result { + open_options.read(true).write(true).create_new(true); + + #[cfg(not(target_os = "wasi"))] + { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + open_options.mode(permissions.map(|p| p.mode()).unwrap_or(0o600)); + } + + open_options.open(path) } fn create_unlinked(path: &Path) -> io::Result { @@ -37,7 +45,7 @@ fn create_unlinked(path: &Path) -> io::Result { path = &tmp; } - let f = create_named(path, &mut OpenOptions::new())?; + let f = create_named(path, &mut OpenOptions::new(), None)?; // don't care whether the path has already been unlinked, // but perhaps there are some IO error conditions we should send up? let _ = fs::remove_file(path); diff --git a/src/file/imp/windows.rs b/src/file/imp/windows.rs index 9df65f9e8..a8a6f76c4 100644 --- a/src/file/imp/windows.rs +++ b/src/file/imp/windows.rs @@ -19,7 +19,11 @@ fn to_utf16(s: &Path) -> Vec { s.as_os_str().encode_wide().chain(iter::once(0)).collect() } -pub fn create_named(path: &Path, open_options: &mut OpenOptions) -> io::Result { +pub fn create_named( + path: &Path, + open_options: &mut OpenOptions, + _permissions: Option<&std::fs::Permissions>, +) -> io::Result { open_options .create_new(true) .read(true) diff --git a/src/file/mod.rs b/src/file/mod.rs index 8a28343e0..f6950f383 100644 --- a/src/file/mod.rs +++ b/src/file/mod.rs @@ -1107,13 +1107,14 @@ impl AsRawHandle for NamedTempFile { pub(crate) fn create_named( mut path: PathBuf, open_options: &mut OpenOptions, + permissions: Option<&std::fs::Permissions>, ) -> io::Result { // Make the path absolute. Otherwise, changing directories could cause us to // delete the wrong file. if !path.is_absolute() { path = env::current_dir()?.join(path) } - imp::create_named(&path, open_options) + imp::create_named(&path, open_options, permissions) .with_err_path(|| path.clone()) .map(|file| NamedTempFile { path: TempPath { diff --git a/src/lib.rs b/src/lib.rs index 1402f16f3..798c2276d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -533,14 +533,11 @@ impl<'a, 'b> Builder<'a, 'b> { self.suffix, self.random_len, |path| { - let mut open_options = OpenOptions::new(); - open_options.append(self.append); - #[cfg(all(unix, not(target_os = "wasi")))] - { - use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; - open_options.mode(self.permissions.as_ref().map(|p| p.mode()).unwrap_or(0o600)); - } - file::create_named(path, &mut open_options) + file::create_named( + path, + OpenOptions::new().append(self.append), + self.permissions.as_ref(), + ) }, ) } From 7a80c7702d648aca88c1604d79bbd6aeedd72d9d Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 4 Feb 2024 20:33:32 +0100 Subject: [PATCH 4/5] Fail on Windows if somebody tries to set permissions there. That way, there will be no surprises as would be the case with doing nothing. --- src/file/imp/windows.rs | 9 ++++++++- src/lib.rs | 3 ++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/file/imp/windows.rs b/src/file/imp/windows.rs index a8a6f76c4..e7f603843 100644 --- a/src/file/imp/windows.rs +++ b/src/file/imp/windows.rs @@ -19,11 +19,18 @@ fn to_utf16(s: &Path) -> Vec { s.as_os_str().encode_wide().chain(iter::once(0)).collect() } +fn not_supported(msg: &str) -> io::Result { + Err(io::Error::new(io::ErrorKind::Other, msg)) +} + pub fn create_named( path: &Path, open_options: &mut OpenOptions, - _permissions: Option<&std::fs::Permissions>, + permissions: Option<&std::fs::Permissions>, ) -> io::Result { + if permissions.map_or(false, |p| p.readonly()) { + return not_supported("changing permissions is not supported on this platform"); + } open_options .create_new(true) .read(true) diff --git a/src/lib.rs b/src/lib.rs index 798c2276d..4fdf2449a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -420,7 +420,8 @@ impl<'a, 'b> Builder<'a, 'b> { /// /// ## Windows and others /// - /// This setting is ignored. + /// This setting is unsupported and trying to set a file or directory read-only + /// will cause an error to be returned.. /// /// # Limitations /// From e3740914858b564590fb3d1ca698469b7b76f079 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 4 Feb 2024 21:02:04 +0100 Subject: [PATCH 5/5] Add support for setting permissions on directories as well. --- src/dir/imp/any.rs | 19 ++++++++++++ src/dir/imp/mod.rs | 9 ++++++ src/dir/imp/unix.rs | 21 +++++++++++++ src/{dir.rs => dir/mod.rs} | 15 ++++----- src/file/imp/unix.rs | 3 +- src/file/imp/windows.rs | 3 +- src/lib.rs | 62 ++++++++++++++++++++++++++++---------- src/util.rs | 5 +-- 8 files changed, 110 insertions(+), 27 deletions(-) create mode 100644 src/dir/imp/any.rs create mode 100644 src/dir/imp/mod.rs create mode 100644 src/dir/imp/unix.rs rename src/{dir.rs => dir/mod.rs} (98%) diff --git a/src/dir/imp/any.rs b/src/dir/imp/any.rs new file mode 100644 index 000000000..015f9f87d --- /dev/null +++ b/src/dir/imp/any.rs @@ -0,0 +1,19 @@ +use crate::error::IoResultExt; +use crate::TempDir; +use std::path::PathBuf; +use std::{fs, io}; + +fn not_supported(msg: &str) -> io::Result { + Err(io::Error::new(io::ErrorKind::Other, msg)) +} + +pub fn create(path: PathBuf, permissions: Option<&std::fs::Permissions>) -> io::Result { + if permissions.map_or(false, |p| p.readonly()) { + return not_supported("changing permissions is not supported on this platform"); + } + fs::create_dir(&path) + .with_err_path(|| &path) + .map(|_| TempDir { + path: path.into_boxed_path(), + }) +} diff --git a/src/dir/imp/mod.rs b/src/dir/imp/mod.rs new file mode 100644 index 000000000..26d0a2273 --- /dev/null +++ b/src/dir/imp/mod.rs @@ -0,0 +1,9 @@ +#[cfg(unix)] +mod unix; +#[cfg(unix)] +pub use unix::*; + +#[cfg(not(unix))] +mod any; +#[cfg(not(unix))] +pub use any::*; diff --git a/src/dir/imp/unix.rs b/src/dir/imp/unix.rs new file mode 100644 index 000000000..47dd125c1 --- /dev/null +++ b/src/dir/imp/unix.rs @@ -0,0 +1,21 @@ +use crate::error::IoResultExt; +use crate::TempDir; +use std::io; +use std::path::PathBuf; + +pub fn create(path: PathBuf, permissions: Option<&std::fs::Permissions>) -> io::Result { + let mut dir_options = std::fs::DirBuilder::new(); + #[cfg(not(target_os = "wasi"))] + { + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + if let Some(p) = permissions { + dir_options.mode(p.mode()); + } + } + dir_options + .create(&path) + .with_err_path(|| &path) + .map(|_| TempDir { + path: path.into_boxed_path(), + }) +} diff --git a/src/dir.rs b/src/dir/mod.rs similarity index 98% rename from src/dir.rs rename to src/dir/mod.rs index 1b79be445..db70dd52e 100644 --- a/src/dir.rs +++ b/src/dir/mod.rs @@ -12,7 +12,7 @@ use std::ffi::OsStr; use std::fs::remove_dir_all; use std::mem; use std::path::{self, Path, PathBuf}; -use std::{fmt, fs, io}; +use std::{fmt, io}; use crate::error::IoResultExt; use crate::Builder; @@ -468,10 +468,11 @@ impl Drop for TempDir { } } -pub(crate) fn create(path: PathBuf) -> io::Result { - fs::create_dir(&path) - .with_err_path(|| &path) - .map(|_| TempDir { - path: path.into_boxed_path(), - }) +pub(crate) fn create( + path: PathBuf, + permissions: Option<&std::fs::Permissions>, +) -> io::Result { + imp::create(path, permissions) } + +mod imp; diff --git a/src/file/imp/unix.rs b/src/file/imp/unix.rs index 666996a63..5f2cb5dcb 100644 --- a/src/file/imp/unix.rs +++ b/src/file/imp/unix.rs @@ -83,7 +83,8 @@ fn create_unix(dir: &Path) -> io::Result { OsStr::new(".tmp"), OsStr::new(""), crate::NUM_RAND_CHARS, - |path| create_unlinked(&path), + None, + |path, _| create_unlinked(&path), ) } diff --git a/src/file/imp/windows.rs b/src/file/imp/windows.rs index e7f603843..bed62090f 100644 --- a/src/file/imp/windows.rs +++ b/src/file/imp/windows.rs @@ -45,7 +45,8 @@ pub fn create(dir: &Path) -> io::Result { OsStr::new(".tmp"), OsStr::new(""), crate::NUM_RAND_CHARS, - |path| { + None, + |path, _permissions| { OpenOptions::new() .create_new(true) .read(true) diff --git a/src/lib.rs b/src/lib.rs index 4fdf2449a..2072f7a12 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -401,7 +401,7 @@ impl<'a, 'b> Builder<'a, 'b> { self } - /// The permissions to create the tempfile with. + /// The permissions to create the tempfile or [tempdir](Self::tempdir) with. /// This allows to them differ from the default mode of `0o600` on Unix. /// /// # Security @@ -415,21 +415,19 @@ impl<'a, 'b> Builder<'a, 'b> { /// # Platform Notes /// ## Unix /// - /// The actual permission bits set on the tempfile will be affected by the - /// `umask` applied by the underlying `open` syscall. + /// The actual permission bits set on the tempfile or tempdir will be affected by the + /// `umask` applied by the underlying syscall. + /// /// /// ## Windows and others /// /// This setting is unsupported and trying to set a file or directory read-only /// will cause an error to be returned.. /// - /// # Limitations - /// - /// Permissions for directories aren't currently set even though it would - /// be possible on Unix systems. - /// /// # Examples /// + /// Create a named temporary file that is world-readable. + /// /// ``` /// # use std::io; /// # fn main() { @@ -454,6 +452,33 @@ impl<'a, 'b> Builder<'a, 'b> { /// # Ok(()) /// # } /// ``` + /// + /// Create a named temporary directory that is restricted to the owner. + /// + /// ``` + /// # use std::io; + /// # fn main() { + /// # if let Err(_) = run() { + /// # ::std::process::exit(1); + /// # } + /// # } + /// # fn run() -> Result<(), io::Error> { + /// # use tempfile::Builder; + /// #[cfg(unix)] + /// { + /// use std::os::unix::fs::PermissionsExt; + /// let owner_rwx = std::fs::Permissions::from_mode(0o700); + /// let tempdir = Builder::new().permissions(owner_rwx).tempdir()?; + /// let actual_permissions = tempdir.path().metadata()?.permissions(); + /// assert_eq!( + /// actual_permissions.mode() & !0o170000, + /// 0o700, + /// "we get the narrow permissions we asked for" + /// ); + /// } + /// # Ok(()) + /// # } + /// ``` pub fn permissions(&mut self, permissions: std::fs::Permissions) -> &mut Self { self.permissions = Some(permissions); self @@ -533,12 +558,9 @@ impl<'a, 'b> Builder<'a, 'b> { self.prefix, self.suffix, self.random_len, - |path| { - file::create_named( - path, - OpenOptions::new().append(self.append), - self.permissions.as_ref(), - ) + self.permissions.as_ref(), + |path, permissions| { + file::create_named(path, OpenOptions::new().append(self.append), permissions) }, ) } @@ -611,7 +633,14 @@ impl<'a, 'b> Builder<'a, 'b> { dir = &storage; } - util::create_helper(dir, self.prefix, self.suffix, self.random_len, dir::create) + util::create_helper( + dir, + self.prefix, + self.suffix, + self.random_len, + self.permissions.as_ref(), + dir::create, + ) } /// Attempts to create a temporary file (or file-like object) using the @@ -756,7 +785,8 @@ impl<'a, 'b> Builder<'a, 'b> { self.prefix, self.suffix, self.random_len, - move |path| { + None, + move |path, _permissions| { Ok(NamedTempFile::from_parts( f(&path)?, TempPath::from_path(path), diff --git a/src/util.rs b/src/util.rs index d426ba3d7..8c04953a3 100644 --- a/src/util.rs +++ b/src/util.rs @@ -20,7 +20,8 @@ pub fn create_helper( prefix: &OsStr, suffix: &OsStr, random_len: usize, - mut f: impl FnMut(PathBuf) -> io::Result, + permissions: Option<&std::fs::Permissions>, + mut f: impl FnMut(PathBuf, Option<&std::fs::Permissions>) -> io::Result, ) -> io::Result { let num_retries = if random_len != 0 { crate::NUM_RETRIES @@ -30,7 +31,7 @@ pub fn create_helper( for _ in 0..num_retries { let path = base.join(tmpname(prefix, suffix, random_len)); - return match f(path) { + return match f(path, permissions) { Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists && num_retries > 1 => continue, // AddrInUse can happen if we're creating a UNIX domain socket and // the path already exists.