From 8e13a04f070a2c99ac03a05c719fe7b9ec34ac9e Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Mon, 20 Jul 2026 12:42:13 -0700 Subject: [PATCH] Improve snapshot/oci validation Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- CHANGELOG.md | 1 + .../src/sandbox/snapshot/file/config.rs | 68 ++++++--- .../src/sandbox/snapshot/file/fsutil.rs | 42 +++++- .../src/sandbox/snapshot/file/mod.rs | 27 +++- .../src/sandbox/snapshot/file_tests.rs | 134 ++++++++++++++++++ 5 files changed, 245 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e634063b..571a2ec98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed * Fix symbol resolution in guest core dumps for sandboxes created from snapshots by @ludfjig in https://github.com/hyperlight-dev/hyperlight/pull/1618 +* Reject malformed OCI snapshot metadata and non-regular artifact files during load. ## [v0.16.0] - 2026-06-26 diff --git a/src/hyperlight_host/src/sandbox/snapshot/file/config.rs b/src/hyperlight_host/src/sandbox/snapshot/file/config.rs index 5e9fe6c02..fef71263a 100644 --- a/src/hyperlight_host/src/sandbox/snapshot/file/config.rs +++ b/src/hyperlight_host/src/sandbox/snapshot/file/config.rs @@ -426,7 +426,13 @@ impl OciSnapshotConfig { self.layout.snapshot_size )); } - let pt = self.layout.pt_size.unwrap_or(0); + let pt = self + .layout + .pt_size + .ok_or_else(|| crate::new_error!("snapshot pt_size is missing"))?; + if pt == 0 { + return Err(crate::new_error!("snapshot pt_size must be nonzero")); + } if !pt.is_multiple_of(PAGE_SIZE) { return Err(crate::new_error!( "snapshot pt_size ({}) is not a multiple of PAGE_SIZE", @@ -464,48 +470,58 @@ impl OciSnapshotConfig { } } - // Entrypoint address must point inside the guest snapshot - // region `[BASE_ADDRESS, BASE_ADDRESS + snapshot_size)`. The - // address is a GVA, bounded by the same range because guests - // identity-map the snapshot region at low VAs. A guest - // dispatching from a non-identity-mapped VA must relax this - // check. - let snap_lo = SandboxMemoryLayout::BASE_ADDRESS as u64; - let snap_hi = snap_lo - .checked_add(self.layout.snapshot_size as u64) + // The saved dispatch entrypoint must be in the executable code + // region. Code occupies the page-rounded prefix of the snapshot. + let code_lo = SandboxMemoryLayout::BASE_ADDRESS as u64; + let code_hi = code_lo + .checked_add(self.layout.code_size.next_multiple_of(PAGE_SIZE) as u64) .ok_or_else(|| { crate::new_error!( - "snapshot layout overflow: BASE_ADDRESS + snapshot_size ({}) does not fit in u64", - self.layout.snapshot_size + "snapshot layout overflow: BASE_ADDRESS + code_size ({}) does not fit in u64", + self.layout.code_size ) })?; - if self.entrypoint_addr < snap_lo || self.entrypoint_addr >= snap_hi { + if self.entrypoint_addr < code_lo || self.entrypoint_addr >= code_hi { return Err(crate::new_error!( - "snapshot entrypoint addr {:#x} is outside the snapshot region [{:#x}, {:#x})", + "snapshot entrypoint addr {:#x} is outside the code region [{:#x}, {:#x})", self.entrypoint_addr, - snap_lo, - snap_hi + code_lo, + code_hi + )); + } + #[cfg(target_arch = "aarch64")] + if !self.entrypoint_addr.is_multiple_of(4) { + return Err(crate::new_error!( + "snapshot entrypoint addr {:#x} is not 4-byte aligned", + self.entrypoint_addr )); } // ELF entry point GVA for `AT_ENTRY` in core dumps. 0 means // unknown. Any other value must point inside the snapshot // region, like `entrypoint_addr`. + let snapshot_hi = code_lo + .checked_add(self.layout.snapshot_size as u64) + .ok_or_else(|| { + crate::new_error!( + "snapshot layout overflow: BASE_ADDRESS + snapshot_size ({}) does not fit in u64", + self.layout.snapshot_size + ) + })?; if self.original_entrypoint_addr != 0 - && (self.original_entrypoint_addr < snap_lo || self.original_entrypoint_addr >= snap_hi) + && (self.original_entrypoint_addr < code_lo + || self.original_entrypoint_addr >= snapshot_hi) { return Err(crate::new_error!( "snapshot original entrypoint addr {:#x} is outside the snapshot region [{:#x}, {:#x})", self.original_entrypoint_addr, - snap_lo, - snap_hi + code_lo, + snapshot_hi )); } - // `stack_top_gva` is restored into the guest stack pointer. - // Bound it to the architecture's addressable GVA range so a - // malformed config cannot install an out-of-range stack - // pointer. The range is `(0, MAX_GVA]`. + // `stack_top_gva` is restored directly into the guest stack + // pointer. It must be aligned and in the guest address range. let max_gva = hyperlight_common::layout::SCRATCH_TOP_GVA as u64; if self.stack_top_gva == 0 || self.stack_top_gva > max_gva { return Err(crate::new_error!( @@ -514,6 +530,12 @@ impl OciSnapshotConfig { max_gva )); } + if !self.stack_top_gva.is_multiple_of(16) { + return Err(crate::new_error!( + "snapshot stack_top_gva {:#x} is not 16-byte aligned", + self.stack_top_gva + )); + } Ok(()) } } diff --git a/src/hyperlight_host/src/sandbox/snapshot/file/fsutil.rs b/src/hyperlight_host/src/sandbox/snapshot/file/fsutil.rs index 876bf0e96..d4cd1b8a1 100644 --- a/src/hyperlight_host/src/sandbox/snapshot/file/fsutil.rs +++ b/src/hyperlight_host/src/sandbox/snapshot/file/fsutil.rs @@ -115,7 +115,7 @@ pub(super) fn open_no_follow(path: &Path) -> crate::Result { use std::os::unix::fs::OpenOptionsExt; std::fs::OpenOptions::new() .read(true) - .custom_flags(libc::O_NOFOLLOW) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) .open(path) }; #[cfg(not(unix))] @@ -123,13 +123,21 @@ pub(super) fn open_no_follow(path: &Path) -> crate::Result { reject_symlink(path)?; std::fs::File::open(path) }; - opened.map_err(|e| { + let file = opened.map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { crate::new_error!("blob file {:?} not found", path) } else { crate::new_error!("failed to open {:?}: {}", path, e) } - }) + })?; + let file_type = file + .metadata() + .map_err(|e| crate::new_error!("failed to stat {:?}: {}", path, e))? + .file_type(); + if !file_type.is_file() { + return Err(crate::new_error!("{:?} is not a regular file", path)); + } + Ok(file) } /// Read a file in full, refusing if the file is bigger than `max_size`. @@ -155,3 +163,31 @@ pub(super) fn read_bounded(path: &Path, max_size: u64) -> crate::Result> } Ok(buf) } + +#[cfg(all(test, unix))] +mod tests { + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::OpenOptionsExt; + + use super::*; + + #[test] + fn open_no_follow_rejects_fifo() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("fifo"); + let c_path = CString::new(path.as_os_str().as_bytes()).unwrap(); + // SAFETY: `c_path` is a valid NUL-terminated path and mode has valid permission bits. + assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, 0); + + let _guard = std::fs::OpenOptions::new() + .read(true) + .write(true) + .custom_flags(libc::O_NONBLOCK) + .open(&path) + .unwrap(); + + let err = open_no_follow(&path).unwrap_err(); + assert!(format!("{err}").contains("regular file")); + } +} diff --git a/src/hyperlight_host/src/sandbox/snapshot/file/mod.rs b/src/hyperlight_host/src/sandbox/snapshot/file/mod.rs index 6e13085da..7ea72d919 100644 --- a/src/hyperlight_host/src/sandbox/snapshot/file/mod.rs +++ b/src/hyperlight_host/src/sandbox/snapshot/file/mod.rs @@ -167,10 +167,26 @@ fn load_manifest( .map_err(|e| crate::new_error!("failed to read index.json: {}", e))?; let index: ImageIndex = serde_json::from_slice(&index_bytes) .map_err(|e| crate::new_error!("failed to parse index.json: {}", e))?; + if index.schema_version() != SCHEMA_VERSION { + return Err(crate::new_error!( + "unsupported OCI index schemaVersion {} (expected {})", + index.schema_version(), + SCHEMA_VERSION + )); + } + if let Some(media_type) = index.media_type() + && !matches!(media_type, MediaType::ImageIndex) + { + return Err(crate::new_error!( + "OCI index has unexpected media type {} (expected {})", + media_type.to_string(), + MediaType::ImageIndex.to_string() + )); + } let manifest_desc = select_manifest(&index, reference, path)?; if !matches!(manifest_desc.media_type(), MediaType::ImageManifest) { return Err(crate::new_error!( - "manifest descriptor for {} has unexpected media type {:?} (expected {:?})", + "manifest descriptor for {} has unexpected media type {} (expected {})", reference, manifest_desc.media_type().to_string(), MediaType::ImageManifest.to_string() @@ -198,6 +214,15 @@ fn load_manifest( SCHEMA_VERSION )); } + if let Some(media_type) = manifest.media_type() + && !matches!(media_type, MediaType::ImageManifest) + { + return Err(crate::new_error!( + "OCI manifest has unexpected media type {} (expected {})", + media_type.to_string(), + MediaType::ImageManifest.to_string() + )); + } Ok(manifest) } diff --git a/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs b/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs index 4dae80a9e..e577b0d77 100644 --- a/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs +++ b/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs @@ -25,6 +25,7 @@ use serde_json::Value; use sha2::{Digest as _, Sha256}; use crate::func::Registerable; +use crate::mem::layout::SandboxMemoryLayout; use crate::sandbox::snapshot::{OciDigest, OciReference, OciTag, Snapshot}; use crate::{GuestBinary, HostFunctions, MultiUseSandbox, UninitializedSandbox}; @@ -929,6 +930,36 @@ fn snapshot_layout_pt_size_unaligned_rejected() { ); } +#[test] +fn snapshot_layout_missing_pt_size_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_config(&path, |cfg| { + let memory_size = cfg["memory_size"].as_u64().unwrap(); + cfg["layout"]["snapshot_size"] = Value::from(memory_size); + cfg["layout"]["pt_size"] = Value::Null; + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "pt_size"); +} + +#[test] +fn snapshot_layout_zero_pt_size_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_config(&path, |cfg| { + let memory_size = cfg["memory_size"].as_u64().unwrap(); + cfg["layout"]["snapshot_size"] = Value::from(memory_size); + cfg["layout"]["pt_size"] = Value::from(0u64); + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "pt_size"); +} + #[test] fn missing_snapshot_blob_rejected() { let snapshot = create_snapshot(); @@ -1470,6 +1501,63 @@ fn malformed_index_json_rejected() { assert_err_contains(err, "index.json"); } +#[test] +fn wrong_index_schema_version_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_index(&path, |idx| { + idx["schemaVersion"] = Value::from(99u32); + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "schemaVersion"); +} + +#[test] +fn wrong_index_media_type_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_index(&path, |idx| { + idx["mediaType"] = Value::from("application/vnd.oci.image.manifest.v1+json"); + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "media type"); +} + +#[test] +fn missing_index_media_type_accepted() { + let (_dir, path) = save_for_mutation(); + rewrite_index(&path, |idx| { + idx.as_object_mut().unwrap().remove("mediaType"); + }); + Snapshot::checked_load(&path, OciTag::new("latest").unwrap()).unwrap(); +} + +#[test] +fn wrong_manifest_media_type_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_manifest(&path, |manifest| { + manifest["mediaType"] = Value::from("application/vnd.oci.image.index.v1+json"); + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "media type"); +} + +#[test] +fn missing_manifest_media_type_accepted() { + let (_dir, path) = save_for_mutation(); + rewrite_manifest(&path, |manifest| { + manifest.as_object_mut().unwrap().remove("mediaType"); + }); + Snapshot::checked_load(&path, OciTag::new("latest").unwrap()).unwrap(); +} + #[test] fn empty_index_rejected() { let (_dir, path) = save_for_mutation(); @@ -1787,6 +1875,38 @@ fn original_entrypoint_addr_zero_accepted() { Snapshot::checked_load(&path, OciTag::new("latest").unwrap()).unwrap(); } +#[test] +fn entrypoint_addr_outside_code_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_config(&path, |cfg| { + let code_size = cfg["layout"]["code_size"].as_u64().unwrap(); + let page_size = hyperlight_common::vmem::PAGE_SIZE as u64; + let peb_addr = + SandboxMemoryLayout::BASE_ADDRESS as u64 + code_size.next_multiple_of(page_size); + cfg["entrypoint_addr"] = Value::from(peb_addr); + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "entrypoint addr"); +} + +#[cfg(target_arch = "aarch64")] +#[test] +fn unaligned_entrypoint_addr_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_config(&path, |cfg| { + let entrypoint = cfg["entrypoint_addr"].as_u64().unwrap(); + cfg["entrypoint_addr"] = Value::from(entrypoint + 1); + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "entrypoint addr"); +} + // `load`: skips blob digest verification, runs every // other validator. @@ -2355,6 +2475,20 @@ fn stack_top_gva_out_of_range_rejected() { assert_err_contains(err, "stack_top_gva"); } +#[test] +fn stack_top_gva_misaligned_rejected() { + let (_dir, path) = save_for_mutation(); + rewrite_config(&path, |cfg| { + let stack_top = cfg["stack_top_gva"].as_u64().unwrap(); + cfg["stack_top_gva"] = Value::from(stack_top - 1); + }); + let err = unwrap_err_snapshot(Snapshot::checked_load( + &path, + OciTag::new("latest").unwrap(), + )); + assert_err_contains(err, "stack_top_gva"); +} + #[test] #[cfg(unix)] fn symlink_snapshot_blob_rejected() {