From f2737ff6ce81b8e1be2cf02db82db03328495b43 Mon Sep 17 00:00:00 2001 From: Vineeth Sai Narajala Date: Mon, 2 Nov 2026 19:09:43 +0000 Subject: [PATCH] fix(vm): make guest root modes independent of umask Signed-off-by: Vineeth Sai Narajala --- crates/openshell-driver-vm/src/driver.rs | 106 +++++++++++++++++--- crates/openshell-driver-vm/src/rootfs.rs | 122 ++++++++++++++++++++++- 2 files changed, 211 insertions(+), 17 deletions(-) diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index 6260a52842..dc8fc48679 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -92,7 +92,7 @@ use std::io::{BufRead, BufReader, BufWriter, Read, Seek, SeekFrom, Write}; #[cfg(unix)] use std::os::fd::{AsRawFd as _, OwnedFd}; #[cfg(unix)] -use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt}; +use std::os::unix::fs::{DirBuilderExt as _, OpenOptionsExt as _, PermissionsExt}; #[cfg(unix)] use std::os::unix::process::ExitStatusExt as _; use std::path::{Component, Path, PathBuf}; @@ -7569,12 +7569,10 @@ fn create_empty_sandbox_overlay_image(overlay_disk: &Path, size_bytes: u64) -> R } let result = (|| { - fs::create_dir_all(staging_dir.join("upper")) - .map_err(|err| format!("create overlay upper dir: {err}"))?; - fs::create_dir_all(staging_dir.join("work")) - .map_err(|err| format!("create overlay work dir: {err}"))?; - fs::create_dir_all(staging_dir.join("config")) - .map_err(|err| format!("create overlay config dir: {err}"))?; + create_disk_staging_dir(&staging_dir, 0o700)?; + create_disk_staging_dir(&staging_dir.join("upper"), 0o755)?; + create_disk_staging_dir(&staging_dir.join("work"), 0o700)?; + create_disk_staging_dir(&staging_dir.join("config"), 0o700)?; create_ext4_image_from_dir_with_size(&staging_dir, overlay_disk, size_bytes) })(); @@ -7858,12 +7856,11 @@ fn create_image_prep_disk( } let result = (|| { - fs::create_dir_all(staging_dir.join("upper").join("srv")) - .map_err(|err| format!("create image-prep env dir: {err}"))?; - fs::create_dir_all(staging_dir.join("work")) - .map_err(|err| format!("create image-prep work dir: {err}"))?; - fs::create_dir_all(staging_dir.join("config")) - .map_err(|err| format!("create image-prep config dir: {err}"))?; + create_disk_staging_dir(&staging_dir, 0o700)?; + create_disk_staging_dir(&staging_dir.join("upper"), 0o755)?; + create_disk_staging_dir(&staging_dir.join("upper/srv"), 0o755)?; + create_disk_staging_dir(&staging_dir.join("work"), 0o700)?; + create_disk_staging_dir(&staging_dir.join("config"), 0o700)?; stage_guest_image_payload(&staging_dir, payload)?; create_ext4_image_from_dir_with_size(&staging_dir, image_path, size_bytes) })(); @@ -7872,6 +7869,15 @@ fn create_image_prep_disk( result } +fn create_disk_staging_dir(path: &Path, mode: u32) -> Result<(), String> { + fs::DirBuilder::new() + .mode(mode) + .create(path) + .map_err(|err| format!("create {}: {err}", path.display()))?; + fs::set_permissions(path, fs::Permissions::from_mode(mode)) + .map_err(|err| format!("chmod {}: {err}", path.display())) +} + fn stage_guest_image_payload( staging_dir: &Path, payload: &GuestImagePayload, @@ -8362,6 +8368,80 @@ mod tests { static ENV_LOCK: std::sync::LazyLock> = std::sync::LazyLock::new(|| std::sync::Mutex::new(())); + #[test] + fn guest_disk_directories_ignore_restrictive_umask() { + const CHILD_ENV: &str = "OPENSHELL_VM_UMASK_TEST_CHILD"; + if std::env::var_os(CHILD_ENV).is_none() { + let output = std::process::Command::new("sh") + .arg("-c") + .arg("umask 077; exec \"$@\"") + .arg("sh") + .arg(std::env::current_exe().expect("test binary")) + .arg("--exact") + .arg("driver::tests::guest_disk_directories_ignore_restrictive_umask") + .env(CHILD_ENV, "1") + .output() + .expect("run test under umask 0077"); + assert!( + output.status.success(), + "child test failed:\n{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let directory = tempfile::tempdir().expect("test directory"); + let overlay_disk = directory.path().join("overlay.ext4"); + create_empty_sandbox_overlay_image(&overlay_disk, 32 * 1024 * 1024) + .expect("create guest overlay disk"); + + let image_archive = directory.path().join("source.tar"); + fs::write(&image_archive, b"test archive").expect("write image payload"); + let prep_disk = directory.path().join("image-prep.ext4"); + let payload = GuestImagePayload { + image_ref: "example.test/image:latest".to_string(), + image_identity: "test-image".to_string(), + source: GuestImagePayloadSource::LocalDocker { + rootfs_archive: image_archive, + }, + }; + create_image_prep_disk(&prep_disk, 32 * 1024 * 1024, &payload) + .expect("create image preparation disk"); + + let assert_disk_mode = |disk: &Path, path: &str, expected: &str| { + let output = std::process::Command::new( + openshell_core::e2fsprogs::resolve(&["debugfs"]).expect("debugfs"), + ) + .arg("-R") + .arg(format!("stat {path}")) + .arg(disk) + .output() + .expect("inspect guest disk"); + assert!(output.status.success(), "{output:?}"); + let metadata = String::from_utf8(output.stdout).expect("debugfs output"); + let mode = metadata + .split("Mode:") + .nth(1) + .expect("inode mode") + .split_whitespace() + .next() + .expect("mode value"); + assert_eq!(mode, expected, "{}:{path}", disk.display()); + }; + for disk in [&overlay_disk, &prep_disk] { + for (path, expected) in [ + ("/", "0755"), + ("/upper", "0755"), + ("/work", "0700"), + ("/config", "0700"), + ] { + assert_disk_mode(disk, path, expected); + } + } + assert_disk_mode(&prep_disk, "/upper/srv", "0755"); + } + #[test] fn vm_console_diagnostic_is_bounded_to_the_tail() { let directory = tempfile::tempdir().unwrap(); diff --git a/crates/openshell-driver-vm/src/rootfs.rs b/crates/openshell-driver-vm/src/rootfs.rs index 138c7e1a90..bcbc2e554f 100644 --- a/crates/openshell-driver-vm/src/rootfs.rs +++ b/crates/openshell-driver-vm/src/rootfs.rs @@ -546,6 +546,17 @@ fn prepare_sandbox_rootfs( sandbox_uid: Option, sandbox_gid: Option, ) -> Result<(), String> { + fs::create_dir_all(rootfs).map_err(|e| format!("create {}: {e}", rootfs.display()))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + fs::set_permissions(rootfs, fs::Permissions::from_mode(0o755)) + .map_err(|e| format!("chmod {}: {e}", rootfs.display()))?; + } + for relative in ["srv", "opt/openshell/bin", "etc"] { + create_guest_rootfs_directory(rootfs, relative)?; + } + for relative in ["opt/openshell/.initialized", "opt/openshell/.rootfs-type"] { remove_rootfs_path(rootfs, relative)?; } @@ -585,6 +596,31 @@ fn prepare_sandbox_rootfs( Ok(()) } +/// Give only newly created guest directories traversal permissions. Existing +/// image directories retain their authored modes. +fn create_guest_rootfs_directory(rootfs: &Path, relative: &str) -> Result<(), String> { + let mut path = rootfs.to_path_buf(); + for component in relative.split('/') { + path.push(component); + if path.exists() { + continue; + } + #[cfg(unix)] + { + use std::os::unix::fs::{DirBuilderExt as _, PermissionsExt as _}; + fs::DirBuilder::new() + .mode(0o755) + .create(&path) + .map_err(|e| format!("create {}: {e}", path.display()))?; + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)) + .map_err(|e| format!("chmod {}: {e}", path.display()))?; + } + #[cfg(not(unix))] + fs::create_dir(&path).map_err(|e| format!("create {}: {e}", path.display()))?; + } + Ok(()) +} + pub fn validate_sandbox_rootfs(rootfs: &Path) -> Result<(), String> { require_rootfs_path(rootfs, SANDBOX_GUEST_INIT_PATH)?; require_rootfs_path(rootfs, SANDBOX_SUPERVISOR_PATH)?; @@ -1039,19 +1075,25 @@ fn ensure_sandbox_guest_user( &etc_dir.join("group"), &format!("sandbox:x:{sandbox_gid}:"), |line| line.starts_with("sandbox:"), + 0o644, + )?; + replace_or_append_line( + &etc_dir.join("gshadow"), + "sandbox:!::", + |line| line.starts_with("sandbox:"), + 0o600, )?; - replace_or_append_line(&etc_dir.join("gshadow"), "sandbox:!::", |line| { - line.starts_with("sandbox:") - })?; replace_or_append_line( &etc_dir.join("passwd"), &format!("sandbox:x:{sandbox_uid}:{sandbox_gid}:OpenShell Sandbox:/sandbox:/bin/bash"), |line| line.starts_with("sandbox:"), + 0o644, )?; replace_or_append_line( &etc_dir.join("shadow"), "sandbox:!:20123:0:99999:7:::", |line| line.starts_with("sandbox:"), + 0o600, )?; Ok(()) @@ -1061,7 +1103,13 @@ fn replace_or_append_line( path: &Path, line: &str, matches: impl Fn(&str) -> bool, + mode: u32, ) -> Result<(), String> { + let new_file = match fs::symlink_metadata(path) { + Ok(_) => false, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => true, + Err(error) => return Err(format!("stat {}: {error}", path.display())), + }; let contents = if path.exists() { fs::read_to_string(path).map_err(|e| format!("read {}: {e}", path.display()))? } else { @@ -1086,7 +1134,16 @@ fn replace_or_append_line( output.push('\n'); } - fs::write(path, output).map_err(|e| format!("write {}: {e}", path.display())) + fs::write(path, output).map_err(|e| format!("write {}: {e}", path.display()))?; + #[cfg(unix)] + if new_file { + use std::os::unix::fs::PermissionsExt as _; + fs::set_permissions(path, fs::Permissions::from_mode(mode)) + .map_err(|e| format!("chmod {}: {e}", path.display()))?; + } + #[cfg(not(unix))] + let _ = (new_file, mode); + Ok(()) } fn ensure_supervisor_binary(rootfs: &Path) -> Result<(), String> { @@ -1227,6 +1284,63 @@ mod tests { use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{SystemTime, UNIX_EPOCH}; + #[cfg(target_os = "linux")] + #[test] + fn prepared_rootfs_modes_ignore_restrictive_umask() { + const CHILD_ENV: &str = "OPENSHELL_VM_ROOTFS_UMASK_TEST_CHILD"; + if std::env::var_os(CHILD_ENV).is_none() { + let output = Command::new("sh") + .arg("-c") + .arg("umask 077; exec \"$@\"") + .arg("sh") + .arg(std::env::current_exe().expect("test binary")) + .arg("--exact") + .arg("rootfs::tests::prepared_rootfs_modes_ignore_restrictive_umask") + .env(CHILD_ENV, "1") + .output() + .expect("run test under umask 0077"); + assert!( + output.status.success(), + "child test failed:\n{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let directory = tempfile::tempdir().expect("test directory"); + let rootfs = directory.path().join("rootfs"); + fs::create_dir_all(rootfs.join("opt/openshell/bin")).expect("image runtime directory"); + for (relative, mode) in [ + ("opt", 0o711), + ("opt/openshell", 0o755), + ("opt/openshell/bin", 0o755), + ] { + fs::set_permissions(rootfs.join(relative), fs::Permissions::from_mode(mode)) + .expect("set image-authored mode"); + } + write_fake_runtime_binaries(&rootfs); + + prepare_sandbox_rootfs(&rootfs, Some(1000), Some(1000)).expect("prepare rootfs"); + for (relative, expected) in [ + ("", 0o755), + ("srv", 0o755), + ("etc", 0o755), + ("etc/passwd", 0o644), + ("etc/group", 0o644), + ("etc/shadow", 0o600), + ("etc/gshadow", 0o600), + ("opt", 0o711), + ] { + let actual = fs::metadata(rootfs.join(relative)) + .expect("stat guest path") + .permissions() + .mode() + & 0o7777; + assert_eq!(actual, expected, "{relative}"); + } + } + #[cfg(target_os = "linux")] #[test] fn host_supervisor_cache_rejects_wrong_content_and_installs_atomically() {