From bb218975adf80029f1c26e6978e9eab5fb0136c9 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Mon, 9 Nov 2026 20:03:58 -0800 Subject: [PATCH] host: read strict snapshots through the platform layer SnapshotManifest::read and Snapshot::open read a snapshot's manifest and key with open_regular and read_at_most from host::store::regular, so the strict snapshot reads depended on file helpers and a refusal classification that the store owns, and later pull requests delete the store. Move open_regular, read_at_most, is_refused, the private Refused type and their regular-file support (not_regular, refused_open and regular) to host::platform, which stays and already holds the host's file helpers. regular() needs is_reparse_point, which moves down from host::filesystem::host_path to host::platform and stays re-exported where it was, since filesystem builds on platform and the children of host depend on one another without cycles. The store keeps open_regular_in, which builds its link and non-regular-file refusals with the moved refused_open and regular, and keeps open_regular, read_at_most and is_refused as aliases for its remaining callers until #1121 deletes them. SnapshotManifest::read, and read_attachments, which shares its read, and Snapshot::open's read of key.bin use host::platform directly. The maximum-length error, the refusal of links, directories and other files that are not regular files, and the refusal classification stay as they were. finish_capture, the CLI's sandbox store and the CLI's capture still read through the aliases, since they also use the store's private-file helpers, which this change does not move. The manifest reader's tests, which refuse a missing manifest, one over MAX_MANIFEST_LEN, a directory, a link, a junction and a FIFO, the snapshot tests, and the clone path's keys_that_their_read_refuses_for_what_they_are_are_failures_of_the_template pass unchanged. New platform tests pin the moved reads' bound, messages and refusal classification, and a store test checks that open_regular_in refuses a link and a directory with refusals that host::platform recognizes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nvx-rs/src/host/filesystem/host_path.rs | 14 +-- nvx-rs/src/host/platform/mod.rs | 111 +++++++++++++++++++++++- nvx-rs/src/host/platform/tests.rs | 40 +++++++++ nvx-rs/src/host/snapshot.rs | 5 +- nvx-rs/src/host/snapshot/manifest.rs | 6 +- nvx-rs/src/host/store/regular.rs | 104 ++-------------------- nvx-rs/src/host/store/tests.rs | 34 ++++++++ 7 files changed, 199 insertions(+), 115 deletions(-) diff --git a/nvx-rs/src/host/filesystem/host_path.rs b/nvx-rs/src/host/filesystem/host_path.rs index e77f8f820..e5ab9ad0a 100644 --- a/nvx-rs/src/host/filesystem/host_path.rs +++ b/nvx-rs/src/host/filesystem/host_path.rs @@ -6,19 +6,7 @@ use std::path::{Path, PathBuf}; use super::FileIdentity; use crate::error::{Error, Result}; use crate::host::platform; - -#[cfg(windows)] -/// Returns whether Windows marks `metadata` as a reparse point. -pub(crate) fn is_reparse_point(metadata: &fs::Metadata) -> bool { - use std::os::windows::fs::MetadataExt; - metadata.file_attributes() & 0x400 != 0 -} - -#[cfg(not(windows))] -/// Reports no reparse points on hosts whose metadata has no such Windows attribute. -pub(crate) fn is_reparse_point(_metadata: &fs::Metadata) -> bool { - false -} +pub(crate) use crate::host::platform::is_reparse_point; /// Returns the stable device-and-file identity of `path` for rename detection. pub(super) fn identity(path: &Path) -> Result { diff --git a/nvx-rs/src/host/platform/mod.rs b/nvx-rs/src/host/platform/mod.rs index b990f92d6..7f61aed12 100644 --- a/nvx-rs/src/host/platform/mod.rs +++ b/nvx-rs/src/host/platform/mod.rs @@ -7,8 +7,14 @@ //! with its recorded identity, or a child that the caller has not reaped yet: a pidfd on Linux, //! when the host provides them, and a process handle on Windows. Waits and termination go through //! it, so they reach that process and never another that reuses its ID. +//! +//! Its crate-private reads of regular files open a file without following a link, or on Windows +//! any other reparse point, refuse one that is not a regular file before reading anything from +//! it, and read it within a bound; `is_refused` tells their refusals from failures of the host's. -use std::io; +use std::fmt; +use std::fs::{self, File, OpenOptions}; +use std::io::{self, Read}; use std::path::Path; use std::time::Duration; @@ -37,6 +43,109 @@ use windows as imp; pub(crate) use imp::*; pub use imp::{SUPPORTED, create_private_dir, file_identity, process_start_time}; +/// Why this module refused a file for what the file is, as [`is_refused`] tells. +#[derive(Debug)] +struct Refused(String); + +impl fmt::Display for Refused { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.0) + } +} + +impl std::error::Error for Refused {} + +/// Returns whether `error` is one with which this module refused a file for what the file is, +/// rather than an operation of the host's that failed: a link, or a file that is not a regular +/// file, which [`open_regular`] refuses, as [`refused_open`] and [`regular`] do for other opens +/// that follow no link, and a file that holds more than its bound, which [`read_at_most`] +/// refuses. +pub(crate) fn is_refused(error: &io::Error) -> bool { + error.get_ref().is_some_and(|inner| inner.is::()) +} + +/// Returns the error of a file that is a link, or not a regular file. +fn not_regular() -> io::Error { + io::Error::other(Refused("it is a link, or not a regular file".to_owned())) +} + +/// Opens the file at `path` for reading, without following a link, or on Windows any other +/// reparse point, if it is a regular file. +/// +/// The open does not wait, as it would on Unix for a FIFO without a writer: a file that is not a +/// regular file is refused before anything is read from it. +pub(crate) fn open_regular(path: &Path) -> io::Result { + let mut options = OpenOptions::new(); + options.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + + // Reads of a regular file do not depend on `O_NONBLOCK`. + options + .custom_flags((nix::fcntl::OFlag::O_NOFOLLOW | nix::fcntl::OFlag::O_NONBLOCK).bits()); + } + #[cfg(windows)] + { + use std::os::windows::fs::OpenOptionsExt; + + /// `FILE_FLAG_OPEN_REPARSE_POINT`, which opens a link as itself. + const OPEN_REPARSE_POINT: u32 = 0x0020_0000; + options.custom_flags(OPEN_REPARSE_POINT); + } + let file = options.open(path).map_err(refused_open)?; + regular(file) +} + +/// Returns `error`, with which an open failed, as the error of a file that is not a regular file if +/// the open refused one: on Unix, a link, which `O_NOFOLLOW` refuses, or a socket, which an open +/// that does not wait refuses. +pub(crate) fn refused_open(error: io::Error) -> io::Error { + #[cfg(unix)] + if [nix::errno::Errno::ELOOP, nix::errno::Errno::ENXIO] + .into_iter() + .any(|errno| error.raw_os_error() == Some(errno as i32)) + { + return not_regular(); + } + error +} + +#[cfg(windows)] +/// Returns whether Windows marks `metadata` as a reparse point. +pub(crate) fn is_reparse_point(metadata: &fs::Metadata) -> bool { + use std::os::windows::fs::MetadataExt; + metadata.file_attributes() & 0x400 != 0 +} + +#[cfg(not(windows))] +/// Reports no reparse points on hosts whose metadata has no such Windows attribute. +pub(crate) fn is_reparse_point(_metadata: &fs::Metadata) -> bool { + false +} + +/// Returns `file`, which an open that follows no link opened, if it is a regular file. +pub(crate) fn regular(file: File) -> io::Result { + let metadata = file.metadata()?; + if !metadata.file_type().is_file() || is_reparse_point(&metadata) { + return Err(not_regular()); + } + Ok(file) +} + +/// Reads `file` to its end, failing if it holds more than `limit` bytes, the most that `what` may +/// hold, without reading more than one byte past the limit. +pub(crate) fn read_at_most(file: File, limit: u64, what: &str) -> io::Result> { + let mut bytes = Vec::new(); + file.take(limit + 1).read_to_end(&mut bytes)?; + if bytes.len() as u64 > limit { + return Err(io::Error::other(Refused(format!( + "it holds more than the {limit} bytes that {what} may hold" + )))); + } + Ok(bytes) +} + /// Refuses a VMM working directory that exceeds Windows' process-creation limit, without /// accessing the filesystem. Other hosts have no such limit. pub(crate) fn check_working_dir(path: &Path) -> Result<()> { diff --git a/nvx-rs/src/host/platform/tests.rs b/nvx-rs/src/host/platform/tests.rs index 37392e1ba..4681fdac6 100644 --- a/nvx-rs/src/host/platform/tests.rs +++ b/nvx-rs/src/host/platform/tests.rs @@ -14,6 +14,46 @@ pub(crate) use super::windows::tests::{ }; use super::*; +#[test] +fn a_bounded_read_refuses_a_regular_file_that_holds_more_than_its_bound() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("record.bin"); + std::fs::write(&path, b"data").unwrap(); + assert_eq!( + read_at_most(open_regular(&path).unwrap(), 4, "a record").unwrap(), + b"data" + ); + let error = read_at_most(open_regular(&path).unwrap(), 3, "a record").unwrap_err(); + assert_eq!( + error.to_string(), + "it holds more than the 3 bytes that a record may hold" + ); + assert!(is_refused(&error), "{error}"); + // A file that is missing is a failure of the host's, not a refusal of what a file is. + let error = open_regular(&directory.path().join("missing")).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::NotFound, "{error}"); + assert!(!is_refused(&error), "{error}"); +} + +#[cfg(unix)] +#[test] +fn an_open_of_a_regular_file_refuses_a_link_and_a_directory() { + let directory = tempfile::tempdir().unwrap(); + let target = directory.path().join("target.bin"); + std::fs::write(&target, b"data").unwrap(); + std::os::unix::fs::symlink(&target, directory.path().join("link")).unwrap(); + std::fs::create_dir(directory.path().join("directory")).unwrap(); + for name in ["link", "directory"] { + let error = open_regular(&directory.path().join(name)).unwrap_err(); + assert_eq!( + error.to_string(), + "it is a link, or not a regular file", + "{name}" + ); + assert!(is_refused(&error), "{name}: {error}"); + } +} + #[cfg(any(target_os = "linux", windows))] #[test] fn a_transport_without_a_native_handle_needs_no_clone_implementation() { diff --git a/nvx-rs/src/host/snapshot.rs b/nvx-rs/src/host/snapshot.rs index 694539b4a..445c674f2 100644 --- a/nvx-rs/src/host/snapshot.rs +++ b/nvx-rs/src/host/snapshot.rs @@ -54,7 +54,6 @@ use crate::host::config::{ }; use crate::host::paths::{absolute, normalized}; use crate::host::runtime::{Runtime, RuntimeIdentity}; -use crate::host::store::regular; use crate::host::{filesystem, platform, sandbox, seal}; use crate::net::Ipv4Cidr; use crate::{Error, Hypervisor, Result}; @@ -141,9 +140,9 @@ impl Snapshot { } let manifest = check_openvmm(&dir.join(OPENVMM_DIR))?; let path = dir.join(KEY_FILE); - let bytes = regular::open_regular(&path) + let bytes = platform::open_regular(&path) .and_then(|file| { - regular::read_at_most(file, SnapshotKey::MAX_LEN as u64, "a snapshot key") + platform::read_at_most(file, SnapshotKey::MAX_LEN as u64, "a snapshot key") }) .map_err(|error| entry_error(&path, error))?; let key = SnapshotKey::from_bytes(&bytes)?; diff --git a/nvx-rs/src/host/snapshot/manifest.rs b/nvx-rs/src/host/snapshot/manifest.rs index 0249212ff..f3ec3fe5d 100644 --- a/nvx-rs/src/host/snapshot/manifest.rs +++ b/nvx-rs/src/host/snapshot/manifest.rs @@ -20,9 +20,9 @@ use sha2::{Digest, Sha256}; use super::wire::{Field, Message, WireError}; use crate::contract; use crate::error::{Error, ErrorCode, Result}; +use crate::host::platform; use crate::host::process::BootId; pub use crate::host::sandbox::geometry::BlockGeometry; -use crate::host::store::regular; use crate::hypervisor::Hypervisor; /// Name of the manifest in a snapshot's directory. @@ -827,8 +827,8 @@ const CAPTURE_TEMPLATE: &str = "the template again with the runtime of this buil fn read_manifest(snapshot: &Path) -> Result<(Vec, String)> { let path = snapshot.join(MANIFEST_NAME); let description = format!("the snapshot manifest {}", path.display()); - let bytes = regular::open_regular(&path) - .and_then(|file| regular::read_at_most(file, MAX_MANIFEST_LEN, "a manifest")) + let bytes = platform::open_regular(&path) + .and_then(|file| platform::read_at_most(file, MAX_MANIFEST_LEN, "a manifest")) .map_err(|error| { let code = match error.kind() { io::ErrorKind::NotFound => ErrorCode::NotFound, diff --git a/nvx-rs/src/host/store/regular.rs b/nvx-rs/src/host/store/regular.rs index da503f91b..d0721edfe 100644 --- a/nvx-rs/src/host/store/regular.rs +++ b/nvx-rs/src/host/store/regular.rs @@ -1,39 +1,17 @@ //! The files that the template modules read, and that a sandbox's store reads as its clone //! binding: regular files that are not links, which they read within a bound, and the identity of //! a file that a template module holds open. +//! +//! The reads themselves are [`platform`]'s: this module re-exports its `open_regular`, +//! `read_at_most` and `is_refused` for its callers until #1121 deletes them. -use std::fmt; -use std::fs::{File, OpenOptions}; -use std::io::{self, Read}; +use std::fs::File; +use std::io; use std::path::Path; +use crate::host::platform; +pub(crate) use crate::host::platform::{is_refused, open_regular, read_at_most}; use crate::host::seal::FileSeal; -use crate::host::{filesystem, platform}; - -/// Why a read of this module refused a file for what the file is, as [`is_refused`] tells. -#[derive(Debug)] -struct Refused(String); - -impl fmt::Display for Refused { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - formatter.write_str(&self.0) - } -} - -impl std::error::Error for Refused {} - -/// Returns whether `error` is one with which a read of this module refused a file for what the -/// file is, rather than an operation of the host's that failed: a link, or a file that is not a -/// regular file, which [`open_regular`] and [`open_regular_in`] refuse, and a file that holds -/// more than its bound, which [`read_at_most`] refuses. -pub(crate) fn is_refused(error: &io::Error) -> bool { - error.get_ref().is_some_and(|inner| inner.is::()) -} - -/// Returns the error of a file that is a link, or not a regular file. -fn not_regular() -> io::Error { - io::Error::other(Refused("it is a link, or not a regular file".to_owned())) -} /// Returns whether `error`, with which an open or a removal of the file at `path` failed, means /// that the file was removed, by a transaction of another process, say: on Windows, a file system @@ -47,34 +25,6 @@ pub(crate) fn was_removed(path: &Path, error: &io::Error) -> bool { cfg!(windows) && error.kind() == io::ErrorKind::PermissionDenied && platform::is_deleted(path) } -/// Opens the file at `path` for reading, without following a link, or on Windows any other -/// reparse point, if it is a regular file. -/// -/// The open does not wait, as it would on Unix for a FIFO without a writer: a file that is not a -/// regular file is refused before anything is read from it. -pub(crate) fn open_regular(path: &Path) -> io::Result { - let mut options = OpenOptions::new(); - options.read(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - - // Reads of a regular file do not depend on `O_NONBLOCK`. - options - .custom_flags((nix::fcntl::OFlag::O_NOFOLLOW | nix::fcntl::OFlag::O_NONBLOCK).bits()); - } - #[cfg(windows)] - { - use std::os::windows::fs::OpenOptionsExt; - - /// `FILE_FLAG_OPEN_REPARSE_POINT`, which opens a link as itself. - const OPEN_REPARSE_POINT: u32 = 0x0020_0000; - options.custom_flags(OPEN_REPARSE_POINT); - } - let file = options.open(path).map_err(refused_open)?; - regular(file) -} - /// Opens the file `name` of `directory` for reading, as [`open_regular`] opens the file at a /// path, if it is a regular file, or returns `None` if the directory has no such entry, or, on /// Windows, one whose deletion is pending, which is removed. The open names the entry through the @@ -85,46 +35,10 @@ pub(crate) fn open_regular_in( name: &str, ) -> io::Result> { match directory.open_file(name) { - Ok(Some(file)) => regular(file).map(Some), + Ok(Some(file)) => platform::regular(file).map(Some), Ok(None) => Ok(None), - Err(error) => Err(refused_open(error)), - } -} - -/// Returns `error`, with which an open failed, as the error of a file that is not a regular file if -/// the open refused one: on Unix, a link, which `O_NOFOLLOW` refuses, or a socket, which an open -/// that does not wait refuses. -fn refused_open(error: io::Error) -> io::Error { - #[cfg(unix)] - if [nix::errno::Errno::ELOOP, nix::errno::Errno::ENXIO] - .into_iter() - .any(|errno| error.raw_os_error() == Some(errno as i32)) - { - return not_regular(); - } - error -} - -/// Returns `file`, which an open that follows no link opened, if it is a regular file. -fn regular(file: File) -> io::Result { - let metadata = file.metadata()?; - if !metadata.file_type().is_file() || filesystem::is_reparse_point(&metadata) { - return Err(not_regular()); - } - Ok(file) -} - -/// Reads `file` to its end, failing if it holds more than `limit` bytes, the most that `what` may -/// hold, without reading more than one byte past the limit. -pub(crate) fn read_at_most(file: File, limit: u64, what: &str) -> io::Result> { - let mut bytes = Vec::new(); - file.take(limit + 1).read_to_end(&mut bytes)?; - if bytes.len() as u64 > limit { - return Err(io::Error::other(Refused(format!( - "it holds more than the {limit} bytes that {what} may hold" - )))); + Err(error) => Err(platform::refused_open(error)), } - Ok(bytes) } /// Returns the identity of the file that `file` holds open, as its [`FileSeal`] records it: its diff --git a/nvx-rs/src/host/store/tests.rs b/nvx-rs/src/host/store/tests.rs index bffe25bf6..c403f16c0 100644 --- a/nvx-rs/src/host/store/tests.rs +++ b/nvx-rs/src/host/store/tests.rs @@ -410,6 +410,40 @@ fn records_and_capabilities_that_are_not_regular_files_fail_at_once() { } } +// Other hosts open no handle of a directory, through which the store names its entries. +#[cfg(any(target_os = "linux", windows))] +#[test] +fn entries_that_are_links_or_not_regular_files_are_refused_as_the_platform_refuses_them() { + let directory = tempfile::tempdir().unwrap(); + let opened = platform::Directory::open(directory.path()).unwrap(); + let target = directory.path().join("record"); + fs::write(&target, b"data").unwrap(); + let file = regular::open_regular_in(&opened, "record") + .unwrap() + .unwrap(); + assert_eq!(regular::read_at_most(file, 4, "a record").unwrap(), b"data"); + fs::create_dir(directory.path().join("directory")).unwrap(); + let mut refused = vec!["directory"]; + let link = directory.path().join("link"); + #[cfg(unix)] + let linked = std::os::unix::fs::symlink(&target, &link); + #[cfg(windows)] + let linked = std::os::windows::fs::symlink_file(&target, &link); + // Windows lets only Developer Mode or a privilege create a symbolic link. + if linked.is_ok() { + refused.push("link"); + } + for name in refused { + let error = regular::open_regular_in(&opened, name).unwrap_err(); + assert_eq!( + error.to_string(), + "it is a link, or not a regular file", + "{name}" + ); + assert!(platform::is_refused(&error), "{name}: {error}"); + } +} + #[test] fn failures_to_read_the_records_and_the_capability_name_them() { let (_directory, store) = store();