From 09a64b6ab09c3929be07d92cd3f6c0127ab79810 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Sun, 8 Nov 2026 18:10:43 -0800 Subject: [PATCH] sandbox: refuse shares of a sandbox's own files A managed sandbox could share its own files with its guest: a --mount whose host path is, holds, or lies inside the sandbox's state directory, image, or scratch image, or the OpenVMM, kernel, or initramfs that its VM runs. Through such a share, the workload could read or change the sandbox's records and storage, or what the host runs at the next start (#818). provision now refuses such a share before it records the sandbox, and before it creates anything, and start refuses it again, from the recorded configuration, before it claims or launches anything. Both compare each share's host path with the sandbox's own files once host::paths::resolve has resolved them, component by component, so start also refuses a share that came to reach one of them through a link since provision. The comparison drops the Windows verbatim prefix that resolve keeps for a path that only it names, such as that of a directory whose name ends in a period. A provision that finds no artifact root, which it does not need, leaves the artifacts to start. Both protect the artifact root's OpenVMM, which start runs, whatever NVX_OPENVMM names. Hard links and bind mounts of these files are not detected, as the documentation says. The new CLI tests cover each refusal at provision and at start, a state directory that provision would create, a state directory that only a verbatim path names on Windows, the root's OpenVMM with NVX_OPENVMM set, a share that a link redirected after provision, and shares beside the sandbox's files whose names only start as theirs do. The refusals fail without the check, the verbatim case without the prefix's removal, and the last test with a textual prefix comparison. provision_and_deprovision_need_no_artifact_root now provisions a share too. This replays esaurez's first implementation, b1f1446c543 and 3a3a5a7084e, adapted to the typed record and the CLI-owned lifecycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- doc/run.md | 12 ++ doc/usage.md | 2 +- nvx-rs/README.md | 5 + .../src/host/cli/sandbox/lifecycle/start.rs | 4 +- nvx-rs/src/host/cli/sandbox/mod.rs | 87 ++++++++- nvx-rs/tests/cli/download.rs | 10 +- nvx-rs/tests/cli/lifecycle.rs | 181 +++++++++++++++++- 7 files changed, 291 insertions(+), 10 deletions(-) diff --git a/doc/run.md b/doc/run.md index 23e5f1de3..c4eb78cae 100644 --- a/doc/run.md +++ b/doc/run.md @@ -1170,6 +1170,18 @@ as through a hard link or a bind mount. NVX no longer reads earlier configuration formats, including those that recorded the shares as requested; deprovision such a sandbox and provision it again. +A share must not reach the sandbox's own files, which the workload could +otherwise read or change: `provision` refuses a share whose resolved host path +equals, contains, or lies inside the sandbox's state directory, its image, its +scratch image, or the OpenVMM, kernel, or initramfs of the artifact root, before +it records the sandbox, and `start` refuses one again before it launches the VM. +`start` resolves the recorded host paths again, so a share that came to reach +one of these files through a link since `provision` is refused too. A +`provision` that finds no artifact root leaves the artifacts to `start`. Only +resolved paths are compared, so don't share a directory that holds a link +through which the host reaches one of these files, nor a hard link or bind +mount of one. + #### Access policy Each share can narrow what the workload can see and modify inside it. OpenVMM diff --git a/doc/usage.md b/doc/usage.md index 79024eb55..9be8779a4 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -321,7 +321,7 @@ do, and their names cannot be abbreviated either. | `--environment-file PATH` | omitted | Read the exact managed `exec` environment from a strict UTF-8 JSON array of `KEY=VALUE` strings, limited to 1 MiB of input. Invalid JSON, non-finite numbers, and unpaired surrogate escapes are refused. An empty array requests an empty environment. This option is mutually exclusive with `--environment`; omitting both preserves guest defaults. | | `--inherit-default-environment` | off | Layer the managed `exec` environment from `--environment` or `--environment-file` over the guest default environment instead of replacing it; an entry replaces the default variable of the same name. Without either option the workload already receives the defaults, so the flag has no effect. | | `--hypervisor {auto,whp,kvm,mshv}` | `auto` | Select the host hypervisor. | -| `--mount GUEST_TARGET,HOST_PATH[,ro\|rw]` | none | Live-share a host directory, or a single regular host file without its directory, at the absolute target inside the container rootfs for `run` or `provision`; defaults to `ro`. The target cannot contain whitespace, control characters, double quotes, backslashes, `=`, or any UTF-8 byte that Linux treats as whitespace, such as the `0xa0` in `à`, so that it stays one kernel command-line token in the shares of a one-shot `run`. The mode follows the last comma, so `HOST_PATH` may contain commas when the mode is given. Repeat to attach more shares, each with its own target and mode, for example a read-write workspace and a read-only tool cache, as the children of an aggregate that the guest binds at their targets; a shared file always uses the aggregate, and so does every share of a managed sandbox. Targets and host paths must not overlap. A one-shot `run` describes its shares in kernel command-line tokens that count toward the sandbox's 1024-byte budget; a managed sandbox announces only their number and sends their mapping table once its guest agent is ready, for at most 256 shares. An `rw` share accepts guest-created symbolic links, which the host never follows. `/`, `/etc`, `/run`, and `/run/nvx`, and the `/etc/machine-id`, `/etc/resolv.conf`, `/proc`, `/sys`, `/dev`, `/.nvx-agent`, and `/run/nvx/time` trees, are reserved. | +| `--mount GUEST_TARGET,HOST_PATH[,ro\|rw]` | none | Live-share a host directory, or a single regular host file without its directory, at the absolute target inside the container rootfs for `run` or `provision`; defaults to `ro`. The target cannot contain whitespace, control characters, double quotes, backslashes, `=`, or any UTF-8 byte that Linux treats as whitespace, such as the `0xa0` in `à`, so that it stays one kernel command-line token in the shares of a one-shot `run`. The mode follows the last comma, so `HOST_PATH` may contain commas when the mode is given. Repeat to attach more shares, each with its own target and mode, for example a read-write workspace and a read-only tool cache, as the children of an aggregate that the guest binds at their targets; a shared file always uses the aggregate, and so does every share of a managed sandbox. Targets and host paths must not overlap, and `provision` and `start` refuse a host path that equals, contains, or lies inside the sandbox's state directory, image, scratch image, OpenVMM, kernel, or initramfs; see [Live host shares](run.md#live-host-shares). A one-shot `run` describes its shares in kernel command-line tokens that count toward the sandbox's 1024-byte budget; a managed sandbox announces only their number and sends their mapping table once its guest agent is ready, for at most 256 shares. An `rw` share accepts guest-created symbolic links, which the host never follows. `/`, `/etc`, `/run`, and `/run/nvx`, and the `/etc/machine-id`, `/etc/resolv.conf`, `/proc`, `/sys`, `/dev`, `/.nvx-agent`, and `/run/nvx/time` trees, are reserved. | | `--mount-deny HOST_PATH` | none | Hide one existing file or directory inside a `--mount` host directory; relative paths are resolved inside it. It may name the directory itself when `--mount-allow` paths inside it remain. With several shares, it applies to the `--mount` before it, which must share a directory. Repeat to deny multiple paths. | | `--mount-allow HOST_PATH` | none | Expose one existing file or directory inside a `--mount-deny` path of the same share again, for example a readable `mcp-payloads` directory inside hidden logs. The hidden directories on the way list only the entries that lead to it, and the workload cannot modify them. Relative paths are resolved inside the share; with several shares, it applies to the `--mount` before it. Repeat to allow multiple paths. See [Access policy](run.md#access-policy). | | `--mount-write HOST_PATH` | none | Make one existing file or directory one of the only writable parts of an `rw` share; the workload's writes anywhere else in the share fail with `EROFS`. Relative paths are resolved inside the share; with several shares, it applies to the `--mount` before it, which must be `rw`. Repeat to declare multiple writable paths. `provision` persists the denied, allowed, and writable paths. | diff --git a/nvx-rs/README.md b/nvx-rs/README.md index daa22ce03..4bf4ea8c0 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -3042,6 +3042,11 @@ guest agents of NVX advertise bits 0, 4, and 6 in sandbox mode. reported the exit. - `exec` refuses more than 1 MiB of workload output, which the guest agent never forwards. +- `provision`, and `start` again, refuse a share whose resolved host path + equals, contains, or lies inside the sandbox's state directory, image, or + scratch image, or the OpenVMM, kernel, or initramfs of the artifact root, as + [Live host shares](../doc/run.md#live-host-shares) describes, where `nvx.py` + shared them. - The state directory holds the store's files: `sandbox.lock` replaces the `lifecycle.lock` of `nvx.py`, `runtime.json` has the store's format, and each start replaces `openvmm.log`, which `nvx.py` appended to. `exec` holds the diff --git a/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs b/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs index ad76247b3..14bbeda69 100644 --- a/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs +++ b/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs @@ -10,11 +10,12 @@ use super::{ CONFIG_NAME, Found, REQUIRED_FEATURES, connect, find_vm, mismatched, require_config, running_vm, runtime_for, }; +use crate::host::cli::artifacts::locate; use crate::host::cli::deadline::{Deadline, RETRY_INTERVAL}; use crate::host::cli::exec_lock::ExecLock; use crate::host::cli::sandbox::lock::LifecycleLock; use crate::host::cli::sandbox::state::{Config, read_config}; -use crate::host::cli::sandbox::{Artifacts, error_with_cause}; +use crate::host::cli::sandbox::{Artifacts, error_with_cause, refuse_own_files}; use crate::host::cli::session::{ExecRequest, ExecResult, Session}; use crate::host::cli::{Exit, Result, output}; use crate::host::process::KILL_TIMEOUT; @@ -63,6 +64,7 @@ fn start_locked( } let config = Config::new(config)?; let launch = config.launch()?; + refuse_own_files(state_dir, &launch, Some(&locate(artifacts.root)?.dir))?; let binding = config.distro_binding(); let scratch_binding = config.scratch_binding()?; let distro = binding diff --git a/nvx-rs/src/host/cli/sandbox/mod.rs b/nvx-rs/src/host/cli/sandbox/mod.rs index a5415cd89..768fbc456 100644 --- a/nvx-rs/src/host/cli/sandbox/mod.rs +++ b/nvx-rs/src/host/cli/sandbox/mod.rs @@ -22,14 +22,16 @@ mod one_shot; mod options; mod state; +use std::path::{Path, PathBuf}; use std::time::Instant; use super::artifacts::{Located, OPENVMM_VARIABLE, locate}; use super::download::{self, Hold}; use super::shell::format_command; -use super::{Exit, Result, output}; +use super::{Exit, Result, io_error, output}; use crate::ErrorCode; -use crate::host::artifacts::{Guest, KERNEL_NAME, require_artifact}; +use crate::host::artifacts::{ArtifactDir, Guest, KERNEL_NAME, require_artifact}; +use crate::host::paths; use crate::host::sandbox::distro::{DISTRO_POLICY_REVISION, DistroBinding, DistroFormat}; use crate::host::sandbox::mount::{plan_mounts, validate_mounts}; use crate::host::sandbox::options::{ @@ -250,6 +252,14 @@ pub(super) fn run(root: Option<&str>, options: Options) -> Result { // binding, with the manifest beside that path. let distro = RecordedDistro::new(&launch, binding)?; let state_dir = state_dir("provision")?; + // A provision runs no artifact, so it needs no artifact root; without one, `start` + // checks the shares against the artifacts that it runs. + let located = locate(root).ok(); + refuse_own_files( + Path::new(state_dir), + &launch, + located.as_ref().map(|located| &located.dir), + )?; lifecycle::provision( state_dir, &launch, @@ -386,6 +396,79 @@ pub(super) fn run(root: Option<&str>, options: Options) -> Result { } } +/// Refuses a share of the sandbox's own files: one whose host path equals, contains, or lies +/// inside the state directory `state_dir`, the image or scratch image of `launch`, or the OpenVMM, +/// kernel, or Alpine initramfs of `artifacts`, if known. Through it, the guest could read or +/// change the sandbox's records, its storage, or what the host runs. +/// +/// The paths are compared once [`paths::resolve`] has resolved them, without a Windows verbatim +/// prefix. Resolution follows symbolic links and junctions, but not hard links or bind mounts. +/// `provision` checks before it records the sandbox, and `start` again before it launches it, +/// since the shares' paths may have come to resolve elsewhere. +fn refuse_own_files( + state_dir: &Path, + launch: &SandboxLaunch, + artifacts: Option<&ArtifactDir>, +) -> Result { + let Some(export) = launch.export() else { + return Ok(()); + }; + let mut owned = vec![ + ("state directory", state_dir.to_owned()), + ("image", launch.distro().path().to_owned()), + ]; + owned.extend( + launch + .scratch() + .map(|path| ("scratch image", path.to_owned())), + ); + if let Some(dir) = artifacts { + owned.extend([ + ("OpenVMM", dir.openvmm()), + ("kernel", dir.kernel()), + ("initramfs", dir.initramfs(Guest::Alpine)), + ]); + } + let resolve = |path: &Path| { + paths::resolve(path) + .map(without_verbatim_prefix) + .map_err(|error| io_error(ErrorCode::Io, &error, path)) + }; + let owned = owned + .into_iter() + .map(|(name, path)| Ok((name, resolve(&path)?))) + .collect::>>()?; + for child in &export.children { + let shared = resolve(&child.root)?; + let overlap = owned + .iter() + .find(|(_, path)| shared.starts_with(path) || path.starts_with(&shared)); + if let Some((name, path)) = overlap { + return Err(Exit::error( + ErrorCode::Unsupported, + format!( + "sandbox mount host path {} overlaps the sandbox's {name} {}; a share cannot \ + expose the sandbox's own files", + shared.display(), + path.display() + ), + )); + } + } + Ok(()) +} + +/// Returns `path` without the verbatim prefix, `\\?\`, with which [`paths::resolve`] keeps a +/// Windows path that only that prefix names, such as one whose name ends in a period: its +/// components would otherwise differ from those of a path that it lies inside. A network path +/// keeps its prefix, since no share is one. +fn without_verbatim_prefix(path: PathBuf) -> PathBuf { + match path.to_str().and_then(|text| text.strip_prefix(r"\\?\")) { + Some(rest) if !rest.starts_with(r"UNC\") => PathBuf::from(rest), + _ => path, + } +} + /// Checks the launch of a one-shot run again, and returns its plan with the format and identity of /// the distro layer that `binding`, which the run's verification of the layer made, records. /// diff --git a/nvx-rs/tests/cli/download.rs b/nvx-rs/tests/cli/download.rs index d7f67bc85..b60f9c49f 100644 --- a/nvx-rs/tests/cli/download.rs +++ b/nvx-rs/tests/cli/download.rs @@ -201,9 +201,10 @@ fn restores_download_nothing_for_snapshots_that_they_cannot_read() { #[test] fn provision_and_deprovision_need_no_artifact_root() { - // A provision runs no artifact, and a deprovision of a sandbox that records no launch reads - // none, so both succeed where nvx-rs finds no artifact root: from an executable outside any - // release or checkout, without --root. + // A provision runs no artifact, and checks its shares against the artifacts only where it + // finds them, and a deprovision of a sandbox that records no launch reads none, so both + // succeed where nvx-rs finds no artifact root: from an executable outside any release or + // checkout, without --root. let root = Root::new(); let elsewhere = tempfile::tempdir().unwrap(); let executable = elsewhere @@ -239,10 +240,11 @@ fn provision_and_deprovision_need_no_artifact_root() { String::from_utf8_lossy(&output.stderr) ); }; - let provision: Vec = crate::provision(&root, "state", "") + let mut provision: Vec = crate::provision(&root, "state", "") .iter() .map(|argument| root.expand(argument)) .collect(); + provision.extend(["--mount".to_owned(), root.expand("/data,{root/share}")]); nvx_rs(&provision); nvx_rs(&crate::lifecycle("deprovision", &root, "state")); assert!(!root.below("state").exists()); diff --git a/nvx-rs/tests/cli/lifecycle.rs b/nvx-rs/tests/cli/lifecycle.rs index 7f817ae16..8c10bf35b 100644 --- a/nvx-rs/tests/cli/lifecycle.rs +++ b/nvx-rs/tests/cli/lifecycle.rs @@ -20,8 +20,8 @@ use serde_json::{Value, json}; #[cfg(unix)] use crate::harness::mode; use crate::harness::{ - Content, HYPERVISOR, Hosts, LAYER, OPENVMM, Outcome, PROVENANCE, Root, SCRATCH, UUID, entries, - in_parallel, is_hex, json, + Content, HYPERVISOR, Hosts, LAYER, OPENVMM, Outcome, PROVENANCE, Root, SCRATCH, UUID, + canonical, entries, in_parallel, is_hex, json, }; use crate::openvmm::{ NO_NETWORK, NetworkPolicy, Options, WORKLOAD, assert_endpoint, assert_opened_distro, @@ -1559,6 +1559,183 @@ fn refusals_of_lifecycle_operations() { }); } +/// The host paths of shares of a sandbox's own files, which `provision` and `start` refuse, and +/// which of its files each one reaches first: its state directory, image, scratch image, kernel, +/// initramfs, and OpenVMM, a directory that holds one of them, and a directory inside its state +/// directory. +const OWN_FILES: [(&str, &str); 11] = [ + ("{root/control/state}", "state directory"), + ("{root/control}", "state directory"), + ("{root/control/state/inside}", "state directory"), + ("{root/images/distro.erofs}", "image"), + ("{root/images}", "image"), + ("{root/upper/scratch.ext4}", "scratch image"), + ("{root/upper}", "scratch image"), + ("{root/build/vmlinux}", "kernel"), + ("{root/build/initramfs.cpio.gz}", "initramfs"), + ("{root/build}", "kernel"), + ("{root/openvmm/target/release/openvmm{exe}}", "OpenVMM"), +]; + +/// Returns a root whose image and scratch image are in directories of their own. +fn own_files_root() -> Root { + let root = Root::new(); + root.write("images/distro.erofs", "distro\n"); + root.write("upper/scratch.ext4", "scratch\n"); + root +} + +/// Returns the arguments that provision the sandbox of the image and scratch image of +/// [`own_files_root`] in `{root/control/state}`, with a share of `host` at `/data`, if given. +fn own_files_provision(host: Option<&str>) -> Vec { + let mut options = vec![ + "--layer".to_owned(), + format!("distro,{{root/images/distro.erofs}},{UUID}"), + "--scratch".to_owned(), + "{root/upper/scratch.ext4}".to_owned(), + ]; + if let Some(host) = host { + options.extend(["--mount".to_owned(), format!("/data,{host}")]); + } + sandbox("provision", "{root/control/state}", &options) +} + +/// Returns `host`, with its placeholders replaced, and resolved if it is a link, as the root's +/// OpenVMM is on POSIX hosts, since a share's host path must not be one. +fn share_host(root: &Root, host: &str) -> String { + let host = root.expand(host); + if Path::new(&host).is_symlink() { + canonical(Path::new(&host)).to_str().unwrap().to_owned() + } else { + host + } +} + +#[test] +fn provision_refuses_shares_of_its_own_files() { + in_parallel(&OWN_FILES, |(host, name)| { + let root = own_files_root(); + fs::create_dir_all(root.below("control/state/inside")).unwrap(); + let host = share_host(&root, host); + let before = root.snapshot(); + root.nvx_rs(&own_files_provision(Some(&host))) + .assert_error("unsupported", &[&format!("overlaps the sandbox's {name} ")]); + assert_eq!(root.snapshot(), before, "{host}"); + }); +} + +#[test] +fn provision_refuses_a_share_that_holds_the_state_directory_that_it_would_create() { + let root = own_files_root(); + fs::create_dir(root.below("control")).unwrap(); + let before = root.snapshot(); + root.nvx_rs(&own_files_provision(Some("{root/control}"))) + .assert_error("unsupported", &["overlaps the sandbox's state directory "]); + assert_eq!(root.snapshot(), before); +} + +/// A state directory that only a verbatim path names, as one whose name ends in a period, lies +/// inside a share of its parent all the same. +#[cfg(windows)] +#[test] +fn provision_refuses_a_share_that_holds_a_state_directory_that_only_a_verbatim_path_names() { + let root = Root::new(); + let outside = tempfile::tempdir().unwrap(); + let parent = canonical(outside.path()); + let state = format!(r"\\?\{}\state.", parent.display()); + fs::create_dir(&state).unwrap(); + let share = format!("/data,{}", parent.display()); + root.nvx_rs(&provision(&state, &["--mount", &share])) + .assert_error("unsupported", &["overlaps the sandbox's state directory "]); + assert!(fs::read_dir(&state).unwrap().next().is_none()); + fs::remove_dir(&state).unwrap(); +} + +/// The OpenVMM that `provision` protects is the root's, which `start` runs, and not the one that +/// `NVX_OPENVMM` names. +#[test] +fn provision_refuses_a_share_of_the_roots_openvmm_whatever_the_variable_names() { + let root = own_files_root(); + let host = share_host(&root, &format!("{{root/{OPENVMM}}}")); + let before = root.snapshot(); + root.nvx_rs_with( + &own_files_provision(Some(&host)), + &[("NVX_OPENVMM", "{root/single.txt}")], + ) + .assert_error("unsupported", &["overlaps the sandbox's OpenVMM "]); + assert_eq!(root.snapshot(), before); +} + +#[test] +fn start_refuses_recorded_shares_of_its_own_files() { + in_parallel(&OWN_FILES, |(host, name)| { + let root = own_files_root(); + root.nvx_rs(&own_files_provision(None)) + .assert_silent_success(); + fs::create_dir(root.below("control/state/inside")).unwrap(); + let host = share_host(&root, host); + let mut shared = child(&host, false); + shared["file"] = json!(Path::new(&host).is_file()); + let mut config = json(&root.below("control/state/config.json")); + config["export"] = json!({"binds": [bind(0, "/data", true)], "children": [shared]}); + root.write("control/state/config.json", config.to_string()); + let before = root.snapshot(); + root.nvx_rs(&sandbox::<&str>("start", "{root/control/state}", &[])) + .assert_error("unsupported", &[&format!("overlaps the sandbox's {name} ")]); + assert_eq!(root.snapshot(), before, "{host}"); + assert!(root.openvmm_records().is_empty(), "{host}"); + }); +} + +/// `start` resolves the recorded shares again, so a share that came to reach the sandbox's own +/// files through a link since `provision` is refused. +#[test] +fn start_refuses_a_share_that_came_to_reach_its_own_files() { + let root = Root::new(); + root.write("external/config.json", "{}"); + root.nvx_rs(&provision( + "{root/state}", + &["--mount", "/data,{root/external/config.json}"], + )) + .assert_silent_success(); + fs::remove_dir_all(root.below("external")).unwrap(); + #[cfg(unix)] + let linked = std::os::unix::fs::symlink(root.below("state"), root.below("external")); + #[cfg(windows)] + let linked = std::os::windows::fs::symlink_dir(root.below("state"), root.below("external")); + if let Err(error) = linked { + // Windows creates one only with a privilege, or in Developer Mode. + assert!( + cfg!(windows) && error.raw_os_error() == Some(1314), + "cannot create the link: {error}" + ); + eprintln!("skipped: this host does not let the test create a symbolic link"); + return; + } + let before = root.snapshot(); + operate(&root, "start", "state", &[]) + .assert_error("unsupported", &["overlaps the sandbox's state directory "]); + assert_eq!(root.snapshot(), before); + assert!(root.openvmm_records().is_empty()); +} + +/// Shares beside the sandbox's own files, whose names only start as theirs do, are no shares of +/// them. +#[test] +fn shares_beside_its_own_files_provision_and_start() { + let root = own_files_root(); + root.write("control/state2/data.txt", "data\n"); + root.write("images/distro.erofs.txt", "notes\n"); + let mut provision = own_files_provision(Some("{root/control/state2}")); + provision + .extend(["--mount", "/etc/notes.txt,{root/images/distro.erofs.txt}"].map(str::to_owned)); + root.nvx_rs(&provision).assert_silent_success(); + for operation in ["start", "stop", "deprovision"] { + root.nvx_rs(&sandbox::<&str>(operation, "{root/control/state}", &[])) + .assert_silent_success(); + } +} + /// The lock files that Windows refuses to open: every lifecycle operation fails at once for one /// that refuses access for good, and provision waits for a removed one, which a file system /// without POSIX deletion keeps under its name until its last handle closes (microsoft/nvx#464).