Skip to content

fix(vm): give guest-visible directories explicit modes under a strict service umask #5147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vineethsai7 wants to merge 1 commit into from
Closed

fix(vm): give guest-visible directories explicit modes under a strict service umask #5147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 93 additions & 13 deletions crates/openshell-driver-vm/src/driver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)?;

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Bump the overlay-template cache layout version

On an upgraded packaged gateway that already has images/overlay-templates/sandbox-overlay-ext4-v1/<size>.ext4 from the old UMask=0077 code, overlay_template_image_ready and ensure_sandbox_overlay_template_image accept that file solely by its length, so this 0755 creation is never reached and every newly cloned overlay still has /upper at 0700; the sandbox user therefore still cannot traverse /newroot. Bump OVERLAY_TEMPLATE_CACHE_LAYOUT_VERSION (or validate and rebuild the cached inode mode) and cover the populated-cache upgrade path so the required sandbox e2e flow actually exercises the fix.

AGENTS.md reference: AGENTS.md:L163-L163

Useful? React with 👍 / 👎.

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)
})();
Expand Down Expand Up @@ -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)
})();
Expand All @@ -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,
Expand Down Expand Up @@ -8362,6 +8368,80 @@ mod tests {
static ENV_LOCK: std::sync::LazyLock<std::sync::Mutex<()>> =
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();
Expand Down
122 changes: 118 additions & 4 deletions crates/openshell-driver-vm/src/rootfs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,17 @@ fn prepare_sandbox_rootfs(
sandbox_uid: Option<u32>,
sandbox_gid: Option<u32>,
) -> 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)?;
}
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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(())
Expand All @@ -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 {
Expand All @@ -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> {
Expand Down Expand Up @@ -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() {
Expand Down
Loading