From 5e5d324847f57c05c8f7d1aba9cea6bae0cacf7e Mon Sep 17 00:00:00 2001 From: Florent Benoit Date: Tue, 22 Sep 2026 22:10:48 +0200 Subject: [PATCH] fix(vm): relocate per-sandbox Unix sockets to /tmp to fit macOS sun_path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move VM driver per-sandbox Unix sockets (control.sock, ssh.sock) from the state directory to /tmp/openshell--/sandboxes// so that socket paths stay within the 104-byte macOS sun_path limit regardless of home directory depth. The socket namespace is keyed by effective UID and an 8-char SHA-256 prefix of the state directory, so driver instances with distinct state roots do not collide on the same sandbox ID. Every path component — including the per-sandbox leaf — is created individually with create_dir (not create_dir_all), verified with symlink_metadata to be a real directory owned by the expected UID, and set to mode 0700 — preventing symlink-based privilege escalation via pre-planted paths. Closes #3452 Signed-off-by: Florent Benoit --- crates/openshell-driver-vm/src/driver.rs | 227 ++++++++++++++++++++++- 1 file changed, 224 insertions(+), 3 deletions(-) diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index 68f1de47c8..cdd2194677 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -77,7 +77,7 @@ use std::io::{BufRead, BufReader, BufWriter, Read, Seek, SeekFrom, Write}; #[cfg(unix)] use std::os::fd::AsRawFd as _; #[cfg(unix)] -use std::os::unix::fs::PermissionsExt; +use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::{Component, Path, PathBuf}; use std::pin::Pin; use std::process::Stdio; @@ -921,7 +921,7 @@ impl VmDriver { .env(openshell_core::sandbox_env::SANDBOX, &sandbox.name) .env( openshell_core::sandbox_env::SSH_SOCKET_PATH, - state_dir.join("ssh.sock"), + sandbox_socket_dir(&self.config.state_dir, &sandbox.id).join("ssh.sock"), ) .env( openshell_core::sandbox_env::PROXY_TLS_DIR, @@ -1125,10 +1125,18 @@ impl VmDriver { return Err(Status::internal(format!("create state dir failed: {err}"))); } + if let Err(err) = create_sandbox_socket_dir(&self.config.state_dir, &sandbox.id).await { + let mut registry = self.registry.lock().await; + registry.remove(&sandbox.id); + let _ = tokio::fs::remove_dir_all(&state_dir).await; + return Err(Status::internal(format!("create socket dir failed: {err}"))); + } + if let Err(err) = self.ensure_extension_state_dirs(&state_dir).await { let mut registry = self.registry.lock().await; registry.remove(&sandbox.id); let _ = tokio::fs::remove_dir_all(&state_dir).await; + remove_sandbox_socket_dir(&self.config.state_dir, &sandbox.id).await; return Err(err); } @@ -1136,6 +1144,7 @@ impl VmDriver { let mut registry = self.registry.lock().await; registry.remove(&sandbox.id); let _ = tokio::fs::remove_dir_all(&state_dir).await; + remove_sandbox_socket_dir(&self.config.state_dir, &sandbox.id).await; return Err(Status::internal(format!( "write sandbox start metadata failed: {err}" ))); @@ -1211,6 +1220,7 @@ impl VmDriver { if overlay_preparation == OverlayPreparation::Fresh { let _ = tokio::fs::remove_dir_all(&state_dir).await; } + remove_sandbox_socket_dir(&self.config.state_dir, &sandbox_id).await; return; } @@ -1482,7 +1492,11 @@ impl VmDriver { } let console_output = state_dir.join("rootfs-console.log"); - let control_socket = state_dir.join(VM_CONTROL_SOCKET); + create_sandbox_socket_dir(&self.config.state_dir, &sandbox.id) + .await + .map_err(|err| Status::internal(format!("create socket dir failed: {err}")))?; + let control_socket = + sandbox_socket_dir(&self.config.state_dir, &sandbox.id).join(VM_CONTROL_SOCKET); let session_id = launch_authentication.supervisor.session_id; let channel_tls = generate_sandbox_tls_material(session_id) .map_err(|error| Status::internal(error.to_string()))?; @@ -1990,6 +2004,7 @@ impl VmDriver { } remove_sandbox_state_dir(&self.config.state_dir, &state_dir).await?; + remove_sandbox_socket_dir(&self.config.state_dir, &record_id).await; { let mut registry = self.registry.lock().await; @@ -2611,6 +2626,7 @@ impl VmDriver { if remove_state { let _ = tokio::fs::remove_dir_all(state_dir).await; + remove_sandbox_socket_dir(&self.config.state_dir, sandbox_id).await; } self.publish_platform_event( sandbox_id.to_string(), @@ -5658,6 +5674,89 @@ async fn restrict_owner_only_dir(_path: &Path) -> Result<(), std::io::Error> { Ok(()) } +/// Socket directory under `/tmp` for a sandbox's Unix domain sockets +/// (`control.sock`, `ssh.sock`). Hardcoded `/tmp` rather than +/// `std::env::temp_dir()` because macOS `TMPDIR` resolves to +/// `/var/folders/…/T/` (~51 chars), which would re-exceed the 104-byte +/// `sun_path` limit that this relocation exists to fix. +/// Keyed by effective UID and an 8-char SHA-256 prefix of the state +/// directory so driver instances with distinct state roots do not collide. +fn sandbox_socket_dir(state_dir: &Path, sandbox_id: &str) -> PathBuf { + let uid = rustix::process::geteuid().as_raw(); + let hash = state_dir_hash(state_dir); + PathBuf::from(format!("/tmp/openshell-{uid}-{hash}")) + .join("sandboxes") + .join(sandbox_id) +} + +fn state_dir_hash(state_dir: &Path) -> String { + let digest = Sha256::digest(state_dir.as_os_str().as_encoded_bytes()); + format!( + "{:02x}{:02x}{:02x}{:02x}", + digest[0], digest[1], digest[2], digest[3] + ) +} + +/// Create a single directory level and verify it is a real directory owned by +/// `expected_uid`. Uses `create_dir` (not `create_dir_all`) so symlinks in +/// parent components cannot redirect the target, and `symlink_metadata` so +/// a symlink at the leaf is detected rather than followed. +async fn ensure_private_dir(path: &Path, expected_uid: u32) -> Result<(), std::io::Error> { + match tokio::fs::create_dir(path).await { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(e) => return Err(e), + } + let metadata = tokio::fs::symlink_metadata(path).await?; + if metadata.file_type().is_symlink() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("{} is a symlink; refusing to use it", path.display()), + )); + } + if !metadata.file_type().is_dir() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("{} is not a directory", path.display()), + )); + } + if metadata.uid() != expected_uid { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!( + "{} owned by uid {} but expected {}", + path.display(), + metadata.uid(), + expected_uid, + ), + )); + } + tokio::fs::set_permissions(path, fs::Permissions::from_mode(0o700)).await +} + +async fn ensure_socket_parent_dir(state_dir: &Path) -> Result<(), std::io::Error> { + let uid = rustix::process::geteuid().as_raw(); + let hash = state_dir_hash(state_dir); + let root = PathBuf::from(format!("/tmp/openshell-{uid}-{hash}")); + ensure_private_dir(&root, uid).await?; + ensure_private_dir(&root.join("sandboxes"), uid).await +} + +async fn create_sandbox_socket_dir( + state_dir: &Path, + sandbox_id: &str, +) -> Result { + ensure_socket_parent_dir(state_dir).await?; + let uid = rustix::process::geteuid().as_raw(); + let dir = sandbox_socket_dir(state_dir, sandbox_id); + ensure_private_dir(&dir, uid).await?; + Ok(dir) +} + +async fn remove_sandbox_socket_dir(state_dir: &Path, sandbox_id: &str) { + let _ = tokio::fs::remove_dir_all(sandbox_socket_dir(state_dir, sandbox_id)).await; +} + #[allow(clippy::result_large_err)] fn sandbox_state_dir(root: &Path, sandbox_id: &str) -> Result { validate_sandbox_id(sandbox_id)?; @@ -8498,6 +8597,128 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + #[test] + fn sandbox_socket_dir_fits_macos_sun_path() { + let state_dir = Path::new("/var/lib/openshell-vm"); + let sandbox_id = "3eb2ad45-bead-4c2e-bd10-1a4a7f3a2721"; + let control = sandbox_socket_dir(state_dir, sandbox_id).join(VM_CONTROL_SOCKET); + let ssh = sandbox_socket_dir(state_dir, sandbox_id).join("ssh.sock"); + assert!( + control.as_os_str().len() < 104, + "control socket path exceeds macOS sun_path: {}", + control.display() + ); + assert!( + ssh.as_os_str().len() < 104, + "ssh socket path exceeds macOS sun_path: {}", + ssh.display() + ); + let euid = rustix::process::geteuid().as_raw(); + let hash = state_dir_hash(state_dir); + assert_eq!( + sandbox_socket_dir(state_dir, sandbox_id), + PathBuf::from(format!( + "/tmp/openshell-{euid}-{hash}/sandboxes/{sandbox_id}" + )) + ); + // Worst-case: 10-digit UID + 8-char hash still fits sun_path. + let worst = format!("/tmp/openshell-4294967295-{hash}/sandboxes/{sandbox_id}/control.sock"); + assert!( + worst.len() < 104, + "worst-case control socket path exceeds macOS sun_path: {worst}", + ); + } + + #[test] + fn sandbox_socket_dir_distinct_state_roots_differ() { + let sandbox_id = "same-sandbox-id"; + let dir_a = sandbox_socket_dir(Path::new("/state/a"), sandbox_id); + let dir_b = sandbox_socket_dir(Path::new("/state/b"), sandbox_id); + assert_ne!( + dir_a, dir_b, + "distinct state roots must produce distinct socket dirs" + ); + } + + #[test] + fn ensure_private_dir_rejects_symlink() { + let tmp = tempfile::tempdir().unwrap(); + let victim = tmp.path().join("victim"); + std::fs::create_dir(&victim).unwrap(); + let original_mode = std::fs::metadata(&victim).unwrap().permissions().mode() & 0o777; + + let link = tmp.path().join("link"); + std::os::unix::fs::symlink(&victim, &link).unwrap(); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let uid = rustix::process::geteuid().as_raw(); + let err = rt + .block_on(ensure_private_dir(&link, uid)) + .expect_err("symlink must be rejected"); + assert!( + err.to_string().contains("symlink"), + "error should mention symlink: {err}" + ); + + let after_mode = std::fs::metadata(&victim).unwrap().permissions().mode() & 0o777; + assert_eq!( + original_mode, after_mode, + "victim directory permissions must not change" + ); + } + + #[test] + fn ensure_private_dir_rejects_foreign_owner() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("owned"); + std::fs::create_dir(&dir).unwrap(); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let wrong_uid = rustix::process::geteuid().as_raw() + 1; + let err = rt + .block_on(ensure_private_dir(&dir, wrong_uid)) + .expect_err("foreign-owned directory must be rejected"); + assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied); + } + + #[test] + fn create_sandbox_socket_dir_rejects_leaf_symlink() { + let tmp = tempfile::tempdir().unwrap(); + let victim = tmp.path().join("victim"); + std::fs::create_dir(&victim).unwrap(); + let original_mode = std::fs::metadata(&victim).unwrap().permissions().mode() & 0o777; + + let parent = tmp.path().join("parent"); + std::fs::create_dir(&parent).unwrap(); + let leaf = parent.join("leaf"); + std::os::unix::fs::symlink(&victim, &leaf).unwrap(); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let uid = rustix::process::geteuid().as_raw(); + let err = rt + .block_on(ensure_private_dir(&leaf, uid)) + .expect_err("leaf symlink must be rejected"); + assert!( + err.to_string().contains("symlink"), + "error should mention symlink: {err}" + ); + + let after_mode = std::fs::metadata(&victim).unwrap().permissions().mode() & 0o777; + assert_eq!( + original_mode, after_mode, + "symlink target permissions must not change" + ); + } + #[test] fn sandbox_state_dir_rejects_path_unsafe_ids() { let err = sandbox_state_dir(Path::new("/tmp/openshell-vm"), "../escape")