diff --git a/src/api/config_store.rs b/src/api/config_store.rs index 6d1a90d..b6edd02 100644 --- a/src/api/config_store.rs +++ b/src/api/config_store.rs @@ -245,15 +245,24 @@ pub(super) async fn load_candidate_snapshot( } fn normalize_source_path(path: &Path) -> PathBuf { - path.canonicalize().unwrap_or_else(|_| { - if path.is_absolute() { - path.to_path_buf() - } else { - std::env::current_dir() - .map(|cwd| cwd.join(path)) - .unwrap_or_else(|_| path.to_path_buf()) + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + std::env::current_dir() + .map(|cwd| cwd.join(path)) + .unwrap_or_else(|_| path.to_path_buf()) + }; + let mut normalized = PathBuf::new(); + for component in absolute.components() { + match component { + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + normalized.pop(); + } + component => normalized.push(component.as_os_str()), } - }) + } + normalized } pub(super) async fn load_config_from_disk(config_path: &Path) -> Result { diff --git a/src/api/config_store/tests.rs b/src/api/config_store/tests.rs index 7d09de4..5589002 100644 --- a/src/api/config_store/tests.rs +++ b/src/api/config_store/tests.rs @@ -403,3 +403,17 @@ fn render_user_rate_limits_section() { assert!(rendered.starts_with("[access.user_rate_limits]\n")); assert!(rendered.contains("alice = { up_bps = 1024, down_bps = 2048 }")); } + +#[cfg(unix)] +#[test] +fn source_owner_normalization_preserves_symlinks() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let real = dir.path().join("real.toml"); + let linked = dir.path().join("linked.toml"); + std::fs::write(&real, "").unwrap(); + symlink(&real, &linked).unwrap(); + + assert_eq!(normalize_source_path(&linked), linked); +} diff --git a/src/config/hot_reload/watcher.rs b/src/config/hot_reload/watcher.rs index 9197d2b..178691a 100644 --- a/src/config/hot_reload/watcher.rs +++ b/src/config/hot_reload/watcher.rs @@ -52,15 +52,24 @@ impl ReloadState { } fn normalize_watch_path(path: &Path) -> PathBuf { - path.canonicalize().unwrap_or_else(|_| { - if path.is_absolute() { - path.to_path_buf() - } else { - std::env::current_dir() - .map(|cwd| cwd.join(path)) - .unwrap_or_else(|_| path.to_path_buf()) + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + std::env::current_dir() + .map(|cwd| cwd.join(path)) + .unwrap_or_else(|_| path.to_path_buf()) + }; + let mut normalized = PathBuf::new(); + for component in absolute.components() { + match component { + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + normalized.pop(); + } + component => normalized.push(component.as_os_str()), } - }) + } + normalized } fn sync_watch_paths( @@ -433,3 +442,21 @@ pub fn spawn_config_watcher( (config_rx, log_rx, task) } + +#[cfg(all(test, unix))] +mod path_tests { + use std::os::unix::fs::symlink; + + use super::normalize_watch_path; + + #[test] + fn watch_path_normalization_preserves_symlinks() { + let dir = tempfile::tempdir().unwrap(); + let real = dir.path().join("real.toml"); + let linked = dir.path().join("linked.toml"); + std::fs::write(&real, "").unwrap(); + symlink(&real, &linked).unwrap(); + + assert_eq!(normalize_watch_path(&linked), linked); + } +} diff --git a/src/daemon/pid_file.rs b/src/daemon/pid_file.rs index 3536e4d..bc0acdd 100644 --- a/src/daemon/pid_file.rs +++ b/src/daemon/pid_file.rs @@ -1,15 +1,18 @@ -use std::fs::{self, File, OpenOptions}; -use std::io::{ErrorKind, Read, Write}; -use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; +use std::ffi::OsStr; +use std::fs::{self, File}; +use std::io::{self, ErrorKind, Read, Write}; +use std::os::unix::fs::MetadataExt; #[cfg(target_os = "linux")] use std::os::fd::{FromRawFd, OwnedFd}; use std::path::{Path, PathBuf}; -use nix::fcntl::{Flock, FlockArg}; -use nix::unistd::{Pid, getpid}; +use nix::fcntl::{Flock, FlockArg, OFlag, openat}; +use nix::sys::stat::Mode; +use nix::unistd::{Pid, UnlinkatFlags, getpid, unlinkat}; use tracing::{debug, info, warn}; use super::DaemonError; +use crate::util::secure_fs::AnchoredPath; /// PID file manager backed by a persistent sibling lock file. pub struct PidFile { @@ -18,6 +21,7 @@ pub struct PidFile { pid_file: Option, pid_identity: Option, lock_file: Option>, + anchor: Option, } #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -46,6 +50,7 @@ impl PidFile { pid_file: None, pid_identity: None, lock_file: None, + anchor: None, } } @@ -61,37 +66,42 @@ impl PidFile { /// /// Fails if another owner holds the lock or the existing PID names a running process. pub fn acquire(&mut self) -> Result<(), DaemonError> { - if let Some(parent) = self.path.parent() - && !parent.exists() - { - fs::create_dir_all(parent).map_err(|error| { + let anchor = AnchoredPath::open_trusted_parent_or_create(&self.path, 0o755).map_err( + |error| { DaemonError::PidFile(format!( - "cannot create directory {}: {}", - parent.display(), + "cannot open trusted parent for {}: {}", + self.path.display(), error )) - })?; - } - - let lock_file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .mode(0o644) - .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW) - .open(&self.lock_path) - .map_err(|error| { - DaemonError::PidFile(format!( - "cannot open lock file {}: {}", - self.lock_path.display(), - error - )) - })?; + }, + )?; + let lock_name = self.lock_path.file_name().ok_or_else(|| { + DaemonError::PidFile(format!( + "lock path {} has no file name", + self.lock_path.display() + )) + })?; + let lock_file = open_file_at( + &anchor, + lock_name, + OFlag::O_RDWR | OFlag::O_CREAT | OFlag::O_CLOEXEC | OFlag::O_NOFOLLOW, + 0o644, + ) + .map_err(|error| { + DaemonError::PidFile(format!( + "cannot open lock file {}: {}", + self.lock_path.display(), + error + )) + })?; validate_regular_single_link(&lock_file, &self.lock_path)?; let lock_file = Flock::lock(lock_file, FlockArg::LockExclusiveNonblock).map_err(|(_, errno)| { - if let Some(pid) = self.check_running().ok().flatten() { + if let Some(pid) = read_pid_file_at(&anchor, &self.path) + .ok() + .flatten() + .filter(|pid| is_process_running(*pid)) + { DaemonError::AlreadyRunning(pid) } else { DaemonError::PidFile(format!( @@ -102,23 +112,28 @@ impl PidFile { } })?; - if let Some(pid) = self.check_running()? { + if let Some(pid) = read_pid_file_at(&anchor, &self.path)? + && is_process_running(pid) + { return Err(DaemonError::AlreadyRunning(pid)); } - let mut pid_file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(true) - .mode(0o644) - .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW) - .open(&self.path) - .map_err(|error| { - DaemonError::PidFile(format!("cannot open {}: {}", self.path.display(), error)) - })?; + let mut pid_file = open_file_at( + &anchor, + anchor.name(), + OFlag::O_RDWR | OFlag::O_CREAT | OFlag::O_CLOEXEC | OFlag::O_NOFOLLOW, + 0o644, + ) + .map_err(|error| { + DaemonError::PidFile(format!("cannot open {}: {}", self.path.display(), error)) + })?; let pid_metadata = validate_regular_single_link(&pid_file, &self.path)?; let pid_identity = FileIdentity::from_metadata(&pid_metadata); + // Validate the opened inode before modifying it so a hard-link substitution + // cannot turn PID publication into truncation of an unrelated file. + pid_file.set_len(0).map_err(|error| { + DaemonError::PidFile(format!("cannot truncate {}: {}", self.path.display(), error)) + })?; let pid = getpid(); writeln!(pid_file, "{}", pid).map_err(|error| { DaemonError::PidFile(format!( @@ -138,6 +153,7 @@ impl PidFile { self.pid_file = Some(pid_file); self.pid_identity = Some(pid_identity); self.lock_file = Some(lock_file); + self.anchor = Some(anchor); info!(pid = pid.as_raw(), path = %self.path.display(), "PID file created"); Ok(()) } @@ -147,36 +163,20 @@ impl PidFile { if self.lock_file.is_none() { self.pid_file = None; self.pid_identity = None; + self.anchor = None; return Ok(()); } - let removal = match fs::symlink_metadata(&self.path) { - Ok(metadata) - if self.pid_identity == Some(FileIdentity::from_metadata(&metadata)) - && metadata.is_file() => - { - fs::remove_file(&self.path).map_err(|error| { - DaemonError::PidFile(format!( - "cannot remove {}: {}", - self.path.display(), - error - )) - }) - } - Ok(_) => Err(DaemonError::PidFile(format!( - "refusing to remove replaced PID file {}", - self.path.display() - ))), - Err(error) if error.kind() == ErrorKind::NotFound => Ok(()), - Err(error) => Err(DaemonError::PidFile(format!( - "cannot inspect {} before removal: {}", - self.path.display(), - error - ))), + let removal = match self.anchor.as_ref() { + Some(anchor) => remove_owned_pid_file(anchor, &self.path, self.pid_identity), + None => Err(DaemonError::PidFile( + "PID file lock is held without a directory anchor".to_string(), + )), }; self.pid_file = None; self.pid_identity = None; self.lock_file = None; + self.anchor = None; removal?; debug!(path = %self.path.display(), "PID file removed"); Ok(()) @@ -209,12 +209,44 @@ fn sibling_lock_path(path: &Path) -> PathBuf { lock_path.into() } +fn open_file_at( + anchor: &AnchoredPath, + name: &OsStr, + flags: OFlag, + mode: u32, +) -> io::Result { + let descriptor = openat( + anchor.parent(), + name, + flags, + Mode::from_bits_truncate(mode), + ) + .map_err(|error| io::Error::from_raw_os_error(error as i32))?; + Ok(File::from(descriptor)) +} + fn read_pid_file_if_exists(path: &Path) -> Result, DaemonError> { - let mut file = match OpenOptions::new() - .read(true) - .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW) - .open(path) - { + let anchor = match AnchoredPath::open_trusted_parent(path) { + Ok(anchor) => anchor, + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(None), + Err(error) => { + return Err(DaemonError::PidFile(format!( + "cannot open trusted parent for {}: {}", + path.display(), + error + ))); + } + }; + read_pid_file_at(&anchor, path) +} + +fn read_pid_file_at(anchor: &AnchoredPath, path: &Path) -> Result, DaemonError> { + let mut file = match open_file_at( + anchor, + anchor.name(), + OFlag::O_RDONLY | OFlag::O_CLOEXEC | OFlag::O_NOFOLLOW, + 0, + ) { Ok(file) => file, Err(error) if error.kind() == ErrorKind::NotFound => return Ok(None), Err(error) => { @@ -249,6 +281,49 @@ fn read_pid_file_if_exists(path: &Path) -> Result, DaemonError> { Ok(Some(pid)) } +fn remove_owned_pid_file( + anchor: &AnchoredPath, + path: &Path, + expected: Option, +) -> Result<(), DaemonError> { + let file = match open_file_at( + anchor, + anchor.name(), + OFlag::O_RDONLY | OFlag::O_CLOEXEC | OFlag::O_NOFOLLOW, + 0, + ) { + Ok(file) => file, + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(()), + Err(error) => { + return Err(DaemonError::PidFile(format!( + "cannot inspect {} before removal: {}", + path.display(), + error + ))); + } + }; + let metadata = validate_regular_single_link(&file, path)?; + if expected != Some(FileIdentity::from_metadata(&metadata)) { + return Err(DaemonError::PidFile(format!( + "refusing to remove replaced PID file {}", + path.display() + ))); + } + drop(file); + unlinkat( + anchor.parent(), + anchor.name(), + UnlinkatFlags::NoRemoveDir, + ) + .map_err(|error| { + DaemonError::PidFile(format!( + "cannot remove {}: {}", + path.display(), + io::Error::from_raw_os_error(error as i32) + )) + }) +} + fn validate_regular_single_link( file: &File, path: &Path, @@ -329,12 +404,26 @@ pub fn check_status>(path: P) -> DaemonStatus { fn daemon_lock_is_held(path: &Path) -> Result { let lock_path = sibling_lock_path(path); - let file = match OpenOptions::new() - .read(true) - .write(true) - .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW) - .open(&lock_path) - { + let anchor = match AnchoredPath::open_trusted_parent(path) { + Ok(anchor) => anchor, + Err(error) if error.kind() == ErrorKind::NotFound => return Ok(false), + Err(error) => { + return Err(DaemonError::PidFile(format!( + "cannot open trusted parent for {}: {}", + path.display(), + error + ))); + } + }; + let lock_name = lock_path.file_name().ok_or_else(|| { + DaemonError::PidFile(format!("lock path {} has no file name", lock_path.display())) + })?; + let file = match open_file_at( + &anchor, + lock_name, + OFlag::O_RDWR | OFlag::O_CLOEXEC | OFlag::O_NOFOLLOW, + 0, + ) { Ok(file) => file, Err(error) if error.kind() == ErrorKind::NotFound => return Ok(false), Err(error) => { diff --git a/src/daemon/pid_file/tests.rs b/src/daemon/pid_file/tests.rs index 9aad337..db23065 100644 --- a/src/daemon/pid_file/tests.rs +++ b/src/daemon/pid_file/tests.rs @@ -156,6 +156,57 @@ fn acquire_rejects_pid_symlink_without_truncating_target() { assert_eq!(fs::read(&target_path).unwrap(), b"preserve\n"); } +#[test] +fn acquire_rejects_pid_hard_link_without_truncating_target() { + let directory = tempfile::tempdir().unwrap(); + let pid_path = directory.path().join("telemt.pid"); + let target_path = directory.path().join("target"); + fs::write(&target_path, b"preserve\n").unwrap(); + fs::hard_link(&target_path, &pid_path).unwrap(); + let mut pid_file = PidFile::new(&pid_path); + + assert!(pid_file.acquire().is_err()); + assert_eq!(fs::read(&target_path).unwrap(), b"preserve\n"); +} + +#[test] +fn acquire_rejects_symlinked_parent_without_publishing_outside() { + let directory = tempfile::tempdir().unwrap(); + let real_parent = directory.path().join("real"); + let linked_parent = directory.path().join("linked"); + fs::create_dir(&real_parent).unwrap(); + symlink(&real_parent, &linked_parent).unwrap(); + let pid_path = linked_parent.join("telemt.pid"); + let mut pid_file = PidFile::new(&pid_path); + + assert!(pid_file.acquire().is_err()); + assert!(!real_parent.join("telemt.pid").exists()); + assert!(!real_parent.join("telemt.pid.lock").exists()); +} + +#[test] +fn release_remains_anchored_after_parent_path_replacement() { + let directory = tempfile::tempdir().unwrap(); + let active_parent = directory.path().join("active"); + let moved_parent = directory.path().join("moved"); + fs::create_dir(&active_parent).unwrap(); + let pid_path = active_parent.join("telemt.pid"); + let mut pid_file = PidFile::new(&pid_path); + pid_file.acquire().unwrap(); + + fs::rename(&active_parent, &moved_parent).unwrap(); + fs::create_dir(&active_parent).unwrap(); + fs::write(active_parent.join("telemt.pid"), b"replacement\n").unwrap(); + + pid_file.release().unwrap(); + + assert!(!moved_parent.join("telemt.pid").exists()); + assert_eq!( + fs::read(active_parent.join("telemt.pid")).unwrap(), + b"replacement\n" + ); +} + #[test] fn release_does_not_remove_replacement_path() { let directory = tempfile::tempdir().unwrap(); diff --git a/src/logging/file.rs b/src/logging/file.rs index f59c8a1..bdd989e 100644 --- a/src/logging/file.rs +++ b/src/logging/file.rs @@ -67,7 +67,7 @@ impl BoundedFileAppender { let start = now(); let current_path = active_path_for(&dir, &base_name, options.rotation, &start); #[cfg(unix)] - let dir_fd = crate::util::secure_fs::open_dir_nofollow_or_create(&dir, 0o750)?; + let dir_fd = crate::util::secure_fs::open_trusted_dir_nofollow_or_create(&dir, 0o750)?; #[cfg(unix)] let (file, current_size) = open_append_file(&dir_fd, ¤t_path)?; #[cfg(not(unix))] diff --git a/src/logging/file/tests.rs b/src/logging/file/tests.rs index edaf984..a1fba6a 100644 --- a/src/logging/file/tests.rs +++ b/src/logging/file/tests.rs @@ -123,3 +123,24 @@ fn rotation_stays_bound_to_opened_directory_after_path_replacement() { assert!(!matching_logs(&moved).is_empty()); assert!(matching_logs(&redirect).is_empty()); } + +#[cfg(unix)] +#[test] +fn appender_rejects_group_writable_log_directory() { + use std::os::unix::fs::PermissionsExt; + + let current = std::env::current_dir().unwrap(); + let dir = tempfile::Builder::new() + .prefix("telemt-untrusted-log-") + .tempdir_in(current) + .unwrap(); + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o770)).unwrap(); + + assert!( + BoundedFileAppender::with_now( + options(dir.path().join("telemt.log")), + Box::new(fixed_now), + ) + .is_err() + ); +} diff --git a/src/maestro/helpers.rs b/src/maestro/helpers.rs index 3245674..57a38e3 100644 --- a/src/maestro/helpers.rs +++ b/src/maestro/helpers.rs @@ -35,6 +35,20 @@ pub(crate) fn resolve_runtime_config_path( startup_cwd: &Path, config_path_explicit: bool, ) -> PathBuf { + let normalize = |path: PathBuf| { + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + normalized.pop(); + } + component => normalized.push(component.as_os_str()), + } + } + normalized + }; + if config_path_explicit { let raw = PathBuf::from(config_path_cli); let absolute = if raw.is_absolute() { @@ -42,7 +56,7 @@ pub(crate) fn resolve_runtime_config_path( } else { startup_cwd.join(raw) }; - return absolute.canonicalize().unwrap_or(absolute); + return normalize(absolute); } let etc_telemt = std::path::Path::new("/etc/telemt"); @@ -54,7 +68,7 @@ pub(crate) fn resolve_runtime_config_path( ]; for candidate in candidates { if candidate.is_file() { - return candidate.canonicalize().unwrap_or(candidate); + return normalize(candidate); } } @@ -91,7 +105,17 @@ fn normalize_runtime_dir(path: &Path, startup_cwd: &Path) -> PathBuf { } else { startup_cwd.join(path) }; - absolute.canonicalize().unwrap_or(absolute) + let mut normalized = PathBuf::new(); + for component in absolute.components() { + match component { + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + normalized.pop(); + } + component => normalized.push(component.as_os_str()), + } + } + normalized } /// Parsed CLI arguments. diff --git a/src/maestro/helpers/tests.rs b/src/maestro/helpers/tests.rs index de28726..ee770aa 100644 --- a/src/maestro/helpers/tests.rs +++ b/src/maestro/helpers/tests.rs @@ -52,6 +52,35 @@ let _ = std::fs::remove_dir(&startup_cwd); } + #[cfg(unix)] + #[test] + fn runtime_paths_preserve_symlinks_for_descriptor_validation() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let real_dir = dir.path().join("real"); + let linked_dir = dir.path().join("linked"); + std::fs::create_dir(&real_dir).unwrap(); + std::fs::write(real_dir.join("config.toml"), " ").unwrap(); + symlink(&real_dir, &linked_dir).unwrap(); + let linked_config = linked_dir.join("config.toml"); + + let config = resolve_runtime_config_path( + linked_config.to_str().unwrap(), + dir.path(), + true, + ); + let runtime = resolve_runtime_base_dir( + &linked_config, + dir.path(), + true, + Some(&linked_dir), + ); + + assert_eq!(config, linked_config); + assert_eq!(runtime, linked_dir); + } + #[test] fn resolve_runtime_config_path_uses_startup_candidates_when_not_explicit() { let nonce = std::time::SystemTime::now() diff --git a/src/proxy/user_admission.rs b/src/proxy/user_admission.rs index 4c9b835..e4d4139 100644 --- a/src/proxy/user_admission.rs +++ b/src/proxy/user_admission.rs @@ -1,12 +1,16 @@ use std::collections::HashMap; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicU8, Ordering}; use parking_lot::{Mutex, MutexGuard}; use tokio_util::sync::CancellationToken; use crate::crypto::sha256; +const REGISTRATION_PENDING: u8 = 0; +const REGISTRATION_ACTIVE: u8 = 1; +const REGISTRATION_DROPPED: u8 = 2; + /// Stable secret identity used to fence authentication across runtime generations. pub(crate) type UserCredentialId = [u8; 16]; @@ -358,7 +362,7 @@ impl UserAdmissionAuthority { }; let registration_id = state.allocate_registration_id()?; let token = CancellationToken::new(); - let active = Arc::new(AtomicBool::new(false)); + let active = Arc::new(AtomicU8::new(REGISTRATION_PENDING)); Some(UserAdmissionPublication { state, authority: Arc::clone(self), @@ -429,7 +433,7 @@ pub(crate) struct UserAdmissionPublication<'a> { registration_id: u64, incarnation: UserIncarnation, token: CancellationToken, - active: Arc, + active: Arc, registration_taken: bool, } @@ -455,6 +459,11 @@ impl UserAdmissionPublication<'_> { if !self.registration_taken { return; } + if self.active.compare_exchange( + REGISTRATION_PENDING, REGISTRATION_ACTIVE, Ordering::AcqRel, Ordering::Acquire, + ).is_err() { + return; + } self.state .owners_by_user .entry(self.user.clone()) @@ -466,7 +475,6 @@ impl UserAdmissionPublication<'_> { incarnation: self.incarnation, }, ); - self.active.store(true, Ordering::Release); } } @@ -478,7 +486,7 @@ pub(crate) struct UserSessionRegistration { registration_id: u64, incarnation: UserIncarnation, token: CancellationToken, - active: Arc, + active: Arc, } impl UserSessionRegistration { @@ -500,7 +508,7 @@ impl UserSessionRegistration { impl Drop for UserSessionRegistration { fn drop(&mut self) { - if self.active.swap(false, Ordering::AcqRel) { + if self.active.swap(REGISTRATION_DROPPED, Ordering::AcqRel) == REGISTRATION_ACTIVE { self.authority .unregister(&self.user, self.registration_id, self.incarnation); } diff --git a/src/proxy/user_admission/tests.rs b/src/proxy/user_admission/tests.rs index 8cc6592..49bcbb5 100644 --- a/src/proxy/user_admission/tests.rs +++ b/src/proxy/user_admission/tests.rs @@ -54,3 +54,20 @@ fn stale_candidate_cannot_overwrite_newer_mutation() { ); assert!(!authority.is_user_enabled("alice")); } + +#[test] +fn registration_dropped_before_publication_cannot_leave_an_owner() { + let authority = UserAdmissionAuthority::new(); + let secret = "00112233445566778899aabbccddeeff"; + authority.apply_config(&users(secret), &HashMap::new()); + let credential = credential_id_from_hex(secret).unwrap(); + let mut publication = authority + .claim_authenticated("alice", credential) + .unwrap(); + let registration = publication.take_registration().unwrap(); + + drop(registration); + publication.commit(); + + assert_eq!(authority.cancel_user_owners("alice"), 0); +} diff --git a/src/slot_budget.rs b/src/slot_budget.rs index cf1784c..e70a059 100644 --- a/src/slot_budget.rs +++ b/src/slot_budget.rs @@ -61,7 +61,6 @@ impl SlotBudget { pub(crate) fn used(&self) -> usize { self.used.load(Ordering::Acquire) } - } /// Provisional slot ownership that rolls back unless committed to a registry entry. diff --git a/src/util/secure_fs/mod.rs b/src/util/secure_fs/mod.rs index 1806107..e8641fb 100644 --- a/src/util/secure_fs/mod.rs +++ b/src/util/secure_fs/mod.rs @@ -9,7 +9,7 @@ mod write; pub(crate) use path::{ AnchoredPath, chdir_nofollow_or_create, open_dir_nofollow, - open_dir_nofollow_or_create, + open_dir_nofollow_or_create, open_trusted_dir_nofollow_or_create, }; pub(crate) use write::{ atomic_replace, atomic_replace_async, open_append_regular, open_append_regular_at, diff --git a/src/util/secure_fs/path.rs b/src/util/secure_fs/path.rs index 866fc78..f395b44 100644 --- a/src/util/secure_fs/path.rs +++ b/src/util/secure_fs/path.rs @@ -41,6 +41,18 @@ impl AnchoredPath { Ok(Self { parent, name }) } + /// Creates missing parents and opens a chain protected from untrusted renames. + pub(crate) fn open_trusted_parent_or_create(path: &Path, mode: u32) -> io::Result { + let name = path + .file_name() + .filter(|name| !name.is_empty()) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no file name"))? + .to_os_string(); + let parent_path = path.parent().unwrap_or_else(|| Path::new(".")); + let parent = open_trusted_dir_nofollow_or_create(parent_path, mode)?; + Ok(Self { parent, name }) + } + fn open_with_parent_creation(path: &Path, create_mode: Option) -> io::Result { let name = path .file_name() @@ -80,6 +92,14 @@ pub(crate) fn open_dir_nofollow_or_create(path: &Path, mode: u32) -> io::Result< open_or_create_dir_nofollow(path, mode) } +/// Opens or creates a directory chain protected from untrusted entry replacement. +pub(crate) fn open_trusted_dir_nofollow_or_create( + path: &Path, + mode: u32, +) -> io::Result { + open_dir_components(path, Some(mode), true) +} + /// Opens a directory only when its entire path is owned by root or the effective user. fn open_trusted_dir_nofollow(path: &Path) -> io::Result { open_dir_components(path, None, true) @@ -90,6 +110,20 @@ fn open_dir_components( create_mode: Option, require_trusted: bool, ) -> io::Result { + let mut names = Vec::new(); + for component in path.components() { + match component { + Component::RootDir | Component::CurDir => continue, + Component::Normal(name) => names.push(name.to_os_string()), + Component::ParentDir => names.push(OsString::from("..")), + Component::Prefix(_) => { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "unsupported path prefix", + )); + } + } + } let start = if path.is_absolute() { Path::new("/") } else { @@ -97,48 +131,46 @@ fn open_dir_components( }; let mut current = open(start, DIRECTORY_FLAGS, Mode::empty()).map_err(errno_to_io)?; if require_trusted { - validate_trusted_directory(¤t)?; + validate_trusted_directory(¤t, !names.is_empty())?; } - - for component in path.components() { - let name = match component { - Component::RootDir | Component::CurDir => continue, - Component::Normal(name) => name, - Component::ParentDir => OsStr::new(".."), - Component::Prefix(_) => { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - "unsupported path prefix", - )); - } - }; - let next = match openat(¤t, name, DIRECTORY_FLAGS, Mode::empty()) { + let component_count = names.len(); + for (index, name) in names.into_iter().enumerate() { + let next = match openat(¤t, name.as_os_str(), DIRECTORY_FLAGS, Mode::empty()) { Ok(descriptor) => descriptor, Err(nix::errno::Errno::ENOENT) if create_mode.is_some() => { let mode = Mode::from_bits_truncate(create_mode.unwrap_or(0o750)); - match mkdirat(¤t, name, mode) { + match mkdirat(¤t, name.as_os_str(), mode) { Ok(()) | Err(nix::errno::Errno::EEXIST) => {} Err(error) => return Err(errno_to_io(error)), } - openat(¤t, name, DIRECTORY_FLAGS, Mode::empty()).map_err(errno_to_io)? + openat( + ¤t, + name.as_os_str(), + DIRECTORY_FLAGS, + Mode::empty(), + ) + .map_err(errno_to_io)? } Err(error) => return Err(errno_to_io(error)), }; if require_trusted { - validate_trusted_directory(&next)?; + validate_trusted_directory(&next, index + 1 < component_count)?; } current = next; } Ok(current) } -fn validate_trusted_directory(descriptor: &OwnedFd) -> io::Result<()> { +fn validate_trusted_directory(descriptor: &OwnedFd, allow_sticky_parent: bool) -> io::Result<()> { let file = std::fs::File::from(descriptor.try_clone()?); let metadata = file.metadata()?; let effective_uid = nix::unistd::Uid::effective().as_raw(); + let mode = metadata.permissions().mode(); + let writable_by_others = mode & 0o022 != 0; + let protected_sticky_parent = allow_sticky_parent && mode & 0o1000 != 0; if !metadata.is_dir() || (metadata.uid() != 0 && metadata.uid() != effective_uid) - || metadata.permissions().mode() & 0o022 != 0 + || (writable_by_others && !protected_sticky_parent) { return Err(io::Error::new( io::ErrorKind::PermissionDenied,