diff --git a/CHANGELOG.md b/CHANGELOG.md index 3edbabf9..91f18198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -866,6 +866,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 NXDOMAIN. Connecting the socket also means a dead upstream surfaces ECONNREFUSED immediately instead of stalling for five seconds. +#### Control socket + +- The control socket and the directory holding it are now created with a + restrictive mode rather than created wide and narrowed afterwards. `bind(2)` + makes the socket inode `0777 & ~umask`, so under a permissive umask the + socket was world-accessible for the window between the bind and the `chmod` + to 0770 that followed it; the bind now runs under a umask that masks the + "other" bits, so the inode is 0770 from creation and the chmod and chown stay + the authority on its final mode. The parent directory was worse than a + window: it was created with `create_dir_all`, which is also `0777 & ~umask`, + and nothing ever set a mode on it, so under a permissive umask the directory + holding the socket stayed world-writable for the life of the host, and a + world-writable parent lets an unprivileged account plant an entry at the + socket path. Directories this code creates now come out 0750, which is what + the systemd unit (`RuntimeDirectoryMode=0750`) and the FreeBSD rc script + (`install -d -m 0750`) already apply, so no packaged deployment sees a + different mode and no `fipsctl` user loses access. Both the daemon and the + gateway control sockets are covered. **What this does not close**: the window + between the stale-socket probe and the bind is documented at the site rather + than removed. Reaching it needs write access to the socket's parent + directory, which the packaged layouts give to root alone, and an account + holding it can deny the daemon its socket more simply by squatting the path + first. + #### Key material and identity files - Private key writes no longer follow a symlink, and the key file's mode is diff --git a/src/control/mod.rs b/src/control/mod.rs index e26de0c1..f334b516 100644 --- a/src/control/mod.rs +++ b/src/control/mod.rs @@ -151,7 +151,7 @@ mod unix_impl { if let Some(parent) = socket_path.parent() && !parent.exists() { - std::fs::create_dir_all(parent)?; + crate::utils::sockperm::make_parent(parent)?; debug!(path = %parent.display(), "Created control socket directory"); } @@ -160,7 +160,9 @@ mod unix_impl { Self::remove_stale_socket(&socket_path)?; } - let listener = UnixListener::bind(&socket_path)?; + // Bound under a tightened umask, so the inode is never + // world-accessible in the window before the chmod below. + let listener = crate::utils::sockperm::bind(&socket_path)?; // Make the socket and its parent directory group-accessible so // 'fips' group members can use fipsctl/fipstop without root. @@ -183,6 +185,15 @@ mod unix_impl { /// /// If the file exists but no one is listening, remove it so we can /// bind. This handles unclean daemon exits. + /// + /// The gap between the connect probe and the bind that follows is + /// accepted rather than closed. Reaching it needs write access to the + /// socket's parent directory, which the packaged layouts give to root + /// alone (0750 and root-owned under both systemd and the FreeBSD rc + /// script), and an account holding it can deny the daemon its socket + /// more simply by squatting the path before the daemon starts. The + /// removal itself unlinks a symlink rather than its target, so it is + /// not an arbitrary delete. fn remove_stale_socket(path: &Path) -> Result<(), std::io::Error> { // Try connecting to see if someone is listening match std::os::unix::net::UnixStream::connect(path) { diff --git a/src/gateway/control.rs b/src/gateway/control.rs index a77014dd..7a04316b 100644 --- a/src/gateway/control.rs +++ b/src/gateway/control.rs @@ -65,7 +65,7 @@ impl GatewayControlSocket { if let Some(parent) = socket_path.parent() && !parent.exists() { - std::fs::create_dir_all(parent)?; + crate::utils::sockperm::make_parent(parent)?; debug!(path = %parent.display(), "Created gateway control socket directory"); } @@ -74,7 +74,9 @@ impl GatewayControlSocket { Self::remove_stale_socket(&socket_path)?; } - let listener = UnixListener::bind(&socket_path)?; + // Bound under a tightened umask, so the inode is never + // world-accessible in the window before the chmod below. + let listener = crate::utils::sockperm::bind(&socket_path)?; // Set permissions to 0770 and chown to fips group use std::os::unix::fs::PermissionsExt; @@ -93,6 +95,15 @@ impl GatewayControlSocket { } /// Remove a stale socket file from a previous unclean exit. + /// + /// The gap between the connect probe and the bind that follows is + /// accepted rather than closed. Reaching it needs write access to the + /// socket's parent directory, which the packaged layouts give to root + /// alone (0750 and root-owned under both systemd and the FreeBSD rc + /// script), and an account holding it can deny the daemon its socket + /// more simply by squatting the path before the daemon starts. The + /// removal itself unlinks a symlink rather than its target, so it is + /// not an arbitrary delete. fn remove_stale_socket(path: &Path) -> Result<(), std::io::Error> { match std::os::unix::net::UnixStream::connect(path) { Ok(_) => Err(std::io::Error::new( diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 1fd66c36..cb934c02 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -1,6 +1,9 @@ //! Utility modules. //! //! Shared infrastructure that doesn't belong to a specific protocol layer: -//! session index allocation and other cross-cutting concerns. +//! session index allocation, socket permission handling, and other +//! cross-cutting concerns. pub mod index; +#[cfg(unix)] +pub mod sockperm; diff --git a/src/utils/sockperm.rs b/src/utils/sockperm.rs new file mode 100644 index 00000000..2e1d1d08 --- /dev/null +++ b/src/utils/sockperm.rs @@ -0,0 +1,157 @@ +//! Permission-safe creation of Unix domain sockets and the directories +//! holding them. +//! +//! The socket inode and its parent directory are created with a mode the +//! process umask can only tighten, rather than created wide and narrowed +//! afterwards. The caller's own chmod and chown stay where they are and +//! remain the authority on the socket's final mode; this closes the window +//! between creation and that fix-up, and the case of an intermediate +//! directory that nothing fixes up at all. + +use std::path::Path; +use tokio::net::UnixListener; + +/// Mode for a directory this module creates to hold a control socket. +/// +/// Matches what the packaging already applies (systemd's +/// `RuntimeDirectoryMode=0750`, `install -d -m 0750` in the FreeBSD rc +/// script), so no packaged deployment sees a different directory mode than +/// it does today. Widening it would expose the socket path to accounts that +/// cannot reach it now; the umask can still tighten it further. +const SOCKET_DIR_MODE: u32 = 0o750; + +/// umask held across the socket bind. +/// +/// `bind(2)` creates the socket inode with `0777 & !umask`, so under a +/// permissive umask the socket is world-accessible until the chmod that +/// follows it. Masking the "other" bits makes the inode 0770 at creation, +/// which is the mode the caller applies a moment later anyway. Changing +/// this changes the mode the socket is created with, not the mode it ends +/// up with. +const BIND_UMASK: libc::mode_t = 0o007; + +/// Restores the process umask when dropped. +struct UmaskGuard(libc::mode_t); + +impl UmaskGuard { + /// Install `mask` as the process umask, remembering the previous one. + fn tighten(mask: libc::mode_t) -> Self { + // SAFETY: umask(2) cannot fail and touches only process state. + Self(unsafe { libc::umask(mask) }) + } +} + +impl Drop for UmaskGuard { + fn drop(&mut self) { + // SAFETY: as above; restoring the mask this guard replaced. + unsafe { + libc::umask(self.0); + } + } +} + +/// Create the directory that will hold a socket, and any missing ancestors. +/// +/// Directories come out 0750 rather than `0777 & !umask`. Nothing chmods an +/// intermediate directory afterwards, so one created under a permissive +/// umask would stay world-writable for the life of the host, and a +/// world-writable parent lets an unprivileged account plant an entry at the +/// socket path. +pub fn make_parent(parent: &Path) -> Result<(), std::io::Error> { + use std::os::unix::fs::DirBuilderExt; + + std::fs::DirBuilder::new() + .recursive(true) + .mode(SOCKET_DIR_MODE) + .create(parent) +} + +/// Bind a Unix listener whose inode is never world-accessible. +/// +/// The umask is process-global, so it is held across the bind alone. It +/// only clears bits, so anything else created inside that window comes out +/// more restrictive, never less. +pub fn bind(path: &Path) -> Result { + let _umask = UmaskGuard::tighten(BIND_UMASK); + UnixListener::bind(path) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + use std::sync::Mutex; + + /// The umask is process-global, so the tests that set it run one at a + /// time. This does not serialize against the rest of the test binary; + /// the mask used is 0o022, the ordinary default, so a file another test + /// creates in the window is unaffected. + static UMASK_LOCK: Mutex<()> = Mutex::new(()); + + /// Take the umask lock, ignoring poisoning: a test that fails while + /// holding it must not turn its siblings red for an unrelated reason. + fn umask_lock() -> std::sync::MutexGuard<'static, ()> { + UMASK_LOCK.lock().unwrap_or_else(|e| e.into_inner()) + } + + /// Read the current umask, which is only observable by replacing it. + fn current_umask() -> libc::mode_t { + // SAFETY: umask(2) cannot fail; the value read is put straight back. + unsafe { + let old = libc::umask(0o022); + libc::umask(old); + old + } + } + + fn mode_of(path: &Path) -> u32 { + std::fs::symlink_metadata(path) + .unwrap() + .permissions() + .mode() + } + + #[tokio::test] + async fn socket_is_created_without_other_access_under_a_permissive_umask() { + let _lock = umask_lock(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("control.sock"); + + let restore = UmaskGuard::tighten(0o022); + let listener = bind(&path).unwrap(); + drop(restore); + + assert_eq!(mode_of(&path) & 0o007, 0); + drop(listener); + } + + #[tokio::test] + async fn socket_bind_leaves_the_process_umask_as_it_found_it() { + let _lock = umask_lock(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("control.sock"); + + let restore = UmaskGuard::tighten(0o022); + let listener = bind(&path).unwrap(); + let after = current_umask(); + drop(restore); + + assert_eq!(after, 0o022); + drop(listener); + } + + #[test] + fn socket_parent_and_its_ancestors_are_created_without_other_access() { + let _lock = umask_lock(); + let dir = tempfile::tempdir().unwrap(); + let intermediate = dir.path().join("run"); + let parent = intermediate.join("fips"); + + let restore = UmaskGuard::tighten(0o022); + make_parent(&parent).unwrap(); + drop(restore); + + assert_eq!(mode_of(&intermediate) & 0o007, 0); + assert_eq!(mode_of(&parent) & 0o007, 0); + } +}