From 9a5b09cd7f5d4e6bd4dfd3d8961c62f531f73411 Mon Sep 17 00:00:00 2001 From: dailz Date: Sat, 6 Jun 2026 11:05:00 +0800 Subject: [PATCH] fix(security): harden token file permissions (closes #2) - save_restore_token: use create_new(true) + mode(0o600) for exclusive atomic file creation, preventing symlink attacks and predictable temp file exploitation - token_path: return Option, eliminate insecure /tmp fallback - load_restore_token: reject insecure files (symlinks, wrong owner, group/world-readable permissions) - Directory creation uses DirBuilderExt::mode(0o700) bypassing umask - Added verify_secure_dir and ensure_secure_parent with full metadata validation (owner, permissions, symlink rejection) - Added 11 regression tests covering all security scenarios --- Cargo.lock | 1 + Cargo.toml | 3 + src/cap_portal.rs | 312 ++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 306 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c8823de..dc87f7b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2505,6 +2505,7 @@ dependencies = [ "signal-hook", "signal-hook-mio", "str0m", + "tempfile", "tokio", "tracing", "tracing-subscriber", diff --git a/Cargo.toml b/Cargo.toml index 15f0785..e9e8f7f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,3 +28,6 @@ crossbeam-channel = "0.5" str0m = "0.20" serde_json = "1" dirs = "6" + +[dev-dependencies] +tempfile = "3.27.0" diff --git a/src/cap_portal.rs b/src/cap_portal.rs index d8e8592..bb15feb 100644 --- a/src/cap_portal.rs +++ b/src/cap_portal.rs @@ -246,27 +246,162 @@ impl CapPortal { } } -fn token_path() -> PathBuf { - let base = dirs::cache_dir() - .unwrap_or_else(|| PathBuf::from("/tmp")); - base.join("wl-webrtc").join("portal-restore-token") +fn token_path() -> Option { + dirs::cache_dir().map(|base| base.join("wl-webrtc").join("portal-restore-token")) +} + +/// Verify that `path` is a directory owned by the current user with no group/other permissions. +/// Rejects symlinks at the path itself (but allows the resolved target to be a real dir). +fn verify_secure_dir(path: &std::path::Path) -> bool { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + match std::fs::symlink_metadata(path) { + Ok(meta) => { + if meta.file_type().is_symlink() { + tracing::warn!("Token parent dir is a symlink, rejecting: {}", path.display()); + return false; + } + // Must be a directory + if !meta.is_dir() { + tracing::warn!("Token parent path is not a directory: {}", path.display()); + return false; + } + // Must be owned by current user + if meta.uid() != unsafe { libc::getuid() } { + tracing::warn!("Token parent dir not owned by current user: {}", path.display()); + return false; + } + // No group or other permissions (mode must be 0o700 exactly within the 0o777 mask) + let mode = meta.permissions().mode() & 0o777; + if mode != 0o700 { + tracing::warn!( + "Token parent dir has insecure permissions {:o}, expected 0700: {}", + mode, + path.display() + ); + return false; + } + true + } + Err(e) => { + tracing::warn!("Failed to stat token parent dir: {e}"); + false + } + } +} + +/// Ensure the parent directory exists with restrictive permissions (0o700). +/// Returns false if the directory could not be created or is insecure. +fn ensure_secure_parent(parent: &std::path::Path) -> bool { + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + + if parent.exists() { + // Directory exists — try to tighten permissions, then verify. + // set_permissions follows symlinks, which is fine here since + // we verify with symlink_metadata in verify_secure_dir. + if let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) { + tracing::warn!("Failed to set directory permissions: {e}"); + return false; + } + return verify_secure_dir(parent); + } + + // Create with restrictive mode — DirBuilderExt::mode bypasses umask. + let mut builder = std::fs::DirBuilder::new(); + builder.recursive(true); + builder.mode(0o700); + if let Err(e) = builder.create(parent) { + tracing::warn!("Failed to create token directory: {e}"); + return false; + } + + // Verify after creation (belt-and-suspenders) + verify_secure_dir(parent) } fn load_restore_token() -> Option { - let path = token_path(); + load_restore_token_from(token_path()?) +} + +fn load_restore_token_from(path: PathBuf) -> Option { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let meta = match std::fs::symlink_metadata(&path) { + Ok(m) => m, + Err(_) => return None, + }; + + if meta.file_type().is_symlink() { + tracing::warn!("Token file is a symlink, refusing to read: {}", path.display()); + return None; + } + if !meta.is_file() { + tracing::warn!("Token path is not a regular file: {}", path.display()); + return None; + } + if meta.uid() != unsafe { libc::getuid() } { + tracing::warn!("Token file not owned by current user: {}", path.display()); + return None; + } + let mode = meta.permissions().mode() & 0o777; + if mode & 0o077 != 0 { + tracing::warn!( + "Token file has insecure permissions {:o}, refusing to read: {}", + mode, + path.display() + ); + return None; + } + let token = std::fs::read_to_string(&path).ok()?; let trimmed = token.trim().to_string(); if trimmed.is_empty() { None } else { Some(trimmed) } } fn save_restore_token(token: &str) { - let path = token_path(); - if let Some(parent) = path.parent() { - let _ = std::fs::create_dir_all(parent); + let Some(path) = token_path() else { + tracing::warn!("No secure cache directory available, skipping token save"); + return; + }; + save_restore_token_to(token, &path); +} + +fn save_restore_token_to(token: &str, path: &std::path::Path) { + use std::fs::OpenOptions; + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + + let Some(parent) = path.parent() else { + tracing::warn!("Token path has no parent directory"); + return; + }; + + if !ensure_secure_parent(parent) { + tracing::warn!("Parent directory is insecure, refusing to save token"); + return; } - match std::fs::write(&path, token) { + + // Use a unique temp file to prevent symlink attacks. + // create_new(true) guarantees exclusive creation — fails if file already exists, + // and does NOT follow existing symlinks. + let tmp_path = path.with_extension(format!("{}.tmp", std::process::id())); + let result = (|| -> std::io::Result<()> { + let mut f = OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&tmp_path)?; + f.write_all(token.as_bytes())?; + f.sync_all()?; + std::fs::rename(&tmp_path, path)?; + Ok(()) + })(); + match result { Ok(()) => tracing::info!("Saved portal restore token"), - Err(e) => tracing::warn!("Failed to save restore token: {e}"), + Err(e) => { + let _ = std::fs::remove_file(&tmp_path); + tracing::warn!("Failed to save restore token: {e}"); + } } } @@ -645,6 +780,7 @@ fn spa_to_drm_fourcc(format: libspa::param::video::VideoFormat) -> u32 { mod tests { use super::*; use drm_fourcc::DrmFourcc; + use std::os::unix::fs::PermissionsExt; #[test] fn spa_to_drm_fourcc_all_32bit() { @@ -688,4 +824,160 @@ mod tests { use libspa::param::video::VideoFormat; assert_eq!(spa_to_drm_fourcc(VideoFormat::NV12), 0); } + + #[test] + fn token_path_never_uses_tmp() { + assert!(token_path().is_some(), "token_path should resolve on Linux"); + let path = token_path().unwrap(); + assert!(!path.starts_with("/tmp"), "must not fallback to /tmp"); + } + + #[test] + fn verify_secure_dir_rejects_wrong_permissions() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path(); + + // 0o700 should pass + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)).unwrap(); + assert!(verify_secure_dir(path)); + + // 0o755 should fail + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert!(!verify_secure_dir(path)); + + // 0o777 should fail + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o777)).unwrap(); + assert!(!verify_secure_dir(path)); + } + + #[test] + fn verify_secure_dir_rejects_non_directory() { + let dir = tempfile::tempdir().unwrap(); + let file_path = dir.path().join("not-a-dir"); + std::fs::write(&file_path, b"test").unwrap(); + assert!(!verify_secure_dir(&file_path)); + } + + #[test] + fn ensure_secure_parent_creates_with_0700() { + let base = tempfile::tempdir().unwrap(); + let new_dir = base.path().join("wl-test-new-dir"); + assert!(!new_dir.exists()); + + assert!(ensure_secure_parent(&new_dir)); + assert!(new_dir.is_dir()); + + let meta = std::fs::symlink_metadata(&new_dir).unwrap(); + let mode = meta.permissions().mode() & 0o777; + assert_eq!(mode, 0o700, "created directory should be 0700, got {mode:o}"); + } + + #[test] + fn ensure_secure_parent_tightens_existing_dir() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path(); + + // Simulate an existing directory with loose permissions + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).unwrap(); + + assert!(ensure_secure_parent(path)); + + let meta = std::fs::symlink_metadata(path).unwrap(); + let mode = meta.permissions().mode() & 0o777; + assert_eq!(mode, 0o700, "tightened directory should be 0700, got {mode:o}"); + } + + #[test] + fn save_creates_file_with_0600() { + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join("portal-restore-token"); + + save_restore_token_to("secret-token-123", &token_path); + + assert!(token_path.exists()); + let meta = std::fs::symlink_metadata(&token_path).unwrap(); + let mode = meta.permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "token file should be 0600, got {mode:o}"); + assert_eq!(std::fs::read_to_string(&token_path).unwrap(), "secret-token-123"); + } + + #[test] + fn load_reads_secure_file() { + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join("portal-restore-token"); + + // Write a valid 0o600 token file + use std::os::unix::fs::OpenOptionsExt; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&token_path) + .unwrap(); + std::io::Write::write_all(&mut f, b"my-secret\n").unwrap(); + + let result = load_restore_token_from(token_path); + assert_eq!(result, Some("my-secret".to_string())); + } + + #[test] + fn load_rejects_group_readable_file() { + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join("portal-restore-token"); + + // Write with 0o640 (group readable) — should be rejected + use std::os::unix::fs::OpenOptionsExt; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o640) + .open(&token_path) + .unwrap(); + std::io::Write::write_all(&mut f, b"leaked-token\n").unwrap(); + + let result = load_restore_token_from(token_path); + assert!(result.is_none(), "should reject group-readable token file"); + } + + #[test] + fn load_rejects_world_readable_file() { + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join("portal-restore-token"); + + use std::os::unix::fs::OpenOptionsExt; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o604) + .open(&token_path) + .unwrap(); + std::io::Write::write_all(&mut f, b"leaked-token\n").unwrap(); + + let result = load_restore_token_from(token_path); + assert!(result.is_none(), "should reject world-readable token file"); + } + + #[test] + fn load_rejects_symlink() { + let dir = tempfile::tempdir().unwrap(); + let real_path = dir.path().join("real-file"); + let link_path = dir.path().join("portal-restore-token"); + + std::fs::write(&real_path, b"target-content\n").unwrap(); + std::os::unix::fs::symlink(&real_path, &link_path).unwrap(); + + let result = load_restore_token_from(link_path); + assert!(result.is_none(), "should reject symlinked token file"); + } + + #[test] + fn save_then_load_roundtrip() { + let dir = tempfile::tempdir().unwrap(); + let token_path = dir.path().join("portal-restore-token"); + + save_restore_token_to("roundtrip-token", &token_path); + let loaded = load_restore_token_from(token_path); + + assert_eq!(loaded, Some("roundtrip-token".to_string())); + } }