From 58bd8756848477c75dd2491aa5c0e98e1d2c1dc0 Mon Sep 17 00:00:00 2001 From: Pedro Henrique Penna Date: Tue, 20 Oct 2026 08:43:49 -0300 Subject: [PATCH] nvx-rs: check guest-path containment with one function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four checks asked whether a guest path is a directory or lies below it, comparing whole components, so that /runner is not inside /run. filesystem::within answered it for the guest directories that a host mapping must not cover, and the sandbox's share checks wrote the same expression out three more times: validate_mount_target for the reserved mount targets, targets_overlap as an `inside` closure applied both ways, and validate_run_share_targets with a second copy of that closure and of targets_overlap's condition instead of a call to targets_overlap. - within is pub(crate), and its documentation states the rule, which its callers rely on. - validate_mount_target calls within for each reserved target. - targets_overlap is within applied in either direction. - validate_run_share_targets calls targets_overlap, as validate_mounts already does. The rule now has one definition, so the checks cannot drift apart, and each call names the rule that it applies. Each replaced expression is the body of the function that replaces it, or the disjunction of two such bodies, and none has side effects, so every check accepts and refuses the same targets, in the same order, with the same messages. The golden fixtures of the sandbox shares and options, sandbox.json and sandbox_options.json, pin those messages, and a new filesystem test pins the whole-component rule. A throwaway differential test compared the old expressions with within and targets_overlap on all 1,863,225 ordered pairs of the 1,365 strings of up to five characters from `/`, `a`, `.`, and `é`, and found no difference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nvx-rs/src/host/filesystem.rs | 25 +++++++++++++++++++++++-- nvx-rs/src/host/sandbox/mount.rs | 13 +++---------- nvx-rs/src/host/sandbox/options.rs | 7 ++----- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/nvx-rs/src/host/filesystem.rs b/nvx-rs/src/host/filesystem.rs index a8df69f37..8d5fe8071 100644 --- a/nvx-rs/src/host/filesystem.rs +++ b/nvx-rs/src/host/filesystem.rs @@ -781,8 +781,9 @@ struct Mapped { directory: bool, } -/// Returns whether `path` is `directory` or one of its descendants. -fn within(path: &str, directory: &str) -> bool { +/// Returns whether the guest path `path` is `directory` or one of its descendants, comparing +/// whole components: `/run/nvx` is within `/run`, but `/runner` is not. +pub(crate) fn within(path: &str, directory: &str) -> bool { path == directory || path .strip_prefix(directory) @@ -951,6 +952,26 @@ mod tests { assert_eq!(guest_path(Path::new("..")), None); } + /// Verifies that a guest path is within itself and its ancestors only, comparing whole + /// components, as the reserved targets here and the sandbox's share checks require. + #[test] + fn guest_paths_are_within_themselves_and_their_ancestors() { + for (path, directory) in [ + ("/run", "/run"), + ("/run/nvx", "/run"), + ("/run/nvx/t", "/run"), + ] { + assert!(within(path, directory), "{path} {directory}"); + } + for (path, directory) in [ + ("/runner", "/run"), + ("/run", "/run/nvx"), + ("/srv/run", "/run"), + ] { + assert!(!within(path, directory), "{path} {directory}"); + } + } + /// The layout of microsoft/nvx#281: directories that share no directory but the volume's /// root, each with its own access. #[test] diff --git a/nvx-rs/src/host/sandbox/mount.rs b/nvx-rs/src/host/sandbox/mount.rs index ce6528df5..c90e30184 100644 --- a/nvx-rs/src/host/sandbox/mount.rs +++ b/nvx-rs/src/host/sandbox/mount.rs @@ -20,6 +20,7 @@ use sha2::{Digest, Sha256}; use super::refused; use crate::error::{Error, Result}; +use crate::host::filesystem::within; use crate::host::python::builtins::{is_space, repr}; use crate::host::python::fs; use crate::host::python::path::{Flavor, PurePath}; @@ -310,11 +311,7 @@ pub fn validate_mount_target(target: &str) -> Result<()> { ))); } for reserved in RESERVED_MOUNT_TARGETS { - if target == reserved - || target - .strip_prefix(reserved) - .is_some_and(|rest| rest.starts_with('/')) - { + if within(target, reserved) { return Err(refused(format!( "sandbox mount target {target} overlaps the reserved {reserved} tree" ))); @@ -349,11 +346,7 @@ pub(crate) fn require_mount_owner_supported_on(owner: MountOwner, windows: bool) /// Returns whether two guest targets overlap: whether they are equal, or one contains the other. pub(crate) fn targets_overlap(left: &str, right: &str) -> bool { - let inside = |path: &str, directory: &str| { - path.strip_prefix(directory) - .is_some_and(|rest| rest.starts_with('/')) - }; - left == right || inside(left, right) || inside(right, left) + within(left, right) || within(right, left) } /// Returns the kind of share that `host_path` requests: a file for a regular file, and diff --git a/nvx-rs/src/host/sandbox/options.rs b/nvx-rs/src/host/sandbox/options.rs index dfdb1afb3..470c88c63 100644 --- a/nvx-rs/src/host/sandbox/options.rs +++ b/nvx-rs/src/host/sandbox/options.rs @@ -22,6 +22,7 @@ use super::layer::LayerRole; use super::mount::{ AGGREGATE_MOUNT_TARGET, MountKind, MountOwner, MountPolicy, POLICY_KINDS, PolicyKind, SandboxMount, has_forbidden_character, has_irregular_component, mount_kind, os_error, + targets_overlap, }; use super::{SandboxLaunch, refused}; use crate::error::Result; @@ -335,11 +336,7 @@ pub fn validate_run_share_targets(targets: &[&str]) -> Result<()> { ))); } for other in std::iter::once(&AGGREGATE_MOUNT_TARGET).chain(&targets[..index]) { - let inside = |path: &str, directory: &str| { - path.strip_prefix(directory) - .is_some_and(|rest| rest.starts_with('/')) - }; - if target == other || inside(target, other) || inside(other, target) { + if targets_overlap(target, other) { return Err(refused(format!( "--mount targets {other} and {target} overlap; a share cannot hide another" )));