From cfacd7a9ddd918a21d3a1701bc61e7db45d0b6ef Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Mon, 5 Oct 2026 13:30:29 +0000 Subject: [PATCH 1/7] test: property-test every archive extractor against the filesystem Every place rocm unpacks a downloaded archive (the Lemonade embeddable tar.gz/zip, the TheRock SDK tarball, the uv bootstrap, the ComfyUI source) gets a proptest property judged on the real disk. Hostile archives are built byte-by-byte from an alphabet weighted toward `..`, empty and `.` components, mixed separators, absolute names, symlinks, hard links and setuid/world-writable modes. The oracle snapshots the directory around the extraction root before and after and walks what was actually written, rather than re-deriving the extractor's own path checks. Two calibration tests require the same property to catch a traversing extractor and a name-sanitising but link-following one, and each property asserts a floor on how often every dangerous shape was generated. The Lemonade extractor, and GNU tar as used for the SDK and uv archives, hold; those properties are regression guards. The defects these properties found after extraction are filed as #522 and pinned here by ignored tests that fail today and should be un-ignored by its fix: the ComfyUI source unpack following a top-level symlink and keeping world-writable modes, and the uv binary lookup following symlinks out of staging and recursing without bound. The properties run from a fixed seed, so the reach floors cannot fail on seed luck and any failure reproduces as-is; `PROPTEST_RNG_SEED` picks another seed and `PROPTEST_CASES` raises the case count. Signed-off-by: Roman Inflianskas --- Cargo.lock | 5 + apps/rocm/Cargo.toml | 5 + apps/rocm/src/comfyui.rs | 138 +++++ apps/rocm/src/main.rs | 5 + apps/rocm/src/therock.rs | 31 + crates/rocm-core/Cargo.toml | 5 + crates/rocm-core/src/archive_props.rs | 860 ++++++++++++++++++++++++++ crates/rocm-core/src/lib.rs | 2 + crates/rocm-core/src/uv.rs | 155 +++++ engines/lemonade/Cargo.toml | 4 + engines/lemonade/src/install.rs | 248 ++++++++ 11 files changed, 1458 insertions(+) create mode 100644 crates/rocm-core/src/archive_props.rs diff --git a/Cargo.lock b/Cargo.lock index 9f2e1c4a8..f760ebf10 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3669,6 +3669,7 @@ dependencies = [ "serde_json", "sha2", "tar", + "tempfile", "tokio", "tracing", "tracing-appender", @@ -3687,6 +3688,7 @@ dependencies = [ "anyhow", "cc", "directories", + "flate2", "libc", "proptest", "rand 0.9.4", @@ -3696,6 +3698,8 @@ dependencies = [ "serde_json", "sha2", "sysinfo", + "tar", + "tempfile", "toml", "ureq", "windows-sys 0.61.2", @@ -3799,6 +3803,7 @@ dependencies = [ "clap", "flate2", "libc", + "proptest", "rocm-core", "rocm-deps", "rocm-engine-protocol", diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index 548e9657a..b4420fbb7 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -46,6 +46,11 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } unicode-width = "0.2" ureq = { version = "2.12", features = ["native-certs"] } +[dev-dependencies] +# Archive-extraction property tests; see `crates/rocm-core/src/archive_props.rs`. +proptest = { version = "1", default-features = false, features = ["std"] } +tempfile = "3" + [target.'cfg(unix)'.dependencies] libc.workspace = true diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 14ac0f625..c654289c7 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -3074,3 +3074,141 @@ mod tests { Ok(()) } } + +/// Property tests for the ComfyUI source unpack, judged on the filesystem by +/// `crate::archive_props`. The archive is pre-placed where the installer +/// caches it, so `download_and_extract_source` reads it instead of fetching. +#[cfg(all(test, unix))] +mod archive_properties { + use std::fs; + use std::path::Path; + + use super::{ + COMFYUI_SOURCE_ARCHIVE_NAME, download_and_extract_source, models_folder_for_source, + source_path_from_app_root, + }; + use crate::archive_props::{Expect, Layout, entries, run_case, run_property, tar_gz_bytes}; + + /// The unpack plus the first write `install` makes into the result: the + /// models folder under the source tree. + fn unpack_then_first_install_write(layout: &Layout, archive: &Path) -> anyhow::Result<()> { + let app_root = &layout.dest; + let cached = app_root.join("downloads").join(COMFYUI_SOURCE_ARCHIVE_NAME); + fs::create_dir_all(cached.parent().unwrap())?; + fs::copy(archive, &cached)?; + let mut log = fs::File::create(layout.base.join("install.log"))?; + let source_path = source_path_from_app_root(app_root); + download_and_extract_source(app_root, &source_path, &mut log)?; + fs::create_dir_all(models_folder_for_source(&source_path))?; + Ok(()) + } + + fn property(label: &str, expect: Expect, source_must_be_real_dir: bool) -> Result<(), String> { + run_property(label, 512, entries(), Some(5), |layout, entries| { + let log = layout.base.join("install.log"); + let outcome = run_case( + layout, + "source.tar.gz", + &tar_gz_bytes(entries), + &[&layout.dest, &log], + &layout.dest, + unpack_then_first_install_write, + ); + outcome.judge(expect)?; + let source = source_path_from_app_root(&layout.dest); + if source_must_be_real_dir + && outcome.error.is_none() + && fs::symlink_metadata(&source).is_ok_and(|meta| meta.file_type().is_symlink()) + { + return Err(format!( + "the installed source path is a symlink to {:?}: {outcome:#?}", + fs::read_link(&source) + )); + } + Ok(()) + }) + } + + /// Known defect: `first_child_dir` tests `is_dir`, which follows symlinks, + /// and the result is renamed to `source/` as-is. An archive whose first + /// top-level entry is a symlink therefore installs `source` as a link out + /// of the app root, and the very next install step creates `models/` + /// through it. Minimal shrunk counterexample: one entry, `l -> ..`. + #[test] + #[ignore = "known defect: a top-level symlink in the source archive becomes the install's source dir (#522)"] + fn source_unpack_never_writes_outside_the_app_root() { + property( + "comfyui", + Expect { + safe_modes: false, + no_outward_links: false, + }, + false, + ) + .unwrap(); + } + + #[test] + #[ignore = "known defect: a top-level symlink in the source archive becomes the install's source dir (#522)"] + fn installed_source_path_is_a_real_directory() { + property( + "comfyui-source-dir", + Expect { + safe_modes: false, + no_outward_links: false, + }, + true, + ) + .unwrap(); + } + + /// Known gap: `tar::Archive::unpack` applies `mode & 0o777` (setuid and + /// setgid are dropped) without the process umask, so a `0777`/`0666` + /// entry stays world-writable. Shrunk counterexample: one file, mode 0777. + #[test] + #[ignore = "known gap: the ComfyUI unpack keeps world-writable modes from the archive (#522)"] + fn source_unpack_leaves_no_setuid_or_world_writable_entry() { + property( + "comfyui-modes", + Expect { + safe_modes: true, + no_outward_links: false, + }, + false, + ) + .unwrap(); + } + + /// Deterministic form of the shrunk counterexample: a single top-level + /// symlink to the sibling `outside/` directory. After the unpack and the + /// first install write, nothing may exist under `outside/` and `source` + /// must be a real directory. + #[test] + #[ignore = "known defect: a top-level symlink in the source archive becomes the install's source dir (#522)"] + fn top_level_symlink_in_source_archive_does_not_redirect_the_install() { + use crate::archive_props::{Entry, Kind, OUTSIDE_TOKEN, concretize}; + let layout = Layout::new(); + let entries = concretize( + &[Entry { + name: b"ComfyUI-master".to_vec(), + kind: Kind::Symlink(OUTSIDE_TOKEN.as_bytes().to_vec()), + mode: 0o777, + }], + &layout.outside, + ); + let archive = layout.base.join("source.tar.gz"); + fs::write(&archive, tar_gz_bytes(&entries)).unwrap(); + let result = unpack_then_first_install_write(&layout, &archive); + let source = source_path_from_app_root(&layout.dest); + assert!( + !layout.outside.join("models").exists(), + "the install created models/ outside the app root (result: {result:?}, source -> {:?})", + fs::read_link(&source) + ); + assert!( + !fs::symlink_metadata(&source).is_ok_and(|meta| meta.file_type().is_symlink()), + "source is a symlink to {:?}", + fs::read_link(&source) + ); + } +} diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index c794e003e..5c55cd1fc 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -2,6 +2,11 @@ // // SPDX-License-Identifier: MIT +/// Shared archive generator and filesystem oracle for the extraction property +/// tests in `comfyui` and `therock`. +#[cfg(all(test, unix))] +#[path = "../../../crates/rocm-core/src/archive_props.rs"] +mod archive_props; mod automations; mod bootstrap; mod chat_host_facts; diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 41820d119..c92d384fa 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -11186,3 +11186,34 @@ exit 1 ); } } + +/// Property tests for the SDK tarball unpack, judged on the filesystem by +/// `crate::archive_props`. Extraction is the system `tar` resolved from +/// `PATH` (GNU tar on the Linux CI runners), so these pin what that `tar` +/// guarantees through `extract_tarball_and_discard_archive`. +#[cfg(all(test, unix))] +mod archive_properties { + use super::extract_tarball_and_discard_archive; + use crate::archive_props::{Expect, entries, run_case, run_property, tar_gz_bytes}; + + #[test] + fn sdk_tarball_unpack_never_writes_outside_the_install_root() { + run_property("therock", 256, entries(), Some(5), |layout, entries| { + run_case( + layout, + "sdk.tar.gz", + &tar_gz_bytes(entries), + &[&layout.dest], + &layout.dest, + |layout, archive| { + extract_tarball_and_discard_archive(archive, &layout.dest).map(|_| ()) + }, + ) + .judge(Expect { + safe_modes: true, + no_outward_links: false, + }) + }) + .unwrap(); + } +} diff --git a/crates/rocm-core/Cargo.toml b/crates/rocm-core/Cargo.toml index 9b4b22d7e..867e7dccb 100644 --- a/crates/rocm-core/Cargo.toml +++ b/crates/rocm-core/Cargo.toml @@ -40,6 +40,11 @@ ureq = { version = "2.12", features = ["native-certs"] } # easy to state over ALL inputs and awkward to cover with examples. # Test-only, so it adds nothing to any shipped binary. proptest = { version = "1", default-features = false, features = ["std"] } +# Archive-extraction property tests (`src/archive_props.rs`) build hostile +# archives byte-by-byte and judge the result on the filesystem. +flate2 = "1" +tar = "0.4" +tempfile = "3" [target.'cfg(target_os = "windows")'.dependencies] windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_System_Registry", "Win32_System_SystemInformation", "Win32_System_Threading"] } diff --git a/crates/rocm-core/src/archive_props.rs b/crates/rocm-core/src/archive_props.rs new file mode 100644 index 000000000..fc0278f3d --- /dev/null +++ b/crates/rocm-core/src/archive_props.rs @@ -0,0 +1,860 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Property-test support for the archive extractors (test-only). +//! +//! Every place `rocm` unpacks a downloaded archive (the Lemonade embeddable, +//! the TheRock SDK tarball, the `uv` bootstrap, the ComfyUI source) shares one +//! safety contract: whatever the entry names and kinds, nothing is written +//! outside the extraction root, and the tree left behind carries no link that +//! resolves out of it and no setuid/setgid or world-writable entry. +//! +//! This module is shared by `#[path]` include from each crate that owns an +//! extractor, so the generator and the oracle are one definition: +//! +//! - the **generator** builds archives byte-by-byte from a small alphabet that +//! is mostly `..`, `.`, empty components, mixed separators and links, so the +//! dangerous shapes actually occur instead of being drowned in random text; +//! - the **oracle** never re-derives the extractor's path logic: it snapshots +//! the real filesystem around the extraction root before and after, and +//! walks/canonicalizes what is actually on disk. +//! +//! Unix-only: the oracle reads mode bits, inode numbers and symlinks. + +#![cfg(all(test, unix))] +#![allow( + dead_code, + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + reason = "test-only support shared by #[path] include; not every includer uses every helper" +)] + +use std::collections::BTreeMap; +use std::fmt::Write as _; +use std::fs; +use std::io::Write as _; +use std::os::unix::ffi::OsStrExt as _; +use std::os::unix::fs::MetadataExt as _; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use proptest::prelude::*; + +/// Placeholder for "an absolute path into the sibling `outside/` directory"; +/// substituted per case once the temporary layout exists. +pub const OUTSIDE_TOKEN: &str = "@OUT"; + +/// What one archive member is. +#[derive(Clone, Debug)] +pub enum Kind { + File(Vec), + Dir, + Symlink(Vec), + Hardlink(Vec), +} + +/// One archive member: raw name bytes (not validated, not normalised), kind, +/// and mode. +#[derive(Clone, Debug)] +pub struct Entry { + pub name: Vec, + pub kind: Kind, + pub mode: u32, +} + +/// Component alphabet. Weighted toward traversal and separator shapes; `l` is +/// the name links are usually given, so "link then traverse the link" occurs. +fn component() -> impl Strategy> { + prop_oneof![ + 6 => Just(b"..".to_vec()), + 2 => Just(b".".to_vec()), + 2 => Just(Vec::new()), + 4 => Just(b"l".to_vec()), + 3 => Just(b"a".to_vec()), + 1 => Just(b"b".to_vec()), + 1 => Just(b"..\\..\\x".to_vec()), + 1 => Just(b"a\\b".to_vec()), + 1 => Just(b"C:".to_vec()), + 1 => Just(b"C:\\x".to_vec()), + 1 => Just(b"\\\\srv\\share".to_vec()), + 1 => Just(vec![0xff, 0xfe]), + 1 => Just(b"x\0y".to_vec()), + 1 => Just(vec![b'n'; 120]), + ] +} + +fn separator() -> impl Strategy { + prop_oneof![ + 6 => Just(&b"/"[..]), + 2 => Just(&b"\\"[..]), + 1 => Just(&b"//"[..]), + ] +} + +fn prefix() -> impl Strategy> { + prop_oneof![ + 6 => Just(Vec::new()), + 2 => Just(b"./".to_vec()), + 2 => Just(b"/".to_vec()), + 2 => Just(format!("{OUTSIDE_TOKEN}/").into_bytes()), + 1 => Just(b"l/".to_vec()), + ] +} + +/// A raw entry name: optional prefix, then 1-4 components joined by mixed +/// separators. +pub fn name() -> impl Strategy> { + ( + prefix(), + prop::collection::vec((component(), separator()), 1..=4), + ) + .prop_map(|(prefix, parts)| { + let mut out = prefix; + for (index, (part, sep)) in parts.iter().enumerate() { + if index > 0 { + out.extend_from_slice(sep); + } + out.extend_from_slice(part); + } + out + }) +} + +fn link_target() -> impl Strategy> { + prop_oneof![ + 3 => Just(OUTSIDE_TOKEN.as_bytes().to_vec()), + 2 => Just(format!("{OUTSIDE_TOKEN}/sentinel").into_bytes()), + 2 => Just(b"../outside".to_vec()), + 2 => Just(b"../../outside".to_vec()), + 1 => Just(b"/".to_vec()), + 1 => Just(b".".to_vec()), + 3 => name(), + ] +} + +fn mode() -> impl Strategy { + prop_oneof![ + 4 => Just(0o644), + 2 => Just(0o755), + 1 => Just(0o4755), + 1 => Just(0o2755), + 1 => Just(0o777), + 1 => Just(0o1777), + 1 => Just(0o666), + ] +} + +fn kind() -> impl Strategy { + prop_oneof![ + 4 => prop::collection::vec(any::(), 0..8).prop_map(Kind::File), + 2 => Just(Kind::Dir), + 3 => link_target().prop_map(Kind::Symlink), + 1 => link_target().prop_map(Kind::Hardlink), + ] +} + +fn entry() -> impl Strategy { + // A link is usually named `l` (or `a/l`) so a later entry under `l/` walks + // through it. + let link_name = prop_oneof![ + 3 => Just(b"l".to_vec()), + 1 => Just(b"a/l".to_vec()), + 1 => name(), + ]; + prop_oneof![ + 3 => (name(), kind(), mode()).prop_map(|(name, kind, mode)| Entry { name, kind, mode }), + 2 => (link_name, link_target()).prop_map(|(name, target)| Entry { + name, + kind: Kind::Symlink(target), + mode: 0o777, + }), + 2 => (prop_oneof![Just(b"l/".to_vec()), Just(b"a/l/".to_vec())], name(), mode()) + .prop_map(|(mut prefix, rest, mode)| { + prefix.extend_from_slice(&rest); + Entry { name: prefix, kind: Kind::File(b"pwned".to_vec()), mode } + }), + ] +} + +/// A whole archive: 1-6 members. +pub fn entries() -> impl Strategy> { + prop::collection::vec(entry(), 1..=6) +} + +/// Archives that every extractor must accept: plain relative names built only +/// from safe components, regular files and directories, ordinary modes. Used +/// for the "does not over-reject / extracts what it was given" direction. +pub fn benign_entries() -> impl Strategy> { + let part = prop_oneof![Just("a"), Just("b"), Just("c"), Just("d.txt")]; + prop::collection::vec( + ( + prop::collection::vec(part, 1..=3), + prop::collection::vec(any::(), 0..16), + ), + 1..=6, + ) + .prop_map(|items| { + // `top/` keeps every benign archive single-rooted, like the real + // artifacts, and file names get a unique suffix so no file collides + // with a directory of the same name. + let mut out = vec![Entry { + name: b"top/".to_vec(), + kind: Kind::Dir, + mode: 0o755, + }]; + for (index, (parts, data)) in items.into_iter().enumerate() { + let name = format!("top/{}.f{index}", parts.join("/")); + out.push(Entry { + name: name.into_bytes(), + kind: Kind::File(data), + mode: 0o644, + }); + } + out + }) +} + +fn substitute(bytes: &[u8], outside: &Path) -> Vec { + let token = OUTSIDE_TOKEN.as_bytes(); + let replacement = outside.as_os_str().as_bytes(); + let mut out = Vec::with_capacity(bytes.len()); + let mut index = 0; + while index < bytes.len() { + if bytes[index..].starts_with(token) { + out.extend_from_slice(replacement); + index += token.len(); + } else { + out.push(bytes[index]); + index += 1; + } + } + out +} + +/// Replace the `@OUT` placeholder in names and link targets with the real +/// absolute path of the case's `outside/` directory. +pub fn concretize(entries: &[Entry], outside: &Path) -> Vec { + entries + .iter() + .map(|entry| Entry { + name: substitute(&entry.name, outside), + kind: match &entry.kind { + Kind::Symlink(target) => Kind::Symlink(substitute(target, outside)), + Kind::Hardlink(target) => Kind::Hardlink(substitute(target, outside)), + other => other.clone(), + }, + mode: entry.mode, + }) + .collect() +} + +fn tar_block(header: &mut tar::Header) -> [u8; 512] { + header.set_cksum(); + let mut block = [0u8; 512]; + block.copy_from_slice(header.as_bytes()); + block +} + +fn push_padded(out: &mut Vec, data: &[u8]) { + out.extend_from_slice(data); + let pad = (512 - data.len() % 512) % 512; + out.extend(std::iter::repeat_n(0u8, pad)); +} + +fn push_long(out: &mut Vec, kind: tar::EntryType, value: &[u8]) { + let mut header = tar::Header::new_gnu(); + header.as_gnu_mut().unwrap().name[..13].copy_from_slice(b"././@LongLink"); + header.set_mode(0o644); + header.set_entry_type(kind); + let mut data = value.to_vec(); + data.push(0); + header.set_size(data.len() as u64); + out.extend_from_slice(&tar_block(&mut header)); + push_padded(out, &data); +} + +/// Serialise entries as a GNU tar stream, writing names and link targets into +/// the header bytes directly — `tar::Builder` refuses `..` and absolute names, +/// which is exactly what this needs to produce. Names longer than the 100-byte +/// field get a GNU long-name record, as a real `tar` would emit. +pub fn tar_bytes(entries: &[Entry]) -> Vec { + let mut out = Vec::new(); + for entry in entries { + if entry.name.len() > 100 { + push_long(&mut out, tar::EntryType::GNULongName, &entry.name); + } + let target = match &entry.kind { + Kind::Symlink(target) | Kind::Hardlink(target) => Some(target.clone()), + _ => None, + }; + if let Some(target) = &target + && target.len() > 100 + { + push_long(&mut out, tar::EntryType::GNULongLink, target); + } + let mut header = tar::Header::new_gnu(); + { + let gnu = header.as_gnu_mut().unwrap(); + let len = entry.name.len().min(100); + gnu.name[..len].copy_from_slice(&entry.name[..len]); + if let Some(target) = &target { + let len = target.len().min(100); + gnu.linkname[..len].copy_from_slice(&target[..len]); + } + } + header.set_mode(entry.mode); + header.set_mtime(1_700_000_000); + let data: &[u8] = match &entry.kind { + Kind::File(data) => { + header.set_entry_type(tar::EntryType::Regular); + data + } + Kind::Dir => { + header.set_entry_type(tar::EntryType::Directory); + &[] + } + Kind::Symlink(_) => { + header.set_entry_type(tar::EntryType::Symlink); + &[] + } + Kind::Hardlink(_) => { + header.set_entry_type(tar::EntryType::Link); + &[] + } + }; + header.set_size(data.len() as u64); + out.extend_from_slice(&tar_block(&mut header)); + push_padded(&mut out, data); + } + out.extend(std::iter::repeat_n(0u8, 1024)); + out +} + +/// [`tar_bytes`], gzip-compressed. +pub fn tar_gz_bytes(entries: &[Entry]) -> Vec { + let mut encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); + encoder.write_all(&tar_bytes(entries)).unwrap(); + encoder.finish().unwrap() +} + +/// Per-case filesystem layout: `base/dest` is the extraction root; +/// `base/outside/sentinel` and `base/victim` are what an escape would touch. +pub struct Layout { + pub _temp: tempfile::TempDir, + pub base: PathBuf, + pub dest: PathBuf, + pub outside: PathBuf, +} + +impl Layout { + pub fn new() -> Self { + let temp = tempfile::Builder::new() + .prefix("archive-props-") + .tempdir() + .unwrap(); + let base = fs::canonicalize(temp.path()).unwrap(); + let dest = base.join("dest"); + let outside = base.join("outside"); + fs::create_dir_all(&dest).unwrap(); + fs::create_dir_all(&outside).unwrap(); + fs::write(outside.join("sentinel"), b"original").unwrap(); + fs::write(base.join("victim"), b"original").unwrap(); + Self { + _temp: temp, + base, + dest, + outside, + } + } +} + +#[derive(Debug, PartialEq, Eq)] +struct Stamp { + kind: &'static str, + mode: u32, + nlink: u64, + content: Vec, + link: Option, +} + +fn stamp(path: &Path) -> Stamp { + let Ok(meta) = fs::symlink_metadata(path) else { + return Stamp { + kind: "missing", + mode: 0, + nlink: 0, + content: Vec::new(), + link: None, + }; + }; + let file_type = meta.file_type(); + let (kind, content, link) = if file_type.is_symlink() { + ("symlink", Vec::new(), fs::read_link(path).ok()) + } else if file_type.is_dir() { + ("dir", Vec::new(), None) + } else { + ("file", fs::read(path).unwrap_or_default(), None) + }; + Stamp { + kind, + mode: meta.mode(), + nlink: if file_type.is_dir() { 0 } else { meta.nlink() }, + content, + link, + } +} + +/// Make a real (non-symlink) directory listable again after an archive set +/// its mode to something like `0644`, so the oracle — and the temporary +/// directory's cleanup — can still walk it. Called only after the mode was +/// recorded. +fn ensure_listable(dir: &Path) { + use std::os::unix::fs::PermissionsExt as _; + if let Ok(meta) = fs::symlink_metadata(dir) + && meta.file_type().is_dir() + && meta.mode() & 0o700 != 0o700 + { + let _ = fs::set_permissions(dir, fs::Permissions::from_mode(meta.mode() | 0o700)); + } +} + +fn read_dir_paths(dir: &Path) -> Vec { + ensure_listable(dir); + fs::read_dir(dir) + .map(|read| { + read.filter_map(|entry| entry.ok().map(|e| e.path())) + .collect() + }) + .unwrap_or_default() +} + +/// Snapshot of every path under `base` except those under the given roots. +pub fn snapshot_outside(base: &Path, inside: &[&Path]) -> BTreeMap { + let mut out = BTreeMap::new(); + let mut stack = vec![base.to_path_buf()]; + while let Some(dir) = stack.pop() { + for path in read_dir_paths(&dir) { + if inside.iter().any(|root| path.starts_with(root)) { + continue; + } + let stamp = stamp(&path); + if stamp.kind == "dir" { + stack.push(path.clone()); + } + out.insert(path, format!("{stamp:?}")); + } + } + out +} + +/// What the oracle found wrong in the tree an extraction left on disk, split +/// by how serious it is. +#[derive(Debug, Default)] +pub struct Violations { + /// A file inside the root shares an inode with one outside it: writing + /// the "installed" file writes the outside one. An escape. + pub shared_inodes: Vec, + /// A symlink inside the root resolves outside it. Not itself a write + /// outside, but every later step that follows links (a directory walk, a + /// copy, a chmod, writing into the installed tree) inherits it. + pub outward_links: Vec, + /// setuid/setgid or world-writable entries in the installed tree, + /// including the root itself. + pub bad_modes: Vec, +} + +fn check_mode(path: &Path, mode: u32, violations: &mut Violations) { + if mode & 0o6000 != 0 { + violations.bad_modes.push(format!( + "{} has setuid/setgid bits ({mode:o})", + path.display() + )); + } + if mode & 0o002 != 0 { + violations + .bad_modes + .push(format!("{} is world-writable ({mode:o})", path.display())); + } +} + +/// Walk `root` on the real filesystem (never following links) and report any +/// symlink whose target resolves outside `root`, any hard link shared with a +/// file outside, and any setuid/setgid or world-writable entry. +pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { + let mut violations = Violations::default(); + let Ok(root_meta) = fs::symlink_metadata(root) else { + return violations; + }; + if root_meta.file_type().is_symlink() { + violations.outward_links.push(format!( + "the root {} was replaced by a symlink", + root.display() + )); + return violations; + } + check_mode(root, root_meta.mode(), &mut violations); + ensure_listable(root); + let Ok(canonical_root) = fs::canonicalize(root) else { + return violations; + }; + let mut stack = vec![canonical_root.clone()]; + while let Some(dir) = stack.pop() { + for path in read_dir_paths(&dir) { + let Ok(meta) = fs::symlink_metadata(&path) else { + continue; + }; + let file_type = meta.file_type(); + if file_type.is_symlink() { + // `canonicalize` resolves the whole chain on disk; a dangling + // link is reported by its lexical target instead. + let target = fs::read_link(&path).unwrap_or_default(); + match fs::canonicalize(&path) { + Ok(resolved) if !resolved.starts_with(&canonical_root) => { + violations.outward_links.push(format!( + "symlink {} -> {} resolves outside the root to {}", + path.display(), + target.display(), + resolved.display() + )); + } + Err(_) if target.is_absolute() && !target.starts_with(&canonical_root) => { + violations.outward_links.push(format!( + "dangling symlink {} -> {} points outside the root", + path.display(), + target.display() + )); + } + _ => {} + } + continue; + } + check_mode(&path, meta.mode(), &mut violations); + if file_type.is_file() && outside_inodes.contains(&meta.ino()) { + violations.shared_inodes.push(format!( + "{} is a hard link to a file outside the root", + path.display() + )); + } + if file_type.is_dir() { + stack.push(path); + } + } + } + violations +} + +/// Inode numbers of the regular files in the outside area, for the hard-link +/// check. +pub fn outside_inodes(layout: &Layout) -> Vec { + [layout.outside.join("sentinel"), layout.base.join("victim")] + .iter() + .filter_map(|path| fs::symlink_metadata(path).ok().map(|m| m.ino())) + .collect() +} + +/// Generator reach: how often each dangerous shape actually occurred. A +/// property that passes over a generator that never produced the shape proves +/// nothing, so each test prints these and asserts a floor. +#[derive(Default)] +pub struct Reach { + pub cases: AtomicUsize, + pub dotdot: AtomicUsize, + pub absolute: AtomicUsize, + pub backslash: AtomicUsize, + pub symlink_out: AtomicUsize, + pub through_link: AtomicUsize, + pub hardlink: AtomicUsize, + pub special_mode: AtomicUsize, + pub long_name: AtomicUsize, + pub non_utf8: AtomicUsize, +} + +impl Reach { + pub const fn new() -> Self { + Self { + cases: AtomicUsize::new(0), + dotdot: AtomicUsize::new(0), + absolute: AtomicUsize::new(0), + backslash: AtomicUsize::new(0), + symlink_out: AtomicUsize::new(0), + through_link: AtomicUsize::new(0), + hardlink: AtomicUsize::new(0), + special_mode: AtomicUsize::new(0), + long_name: AtomicUsize::new(0), + non_utf8: AtomicUsize::new(0), + } + } + + pub fn record(&self, entries: &[Entry]) { + let bump = |counter: &AtomicUsize, hit: bool| { + if hit { + counter.fetch_add(1, Ordering::Relaxed); + } + }; + let has = |needle: &[u8], hay: &[u8]| hay.windows(needle.len()).any(|w| w == needle); + self.cases.fetch_add(1, Ordering::Relaxed); + bump( + &self.dotdot, + entries + .iter() + .any(|e| e.name.split(|b| *b == b'/').any(|c| c == b"..")), + ); + bump( + &self.absolute, + entries.iter().any(|e| e.name.first() == Some(&b'/')), + ); + bump(&self.backslash, entries.iter().any(|e| has(b"\\", &e.name))); + bump( + &self.symlink_out, + entries.iter().any(|e| match &e.kind { + Kind::Symlink(t) => t.first() == Some(&b'/') || has(b"..", t), + _ => false, + }), + ); + // A later entry whose name walks through an earlier symlink's name. + let mut through = false; + for (index, entry) in entries.iter().enumerate() { + if let Kind::Symlink(_) = entry.kind { + let mut prefix = entry.name.clone(); + prefix.push(b'/'); + if entries[index + 1..] + .iter() + .any(|later| later.name.starts_with(&prefix)) + { + through = true; + } + } + } + bump(&self.through_link, through); + bump( + &self.hardlink, + entries.iter().any(|e| matches!(e.kind, Kind::Hardlink(_))), + ); + bump( + &self.special_mode, + entries.iter().any(|e| e.mode & 0o6002 != 0), + ); + bump(&self.long_name, entries.iter().any(|e| e.name.len() > 100)); + bump( + &self.non_utf8, + entries + .iter() + .any(|e| std::str::from_utf8(&e.name).is_err()), + ); + } + + /// Print the counts and fail if any dangerous shape was generated in fewer + /// than `floor_percent` of cases. + pub fn report_and_check(&self, label: &str, floor_percent: usize) { + let cases = self.cases.load(Ordering::Relaxed).max(1); + let rows = [ + ("dotdot", &self.dotdot), + ("absolute", &self.absolute), + ("backslash", &self.backslash), + ("symlink_out", &self.symlink_out), + ("through_link", &self.through_link), + ("hardlink", &self.hardlink), + ("special_mode", &self.special_mode), + ("long_name", &self.long_name), + ("non_utf8", &self.non_utf8), + ]; + let mut line = format!("[{label}] generator reach over {cases} cases:"); + for (name, counter) in rows { + let hits = counter.load(Ordering::Relaxed); + let _ = write!(line, " {name}={}%", hits * 100 / cases); + } + eprintln!("{line}"); + for (name, counter) in rows { + let hits = counter.load(Ordering::Relaxed); + assert!( + hits * 100 / cases >= floor_percent, + "[{label}] generator reach too low for {name}: {hits}/{cases}" + ); + } + } +} + +/// Outcome of one extraction case, judged on the real filesystem. +#[derive(Debug)] +pub struct CaseOutcome { + /// The extractor's error, if it reported one. Failing is allowed; what a + /// failure leaves behind is still judged. + pub error: Option, + /// Writes that landed outside the root: paths created, modified or + /// removed outside it, and hard links shared with outside files. + pub escapes: Vec, + /// The state of the tree left inside the root. + pub tree: Violations, +} + +/// Write `archive_bytes` to `base/`, run `extract`, and judge +/// the result: nothing outside `inside_roots` (which must include the +/// extraction root) may have changed, and the tree under `inspect_root` is +/// walked by [`inspect_tree`]. `extract` receives the layout and the archive +/// path. +pub fn run_case( + layout: &Layout, + archive_name: &str, + archive_bytes: &[u8], + inside_roots: &[&Path], + inspect_root: &Path, + extract: impl FnOnce(&Layout, &Path) -> anyhow::Result<()>, +) -> CaseOutcome { + let archive = layout.base.join(archive_name); + fs::write(&archive, archive_bytes).unwrap(); + let mut excluded: Vec<&Path> = inside_roots.to_vec(); + excluded.push(&archive); + let inodes = outside_inodes(layout); + let before = snapshot_outside(&layout.base, &excluded); + let result = extract(layout, &archive); + let after = snapshot_outside(&layout.base, &excluded); + let mut escapes = Vec::new(); + for (path, stamp) in &after { + match before.get(path) { + None => escapes.push(format!( + "created outside the root: {} {stamp}", + path.display() + )), + Some(old) if old != stamp => { + escapes.push(format!( + "modified outside the root: {} {old} -> {stamp}", + path.display() + )); + } + Some(_) => {} + } + } + for path in before.keys() { + if !after.contains_key(path) { + escapes.push(format!("removed outside the root: {}", path.display())); + } + } + let mut tree = inspect_tree(inspect_root, &inodes); + escapes.append(&mut tree.shared_inodes); + CaseOutcome { + error: result.err().map(|error| format!("{error:#}")), + escapes, + tree, + } +} + +/// Which parts of the contract a given extractor promises. Escapes (writes +/// outside the root) are always checked; the tree-hygiene parts are opt-in, +/// because the system-`tar` extractors deliberately recreate symlinks as +/// archived. +#[derive(Clone, Copy, Debug)] +pub struct Expect { + /// No setuid/setgid or world-writable entry in the extracted tree. + pub safe_modes: bool, + /// No symlink in the extracted tree resolves outside it. + pub no_outward_links: bool, +} + +impl CaseOutcome { + /// `Ok` when the outcome meets `expect`, else the outcome as text. + pub fn judge(&self, expect: Expect) -> Result<(), String> { + let broken = !self.escapes.is_empty() + || (expect.safe_modes && !self.tree.bad_modes.is_empty()) + || (expect.no_outward_links && !self.tree.outward_links.is_empty()); + if broken { + Err(format!("{self:#?}")) + } else { + Ok(()) + } + } +} + +/// Run `check` over `cases` archives from `strategy`, each in a fresh +/// [`Layout`] with the `@OUT` placeholder already substituted. Returns the +/// shrunk counterexample as text on failure. On success, when `reach_floor` +/// is set, also fails if any dangerous shape occurred in fewer than that +/// percentage of cases — a pass over a generator that never produced the +/// shape would prove nothing. +pub fn run_property( + label: &str, + cases: u32, + strategy: impl Strategy>, + reach_floor: Option, + check: impl Fn(&Layout, &[Entry]) -> Result<(), String>, +) -> Result<(), String> { + let reach = Reach::new(); + // `PROPTEST_CASES` (proptest's own knob, which an explicit case count + // would otherwise override) scales every property up for a longer hunt. + let cases = std::env::var("PROPTEST_CASES") + .ok() + .and_then(|value| value.parse().ok()) + .unwrap_or(cases); + let mut config = proptest::test_runner::Config { + cases, + failure_persistence: None, + ..proptest::test_runner::Config::default() + }; + // A fixed seed unless `PROPTEST_RNG_SEED` picks another: the reach floors + // below are statements about these exact cases, so a run cannot fail on + // seed luck, and any failure reproduces as-is. + if config.rng_seed == proptest::test_runner::RngSeed::Random { + config.rng_seed = proptest::test_runner::RngSeed::Fixed(0x00a2_c41e); + } + let result = proptest::test_runner::TestRunner::new(config).run(&strategy, |entries| { + let layout = Layout::new(); + let entries = concretize(&entries, &layout.outside); + reach.record(&entries); + check(&layout, &entries).map_err(proptest::test_runner::TestCaseError::fail) + }); + if let Err(error) = result { + return Err(format!("[{label}] {error}")); + } + if let Some(floor) = reach_floor { + reach.report_and_check(label, floor); + } + Ok(()) +} + +/// A second calibration extractor that does sanitise names — it keeps only +/// normal components, so no `..` or absolute name gets through — but still +/// follows a symlink planted by an earlier entry. Only the "link, then write +/// through the link" shape can catch it, so this calibrates that shape +/// specifically. +pub fn naive_sanitizing_unpack(archive: &Path, destination: &Path) -> anyhow::Result<()> { + let file = fs::File::open(archive)?; + let mut tar = tar::Archive::new(flate2::read::GzDecoder::new(file)); + for entry in tar.entries()? { + let mut entry = entry?; + let raw = entry.path_bytes().into_owned(); + let mut target = destination.to_path_buf(); + for component in Path::new(std::ffi::OsStr::from_bytes(&raw)).components() { + if let std::path::Component::Normal(part) = component { + target.push(part); + } + } + if target == destination { + continue; + } + if let Some(parent) = target.parent() { + let _ = fs::create_dir_all(parent); + } + let _ = entry.unpack(&target); + } + Ok(()) +} + +/// The deliberately unsafe extractor used to calibrate the generator: it +/// joins each raw entry name onto the root and unpacks there, following any +/// link an earlier entry planted. If the property does not fail against this, +/// the generator is not reaching the shapes it claims to. +pub fn naive_unpack(archive: &Path, destination: &Path) -> anyhow::Result<()> { + let file = fs::File::open(archive)?; + let mut tar = tar::Archive::new(flate2::read::GzDecoder::new(file)); + for entry in tar.entries()? { + let mut entry = entry?; + let raw = entry.path_bytes().into_owned(); + let target = destination.join(std::ffi::OsStr::from_bytes(&raw)); + if let Some(parent) = target.parent() { + let _ = fs::create_dir_all(parent); + } + let _ = entry.unpack(&target); + } + Ok(()) +} diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index a39270271..dfa0cdc4d 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -27,6 +27,8 @@ use windows_sys::Win32::System::Threading::{ WaitForSingleObject, }; +#[cfg(all(test, unix))] +mod archive_props; pub mod browser; pub mod diagnose; pub mod disk_space; diff --git a/crates/rocm-core/src/uv.rs b/crates/rocm-core/src/uv.rs index a3b739980..ab0a78ee0 100644 --- a/crates/rocm-core/src/uv.rs +++ b/crates/rocm-core/src/uv.rs @@ -1314,3 +1314,158 @@ All installed packages are compatible Ok(()) } } + +/// Property tests for the `uv` bootstrap archive, judged on the filesystem by +/// `crate::archive_props`. Extraction here is the system `tar` resolved from +/// `PATH`, so these exercise the host's `tar` (GNU tar on the Linux CI +/// runners) through `extract_archive`. +#[cfg(all(test, unix))] +mod archive_properties { + use std::path::Path; + + use super::{extract_archive, find_binary_in, make_executable, uv_binary_name}; + use crate::archive_props::{ + Entry, Expect, Kind, Layout, OUTSIDE_TOKEN, concretize, entries, naive_sanitizing_unpack, + naive_unpack, run_case, run_property, tar_gz_bytes, + }; + + fn property( + label: &str, + expect: Expect, + extract: fn(&Path, &Path) -> anyhow::Result<()>, + ) -> Result<(), String> { + run_property(label, 256, entries(), Some(5), |layout, entries| { + run_case( + layout, + "archive.tar.gz", + &tar_gz_bytes(entries), + &[&layout.dest], + &layout.dest, + |layout, archive| extract(archive, &layout.dest), + ) + .judge(expect) + }) + } + + const ESCAPES_ONLY: Expect = Expect { + safe_modes: false, + no_outward_links: false, + }; + + #[test] + fn uv_archive_extraction_never_writes_outside_the_staging_dir() { + property("uv", ESCAPES_ONLY, extract_archive).unwrap(); + } + + #[test] + fn uv_archive_extraction_leaves_no_setuid_or_world_writable_entry() { + property( + "uv-modes", + Expect { + safe_modes: true, + no_outward_links: false, + }, + extract_archive, + ) + .unwrap(); + } + + /// Unpack a one-off archive into the layout's staging dir and run the + /// lookup-and-chmod half of `ensure_uv_binary` on it. + fn stage_and_locate(layout: &Layout, entries: &[Entry]) -> Option { + let archive = layout.base.join("uv.tar.gz"); + std::fs::write( + &archive, + tar_gz_bytes(&concretize(entries, &layout.outside)), + ) + .unwrap(); + extract_archive(&archive, &layout.dest).unwrap(); + let binary = find_binary_in(&layout.dest, uv_binary_name())?; + let _ = make_executable(&binary); + Some(binary) + } + + /// Known defect: `find_binary_in` tests `is_file`/`is_dir`, which follow + /// symlinks, and `make_executable` then chmods whatever was found. An + /// archive whose `uv` entry is a symlink to a file outside the staging + /// directory gets that outside file chmodded to 0755 — the system `tar` + /// recreates the symlink as archived, so nothing upstream of the lookup + /// stops it. + #[test] + #[ignore = "known defect: uv binary lookup follows symlinks out of the staging dir (#522)"] + fn uv_binary_lookup_never_chmods_a_file_outside_staging() { + use std::os::unix::fs::PermissionsExt as _; + let layout = Layout::new(); + let sentinel = layout.outside.join("sentinel"); + std::fs::set_permissions(&sentinel, std::fs::Permissions::from_mode(0o600)).unwrap(); + let entries = [Entry { + name: b"uv".to_vec(), + kind: Kind::Symlink(format!("{OUTSIDE_TOKEN}/sentinel").into_bytes()), + mode: 0o777, + }]; + let found = stage_and_locate(&layout, &entries); + let mode = std::fs::metadata(&sentinel).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o600, + "a file outside staging was chmodded via {found:?} (now {mode:o})" + ); + } + + /// Known defect: the same link-following walk recurses through directory + /// symlinks with no depth bound and no cycle check, so two `-> .` links + /// make it branch at every level until the kernel's 40-hop symlink limit: + /// on the order of 2^40 directory reads. (A link to `/` instead walks the + /// whole host filesystem.) This runs the lookup on a worker thread and + /// fails if it has not returned in 20 s; the thread is leaked on failure. + #[test] + #[ignore = "known defect: uv binary lookup does not terminate on a symlink loop (#522)"] + fn uv_binary_lookup_terminates_on_a_symlink_loop() { + let layout = Layout::new(); + let entries = [ + Entry { + name: b"a".to_vec(), + kind: Kind::Symlink(b".".to_vec()), + mode: 0o777, + }, + Entry { + name: b"b".to_vec(), + kind: Kind::Symlink(b".".to_vec()), + mode: 0o777, + }, + ]; + let archive = layout.base.join("uv.tar.gz"); + std::fs::write(&archive, tar_gz_bytes(&entries)).unwrap(); + extract_archive(&archive, &layout.dest).unwrap(); + let staging = layout.dest; + let (done, wait) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let _ = done.send(find_binary_in(&staging, uv_binary_name())); + }); + let result = wait.recv_timeout(std::time::Duration::from_secs(20)); + assert!( + result.is_ok(), + "find_binary_in did not finish within 20 s on a two-link symlink loop" + ); + } + + /// Calibration: the same property over the same generator must catch an + /// extractor that joins raw names. If this starts passing, the generator + /// has stopped producing traversal names and every "no escape" result + /// above is meaningless. + #[test] + fn generator_catches_a_traversing_extractor() { + let error = property("naive", ESCAPES_ONLY, naive_unpack) + .expect_err("an extractor that honours `..` must be caught"); + eprintln!("{error}"); + } + + /// Calibration for the two-step link shape: an extractor that sanitises + /// every name but follows a symlink an earlier entry planted is only + /// caught if the generator really emits "link, then write through it". + #[test] + fn generator_catches_a_link_following_extractor() { + let error = property("naive-links", ESCAPES_ONLY, naive_sanitizing_unpack) + .expect_err("an extractor that writes through planted links must be caught"); + eprintln!("{error}"); + } +} diff --git a/engines/lemonade/Cargo.toml b/engines/lemonade/Cargo.toml index 94e37252e..fcbd2422e 100644 --- a/engines/lemonade/Cargo.toml +++ b/engines/lemonade/Cargo.toml @@ -36,3 +36,7 @@ ureq = { version = "2.12", features = ["json", "native-certs"] } # entries with deflate or no compression, and the narrow feature set keeps the # extractor's dependency surface small. zip = { version = "2.2", default-features = false, features = ["deflate"] } + +[dev-dependencies] +# Archive-extraction property tests; see `crates/rocm-core/src/archive_props.rs`. +proptest = { version = "1", default-features = false, features = ["std"] } diff --git a/engines/lemonade/src/install.rs b/engines/lemonade/src/install.rs index e03b7e8cd..633419fae 100644 --- a/engines/lemonade/src/install.rs +++ b/engines/lemonade/src/install.rs @@ -1954,3 +1954,251 @@ mod tests { fs::remove_dir_all(&dir).ok(); } } + +/// Shared archive generator and filesystem oracle; see the module docs. +#[cfg(all(test, unix))] +#[path = "../../../crates/rocm-core/src/archive_props.rs"] +mod archive_props; + +/// Property tests for the in-process Lemonade extractor, judged on the +/// filesystem by `archive_props` rather than by re-deriving the extractor's +/// own path checks. +#[cfg(all(test, unix))] +mod archive_properties { + use std::io::Write as _; + + use proptest::prelude::*; + use sha2::{Digest as _, Sha256}; + + use super::archive_props::{ + Entry, Expect, Kind, benign_entries, entries, run_case, run_property, tar_gz_bytes, + }; + use super::{ + AppPaths, extract_tar_gz, extract_zip, installed_runtime_version, lemonade_path_in, + lemonade_root, lemond_path_in, prepare_embeddable_with, runtime_dir_in, + }; + + /// The in-process extractor rejects every link kind and masks modes, so it + /// promises the whole contract, not just "no escape". + const FULL: Expect = Expect { + safe_modes: true, + no_outward_links: true, + }; + + #[test] + fn tar_extraction_keeps_every_entry_inside_the_root() { + run_property( + "lemonade-tar", + 512, + entries(), + Some(5), + |layout, entries| { + run_case( + layout, + "embeddable.tar.gz", + &tar_gz_bytes(entries), + &[&layout.dest], + &layout.dest, + |layout, archive| extract_tar_gz(archive, &layout.dest), + ) + .judge(FULL) + }, + ) + .unwrap(); + } + + /// Zip names are strings, so non-UTF-8 names are carried lossily; hard + /// links do not exist in zip and are written as regular files. + fn zip_bytes(entries: &[Entry]) -> Vec { + let mut writer = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); + for entry in entries { + let name = String::from_utf8_lossy(&entry.name).into_owned(); + let options = zip::write::SimpleFileOptions::default().unix_permissions(entry.mode); + // The writer refuses a few names (and duplicates); a refused entry + // is simply left out of the archive. + match &entry.kind { + Kind::File(data) | Kind::Hardlink(data) => { + if writer.start_file(name, options).is_ok() { + writer.write_all(data).unwrap(); + } + } + Kind::Dir => { + let _ = writer.add_directory(name, options); + } + Kind::Symlink(target) => { + let _ = writer.add_symlink( + name, + String::from_utf8_lossy(target).into_owned(), + options, + ); + } + } + } + writer.finish().unwrap().into_inner() + } + + #[test] + fn zip_extraction_keeps_every_entry_inside_the_root() { + use std::sync::atomic::{AtomicUsize, Ordering}; + // Reach is measured on what the zip writer actually stored, since it + // may drop or normalise some generated names. + let (cases, traversal, symlink) = ( + AtomicUsize::new(0), + AtomicUsize::new(0), + AtomicUsize::new(0), + ); + run_property("lemonade-zip", 512, entries(), None, |layout, entries| { + let bytes = zip_bytes(entries); + let mut stored = zip::ZipArchive::new(std::io::Cursor::new(&bytes)).unwrap(); + cases.fetch_add(1, Ordering::Relaxed); + let (mut has_traversal, mut has_symlink) = (false, false); + for index in 0..stored.len() { + let entry = stored.by_index_raw(index).unwrap(); + let name = entry.name(); + has_traversal |= + name.starts_with('/') || name.split(['/', '\\']).any(|part| part == ".."); + has_symlink |= entry.is_symlink(); + } + if has_traversal { + traversal.fetch_add(1, Ordering::Relaxed); + } + if has_symlink { + symlink.fetch_add(1, Ordering::Relaxed); + } + run_case( + layout, + "embeddable.zip", + &bytes, + &[&layout.dest], + &layout.dest, + |layout, archive| extract_zip(archive, &layout.dest), + ) + .judge(FULL) + }) + .unwrap(); + let cases = cases.into_inner().max(1); + let (traversal, symlink) = (traversal.into_inner(), symlink.into_inner()); + eprintln!( + "[lemonade-zip] stored traversal/absolute names in {}%, symlinks in {}% of {cases} archives", + traversal * 100 / cases, + symlink * 100 / cases + ); + assert!(traversal * 100 / cases >= 5 && symlink * 100 / cases >= 5); + } + + /// Hostile entries — or benign names carrying hostile modes, which the + /// extractor accepts — sometimes alongside a well-formed embeddable root, + /// so the search-and-copy half of the install is reached too. Fully + /// hostile archives alone almost never get past extraction (the extractor + /// fails closed on the first bad entry), which would leave the copy step + /// untested. + fn install_entries() -> impl Strategy> { + let modes = prop::sample::select(vec![0o644, 0o755, 0o4755, 0o2775, 0o777, 0o666]); + let benign = (benign_entries(), modes).prop_map(|(mut entries, mode)| { + for entry in &mut entries { + entry.mode = mode; + } + entries + }); + let body = prop_oneof![entries(), benign]; + (body, any::(), any::()).prop_map(|(mut entries, with_root, first)| { + if with_root { + let root = [ + Entry { + name: b"e/lemond".to_vec(), + kind: Kind::File(b"lemond".to_vec()), + mode: 0o755, + }, + Entry { + name: b"e/lemonade".to_vec(), + kind: Kind::File(b"lemonade".to_vec()), + mode: 0o755, + }, + ]; + if first { + entries.splice(0..0, root); + } else { + entries.extend(root); + } + } + entries + }) + } + + /// The whole `prepare_embeddable` path — digest check, extract, root + /// search, copy into the runtime tree, version marker — over hostile + /// archives. Beyond "nothing outside the engine root changes", a failed + /// install must never leave the version marker that tells the next run the + /// runtime is complete. + #[test] + fn install_never_escapes_and_never_marks_a_failed_install_complete() { + use std::sync::atomic::{AtomicUsize, Ordering}; + // Reach of the install half: a property over installs that all fail + // at extraction would say nothing about the copy step. + let (cases, installed) = (AtomicUsize::new(0), AtomicUsize::new(0)); + run_property( + "lemonade-install", + 256, + install_entries(), + None, + |layout, entries| { + cases.fetch_add(1, Ordering::Relaxed); + let bytes = tar_gz_bytes(entries); + let digest = format!("{:x}", Sha256::digest(&bytes)); + let paths = AppPaths { + config_dir: layout.dest.join("config"), + data_dir: layout.dest.join("data"), + cache_dir: layout.dest.join("cache"), + }; + let mut install_result = None; + let outcome = run_case( + layout, + "staged.tar.gz", + &bytes, + &[&layout.dest], + &layout.dest, + |layout, archive| { + let result = prepare_embeddable_with( + &paths, + Some(&layout.dest), + false, + "9.9.9", + &digest, + |_, destination| { + std::fs::copy(archive, destination)?; + Ok(()) + }, + ); + install_result = Some(result.is_ok()); + if result.is_ok() { + installed.fetch_add(1, Ordering::Relaxed); + } + result.map(|_| ()) + }, + ); + outcome.judge(FULL)?; + let runtime_dir = runtime_dir_in(&lemonade_root(&paths, Some(&layout.dest))); + let marker = installed_runtime_version(&runtime_dir); + if install_result == Some(false) && marker.is_some() { + return Err(format!( + "failed install left version marker {marker:?}: {outcome:#?}" + )); + } + if marker.is_some() + && !(lemond_path_in(&runtime_dir).is_file() + && lemonade_path_in(&runtime_dir).is_file()) + { + return Err(format!("marker without binaries: {outcome:#?}")); + } + Ok(()) + }, + ) + .unwrap(); + let (cases, installed) = (cases.into_inner(), installed.into_inner()); + eprintln!("[lemonade-install] {installed}/{cases} hostile archives installed successfully"); + assert!( + installed * 100 / cases.max(1) >= 5, + "install reach too low: {installed}/{cases} reached a completed install" + ); + } +} From feaa3ad83400c52931b18c00c504caa169e561c7 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 9 Oct 2026 11:23:33 +0000 Subject: [PATCH 2/7] test: keep hostile archives inside the watched tree, judge modes per runner Review of the archive properties found the oracle watched only the case directory while the generator could name paths beyond it: an extractor that ignored its destination, or a deep enough `..` chain or absolute name, wrote past the snapshot unseen, and the calibration extractors could change real host directories with another seed. Each case now runs at the bottom of a fence of nested directories deeper than any chain the generator can build, absolute names and link targets are re-rooted inside it, and the snapshot covers the whole fence. A fence is reused across a property's cases until one changes it. Every extractor also gets a benign round trip, which catches extraction that ignores its destination and an extractor that rejects everything. Mode checks no longer depend on the runner by accident: the fixture sets its own modes, world-write is judged only while the umask clears it, and the system-tar properties skip modes as root, where GNU tar restores archived modes, and say so. `base/victim` is now reachable. ComfyUI gains active coverage: an escape property and a round trip over archives without the top-level symlink #522 is about, and the ignored modes test now fails for the modes alone. Signed-off-by: Roman Inflianskas --- apps/rocm/src/comfyui.rs | 123 ++++++++-- apps/rocm/src/therock.rs | 39 ++- crates/rocm-core/src/archive_props.rs | 339 ++++++++++++++++++++++---- crates/rocm-core/src/uv.rs | 38 ++- engines/lemonade/src/install.rs | 42 +++- 5 files changed, 494 insertions(+), 87 deletions(-) diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index c654289c7..0d5191cfb 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -3087,7 +3087,12 @@ mod archive_properties { COMFYUI_SOURCE_ARCHIVE_NAME, download_and_extract_source, models_folder_for_source, source_path_from_app_root, }; - use crate::archive_props::{Expect, Layout, entries, run_case, run_property, tar_gz_bytes}; + use proptest::prelude::*; + + use crate::archive_props::{ + Entry, Expect, Kind, Layout, Modes, benign_entries, check_round_trip, entries, + note_mode_scope, run_case, run_property, tar_gz_bytes, + }; /// The unpack plus the first write `install` makes into the result: the /// models folder under the source tree. @@ -3103,8 +3108,53 @@ mod archive_properties { Ok(()) } - fn property(label: &str, expect: Expect, source_must_be_real_dir: bool) -> Result<(), String> { - run_property(label, 512, entries(), Some(5), |layout, entries| { + /// [`entries`] with every relative name moved one level down, under + /// `a/`, and any symlink that would still land directly in the extraction + /// root dropped: that is the one shape that hits the known #522 defect + /// (`first_child_dir` follows it). Moving names down rather than only + /// dropping top-level links keeps "link, then write through it" in play, + /// one level down where the source tree is. + fn entries_without_top_level_symlink() -> impl Strategy> { + fn lands_top_level(name: &[u8]) -> bool { + let mut depth = 0usize; + for part in name.split(|byte| *byte == b'/') { + match part { + b"" | b"." => {} + b".." => { + let Some(up) = depth.checked_sub(1) else { + return true; + }; + depth = up; + } + _ => depth += 1, + } + } + depth <= 1 + } + entries().prop_map(|entries| { + entries + .into_iter() + .map(|mut entry| { + if !entry.name.starts_with(b"/") && !entry.name.starts_with(b"@") { + entry.name.splice(0..0, *b"a/"); + } + entry + }) + .filter(|entry| { + !(matches!(entry.kind, Kind::Symlink(_)) && lands_top_level(&entry.name)) + }) + .collect() + }) + } + + fn property( + label: &str, + strategy: impl Strategy>, + expect: Expect, + source_must_be_real_dir: bool, + ) -> Result<(), String> { + note_mode_scope(label, expect.modes); + run_property(label, 512, strategy, Some(5), |layout, entries| { let log = layout.base.join("install.log"); let outcome = run_case( layout, @@ -3129,6 +3179,44 @@ mod archive_properties { }) } + const ESCAPES_ONLY: Expect = Expect { + modes: Modes::Unchecked, + no_outward_links: false, + }; + + /// What holds today: with the #522 shape (a top-level symlink) left out, + /// the unpack and the first install write stay inside the app root. + #[test] + fn source_unpack_without_a_top_level_symlink_never_writes_outside_the_app_root() { + property( + "comfyui-no-top-link", + entries_without_top_level_symlink(), + ESCAPES_ONLY, + true, + ) + .unwrap(); + } + + /// The other direction: a well-formed source archive is installed in + /// full, its single top-level directory becoming `source/`. + #[test] + fn source_unpack_installs_a_benign_archive_as_the_source_dir() { + run_property( + "comfyui-benign", + 64, + benign_entries(), + None, + |layout, entries| { + let archive = layout.base.join("source.tar.gz"); + fs::write(&archive, tar_gz_bytes(entries)).unwrap(); + unpack_then_first_install_write(layout, &archive) + .map_err(|error| format!("{error:#}"))?; + check_round_trip(&source_path_from_app_root(&layout.dest), entries) + }, + ) + .unwrap(); + } + /// Known defect: `first_child_dir` tests `is_dir`, which follows symlinks, /// and the result is renamed to `source/` as-is. An archive whose first /// top-level entry is a symlink therefore installs `source` as a link out @@ -3137,41 +3225,28 @@ mod archive_properties { #[test] #[ignore = "known defect: a top-level symlink in the source archive becomes the install's source dir (#522)"] fn source_unpack_never_writes_outside_the_app_root() { - property( - "comfyui", - Expect { - safe_modes: false, - no_outward_links: false, - }, - false, - ) - .unwrap(); + property("comfyui", entries(), ESCAPES_ONLY, false).unwrap(); } #[test] #[ignore = "known defect: a top-level symlink in the source archive becomes the install's source dir (#522)"] fn installed_source_path_is_a_real_directory() { - property( - "comfyui-source-dir", - Expect { - safe_modes: false, - no_outward_links: false, - }, - true, - ) - .unwrap(); + property("comfyui-source-dir", entries(), ESCAPES_ONLY, true).unwrap(); } /// Known gap: `tar::Archive::unpack` applies `mode & 0o777` (setuid and /// setgid are dropped) without the process umask, so a `0777`/`0666` /// entry stays world-writable. Shrunk counterexample: one file, mode 0777. + /// The top-level symlink shape is left out, so this fails for the modes + /// alone and can be un-ignored on its own when they are fixed. #[test] #[ignore = "known gap: the ComfyUI unpack keeps world-writable modes from the archive (#522)"] fn source_unpack_leaves_no_setuid_or_world_writable_entry() { property( "comfyui-modes", + entries_without_top_level_symlink(), Expect { - safe_modes: true, + modes: Modes::Safe, no_outward_links: false, }, false, @@ -3186,7 +3261,7 @@ mod archive_properties { #[test] #[ignore = "known defect: a top-level symlink in the source archive becomes the install's source dir (#522)"] fn top_level_symlink_in_source_archive_does_not_redirect_the_install() { - use crate::archive_props::{Entry, Kind, OUTSIDE_TOKEN, concretize}; + use crate::archive_props::{OUTSIDE_TOKEN, concretize}; let layout = Layout::new(); let entries = concretize( &[Entry { @@ -3194,7 +3269,7 @@ mod archive_properties { kind: Kind::Symlink(OUTSIDE_TOKEN.as_bytes().to_vec()), mode: 0o777, }], - &layout.outside, + &layout, ); let archive = layout.base.join("source.tar.gz"); fs::write(&archive, tar_gz_bytes(&entries)).unwrap(); diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index c92d384fa..4e2b96070 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -11194,10 +11194,21 @@ exit 1 #[cfg(all(test, unix))] mod archive_properties { use super::extract_tarball_and_discard_archive; - use crate::archive_props::{Expect, entries, run_case, run_property, tar_gz_bytes}; + use crate::archive_props::{ + Expect, Modes, benign_entries, check_round_trip, entries, note_mode_scope, run_case, + run_property, tar_gz_bytes, + }; + /// No escape, and no setuid/setgid or world-writable entry. The modes are + /// what the system `tar` gives an unprivileged user, so they are not + /// judged when this runs as root (see `Modes`). #[test] fn sdk_tarball_unpack_never_writes_outside_the_install_root() { + let expect = Expect { + modes: Modes::SafeUnlessRoot, + no_outward_links: false, + }; + note_mode_scope("therock", expect.modes); run_property("therock", 256, entries(), Some(5), |layout, entries| { run_case( layout, @@ -11209,11 +11220,29 @@ mod archive_properties { extract_tarball_and_discard_archive(archive, &layout.dest).map(|_| ()) }, ) - .judge(Expect { - safe_modes: true, - no_outward_links: false, - }) + .judge(expect) }) .unwrap(); } + + /// The other direction: a well-formed tarball is extracted in full, into + /// the install root it was given. An unpack that wrote somewhere else + /// entirely escapes the oracle above; this catches it. + #[test] + fn sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root() { + run_property( + "therock-benign", + 64, + benign_entries(), + None, + |layout, entries| { + let archive = layout.base.join("sdk.tar.gz"); + std::fs::write(&archive, tar_gz_bytes(entries)).unwrap(); + extract_tarball_and_discard_archive(&archive, &layout.dest) + .map_err(|error| format!("{error:#}"))?; + check_round_trip(&layout.dest.join("top"), entries) + }, + ) + .unwrap(); + } } diff --git a/crates/rocm-core/src/archive_props.rs b/crates/rocm-core/src/archive_props.rs index fc0278f3d..3e3cf0f28 100644 --- a/crates/rocm-core/src/archive_props.rs +++ b/crates/rocm-core/src/archive_props.rs @@ -17,10 +17,22 @@ //! is mostly `..`, `.`, empty components, mixed separators and links, so the //! dangerous shapes actually occur instead of being drowned in random text; //! - the **oracle** never re-derives the extractor's path logic: it snapshots -//! the real filesystem around the extraction root before and after, and +//! the whole per-case temporary directory before and after, and //! walks/canonicalizes what is actually on disk. //! -//! Unix-only: the oracle reads mode bits, inode numbers and symlinks. +//! Every case runs in a fresh temporary directory laid out so that nothing +//! the generator can name reaches past it (see [`Layout`]): absolute names and +//! link targets are re-rooted inside it, and the extraction root sits deeper +//! than any `..` chain the generator can build. An extractor that escapes its +//! root therefore writes somewhere the oracle watches, never onto the host. +//! The one escape that leaves the temporary directory regardless is an +//! extractor ignoring its destination altogether (`tar` without `-C` writes +//! into the working directory); [`check_round_trip`] over [`benign_entries`] +//! is what catches that. +//! +//! Unix-only: the oracle reads mode bits, inode numbers and symlinks. Mode +//! checks read the process umask and effective uid from `/proc/self/status`, +//! because what an extractor may leave behind depends on both (see [`Modes`]). #![cfg(all(test, unix))] #![allow( @@ -46,6 +58,28 @@ use proptest::prelude::*; /// substituted per case once the temporary layout exists. pub const OUTSIDE_TOKEN: &str = "@OUT"; +/// Most members in one generated archive. +const MAX_ENTRIES: usize = 6; + +/// Most components in one generated name. +const MAX_COMPONENTS: usize = 4; + +/// Most `..` steps one generated name or link target can take: one per +/// component, even for an extractor that reads `\` as a separator (no +/// component holds more than one `..`, which is what keeps [`NESTING`], and +/// so the cost of every case, small). +const MAX_UP_PER_STRING: usize = MAX_COMPONENTS; + +/// How many directories a [`Fence`] nests each case's fake root under the +/// temporary directory. A path the generator builds starts at the extraction root, at +/// the fake root (absolute names and targets are re-rooted there), or where an +/// earlier member's link lands, and its own name or target then climbs at most +/// [`MAX_UP_PER_STRING`]. Each member contributes a name and at most one +/// target, so no chain over one archive climbs more than +/// `MAX_ENTRIES * 2 * MAX_UP_PER_STRING` above the fake root; one more level +/// keeps even that inside the temporary directory. +const NESTING: usize = MAX_ENTRIES * 2 * MAX_UP_PER_STRING + 1; + /// What one archive member is. #[derive(Clone, Debug)] pub enum Kind { @@ -74,7 +108,7 @@ fn component() -> impl Strategy> { 4 => Just(b"l".to_vec()), 3 => Just(b"a".to_vec()), 1 => Just(b"b".to_vec()), - 1 => Just(b"..\\..\\x".to_vec()), + 1 => Just(b"..\\x".to_vec()), 1 => Just(b"a\\b".to_vec()), 1 => Just(b"C:".to_vec()), 1 => Just(b"C:\\x".to_vec()), @@ -82,6 +116,7 @@ fn component() -> impl Strategy> { 1 => Just(vec![0xff, 0xfe]), 1 => Just(b"x\0y".to_vec()), 1 => Just(vec![b'n'; 120]), + 1 => Just(b"victim".to_vec()), ] } @@ -108,7 +143,7 @@ fn prefix() -> impl Strategy> { pub fn name() -> impl Strategy> { ( prefix(), - prop::collection::vec((component(), separator()), 1..=4), + prop::collection::vec((component(), separator()), 1..=MAX_COMPONENTS), ) .prop_map(|(prefix, parts)| { let mut out = prefix; @@ -128,6 +163,7 @@ fn link_target() -> impl Strategy> { 2 => Just(format!("{OUTSIDE_TOKEN}/sentinel").into_bytes()), 2 => Just(b"../outside".to_vec()), 2 => Just(b"../../outside".to_vec()), + 1 => Just(b"../victim".to_vec()), 1 => Just(b"/".to_vec()), 1 => Just(b".".to_vec()), 3 => name(), @@ -180,12 +216,13 @@ fn entry() -> impl Strategy { /// A whole archive: 1-6 members. pub fn entries() -> impl Strategy> { - prop::collection::vec(entry(), 1..=6) + prop::collection::vec(entry(), 1..=MAX_ENTRIES) } /// Archives that every extractor must accept: plain relative names built only -/// from safe components, regular files and directories, ordinary modes. Used -/// for the "does not over-reject / extracts what it was given" direction. +/// from safe components, regular files and directories, ordinary modes. Each +/// extractor's tests run these through [`check_round_trip`], the "does not +/// over-reject / extracts what it was given, where it was told to" direction. pub fn benign_entries() -> impl Strategy> { let part = prop_oneof![Just("a"), Just("b"), Just("c"), Just("d.txt")]; prop::collection::vec( @@ -216,10 +253,45 @@ pub fn benign_entries() -> impl Strategy> { }) } -fn substitute(bytes: &[u8], outside: &Path) -> Vec { +/// The benign direction, for an archive from [`benign_entries`]: `top` is +/// where the extractor left the archive's single `top/` directory (the +/// extraction root's `top/` for a plain unpack). Every file must be there, +/// byte for byte. A rejecting extractor fails this, and so does one that +/// ignored its destination and wrote somewhere the escape oracle cannot see. +pub fn check_round_trip(top: &Path, entries: &[Entry]) -> Result<(), String> { + for entry in entries { + let Kind::File(data) = &entry.kind else { + continue; + }; + let relative = entry + .name + .strip_prefix(b"top/") + .ok_or_else(|| format!("not a benign entry: {entry:?}"))?; + let path = top.join(std::ffi::OsStr::from_bytes(relative)); + match fs::read(&path) { + Ok(found) if &found == data => {} + Ok(found) => { + return Err(format!( + "{} holds {found:?}, the archive had {data:?}", + path.display() + )); + } + Err(error) => return Err(format!("{} was not extracted: {error}", path.display())), + } + } + Ok(()) +} + +fn substitute(bytes: &[u8], layout: &Layout) -> Vec { let token = OUTSIDE_TOKEN.as_bytes(); - let replacement = outside.as_os_str().as_bytes(); + let replacement = layout.outside.as_os_str().as_bytes(); let mut out = Vec::with_capacity(bytes.len()); + // An absolute name or target is re-rooted at the case's fake root, the way + // a chroot would see it: an extractor that honours it then writes inside + // the watched temporary directory instead of onto the host. + if bytes.first() == Some(&b'/') { + out.extend_from_slice(layout.base.as_os_str().as_bytes()); + } let mut index = 0; while index < bytes.len() { if bytes[index..].starts_with(token) { @@ -234,15 +306,16 @@ fn substitute(bytes: &[u8], outside: &Path) -> Vec { } /// Replace the `@OUT` placeholder in names and link targets with the real -/// absolute path of the case's `outside/` directory. -pub fn concretize(entries: &[Entry], outside: &Path) -> Vec { +/// absolute path of the case's `outside/` directory, and re-root every other +/// absolute name or target at the case's fake root ([`Layout::base`]). +pub fn concretize(entries: &[Entry], layout: &Layout) -> Vec { entries .iter() .map(|entry| Entry { - name: substitute(&entry.name, outside), + name: substitute(&entry.name, layout), kind: match &entry.kind { - Kind::Symlink(target) => Kind::Symlink(substitute(target, outside)), - Kind::Hardlink(target) => Kind::Hardlink(substitute(target, outside)), + Kind::Symlink(target) => Kind::Symlink(substitute(target, layout)), + Kind::Hardlink(target) => Kind::Hardlink(substitute(target, layout)), other => other.clone(), }, mode: entry.mode, @@ -339,30 +412,104 @@ pub fn tar_gz_bytes(entries: &[Entry]) -> Vec { encoder.finish().unwrap() } -/// Per-case filesystem layout: `base/dest` is the extraction root; -/// `base/outside/sentinel` and `base/victim` are what an escape would touch. +/// The guard rail every case runs inside: a temporary directory with a +/// chain of [`NESTING`] directories in it, each case's own directory at the +/// bottom. Nothing the generator can name climbs out of the chain, and the +/// oracle watches all of it. +/// +/// Creating the chain costs a directory per level, so [`run_property`] keeps +/// one fence for all of a property's cases and builds a fresh one only after +/// a case changed something in it: a clean fence is the same fence a new one +/// would be. +pub struct Fence { + _temp: tempfile::TempDir, + top: PathBuf, + bottom: PathBuf, + dirty: std::rc::Rc>, +} + +impl Fence { + pub fn new() -> Self { + use std::os::unix::fs::DirBuilderExt as _; + let temp = tempfile::Builder::new() + .prefix("archive-props-") + .tempdir() + .unwrap(); + let top = fs::canonicalize(temp.path()).unwrap(); + let mut bottom = top.clone(); + for _ in 0..NESTING { + bottom.push("d"); + } + fs::DirBuilder::new() + .recursive(true) + .mode(0o755) + .create(&bottom) + .unwrap(); + Self { + _temp: temp, + top, + bottom, + dirty: std::rc::Rc::default(), + } + } +} + +/// Per-case filesystem layout, at the bottom of a [`Fence`]. `top` is the +/// fence's temporary directory and the oracle watches all of it. `base`, the +/// case's own directory, stands in for `/`; `base/dest` is the extraction +/// root, and `base/outside/sentinel` and `base/victim` are what an escape +/// would touch. +/// +/// Every directory and file the case is judged on gets an explicit mode, so +/// the oracle judges what the extractor did, not what the runner's umask made +/// of the fixture. pub struct Layout { - pub _temp: tempfile::TempDir, + // Declared first so the case directory goes before a fence it owns. + _case: tempfile::TempDir, + _own_fence: Option, + fence_dirty: std::rc::Rc>, + pub top: PathBuf, pub base: PathBuf, pub dest: PathBuf, pub outside: PathBuf, } impl Layout { + /// A one-off case in a fence of its own. pub fn new() -> Self { - let temp = tempfile::Builder::new() - .prefix("archive-props-") - .tempdir() + let fence = Fence::new(); + let mut layout = Self::in_fence(&fence); + layout._own_fence = Some(fence); + layout + } + + /// A fresh case directory at the bottom of `fence`. + pub fn in_fence(fence: &Fence) -> Self { + use std::os::unix::fs::PermissionsExt as _; + let case = tempfile::Builder::new() + .prefix("case-") + .tempdir_in(&fence.bottom) .unwrap(); - let base = fs::canonicalize(temp.path()).unwrap(); + let base = fs::canonicalize(case.path()).unwrap(); + let mkdir = |dir: &Path| { + fs::create_dir(dir).unwrap(); + fs::set_permissions(dir, fs::Permissions::from_mode(0o755)).unwrap(); + }; + let write = |file: &Path| { + fs::write(file, b"original").unwrap(); + fs::set_permissions(file, fs::Permissions::from_mode(0o644)).unwrap(); + }; let dest = base.join("dest"); let outside = base.join("outside"); - fs::create_dir_all(&dest).unwrap(); - fs::create_dir_all(&outside).unwrap(); - fs::write(outside.join("sentinel"), b"original").unwrap(); - fs::write(base.join("victim"), b"original").unwrap(); + mkdir(&dest); + mkdir(&outside); + write(&outside.join("sentinel")); + write(&base.join("victim")); Self { - _temp: temp, + _case: case, + _own_fence: None, + fence_dirty: fence.dirty.clone(), + top: fence.top.clone(), base, dest, outside, @@ -431,6 +578,7 @@ fn read_dir_paths(dir: &Path) -> Vec { } /// Snapshot of every path under `base` except those under the given roots. +/// [`run_case`] passes the whole temporary directory ([`Layout::top`]). pub fn snapshot_outside(base: &Path, inside: &[&Path]) -> BTreeMap { let mut out = BTreeMap::new(); let mut stack = vec![base.to_path_buf()]; @@ -460,21 +608,22 @@ pub struct Violations { /// outside, but every later step that follows links (a directory walk, a /// copy, a chmod, writing into the installed tree) inherits it. pub outward_links: Vec, - /// setuid/setgid or world-writable entries in the installed tree, - /// including the root itself. - pub bad_modes: Vec, + /// setuid/setgid entries in the installed tree, including the root. + pub setid_modes: Vec, + /// World-writable entries in the installed tree, including the root. + pub world_writable: Vec, } fn check_mode(path: &Path, mode: u32, violations: &mut Violations) { if mode & 0o6000 != 0 { - violations.bad_modes.push(format!( + violations.setid_modes.push(format!( "{} has setuid/setgid bits ({mode:o})", path.display() )); } if mode & 0o002 != 0 { violations - .bad_modes + .world_writable .push(format!("{} is world-writable ({mode:o})", path.display())); } } @@ -601,15 +750,15 @@ impl Reach { .iter() .any(|e| e.name.split(|b| *b == b'/').any(|c| c == b"..")), ); - bump( - &self.absolute, - entries.iter().any(|e| e.name.first() == Some(&b'/')), - ); + let absolute = |bytes: &[u8]| { + bytes.first() == Some(&b'/') || bytes.starts_with(OUTSIDE_TOKEN.as_bytes()) + }; + bump(&self.absolute, entries.iter().any(|e| absolute(&e.name))); bump(&self.backslash, entries.iter().any(|e| has(b"\\", &e.name))); bump( &self.symlink_out, entries.iter().any(|e| match &e.kind { - Kind::Symlink(t) => t.first() == Some(&b'/') || has(b"..", t), + Kind::Symlink(t) => absolute(t) || has(b"..", t), _ => false, }), ); @@ -690,8 +839,9 @@ pub struct CaseOutcome { } /// Write `archive_bytes` to `base/`, run `extract`, and judge -/// the result: nothing outside `inside_roots` (which must include the -/// extraction root) may have changed, and the tree under `inspect_root` is +/// the result: nothing in the case's temporary directory outside +/// `inside_roots` (which must include the extraction root) may have changed, +/// and the tree under `inspect_root` is /// walked by [`inspect_tree`]. `extract` receives the layout and the archive /// path. pub fn run_case( @@ -707,9 +857,9 @@ pub fn run_case( let mut excluded: Vec<&Path> = inside_roots.to_vec(); excluded.push(&archive); let inodes = outside_inodes(layout); - let before = snapshot_outside(&layout.base, &excluded); + let before = snapshot_outside(&layout.top, &excluded); let result = extract(layout, &archive); - let after = snapshot_outside(&layout.base, &excluded); + let after = snapshot_outside(&layout.top, &excluded); let mut escapes = Vec::new(); for (path, stamp) in &after { match before.get(path) { @@ -731,6 +881,17 @@ pub fn run_case( escapes.push(format!("removed outside the root: {}", path.display())); } } + // A change above the case's own directory is a change to the fence, which + // the next case must not inherit. + if after + .iter() + .filter(|(path, stamp)| before.get(*path) != Some(*stamp)) + .map(|(path, _)| path) + .chain(before.keys().filter(|path| !after.contains_key(*path))) + .any(|path| !path.starts_with(&layout.base)) + { + layout.fence_dirty.set(true); + } let mut tree = inspect_tree(inspect_root, &inodes); escapes.append(&mut tree.shared_inodes); CaseOutcome { @@ -740,14 +901,92 @@ pub fn run_case( } } +/// What an extractor promises about the modes it leaves behind. +/// +/// Both promises are relative to the process: an extractor that creates +/// directories (or runs `tar`) gets modes through the umask, so under a umask +/// that leaves world-write open (`umask 0`) a world-writable entry is what the +/// user asked for, and only setuid/setgid is judged. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Modes { + /// Not judged. + Unchecked, + /// No setuid/setgid entry, and no world-writable one while the umask + /// clears world-write. Holds whoever runs it. + Safe, + /// [`Modes::Safe`] for the system `tar`, which promises it only to an + /// unprivileged user: run as root, GNU tar restores archived modes, + /// setuid included. So under effective uid 0 the modes are not judged, + /// and [`note_mode_scope`] says so. + SafeUnlessRoot, +} + +/// The process umask and effective uid, from `/proc/self/status` (Linux). +/// `None` for either when it cannot be read. +fn process_umask_and_euid() -> (Option, Option) { + let status = fs::read_to_string("/proc/self/status").unwrap_or_default(); + let field = |key: &str| { + status + .lines() + .find_map(|line| line.strip_prefix(key)) + .map(str::trim) + }; + let umask = field("Umask:").and_then(|value| u32::from_str_radix(value, 8).ok()); + let euid = field("Uid:") + .and_then(|value| value.split_whitespace().nth(1)) + .and_then(|value| value.parse().ok()); + (umask, euid) +} + +impl Modes { + /// Why this run judges less than the whole promise, if it does. + fn relaxed_because(self) -> Option { + let (umask, euid) = process_umask_and_euid(); + match self { + Self::Unchecked => None, + Self::SafeUnlessRoot if euid == Some(0) => Some( + "running as root, where the system tar restores archived modes: modes not judged" + .to_owned(), + ), + Self::Safe | Self::SafeUnlessRoot => { + umask.filter(|umask| umask & 0o002 == 0).map(|umask| { + format!("umask {umask:03o} leaves world-write open: only setuid/setgid judged") + }) + } + } + } + + fn broken_by(self, tree: &Violations) -> bool { + let (umask, euid) = process_umask_and_euid(); + match self { + Self::Unchecked => false, + Self::SafeUnlessRoot if euid == Some(0) => false, + Self::Safe | Self::SafeUnlessRoot => { + let umask_clears_world_write = umask.is_none_or(|umask| umask & 0o002 != 0); + !tree.setid_modes.is_empty() + || (umask_clears_world_write && !tree.world_writable.is_empty()) + } + } + } +} + +/// Print, once per property, which part of the mode promise this run cannot +/// judge (running as root, or under a umask that leaves world-write open), so +/// a pass there is not read as the whole promise holding. +pub fn note_mode_scope(label: &str, modes: Modes) { + if let Some(reason) = modes.relaxed_because() { + eprintln!("[{label}] {reason}"); + } +} + /// Which parts of the contract a given extractor promises. Escapes (writes /// outside the root) are always checked; the tree-hygiene parts are opt-in, /// because the system-`tar` extractors deliberately recreate symlinks as /// archived. #[derive(Clone, Copy, Debug)] pub struct Expect { - /// No setuid/setgid or world-writable entry in the extracted tree. - pub safe_modes: bool, + /// What the modes in the extracted tree may be. + pub modes: Modes, /// No symlink in the extracted tree resolves outside it. pub no_outward_links: bool, } @@ -756,7 +995,7 @@ impl CaseOutcome { /// `Ok` when the outcome meets `expect`, else the outcome as text. pub fn judge(&self, expect: Expect) -> Result<(), String> { let broken = !self.escapes.is_empty() - || (expect.safe_modes && !self.tree.bad_modes.is_empty()) + || expect.modes.broken_by(&self.tree) || (expect.no_outward_links && !self.tree.outward_links.is_empty()); if broken { Err(format!("{self:#?}")) @@ -767,7 +1006,8 @@ impl CaseOutcome { } /// Run `check` over `cases` archives from `strategy`, each in a fresh -/// [`Layout`] with the `@OUT` placeholder already substituted. Returns the +/// [`Layout`] with the archive already [`concretize`]d into it. Reach is +/// counted on the archive as generated, before absolute names are re-rooted. Returns the /// shrunk counterexample as text on failure. On success, when `reach_floor` /// is set, also fails if any dangerous shape occurred in fewer than that /// percentage of cases — a pass over a generator that never produced the @@ -797,10 +1037,14 @@ pub fn run_property( if config.rng_seed == proptest::test_runner::RngSeed::Random { config.rng_seed = proptest::test_runner::RngSeed::Fixed(0x00a2_c41e); } + let fence = std::cell::RefCell::new(Fence::new()); let result = proptest::test_runner::TestRunner::new(config).run(&strategy, |entries| { - let layout = Layout::new(); - let entries = concretize(&entries, &layout.outside); + if fence.borrow().dirty.get() { + *fence.borrow_mut() = Fence::new(); + } + let layout = Layout::in_fence(&fence.borrow()); reach.record(&entries); + let entries = concretize(&entries, &layout); check(&layout, &entries).map_err(proptest::test_runner::TestCaseError::fail) }); if let Err(error) = result { @@ -844,6 +1088,9 @@ pub fn naive_sanitizing_unpack(archive: &Path, destination: &Path) -> anyhow::Re /// joins each raw entry name onto the root and unpacks there, following any /// link an earlier entry planted. If the property does not fail against this, /// the generator is not reaching the shapes it claims to. +/// +/// Absolute names reach it already re-rooted by [`concretize`], so even this +/// extractor writes only inside the case's temporary directory. pub fn naive_unpack(archive: &Path, destination: &Path) -> anyhow::Result<()> { let file = fs::File::open(archive)?; let mut tar = tar::Archive::new(flate2::read::GzDecoder::new(file)); diff --git a/crates/rocm-core/src/uv.rs b/crates/rocm-core/src/uv.rs index ab0a78ee0..cd3270715 100644 --- a/crates/rocm-core/src/uv.rs +++ b/crates/rocm-core/src/uv.rs @@ -1325,8 +1325,9 @@ mod archive_properties { use super::{extract_archive, find_binary_in, make_executable, uv_binary_name}; use crate::archive_props::{ - Entry, Expect, Kind, Layout, OUTSIDE_TOKEN, concretize, entries, naive_sanitizing_unpack, - naive_unpack, run_case, run_property, tar_gz_bytes, + Entry, Expect, Kind, Layout, Modes, OUTSIDE_TOKEN, benign_entries, check_round_trip, + concretize, entries, naive_sanitizing_unpack, naive_unpack, note_mode_scope, run_case, + run_property, tar_gz_bytes, }; fn property( @@ -1334,6 +1335,7 @@ mod archive_properties { expect: Expect, extract: fn(&Path, &Path) -> anyhow::Result<()>, ) -> Result<(), String> { + note_mode_scope(label, expect.modes); run_property(label, 256, entries(), Some(5), |layout, entries| { run_case( layout, @@ -1348,7 +1350,7 @@ mod archive_properties { } const ESCAPES_ONLY: Expect = Expect { - safe_modes: false, + modes: Modes::Unchecked, no_outward_links: false, }; @@ -1357,12 +1359,34 @@ mod archive_properties { property("uv", ESCAPES_ONLY, extract_archive).unwrap(); } + /// The other direction: a well-formed archive is extracted in full, into + /// the staging directory it was given. An extraction that wrote somewhere + /// else entirely (no `-C`) escapes the oracle above; this catches it. + #[test] + fn uv_archive_extraction_extracts_a_benign_archive_into_the_staging_dir() { + run_property( + "uv-benign", + 64, + benign_entries(), + None, + |layout, entries| { + let archive = layout.base.join("archive.tar.gz"); + std::fs::write(&archive, tar_gz_bytes(entries)).unwrap(); + extract_archive(&archive, &layout.dest).map_err(|error| format!("{error:#}"))?; + check_round_trip(&layout.dest.join("top"), entries) + }, + ) + .unwrap(); + } + + /// The system `tar` drops setuid/setgid and applies the umask only for an + /// unprivileged user, so this is judged only there (see `Modes`). #[test] fn uv_archive_extraction_leaves_no_setuid_or_world_writable_entry() { property( "uv-modes", Expect { - safe_modes: true, + modes: Modes::SafeUnlessRoot, no_outward_links: false, }, extract_archive, @@ -1374,11 +1398,7 @@ mod archive_properties { /// lookup-and-chmod half of `ensure_uv_binary` on it. fn stage_and_locate(layout: &Layout, entries: &[Entry]) -> Option { let archive = layout.base.join("uv.tar.gz"); - std::fs::write( - &archive, - tar_gz_bytes(&concretize(entries, &layout.outside)), - ) - .unwrap(); + std::fs::write(&archive, tar_gz_bytes(&concretize(entries, layout))).unwrap(); extract_archive(&archive, &layout.dest).unwrap(); let binary = find_binary_in(&layout.dest, uv_binary_name())?; let _ = make_executable(&binary); diff --git a/engines/lemonade/src/install.rs b/engines/lemonade/src/install.rs index 633419fae..9fa5807be 100644 --- a/engines/lemonade/src/install.rs +++ b/engines/lemonade/src/install.rs @@ -1971,7 +1971,8 @@ mod archive_properties { use sha2::{Digest as _, Sha256}; use super::archive_props::{ - Entry, Expect, Kind, benign_entries, entries, run_case, run_property, tar_gz_bytes, + Entry, Expect, Kind, Modes, benign_entries, check_round_trip, entries, note_mode_scope, + run_case, run_property, tar_gz_bytes, }; use super::{ AppPaths, extract_tar_gz, extract_zip, installed_runtime_version, lemonade_path_in, @@ -1979,14 +1980,15 @@ mod archive_properties { }; /// The in-process extractor rejects every link kind and masks modes, so it - /// promises the whole contract, not just "no escape". + /// promises the whole contract, not just "no escape", whoever runs it. const FULL: Expect = Expect { - safe_modes: true, + modes: Modes::Safe, no_outward_links: true, }; #[test] fn tar_extraction_keeps_every_entry_inside_the_root() { + note_mode_scope("lemonade-tar", FULL.modes); run_property( "lemonade-tar", 512, @@ -2037,9 +2039,42 @@ mod archive_properties { writer.finish().unwrap().into_inner() } + /// The other direction for both formats: a well-formed archive is + /// extracted in full, into the root it was given. + #[test] + fn tar_and_zip_extraction_extract_a_benign_archive_into_the_root() { + run_property( + "lemonade-benign-tar", + 64, + benign_entries(), + None, + |layout, entries| { + let archive = layout.base.join("embeddable.tar.gz"); + std::fs::write(&archive, tar_gz_bytes(entries)).unwrap(); + extract_tar_gz(&archive, &layout.dest).map_err(|error| format!("{error:#}"))?; + check_round_trip(&layout.dest.join("top"), entries) + }, + ) + .unwrap(); + run_property( + "lemonade-benign-zip", + 64, + benign_entries(), + None, + |layout, entries| { + let archive = layout.base.join("embeddable.zip"); + std::fs::write(&archive, zip_bytes(entries)).unwrap(); + extract_zip(&archive, &layout.dest).map_err(|error| format!("{error:#}"))?; + check_round_trip(&layout.dest.join("top"), entries) + }, + ) + .unwrap(); + } + #[test] fn zip_extraction_keeps_every_entry_inside_the_root() { use std::sync::atomic::{AtomicUsize, Ordering}; + note_mode_scope("lemonade-zip", FULL.modes); // Reach is measured on what the zip writer actually stored, since it // may drop or normalise some generated names. let (cases, traversal, symlink) = ( @@ -2136,6 +2171,7 @@ mod archive_properties { // Reach of the install half: a property over installs that all fail // at extraction would say nothing about the copy step. let (cases, installed) = (AtomicUsize::new(0), AtomicUsize::new(0)); + note_mode_scope("lemonade-install", FULL.modes); run_property( "lemonade-install", 256, From a4d8d5034f7423e2632ab6d4c0e62bf4c257101c Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 9 Oct 2026 11:35:46 +0000 Subject: [PATCH 3/7] test: pin that a dirtied archive-property fence is replaced The archive properties reuse one fence across a property's cases while it is clean. Pin both halves: a case that changes only its own directory keeps the fence, and one that changes the fence gets the next case a new one, so no case inherits another's escape. Signed-off-by: Roman Inflianskas --- crates/rocm-core/src/archive_props.rs | 46 +++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/crates/rocm-core/src/archive_props.rs b/crates/rocm-core/src/archive_props.rs index 3e3cf0f28..6d7c2f89e 100644 --- a/crates/rocm-core/src/archive_props.rs +++ b/crates/rocm-core/src/archive_props.rs @@ -1105,3 +1105,49 @@ pub fn naive_unpack(archive: &Path, destination: &Path) -> anyhow::Result<()> { } Ok(()) } + +/// [`run_property`] reuses one [`Fence`] across cases only while it is clean: +/// a case whose extraction changed something above its own directory gets the +/// next case a fresh fence, and one that changed only its own directory does +/// not. +#[test] +fn a_fence_is_replaced_after_a_case_changes_it_and_only_then() { + let tops_seen = |write_above_the_case: bool| { + let tops = std::cell::RefCell::new(Vec::new()); + run_property("fence", 3, Just(Vec::new()), None, |layout, _| { + tops.borrow_mut().push(layout.top.clone()); + let outcome = run_case( + layout, + "a", + b"", + &[&layout.dest], + &layout.dest, + |layout, _| { + let target = if write_above_the_case { + layout.base.parent().unwrap().join("x") + } else { + layout.outside.join("sentinel") + }; + fs::write(target, b"changed")?; + Ok(()) + }, + ); + assert!(!outcome.escapes.is_empty(), "{outcome:#?}"); + Ok(()) + }) + .unwrap(); + let tops = tops.into_inner(); + let distinct: std::collections::BTreeSet<_> = tops.iter().collect(); + (tops.len(), distinct.len()) + }; + let (cases, fences) = tops_seen(false); + assert_eq!( + fences, 1, + "{cases} cases that stayed in their own directory" + ); + let (cases, fences) = tops_seen(true); + assert_eq!( + fences, cases, + "every case after a fence change needs a fresh fence" + ); +} From 27bd6cfcf18dc5905ab2b8718e5624b594ea5113 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 9 Oct 2026 11:49:54 +0000 Subject: [PATCH 4/7] test: tighten the archive-property oracle where a re-read found gaps - A dangling symlink is judged by where its target would resolve relative to the link, so a dangling `../../x` counts as outward, not only an absolute target. - An extraction that deletes its own root is reported as an escape. - Modes::Safe now holds files to "never world-writable" whatever the umask, since the in-process extractor sets file modes itself; only the directories it creates follow the umask. - The calibration tests require the failure to be an escape, so a harness panic can no longer pass for "the generator caught it". - The fence test covers created, modified and removed entries above the case. - PROPTEST_CASES can raise the case count but not lower it below what the reach floors need. - The ComfyUI generator also drops symlinks that walk through an earlier symlink, which could still land at the top level, and leaves absolute names alone. - The benign round trip is one helper instead of five copies. Signed-off-by: Roman Inflianskas --- apps/rocm/src/comfyui.rs | 76 +++++---- apps/rocm/src/therock.rs | 23 +-- crates/rocm-core/src/archive_props.rs | 222 ++++++++++++++++++++------ crates/rocm-core/src/uv.rs | 29 ++-- engines/lemonade/src/install.rs | 44 ++--- 5 files changed, 248 insertions(+), 146 deletions(-) diff --git a/apps/rocm/src/comfyui.rs b/apps/rocm/src/comfyui.rs index 0d5191cfb..01c09b9ff 100644 --- a/apps/rocm/src/comfyui.rs +++ b/apps/rocm/src/comfyui.rs @@ -3090,8 +3090,8 @@ mod archive_properties { use proptest::prelude::*; use crate::archive_props::{ - Entry, Expect, Kind, Layout, Modes, benign_entries, check_round_trip, entries, - note_mode_scope, run_case, run_property, tar_gz_bytes, + Entry, Expect, Kind, Layout, Modes, entries, note_mode_scope, run_case, run_property, + run_round_trip, tar_gz_bytes, }; /// The unpack plus the first write `install` makes into the result: the @@ -3109,30 +3109,34 @@ mod archive_properties { } /// [`entries`] with every relative name moved one level down, under - /// `a/`, and any symlink that would still land directly in the extraction - /// root dropped: that is the one shape that hits the known #522 defect - /// (`first_child_dir` follows it). Moving names down rather than only - /// dropping top-level links keeps "link, then write through it" in play, - /// one level down where the source tree is. + /// `a/`, and every symlink dropped that could still land directly in the + /// extraction root: the shape that hits the known #522 defect + /// (`first_child_dir` follows it). A symlink is dropped when its name, + /// read as text, has at most one component, or when it walks through an + /// earlier symlink's name, which could point anywhere. Absolute names are + /// left alone: re-rooted, they land deep inside the extraction root. + /// Moving names down rather than only dropping top-level links keeps + /// "link, then write through it" in play, one level down where the + /// source tree is. fn entries_without_top_level_symlink() -> impl Strategy> { - fn lands_top_level(name: &[u8]) -> bool { - let mut depth = 0usize; + /// Lexical components, or `None` when the name climbs above the root. + fn components(name: &[u8]) -> Option> { + let mut out = Vec::new(); for part in name.split(|byte| *byte == b'/') { match part { b"" | b"." => {} b".." => { - let Some(up) = depth.checked_sub(1) else { - return true; - }; - depth = up; + out.pop()?; } - _ => depth += 1, + _ => out.push(part), } } - depth <= 1 + Some(out) } entries().prop_map(|entries| { - entries + let mut links: Vec> = Vec::new(); + let mut kept = Vec::new(); + let moved: Vec = entries .into_iter() .map(|mut entry| { if !entry.name.starts_with(b"/") && !entry.name.starts_with(b"@") { @@ -3140,10 +3144,23 @@ mod archive_properties { } entry }) - .filter(|entry| { - !(matches!(entry.kind, Kind::Symlink(_)) && lands_top_level(&entry.name)) - }) - .collect() + .collect(); + for entry in &moved { + if matches!(entry.kind, Kind::Symlink(_)) + && !entry.name.starts_with(b"/") + && !entry.name.starts_with(b"@") + { + let Some(parts) = components(&entry.name) else { + continue; + }; + if parts.len() <= 1 || links.iter().any(|link| parts.starts_with(link)) { + continue; + } + links.push(parts); + } + kept.push(entry.clone()); + } + kept }) } @@ -3201,19 +3218,12 @@ mod archive_properties { /// full, its single top-level directory becoming `source/`. #[test] fn source_unpack_installs_a_benign_archive_as_the_source_dir() { - run_property( - "comfyui-benign", - 64, - benign_entries(), - None, - |layout, entries| { - let archive = layout.base.join("source.tar.gz"); - fs::write(&archive, tar_gz_bytes(entries)).unwrap(); - unpack_then_first_install_write(layout, &archive) - .map_err(|error| format!("{error:#}"))?; - check_round_trip(&source_path_from_app_root(&layout.dest), entries) - }, - ) + run_round_trip("comfyui-benign", |layout, entries| { + let archive = layout.base.join("source.tar.gz"); + fs::write(&archive, tar_gz_bytes(entries))?; + unpack_then_first_install_write(layout, &archive)?; + Ok(source_path_from_app_root(&layout.dest)) + }) .unwrap(); } diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 4e2b96070..e30de5fb2 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -11195,8 +11195,8 @@ exit 1 mod archive_properties { use super::extract_tarball_and_discard_archive; use crate::archive_props::{ - Expect, Modes, benign_entries, check_round_trip, entries, note_mode_scope, run_case, - run_property, tar_gz_bytes, + Expect, Modes, entries, note_mode_scope, run_case, run_property, run_round_trip, + tar_gz_bytes, }; /// No escape, and no setuid/setgid or world-writable entry. The modes are @@ -11230,19 +11230,12 @@ mod archive_properties { /// entirely escapes the oracle above; this catches it. #[test] fn sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root() { - run_property( - "therock-benign", - 64, - benign_entries(), - None, - |layout, entries| { - let archive = layout.base.join("sdk.tar.gz"); - std::fs::write(&archive, tar_gz_bytes(entries)).unwrap(); - extract_tarball_and_discard_archive(&archive, &layout.dest) - .map_err(|error| format!("{error:#}"))?; - check_round_trip(&layout.dest.join("top"), entries) - }, - ) + run_round_trip("therock-benign", |layout, entries| { + let archive = layout.base.join("sdk.tar.gz"); + std::fs::write(&archive, tar_gz_bytes(entries))?; + extract_tarball_and_discard_archive(&archive, &layout.dest)?; + Ok(layout.dest.join("top")) + }) .unwrap(); } } diff --git a/crates/rocm-core/src/archive_props.rs b/crates/rocm-core/src/archive_props.rs index 6d7c2f89e..a5bf036c4 100644 --- a/crates/rocm-core/src/archive_props.rs +++ b/crates/rocm-core/src/archive_props.rs @@ -17,15 +17,15 @@ //! is mostly `..`, `.`, empty components, mixed separators and links, so the //! dangerous shapes actually occur instead of being drowned in random text; //! - the **oracle** never re-derives the extractor's path logic: it snapshots -//! the whole per-case temporary directory before and after, and +//! the whole temporary directory the case runs in before and after, and //! walks/canonicalizes what is actually on disk. //! -//! Every case runs in a fresh temporary directory laid out so that nothing -//! the generator can name reaches past it (see [`Layout`]): absolute names and -//! link targets are re-rooted inside it, and the extraction root sits deeper -//! than any `..` chain the generator can build. An extractor that escapes its -//! root therefore writes somewhere the oracle watches, never onto the host. -//! The one escape that leaves the temporary directory regardless is an +//! Every case runs in a fresh directory at the bottom of a [`Fence`], laid out +//! so that nothing the generator can name reaches past the fence: absolute +//! names and link targets are re-rooted inside it, and the extraction root +//! sits deeper than any `..` chain the generator can build. An extractor that +//! escapes its root therefore writes somewhere the oracle watches, never onto +//! the host. The one escape that leaves the fence regardless is an //! extractor ignoring its destination altogether (`tar` without `-C` writes //! into the working directory); [`check_round_trip`] over [`benign_entries`] //! is what catches that. @@ -282,6 +282,19 @@ pub fn check_round_trip(top: &Path, entries: &[Entry]) -> Result<(), String> { Ok(()) } +/// [`check_round_trip`] over 64 archives from [`benign_entries`]. `extract` +/// writes the archive in its extractor's format, runs the extractor, and +/// returns where the archive's `top/` directory should now be. +pub fn run_round_trip( + label: &str, + extract: impl Fn(&Layout, &[Entry]) -> anyhow::Result, +) -> Result<(), String> { + run_property(label, 64, benign_entries(), None, |layout, entries| { + let top = extract(layout, entries).map_err(|error| format!("{error:#}"))?; + check_round_trip(&top, entries) + }) +} + fn substitute(bytes: &[u8], layout: &Layout) -> Vec { let token = OUTSIDE_TOKEN.as_bytes(); let replacement = layout.outside.as_os_str().as_bytes(); @@ -610,11 +623,14 @@ pub struct Violations { pub outward_links: Vec, /// setuid/setgid entries in the installed tree, including the root. pub setid_modes: Vec, - /// World-writable entries in the installed tree, including the root. - pub world_writable: Vec, + /// World-writable files in the installed tree. + pub world_writable_files: Vec, + /// World-writable directories in the installed tree, including the root. + pub world_writable_dirs: Vec, } -fn check_mode(path: &Path, mode: u32, violations: &mut Violations) { +fn check_mode(path: &Path, meta: &fs::Metadata, violations: &mut Violations) { + let mode = meta.mode(); if mode & 0o6000 != 0 { violations.setid_modes.push(format!( "{} has setuid/setgid bits ({mode:o})", @@ -622,12 +638,31 @@ fn check_mode(path: &Path, mode: u32, violations: &mut Violations) { )); } if mode & 0o002 != 0 { - violations - .world_writable - .push(format!("{} is world-writable ({mode:o})", path.display())); + let found = format!("{} is world-writable ({mode:o})", path.display()); + if meta.is_dir() { + violations.world_writable_dirs.push(found); + } else { + violations.world_writable_files.push(found); + } } } +/// `path` with `.` and `..` resolved by text alone, the way a dangling link's +/// target would resolve once something exists there. +fn lexically_normal(path: &Path) -> PathBuf { + let mut out = PathBuf::new(); + for component in path.components() { + match component { + std::path::Component::CurDir => {} + std::path::Component::ParentDir => { + out.pop(); + } + other => out.push(other), + } + } + out +} + /// Walk `root` on the real filesystem (never following links) and report any /// symlink whose target resolves outside `root`, any hard link shared with a /// file outside, and any setuid/setgid or world-writable entry. @@ -643,7 +678,7 @@ pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { )); return violations; } - check_mode(root, root_meta.mode(), &mut violations); + check_mode(root, &root_meta, &mut violations); ensure_listable(root); let Ok(canonical_root) = fs::canonicalize(root) else { return violations; @@ -657,8 +692,10 @@ pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { let file_type = meta.file_type(); if file_type.is_symlink() { // `canonicalize` resolves the whole chain on disk; a dangling - // link is reported by its lexical target instead. + // link is judged by its target read as text, relative to the + // directory the link is in. let target = fs::read_link(&path).unwrap_or_default(); + let lexical = lexically_normal(&dir.join(&target)); match fs::canonicalize(&path) { Ok(resolved) if !resolved.starts_with(&canonical_root) => { violations.outward_links.push(format!( @@ -668,7 +705,7 @@ pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { resolved.display() )); } - Err(_) if target.is_absolute() && !target.starts_with(&canonical_root) => { + Err(_) if !lexical.starts_with(&canonical_root) => { violations.outward_links.push(format!( "dangling symlink {} -> {} points outside the root", path.display(), @@ -679,7 +716,7 @@ pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { } continue; } - check_mode(&path, meta.mode(), &mut violations); + check_mode(&path, &meta, &mut violations); if file_type.is_file() && outside_inodes.contains(&meta.ino()) { violations.shared_inodes.push(format!( "{} is a hard link to a file outside the root", @@ -892,6 +929,12 @@ pub fn run_case( { layout.fence_dirty.set(true); } + if fs::symlink_metadata(inspect_root).is_err() { + escapes.push(format!( + "removed the extraction root itself: {}", + inspect_root.display() + )); + } let mut tree = inspect_tree(inspect_root, &inodes); escapes.append(&mut tree.shared_inodes); CaseOutcome { @@ -903,21 +946,23 @@ pub fn run_case( /// What an extractor promises about the modes it leaves behind. /// -/// Both promises are relative to the process: an extractor that creates -/// directories (or runs `tar`) gets modes through the umask, so under a umask -/// that leaves world-write open (`umask 0`) a world-writable entry is what the -/// user asked for, and only setuid/setgid is judged. +/// No promise covers a world-writable entry the umask let through: under a +/// umask that leaves world-write open (`umask 0`) that is what the user asked +/// for. What differs is which entries get their mode through the umask. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum Modes { /// Not judged. Unchecked, - /// No setuid/setgid entry, and no world-writable one while the umask - /// clears world-write. Holds whoever runs it. + /// For an extractor that sets every file's mode itself: no setuid/setgid + /// entry and no world-writable file, whatever the umask or uid; + /// directories, which it creates through the umask, are judged only + /// while the umask clears world-write. Safe, - /// [`Modes::Safe`] for the system `tar`, which promises it only to an - /// unprivileged user: run as root, GNU tar restores archived modes, - /// setuid included. So under effective uid 0 the modes are not judged, - /// and [`note_mode_scope`] says so. + /// For the system `tar`, which gives every entry its mode through the + /// umask and drops setuid/setgid only for an unprivileged user: no + /// setuid/setgid entry, and no world-writable entry while the umask + /// clears world-write. Run as root, GNU tar restores archived modes, + /// setuid included, so under effective uid 0 the modes are not judged. SafeUnlessRoot, } @@ -948,11 +993,14 @@ impl Modes { "running as root, where the system tar restores archived modes: modes not judged" .to_owned(), ), - Self::Safe | Self::SafeUnlessRoot => { - umask.filter(|umask| umask & 0o002 == 0).map(|umask| { - format!("umask {umask:03o} leaves world-write open: only setuid/setgid judged") - }) - } + Self::Safe => umask.filter(|umask| umask & 0o002 == 0).map(|umask| { + format!( + "umask {umask:03o} leaves world-write open: world-writable directories not judged" + ) + }), + Self::SafeUnlessRoot => umask.filter(|umask| umask & 0o002 == 0).map(|umask| { + format!("umask {umask:03o} leaves world-write open: only setuid/setgid judged") + }), } } @@ -963,8 +1011,11 @@ impl Modes { Self::SafeUnlessRoot if euid == Some(0) => false, Self::Safe | Self::SafeUnlessRoot => { let umask_clears_world_write = umask.is_none_or(|umask| umask & 0o002 != 0); + let files_follow_umask = self == Self::SafeUnlessRoot; !tree.setid_modes.is_empty() - || (umask_clears_world_write && !tree.world_writable.is_empty()) + || (!tree.world_writable_files.is_empty() + && (umask_clears_world_write || !files_follow_umask)) + || (umask_clears_world_write && !tree.world_writable_dirs.is_empty()) } } } @@ -972,7 +1023,10 @@ impl Modes { /// Print, once per property, which part of the mode promise this run cannot /// judge (running as root, or under a umask that leaves world-write open), so -/// a pass there is not read as the whole promise holding. +/// a pass there is not read as the whole promise holding. It goes to stderr: +/// `cargo test` shows it with `--nocapture`, and nextest only for a failing +/// test unless run with `--success-output`. The default CI runners run +/// unprivileged under umask 022, where nothing is relaxed and nothing prints. pub fn note_mode_scope(label: &str, modes: Modes) { if let Some(reason) = modes.relaxed_because() { eprintln!("[{label}] {reason}"); @@ -1007,8 +1061,8 @@ impl CaseOutcome { /// Run `check` over `cases` archives from `strategy`, each in a fresh /// [`Layout`] with the archive already [`concretize`]d into it. Reach is -/// counted on the archive as generated, before absolute names are re-rooted. Returns the -/// shrunk counterexample as text on failure. On success, when `reach_floor` +/// counted on the archive as generated, before absolute names are re-rooted. +/// Returns the shrunk counterexample as text on failure. On success, when `reach_floor` /// is set, also fails if any dangerous shape occurred in fewer than that /// percentage of cases — a pass over a generator that never produced the /// shape would prove nothing. @@ -1022,10 +1076,12 @@ pub fn run_property( let reach = Reach::new(); // `PROPTEST_CASES` (proptest's own knob, which an explicit case count // would otherwise override) scales every property up for a longer hunt. + // Never down: the reach floors below are percentages, which a handful of + // cases cannot meet. let cases = std::env::var("PROPTEST_CASES") .ok() .and_then(|value| value.parse().ok()) - .unwrap_or(cases); + .map_or(cases, |requested: u32| requested.max(cases)); let mut config = proptest::test_runner::Config { cases, failure_persistence: None, @@ -1107,15 +1163,29 @@ pub fn naive_unpack(archive: &Path, destination: &Path) -> anyhow::Result<()> { } /// [`run_property`] reuses one [`Fence`] across cases only while it is clean: -/// a case whose extraction changed something above its own directory gets the -/// next case a fresh fence, and one that changed only its own directory does -/// not. +/// a case whose extraction created, changed or removed something above its +/// own directory gets the next case a fresh fence, and one that changed only +/// its own directory does not. #[test] fn a_fence_is_replaced_after_a_case_changes_it_and_only_then() { - let tops_seen = |write_above_the_case: bool| { + use std::os::unix::fs::PermissionsExt as _; + #[derive(Clone, Copy, Debug)] + enum Change { + OwnDirectory, + CreateAbove, + ModifyAbove, + RemoveAbove, + } + let fences_over_cases = |change: Change| { let tops = std::cell::RefCell::new(Vec::new()); run_property("fence", 3, Just(Vec::new()), None, |layout, _| { tops.borrow_mut().push(layout.top.clone()); + let above = layout.base.parent().unwrap().to_path_buf(); + // Something to remove that the extraction did not create. + let doomed = above.join(format!("doomed-{}", tops.borrow().len())); + if matches!(change, Change::RemoveAbove) { + fs::write(&doomed, b"x").unwrap(); + } let outcome = run_case( layout, "a", @@ -1123,16 +1193,18 @@ fn a_fence_is_replaced_after_a_case_changes_it_and_only_then() { &[&layout.dest], &layout.dest, |layout, _| { - let target = if write_above_the_case { - layout.base.parent().unwrap().join("x") - } else { - layout.outside.join("sentinel") - }; - fs::write(target, b"changed")?; + match change { + Change::OwnDirectory => fs::write(layout.outside.join("sentinel"), b"x")?, + Change::CreateAbove => fs::write(above.join("created"), b"x")?, + Change::ModifyAbove => { + fs::set_permissions(&above, fs::Permissions::from_mode(0o700))?; + } + Change::RemoveAbove => fs::remove_file(&doomed)?, + } Ok(()) }, ); - assert!(!outcome.escapes.is_empty(), "{outcome:#?}"); + assert!(!outcome.escapes.is_empty(), "{change:?}: {outcome:#?}"); Ok(()) }) .unwrap(); @@ -1140,14 +1212,58 @@ fn a_fence_is_replaced_after_a_case_changes_it_and_only_then() { let distinct: std::collections::BTreeSet<_> = tops.iter().collect(); (tops.len(), distinct.len()) }; - let (cases, fences) = tops_seen(false); + let (cases, fences) = fences_over_cases(Change::OwnDirectory); assert_eq!( fences, 1, "{cases} cases that stayed in their own directory" ); - let (cases, fences) = tops_seen(true); - assert_eq!( - fences, cases, - "every case after a fence change needs a fresh fence" + for change in [ + Change::CreateAbove, + Change::ModifyAbove, + Change::RemoveAbove, + ] { + let (cases, fences) = fences_over_cases(change); + assert_eq!( + fences, cases, + "{change:?}: every case after a fence change needs a fresh fence" + ); + } +} + +/// A dangling symlink is judged by where its target would be, relative to +/// the link: `../../x` from the root leaves it even though nothing is there +/// yet, while a dangling link to a name inside the root does not. +#[test] +fn a_dangling_link_is_judged_by_where_it_would_resolve() { + let layout = Layout::new(); + std::os::unix::fs::symlink("../../not-there", layout.dest.join("out")).unwrap(); + std::os::unix::fs::symlink("not-there-either", layout.dest.join("in")).unwrap(); + let tree = inspect_tree(&layout.dest, &[]); + assert_eq!(tree.outward_links.len(), 1, "{tree:#?}"); + assert!(tree.outward_links[0].contains("/out "), "{tree:#?}"); +} + +/// An extraction that deletes its own root has not stayed inside it, even +/// though nothing outside the root changed. +#[test] +fn removing_the_extraction_root_is_an_escape() { + let layout = Layout::new(); + let outcome = run_case( + &layout, + "a", + b"", + &[&layout.dest], + &layout.dest, + |layout, _| { + fs::remove_dir(&layout.dest)?; + Ok(()) + }, + ); + assert!( + outcome + .escapes + .iter() + .any(|escape| escape.contains("removed the extraction root")), + "{outcome:#?}" ); } diff --git a/crates/rocm-core/src/uv.rs b/crates/rocm-core/src/uv.rs index cd3270715..9ba669296 100644 --- a/crates/rocm-core/src/uv.rs +++ b/crates/rocm-core/src/uv.rs @@ -1325,9 +1325,9 @@ mod archive_properties { use super::{extract_archive, find_binary_in, make_executable, uv_binary_name}; use crate::archive_props::{ - Entry, Expect, Kind, Layout, Modes, OUTSIDE_TOKEN, benign_entries, check_round_trip, - concretize, entries, naive_sanitizing_unpack, naive_unpack, note_mode_scope, run_case, - run_property, tar_gz_bytes, + Entry, Expect, Kind, Layout, Modes, OUTSIDE_TOKEN, concretize, entries, + naive_sanitizing_unpack, naive_unpack, note_mode_scope, run_case, run_property, + run_round_trip, tar_gz_bytes, }; fn property( @@ -1364,18 +1364,12 @@ mod archive_properties { /// else entirely (no `-C`) escapes the oracle above; this catches it. #[test] fn uv_archive_extraction_extracts_a_benign_archive_into_the_staging_dir() { - run_property( - "uv-benign", - 64, - benign_entries(), - None, - |layout, entries| { - let archive = layout.base.join("archive.tar.gz"); - std::fs::write(&archive, tar_gz_bytes(entries)).unwrap(); - extract_archive(&archive, &layout.dest).map_err(|error| format!("{error:#}"))?; - check_round_trip(&layout.dest.join("top"), entries) - }, - ) + run_round_trip("uv-benign", |layout, entries| { + let archive = layout.base.join("archive.tar.gz"); + std::fs::write(&archive, tar_gz_bytes(entries))?; + extract_archive(&archive, &layout.dest)?; + Ok(layout.dest.join("top")) + }) .unwrap(); } @@ -1476,7 +1470,8 @@ mod archive_properties { fn generator_catches_a_traversing_extractor() { let error = property("naive", ESCAPES_ONLY, naive_unpack) .expect_err("an extractor that honours `..` must be caught"); - eprintln!("{error}"); + // Caught for writing outside the root, not for some harness failure. + assert!(error.contains("outside the root"), "{error}"); } /// Calibration for the two-step link shape: an extractor that sanitises @@ -1486,6 +1481,6 @@ mod archive_properties { fn generator_catches_a_link_following_extractor() { let error = property("naive-links", ESCAPES_ONLY, naive_sanitizing_unpack) .expect_err("an extractor that writes through planted links must be caught"); - eprintln!("{error}"); + assert!(error.contains("outside the root"), "{error}"); } } diff --git a/engines/lemonade/src/install.rs b/engines/lemonade/src/install.rs index 9fa5807be..98cd8fd00 100644 --- a/engines/lemonade/src/install.rs +++ b/engines/lemonade/src/install.rs @@ -1971,16 +1971,16 @@ mod archive_properties { use sha2::{Digest as _, Sha256}; use super::archive_props::{ - Entry, Expect, Kind, Modes, benign_entries, check_round_trip, entries, note_mode_scope, - run_case, run_property, tar_gz_bytes, + Entry, Expect, Kind, Modes, benign_entries, entries, note_mode_scope, run_case, + run_property, run_round_trip, tar_gz_bytes, }; use super::{ AppPaths, extract_tar_gz, extract_zip, installed_runtime_version, lemonade_path_in, lemonade_root, lemond_path_in, prepare_embeddable_with, runtime_dir_in, }; - /// The in-process extractor rejects every link kind and masks modes, so it - /// promises the whole contract, not just "no escape", whoever runs it. + /// The in-process extractor rejects every link kind and sets every file's + /// mode itself, so it promises the whole contract, not just "no escape". const FULL: Expect = Expect { modes: Modes::Safe, no_outward_links: true, @@ -2043,31 +2043,19 @@ mod archive_properties { /// extracted in full, into the root it was given. #[test] fn tar_and_zip_extraction_extract_a_benign_archive_into_the_root() { - run_property( - "lemonade-benign-tar", - 64, - benign_entries(), - None, - |layout, entries| { - let archive = layout.base.join("embeddable.tar.gz"); - std::fs::write(&archive, tar_gz_bytes(entries)).unwrap(); - extract_tar_gz(&archive, &layout.dest).map_err(|error| format!("{error:#}"))?; - check_round_trip(&layout.dest.join("top"), entries) - }, - ) + run_round_trip("lemonade-benign-tar", |layout, entries| { + let archive = layout.base.join("embeddable.tar.gz"); + std::fs::write(&archive, tar_gz_bytes(entries))?; + extract_tar_gz(&archive, &layout.dest)?; + Ok(layout.dest.join("top")) + }) .unwrap(); - run_property( - "lemonade-benign-zip", - 64, - benign_entries(), - None, - |layout, entries| { - let archive = layout.base.join("embeddable.zip"); - std::fs::write(&archive, zip_bytes(entries)).unwrap(); - extract_zip(&archive, &layout.dest).map_err(|error| format!("{error:#}"))?; - check_round_trip(&layout.dest.join("top"), entries) - }, - ) + run_round_trip("lemonade-benign-zip", |layout, entries| { + let archive = layout.base.join("embeddable.zip"); + std::fs::write(&archive, zip_bytes(entries))?; + extract_zip(&archive, &layout.dest)?; + Ok(layout.dest.join("top")) + }) .unwrap(); } From 635d93aff0ae824dfa95b0948f7dc651a498176b Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 9 Oct 2026 11:57:25 +0000 Subject: [PATCH 5/7] test: name the archive-props layout's fence field without an underscore It is assigned after construction, which clippy's used_underscore_binding rejects for an underscore-prefixed field. Signed-off-by: Roman Inflianskas --- crates/rocm-core/src/archive_props.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/rocm-core/src/archive_props.rs b/crates/rocm-core/src/archive_props.rs index a5bf036c4..d1b8073ab 100644 --- a/crates/rocm-core/src/archive_props.rs +++ b/crates/rocm-core/src/archive_props.rs @@ -479,7 +479,7 @@ impl Fence { pub struct Layout { // Declared first so the case directory goes before a fence it owns. _case: tempfile::TempDir, - _own_fence: Option, + own_fence: Option, fence_dirty: std::rc::Rc>, pub top: PathBuf, pub base: PathBuf, @@ -492,7 +492,7 @@ impl Layout { pub fn new() -> Self { let fence = Fence::new(); let mut layout = Self::in_fence(&fence); - layout._own_fence = Some(fence); + layout.own_fence = Some(fence); layout } @@ -520,7 +520,7 @@ impl Layout { write(&base.join("victim")); Self { _case: case, - _own_fence: None, + own_fence: None, fence_dirty: fence.dirty.clone(), top: fence.top.clone(), base, From f520cd3facdc1fb52410a5d86adcf26f2d023319 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 9 Oct 2026 12:11:21 +0000 Subject: [PATCH 6/7] test: judge archive-property modes through one tested table The previous round held files to "never world-writable" under any umask, on the premise that the in-process extractor sets every file's mode. It does not for mode-less entries, and the install path writes the version marker, a lock file and the copied archive through the umask, so the install property failed every case under umask 0. World-write is judged only while the umask clears it again, for every promise. Which checks a promise makes for a given umask and uid is now a pure function with a table test, since the live values cannot be injected and CI only ever sees one combination. Also: - the fence's bottom directory gets its mode explicitly, so the fence test's "modified above the case" arm holds under any umask; - the root replaced by a file or symlink is an escape, like its removal; - an unreadable symlink is reported rather than assumed inside, and the dangling-link test covers a nested link and absolute targets; - the calibration tests match the escape text only, not outward links; - a PROPTEST_CASES the property cannot honour is reported, not ignored; - run_case's and CaseOutcome's docs describe the whole-fence snapshot and the root checks. Signed-off-by: Roman Inflianskas --- crates/rocm-core/src/archive_props.rs | 256 +++++++++++++++++--------- crates/rocm-core/src/uv.rs | 7 +- engines/lemonade/src/install.rs | 5 +- 3 files changed, 171 insertions(+), 97 deletions(-) diff --git a/crates/rocm-core/src/archive_props.rs b/crates/rocm-core/src/archive_props.rs index d1b8073ab..b7532f6bb 100644 --- a/crates/rocm-core/src/archive_props.rs +++ b/crates/rocm-core/src/archive_props.rs @@ -444,6 +444,7 @@ pub struct Fence { impl Fence { pub fn new() -> Self { use std::os::unix::fs::DirBuilderExt as _; + use std::os::unix::fs::PermissionsExt as _; let temp = tempfile::Builder::new() .prefix("archive-props-") .tempdir() @@ -458,6 +459,9 @@ impl Fence { .mode(0o755) .create(&bottom) .unwrap(); + // The umask applies to `DirBuilder` too; the directory cases sit in + // gets its mode explicitly, like everything else a case is judged on. + fs::set_permissions(&bottom, fs::Permissions::from_mode(0o755)).unwrap(); Self { _temp: temp, top, @@ -623,14 +627,11 @@ pub struct Violations { pub outward_links: Vec, /// setuid/setgid entries in the installed tree, including the root. pub setid_modes: Vec, - /// World-writable files in the installed tree. - pub world_writable_files: Vec, - /// World-writable directories in the installed tree, including the root. - pub world_writable_dirs: Vec, + /// World-writable entries in the installed tree, including the root. + pub world_writable: Vec, } -fn check_mode(path: &Path, meta: &fs::Metadata, violations: &mut Violations) { - let mode = meta.mode(); +fn check_mode(path: &Path, mode: u32, violations: &mut Violations) { if mode & 0o6000 != 0 { violations.setid_modes.push(format!( "{} has setuid/setgid bits ({mode:o})", @@ -638,17 +639,15 @@ fn check_mode(path: &Path, meta: &fs::Metadata, violations: &mut Violations) { )); } if mode & 0o002 != 0 { - let found = format!("{} is world-writable ({mode:o})", path.display()); - if meta.is_dir() { - violations.world_writable_dirs.push(found); - } else { - violations.world_writable_files.push(found); - } + violations + .world_writable + .push(format!("{} is world-writable ({mode:o})", path.display())); } } -/// `path` with `.` and `..` resolved by text alone, the way a dangling link's -/// target would resolve once something exists there. +/// `path` with `.` and `..` resolved as text. For a dangling link's target +/// this is where it points if no link along the way redirects it; a link +/// earlier in the target is not followed, so this is an estimate. fn lexically_normal(path: &Path) -> PathBuf { let mut out = PathBuf::new(); for component in path.components() { @@ -678,7 +677,7 @@ pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { )); return violations; } - check_mode(root, &root_meta, &mut violations); + check_mode(root, root_meta.mode(), &mut violations); ensure_listable(root); let Ok(canonical_root) = fs::canonicalize(root) else { return violations; @@ -693,8 +692,14 @@ pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { if file_type.is_symlink() { // `canonicalize` resolves the whole chain on disk; a dangling // link is judged by its target read as text, relative to the - // directory the link is in. - let target = fs::read_link(&path).unwrap_or_default(); + // directory the link is in (`dir` is canonical). A link whose + // target cannot be read is reported rather than assumed safe. + let Ok(target) = fs::read_link(&path) else { + violations + .outward_links + .push(format!("symlink {} cannot be read", path.display())); + continue; + }; let lexical = lexically_normal(&dir.join(&target)); match fs::canonicalize(&path) { Ok(resolved) if !resolved.starts_with(&canonical_root) => { @@ -716,7 +721,7 @@ pub fn inspect_tree(root: &Path, outside_inodes: &[u64]) -> Violations { } continue; } - check_mode(&path, &meta, &mut violations); + check_mode(&path, meta.mode(), &mut violations); if file_type.is_file() && outside_inodes.contains(&meta.ino()) { violations.shared_inodes.push(format!( "{} is a hard link to a file outside the root", @@ -869,18 +874,19 @@ pub struct CaseOutcome { /// failure leaves behind is still judged. pub error: Option, /// Writes that landed outside the root: paths created, modified or - /// removed outside it, and hard links shared with outside files. + /// removed outside it, hard links shared with outside files, and the + /// extraction root itself removed or replaced by a non-directory. pub escapes: Vec, /// The state of the tree left inside the root. pub tree: Violations, } /// Write `archive_bytes` to `base/`, run `extract`, and judge -/// the result: nothing in the case's temporary directory outside -/// `inside_roots` (which must include the extraction root) may have changed, -/// and the tree under `inspect_root` is -/// walked by [`inspect_tree`]. `extract` receives the layout and the archive -/// path. +/// the result: nothing in the whole fence outside `inside_roots` (which must +/// include the extraction root) may have changed, `inspect_root` (which must +/// exist before the extraction) must still be a directory, and the tree under +/// it is walked by [`inspect_tree`]. `extract` receives the layout and the +/// archive path. pub fn run_case( layout: &Layout, archive_name: &str, @@ -929,11 +935,16 @@ pub fn run_case( { layout.fence_dirty.set(true); } - if fs::symlink_metadata(inspect_root).is_err() { - escapes.push(format!( + match fs::symlink_metadata(inspect_root) { + Err(_) => escapes.push(format!( "removed the extraction root itself: {}", inspect_root.display() - )); + )), + Ok(meta) if !meta.is_dir() => escapes.push(format!( + "replaced the extraction root with a non-directory: {}", + inspect_root.display() + )), + Ok(_) => {} } let mut tree = inspect_tree(inspect_root, &inodes); escapes.append(&mut tree.shared_inodes); @@ -948,21 +959,19 @@ pub fn run_case( /// /// No promise covers a world-writable entry the umask let through: under a /// umask that leaves world-write open (`umask 0`) that is what the user asked -/// for. What differs is which entries get their mode through the umask. +/// for, and every extractor here creates something through the umask (a +/// directory, the version marker, a lock file, the copied archive). So +/// world-write is judged only while the umask clears it. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum Modes { /// Not judged. Unchecked, - /// For an extractor that sets every file's mode itself: no setuid/setgid - /// entry and no world-writable file, whatever the umask or uid; - /// directories, which it creates through the umask, are judged only + /// No setuid/setgid entry, whoever runs it; no world-writable entry /// while the umask clears world-write. Safe, - /// For the system `tar`, which gives every entry its mode through the - /// umask and drops setuid/setgid only for an unprivileged user: no - /// setuid/setgid entry, and no world-writable entry while the umask - /// clears world-write. Run as root, GNU tar restores archived modes, - /// setuid included, so under effective uid 0 the modes are not judged. + /// [`Modes::Safe`] for the system `tar`, which drops setuid/setgid only + /// for an unprivileged user: run as root, GNU tar restores archived + /// modes, setuid included, so under effective uid 0 nothing is judged. SafeUnlessRoot, } @@ -983,41 +992,40 @@ fn process_umask_and_euid() -> (Option, Option) { (umask, euid) } +/// Which mode checks `modes` makes for a process with this umask and +/// effective uid, as (setuid/setgid judged, world-write judged). An unreadable +/// umask or uid is read strictly: judged, and not root. +fn mode_checks(modes: Modes, umask: Option, euid: Option) -> (bool, bool) { + let umask_clears_world_write = umask.is_none_or(|umask| umask & 0o002 != 0); + match modes { + Modes::Unchecked => (false, false), + Modes::SafeUnlessRoot if euid == Some(0) => (false, false), + Modes::Safe | Modes::SafeUnlessRoot => (true, umask_clears_world_write), + } +} + impl Modes { /// Why this run judges less than the whole promise, if it does. fn relaxed_because(self) -> Option { let (umask, euid) = process_umask_and_euid(); - match self { - Self::Unchecked => None, - Self::SafeUnlessRoot if euid == Some(0) => Some( + match mode_checks(self, umask, euid) { + _ if self == Self::Unchecked => None, + (false, _) => Some( "running as root, where the system tar restores archived modes: modes not judged" .to_owned(), ), - Self::Safe => umask.filter(|umask| umask & 0o002 == 0).map(|umask| { - format!( - "umask {umask:03o} leaves world-write open: world-writable directories not judged" - ) - }), - Self::SafeUnlessRoot => umask.filter(|umask| umask & 0o002 == 0).map(|umask| { - format!("umask {umask:03o} leaves world-write open: only setuid/setgid judged") - }), + (true, false) => Some(format!( + "umask {:03o} leaves world-write open: only setuid/setgid judged", + umask.unwrap_or_default() + )), + (true, true) => None, } } fn broken_by(self, tree: &Violations) -> bool { let (umask, euid) = process_umask_and_euid(); - match self { - Self::Unchecked => false, - Self::SafeUnlessRoot if euid == Some(0) => false, - Self::Safe | Self::SafeUnlessRoot => { - let umask_clears_world_write = umask.is_none_or(|umask| umask & 0o002 != 0); - let files_follow_umask = self == Self::SafeUnlessRoot; - !tree.setid_modes.is_empty() - || (!tree.world_writable_files.is_empty() - && (umask_clears_world_write || !files_follow_umask)) - || (umask_clears_world_write && !tree.world_writable_dirs.is_empty()) - } - } + let (setid, world_write) = mode_checks(self, umask, euid); + (setid && !tree.setid_modes.is_empty()) || (world_write && !tree.world_writable.is_empty()) } } @@ -1078,10 +1086,18 @@ pub fn run_property( // would otherwise override) scales every property up for a longer hunt. // Never down: the reach floors below are percentages, which a handful of // cases cannot meet. - let cases = std::env::var("PROPTEST_CASES") - .ok() - .and_then(|value| value.parse().ok()) - .map_or(cases, |requested: u32| requested.max(cases)); + let requested = std::env::var("PROPTEST_CASES").ok(); + let cases = match requested.as_deref().map(str::parse::) { + Some(Ok(requested)) if requested >= cases => requested, + None => cases, + Some(_) => { + eprintln!( + "[{label}] PROPTEST_CASES={} ignored: this property runs at least {cases} cases", + requested.as_deref().unwrap_or_default() + ); + cases + } + }; let mut config = proptest::test_runner::Config { cases, failure_persistence: None, @@ -1230,40 +1246,96 @@ fn a_fence_is_replaced_after_a_case_changes_it_and_only_then() { } } -/// A dangling symlink is judged by where its target would be, relative to -/// the link: `../../x` from the root leaves it even though nothing is there -/// yet, while a dangling link to a name inside the root does not. +/// A dangling symlink is judged by where its target would be, read as text +/// relative to the directory the link is in: `../x` from a subdirectory stays +/// inside the root, `../../x` from the same place leaves it, and so does an +/// absolute target outside the root, even though nothing is there yet. #[test] fn a_dangling_link_is_judged_by_where_it_would_resolve() { let layout = Layout::new(); - std::os::unix::fs::symlink("../../not-there", layout.dest.join("out")).unwrap(); - std::os::unix::fs::symlink("not-there-either", layout.dest.join("in")).unwrap(); + let sub = layout.dest.join("sub"); + fs::create_dir(&sub).unwrap(); + let link = |target: &Path, name: &str| std::os::unix::fs::symlink(target, sub.join(name)); + link(Path::new("../still-inside"), "in").unwrap(); + link(Path::new("../../not-there"), "up").unwrap(); + link(&layout.outside.join("not-there"), "abs").unwrap(); + link(&layout.dest.join("not-there"), "abs-in").unwrap(); let tree = inspect_tree(&layout.dest, &[]); - assert_eq!(tree.outward_links.len(), 1, "{tree:#?}"); - assert!(tree.outward_links[0].contains("/out "), "{tree:#?}"); + let mut flagged: Vec<&str> = ["in", "up", "abs", "abs-in"] + .into_iter() + .filter(|name| { + let needle = format!("/sub/{name} "); + tree.outward_links + .iter() + .any(|found| found.contains(&needle)) + }) + .collect(); + flagged.sort_unstable(); + assert_eq!(flagged, ["abs", "up"], "{tree:#?}"); } -/// An extraction that deletes its own root has not stayed inside it, even -/// though nothing outside the root changed. +/// An extraction that deletes its own root, or swaps it for a file or a +/// symlink, has not stayed inside it, even though nothing outside the root +/// changed. #[test] -fn removing_the_extraction_root_is_an_escape() { - let layout = Layout::new(); - let outcome = run_case( - &layout, - "a", - b"", - &[&layout.dest], - &layout.dest, - |layout, _| { - fs::remove_dir(&layout.dest)?; - Ok(()) - }, - ); - assert!( - outcome - .escapes - .iter() - .any(|escape| escape.contains("removed the extraction root")), - "{outcome:#?}" - ); +fn removing_or_replacing_the_extraction_root_is_an_escape() { + for (what, expected) in [ + ("remove", "removed the extraction root"), + ("file", "replaced the extraction root"), + ("symlink", "replaced the extraction root"), + ] { + let layout = Layout::new(); + let outcome = run_case( + &layout, + "a", + b"", + &[&layout.dest], + &layout.dest, + |layout, _| { + fs::remove_dir(&layout.dest)?; + match what { + "file" => fs::write(&layout.dest, b"x")?, + "symlink" => std::os::unix::fs::symlink(&layout.outside, &layout.dest)?, + _ => {} + } + Ok(()) + }, + ); + assert!( + outcome + .escapes + .iter() + .any(|escape| escape.contains(expected)), + "{what}: {outcome:#?}" + ); + } +} + +/// Which mode checks each promise makes, for every umask and uid that +/// changes the answer. The checks read the live process, so this pins the +/// table they are read through. +#[test] +fn mode_checks_follow_the_umask_and_the_uid() { + let (open, closed, unknown) = (Some(0o000), Some(0o022), None); + let (root, user) = (Some(0), Some(1000)); + // (promise, umask, euid) -> (setuid/setgid judged, world-write judged) + let table = [ + (Modes::Unchecked, closed, user, (false, false)), + (Modes::Unchecked, open, root, (false, false)), + (Modes::Safe, closed, user, (true, true)), + (Modes::Safe, open, user, (true, false)), + (Modes::Safe, closed, root, (true, true)), + (Modes::Safe, unknown, unknown, (true, true)), + (Modes::SafeUnlessRoot, closed, user, (true, true)), + (Modes::SafeUnlessRoot, open, user, (true, false)), + (Modes::SafeUnlessRoot, closed, root, (false, false)), + (Modes::SafeUnlessRoot, unknown, unknown, (true, true)), + ]; + for (modes, umask, euid, expected) in table { + assert_eq!( + mode_checks(modes, umask, euid), + expected, + "{modes:?} umask {umask:?} euid {euid:?}" + ); + } } diff --git a/crates/rocm-core/src/uv.rs b/crates/rocm-core/src/uv.rs index 9ba669296..d6c2e6e3f 100644 --- a/crates/rocm-core/src/uv.rs +++ b/crates/rocm-core/src/uv.rs @@ -1470,8 +1470,9 @@ mod archive_properties { fn generator_catches_a_traversing_extractor() { let error = property("naive", ESCAPES_ONLY, naive_unpack) .expect_err("an extractor that honours `..` must be caught"); - // Caught for writing outside the root, not for some harness failure. - assert!(error.contains("outside the root"), "{error}"); + // Caught for a write outside the root (an escape, not a harness + // failure or a link the escape check does not judge). + assert!(error.contains("outside the root: "), "{error}"); } /// Calibration for the two-step link shape: an extractor that sanitises @@ -1481,6 +1482,6 @@ mod archive_properties { fn generator_catches_a_link_following_extractor() { let error = property("naive-links", ESCAPES_ONLY, naive_sanitizing_unpack) .expect_err("an extractor that writes through planted links must be caught"); - assert!(error.contains("outside the root"), "{error}"); + assert!(error.contains("outside the root: "), "{error}"); } } diff --git a/engines/lemonade/src/install.rs b/engines/lemonade/src/install.rs index 98cd8fd00..32687b24a 100644 --- a/engines/lemonade/src/install.rs +++ b/engines/lemonade/src/install.rs @@ -1979,8 +1979,9 @@ mod archive_properties { lemonade_root, lemond_path_in, prepare_embeddable_with, runtime_dir_in, }; - /// The in-process extractor rejects every link kind and sets every file's - /// mode itself, so it promises the whole contract, not just "no escape". + /// The in-process extractor rejects every link kind and masks the modes + /// the archive carries, so it promises the whole contract, not just "no + /// escape". const FULL: Expect = Expect { modes: Modes::Safe, no_outward_links: true, From 3190c9a7c5e02ca26399aeca0919d2ebd534ec18 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Mon, 12 Oct 2026 07:10:41 +0000 Subject: [PATCH 7/7] build(rocm): keep one [dev-dependencies] table after the rebase Main now declares proptest in apps/rocm's dev-dependencies itself, so this branch only adds tempfile there. Signed-off-by: Roman Inflianskas --- apps/rocm/Cargo.toml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/apps/rocm/Cargo.toml b/apps/rocm/Cargo.toml index b4420fbb7..20ef18f17 100644 --- a/apps/rocm/Cargo.toml +++ b/apps/rocm/Cargo.toml @@ -46,11 +46,6 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } unicode-width = "0.2" ureq = { version = "2.12", features = ["native-certs"] } -[dev-dependencies] -# Archive-extraction property tests; see `crates/rocm-core/src/archive_props.rs`. -proptest = { version = "1", default-features = false, features = ["std"] } -tempfile = "3" - [target.'cfg(unix)'.dependencies] libc.workspace = true @@ -73,3 +68,4 @@ zbus-secret-service-keyring-store = { version = "1.0", features = ["rt-tokio-cry # Test-only, so it adds nothing to any shipped binary. # Also used for the recursive-deletion paths in deletion_properties. proptest = { version = "1", default-features = false, features = ["std"] } +tempfile = "3"