dirext: rework permissions handling in atomic_* #73
Conversation
`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 <[email protected]>
| /// 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. |
| @@ -347,6 +352,7 @@ pub trait CapStdExtDirExtUtf8 { | |||
| fn atomic_replace_with<F, T, E>( | |||
| &self, | |||
| destname: impl AsRef<Utf8Path>, | |||
| perms: Option<cap_std::fs::Permissions>, | |||
There was a problem hiding this comment.
This would require a semver bump, which is not a problem exactly but I'd prefer new APIs.
It feels to me like there's a bit of a conflict with the existing API promising to preserve permissions on existing files.
For the non-existing file case, a calling process can set the permissions in their write function which is how this was intended to be used.
The use case for overriding the perms on an existing file feels...somewhat obscure.
There was a problem hiding this comment.
Right now atomic_replace_with copy the existing perms, but maybe not the security bits.
It's easy not to know you must flush before setting security bits (not sure it's in a man page)
Also I want to ensure named temporary file are secure, and with the current API i don't see how to do it without leaving some small corner cases.
Overriding the perms of an existing file might not be common, but it could be that you now want to store secrets in your currently world readable config file.
| #[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)?; | ||
| } |
There was a problem hiding this comment.
Since it doesn't hurt to do so, why not unconditionally set zero mode even with O_TMPFILE?
Also, since this is already underneath #[cfg(unix)] I think we could just
#[cfg(unix)]
rustix::fs::fchmod(t.as_file(), Mode::from_raw(0))
or so
There was a problem hiding this comment.
I forgot to convert to draft
This is going to change once we have bytecodealliance/cap-std#390
The idea is that O_TMPFILE is safe, and to get the default permissions we need to create another file (or to depend on /proc), so for O_TMPFILE (the happy path) we create with 0o666 & !umask, and if nobody wants to change the permissions, we already have the right ones, only in the case of named temp files we need some extra steps
I'm trying to have named temp files not to be world readable at any point,
with bytecodealliance/cap-std#390 that would be possible but then a call to atomic_write() with no existing file would require either to read /proc or to create a new file to find out the default permissions.
Current changes might be the best trade-off for Linux users