From b8baab758fcb03cd52012545d4dfd4e2dedb00d5 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 01:34:29 +0000 Subject: [PATCH 01/12] feat(suidhelper): security_gate dual-gated test-seam mechanism --- native/suidhelper/Cargo.toml | 9 +++++ native/suidhelper/src/lib.rs | 1 + native/suidhelper/src/main.rs | 6 ++- native/suidhelper/src/security_gate.rs | 51 ++++++++++++++++++++++++ native/suidhelper/tests/security_gate.rs | 50 +++++++++++++++++++++++ 5 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 native/suidhelper/src/security_gate.rs create mode 100644 native/suidhelper/tests/security_gate.rs diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index b0ac1c4..29c1fef 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -6,6 +6,11 @@ description = "Tiny setuid-root helper that runs a whitelisted set of device com license = "AGPL-3.0-or-later" repository = "https://github.com/harmont-dev/hyper" +[features] +# TEST ONLY. Compiles the security_gate insecure arms. Never enable for a real +# (setuid) build — build.rs refuses to compile a release binary with this on. +insecure_test_seams = [] + [[bin]] name = "hyper-suidhelper" path = "src/main.rs" @@ -21,6 +26,10 @@ path = "tests/util/safe_path.rs" name = "safe_dev" path = "tests/util/safe_dev.rs" +[[test]] +name = "security_gate" +path = "tests/security_gate.rs" + [dependencies] clap = { version = "4", features = ["derive"] } nix = { version = "0.29", features = ["user", "fs", "dir"] } diff --git a/native/suidhelper/src/lib.rs b/native/suidhelper/src/lib.rs index e34e970..ecd09f7 100644 --- a/native/suidhelper/src/lib.rs +++ b/native/suidhelper/src/lib.rs @@ -8,5 +8,6 @@ //! point over these modules. pub mod config; +pub mod security_gate; pub mod tools; pub mod util; diff --git a/native/suidhelper/src/main.rs b/native/suidhelper/src/main.rs index 05732dd..4498d32 100644 --- a/native/suidhelper/src/main.rs +++ b/native/suidhelper/src/main.rs @@ -66,10 +66,12 @@ impl SysTest { } fn main() { + // Resolve the security gate before anything else reads it (the config load + // below consults it). In release this is a no-op: the gate stays secure. + hyper_suidhelper::security_gate::init(); + // Privileges are already dropped to the real uid by a pre-main constructor // (see `setuid_privileged`); root is only re-acquired inside `Privileged`. - // Load the config now - we are post-drop (real uid) and before any - // `Privileged` scope, so the file is read unprivileged, never as root. config::Config::init(); // Each command yields a serializable value (errors stringified to unify); we diff --git a/native/suidhelper/src/security_gate.rs b/native/suidhelper/src/security_gate.rs new file mode 100644 index 0000000..ad05601 --- /dev/null +++ b/native/suidhelper/src/security_gate.rs @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: AGPL-3.0-only +//! Central security gate: the single place that decides whether this process is +//! running in INSECURE TEST MODE, resolved once at startup. +//! +//! Two independent gates, both required to open the insecure path: +//! 1. compile-time — the `insecure_test_seams` Cargo feature. Never enabled in +//! release; `build.rs` refuses a release build with it on. +//! 2. runtime — `HYPER_SETUIDHELPER_IS_INSECURE_MODE=1` in the environment. +//! +//! Seams never test these directly. They call [`split`], which pairs every +//! insecure branch with its production default and can only run the insecure arm +//! when BOTH gates are open. The default — uninitialized, or any release build — +//! is always the secure arm, so a forgotten `init` fails safe. + +use std::sync::atomic::{AtomicBool, Ordering}; + +const INSECURE_MODE_ENV: &str = "HYPER_SETUIDHELPER_IS_INSECURE_MODE"; + +/// Resolved once by [`init`]; defaults to secure so a missing `init` is safe. +static INSECURE: AtomicBool = AtomicBool::new(false); + +/// Resolve the gate from the compile feature and the environment. Call once, as +/// the very first thing in `main`, before any seam (e.g. the config load) runs. +/// Idempotent. +pub fn init() { + let insecure = cfg!(feature = "insecure_test_seams") && env_opts_in(); + if insecure { + eprintln!( + "hyper-suidhelper: WARNING: running in INSECURE TEST MODE \ + ({INSECURE_MODE_ENV}=1); never do this on a real host" + ); + } + INSECURE.store(insecure, Ordering::SeqCst); +} + +fn env_opts_in() -> bool { + std::env::var(INSECURE_MODE_ENV).as_deref() == Ok("1") +} + +/// Run `secure` in production; run `insecure` only when BOTH gates are open. +/// +/// The `cfg!(feature = ...)` is a compile-time constant: in any build without +/// the feature it folds to `false`, so the whole condition is constant-false and +/// the optimizer drops the `insecure` branch entirely. +pub fn split(secure: impl FnOnce() -> T, insecure: impl FnOnce() -> T) -> T { + if cfg!(feature = "insecure_test_seams") && INSECURE.load(Ordering::SeqCst) { + insecure() + } else { + secure() + } +} diff --git a/native/suidhelper/tests/security_gate.rs b/native/suidhelper/tests/security_gate.rs new file mode 100644 index 0000000..6b94e8d --- /dev/null +++ b/native/suidhelper/tests/security_gate.rs @@ -0,0 +1,50 @@ +//! Gate-resolution contract for `security_gate`. nextest runs each test in its +//! own process, so the env mutations and the process-global `INSECURE` flag do +//! not race across tests. + +use hyper_suidhelper::security_gate; + +const ENV: &str = "HYPER_SETUIDHELPER_IS_INSECURE_MODE"; + +// Without the feature compiled in, the insecure arm is unreachable no matter +// what the environment says — this is the production guarantee. +#[cfg(not(feature = "insecure_test_seams"))] +#[test] +fn secure_arm_always_taken_without_feature() { + std::env::set_var(ENV, "1"); + security_gate::init(); + assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); +} + +// With the feature, the env var is still required: feature alone is not enough. +#[cfg(feature = "insecure_test_seams")] +#[test] +fn secure_arm_when_env_absent_even_with_feature() { + std::env::remove_var(ENV); + security_gate::init(); + assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); +} + +// Both gates open → insecure arm. +#[cfg(feature = "insecure_test_seams")] +#[test] +fn insecure_arm_when_feature_and_env() { + std::env::set_var(ENV, "1"); + security_gate::init(); + assert_eq!(security_gate::split(|| "secure", || "insecure"), "insecure"); +} + +// A wrong env value is not opt-in. +#[cfg(feature = "insecure_test_seams")] +#[test] +fn secure_arm_when_env_not_exactly_one() { + std::env::set_var(ENV, "true"); + security_gate::init(); + assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); +} + +// Default (init never called) is secure. +#[test] +fn secure_arm_before_init() { + assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); +} From 9a37a6965e7e6c3e7757a75f99e496ec4dcba58a Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 01:37:42 +0000 Subject: [PATCH 02/12] build(suidhelper): refuse release builds carrying insecure_test_seams --- native/suidhelper/build.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 native/suidhelper/build.rs diff --git a/native/suidhelper/build.rs b/native/suidhelper/build.rs new file mode 100644 index 0000000..c3de1f5 --- /dev/null +++ b/native/suidhelper/build.rs @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: AGPL-3.0-only +//! Build guard: keep the `insecure_test_seams` feature out of any release +//! artifact. Tests build in the `debug` profile, so they are unaffected. + +fn main() { + // Re-run only when the relevant inputs change. + println!("cargo:rerun-if-changed=build.rs"); + + if std::env::var_os("CARGO_FEATURE_INSECURE_TEST_SEAMS").is_some() { + println!( + "cargo:warning=hyper-suidhelper built with `insecure_test_seams` \ + — TEST ONLY. Never install this binary setuid." + ); + if std::env::var("PROFILE").as_deref() == Ok("release") { + panic!( + "refusing to build a RELEASE binary with `insecure_test_seams`: \ + it would ship a setuid security bypass" + ); + } + } +} From 4a645866f102df626dd6ad06f3a6f8fb052e9c3e Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 01:42:03 +0000 Subject: [PATCH 03/12] feat(suidhelper): config-path override behind the security gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route the config file path through security_gate::split so subprocess tests can redirect it via HYPER_SETUIDHELPER_CONFIG_PATH — only when both gates are open (insecure_test_seams feature + env var). Production builds always read /etc/hyper/config.toml with full SafeFile checks. Widened LoadingError path variants from &'static Path to PathBuf; dropped the Copy derive and removed the now-unused CONFIG_PATH static. --- native/suidhelper/Cargo.toml | 4 ++ native/suidhelper/src/config.rs | 54 ++++++++++++++++++-------- native/suidhelper/tests/config_seam.rs | 23 +++++++++++ 3 files changed, 65 insertions(+), 16 deletions(-) create mode 100644 native/suidhelper/tests/config_seam.rs diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index 29c1fef..b6e9963 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -30,6 +30,10 @@ path = "tests/util/safe_dev.rs" name = "security_gate" path = "tests/security_gate.rs" +[[test]] +name = "config_seam" +path = "tests/config_seam.rs" + [dependencies] clap = { version = "4", features = ["derive"] } nix = { version = "0.29", features = ["user", "fs", "dir"] } diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index d8dfbb4..bc94c1f 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -5,26 +5,40 @@ use crate::util::safe_file::{self, IsRegularFile, OnlyRootWritable, RootOwner, S use crate::util::safe_path::{self, IsAbsolute, SafePath, StrictComponents}; use nix::fcntl::OFlag; use serde::Deserialize; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::sync::LazyLock; use thiserror::Error; -#[derive(Debug, Copy, Clone, Error)] +#[derive(Debug, Clone, Error)] pub enum LoadingError { #[error(transparent)] Path(#[from] safe_path::ValidationError), #[error(transparent)] File(#[from] safe_file::ValidationError), #[error("{0:?} could not be read")] - Unreadable(&'static Path), + Unreadable(PathBuf), #[error("{0:?} is not valid TOML")] - Malformed(&'static Path), + Malformed(PathBuf), #[error("work_dir in {0:?} must be an absolute path")] - Relative(&'static Path), + Relative(PathBuf), } const CONFIG_PATHSTR: &str = "/etc/hyper/config.toml"; -static CONFIG_PATH: LazyLock = LazyLock::new(|| PathBuf::from(CONFIG_PATHSTR)); +const INSECURE_CONFIG_PATH_ENV: &str = "HYPER_SETUIDHELPER_CONFIG_PATH"; + +/// The config file path. In production this is the fixed `/etc/hyper/config.toml`. +/// Only in INSECURE TEST MODE (both gates open) may an env var redirect it — the +/// secure arm is always the hardcoded path, so a release build cannot be steered. +fn config_path() -> PathBuf { + crate::security_gate::split( + || PathBuf::from(CONFIG_PATHSTR), + || { + std::env::var(INSECURE_CONFIG_PATH_ENV) + .map(PathBuf::from) + .unwrap_or_else(|_| PathBuf::from(CONFIG_PATHSTR)) + }, + ) +} /// Hyper's /etc/hyper/config.toml file format. #[derive(Debug, Clone, Deserialize)] @@ -51,7 +65,7 @@ impl Config { } /// Hyper's data root. - pub fn hyper_base(&self) -> &Path { + pub fn hyper_base(&self) -> &std::path::Path { self.work_dir.as_path() } @@ -63,17 +77,25 @@ impl Config { /// Read, ownership-check, parse, and validate the config file. See the module /// docs for the trust model. pub fn safe_load() -> Result { - let path = CONFIG_PATH.as_path(); - - let safe_path: SafePath = - PathBuf::from(CONFIG_PATHSTR).try_into()?; + let path = config_path(); - let file: SafeFile = - SafeFile::open(&safe_path, OFlag::O_RDONLY)?; + let body = crate::security_gate::split( + || -> Result { + let safe_path: SafePath = + path.clone().try_into()?; + let file: SafeFile = + SafeFile::open(&safe_path, OFlag::O_RDONLY)?; + std::io::read_to_string(std::fs::File::from(file.into_owned_fd())) + .map_err(|_| LoadingError::Unreadable(path.clone())) + }, + || -> Result { + std::fs::read_to_string(&path) + .map_err(|_| LoadingError::Unreadable(path.clone())) + }, + )?; - let body = std::io::read_to_string(std::fs::File::from(file.into_owned_fd())) - .map_err(|_| LoadingError::Unreadable(path))?; - let config: Config = toml::from_str(&body).map_err(|_| LoadingError::Malformed(path))?; + let config: Config = + toml::from_str(&body).map_err(|_| LoadingError::Malformed(path.clone()))?; if !config.work_dir.is_absolute() { return Err(LoadingError::Relative(path)); diff --git a/native/suidhelper/tests/config_seam.rs b/native/suidhelper/tests/config_seam.rs new file mode 100644 index 0000000..d5529ad --- /dev/null +++ b/native/suidhelper/tests/config_seam.rs @@ -0,0 +1,23 @@ +//! The config-path seam routes through security_gate::split. With both gates +//! open, safe_load consults HYPER_SETUIDHELPER_CONFIG_PATH. This negative case +//! needs no root: pointing at an absent file yields `Unreadable()`, +//! proving the override path — not /etc/hyper — was used. +#![cfg(feature = "insecure_test_seams")] + +use hyper_suidhelper::config::{Config, LoadingError}; +use hyper_suidhelper::security_gate; +use std::path::PathBuf; + +#[test] +fn override_path_is_consulted_when_gates_open() { + let missing = "/tmp/hyper-suidhelper-no-such-config-42.toml"; + std::env::set_var("HYPER_SETUIDHELPER_IS_INSECURE_MODE", "1"); + std::env::set_var("HYPER_SETUIDHELPER_CONFIG_PATH", missing); + security_gate::init(); + + let err = Config::safe_load().expect_err("absent override file must fail"); + assert!( + matches!(err, LoadingError::Unreadable(ref p) if p == &PathBuf::from(missing)), + "expected Unreadable({missing:?}), got {err:?}", + ); +} From 97b2d453bcefee6d6b12523db5847b7cb85a1c78 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 01:46:34 +0000 Subject: [PATCH 04/12] fix(suidhelper): keep config ownership checks enforced in test seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the inner security_gate::split around the SafeFile read in Config::safe_load. The seam was incorrectly bypassing SafePath lexical and SafeFile ownership/type/writability checks in insecure test mode. Now only config_path() is gated — the path changes, but validation is identical in both modes. Update config_seam test to prove the override path is consulted via the real lexical gate: a .. component in the override is rejected as LooseComponents, which cannot come from the clean hardcoded default, confirming the seam without bypassing any security check. --- native/suidhelper/src/config.rs | 20 ++++++-------------- native/suidhelper/tests/config_seam.rs | 23 +++++++++++++---------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index bc94c1f..425ba38 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -79,21 +79,13 @@ impl Config { pub fn safe_load() -> Result { let path = config_path(); - let body = crate::security_gate::split( - || -> Result { - let safe_path: SafePath = - path.clone().try_into()?; - let file: SafeFile = - SafeFile::open(&safe_path, OFlag::O_RDONLY)?; - std::io::read_to_string(std::fs::File::from(file.into_owned_fd())) - .map_err(|_| LoadingError::Unreadable(path.clone())) - }, - || -> Result { - std::fs::read_to_string(&path) - .map_err(|_| LoadingError::Unreadable(path.clone())) - }, - )?; + let safe_path: SafePath = path.clone().try_into()?; + let file: SafeFile = + SafeFile::open(&safe_path, OFlag::O_RDONLY)?; + + let body = std::io::read_to_string(std::fs::File::from(file.into_owned_fd())) + .map_err(|_| LoadingError::Unreadable(path.clone()))?; let config: Config = toml::from_str(&body).map_err(|_| LoadingError::Malformed(path.clone()))?; diff --git a/native/suidhelper/tests/config_seam.rs b/native/suidhelper/tests/config_seam.rs index d5529ad..75b6b35 100644 --- a/native/suidhelper/tests/config_seam.rs +++ b/native/suidhelper/tests/config_seam.rs @@ -1,23 +1,26 @@ //! The config-path seam routes through security_gate::split. With both gates -//! open, safe_load consults HYPER_SETUIDHELPER_CONFIG_PATH. This negative case -//! needs no root: pointing at an absent file yields `Unreadable()`, -//! proving the override path — not /etc/hyper — was used. +//! open, safe_load reads HYPER_SETUIDHELPER_CONFIG_PATH instead of the hardcoded +//! /etc/hyper/config.toml — but with every production check (SafePath lexical +//! gate, SafeFile ownership/type) still enforced. A `..` override is rejected by +//! the real SafePath gate as LooseComponents; the default path is lexically +//! clean, so that error proves the override (not the default) was used. No root +//! needed and no security check is bypassed. #![cfg(feature = "insecure_test_seams")] use hyper_suidhelper::config::{Config, LoadingError}; use hyper_suidhelper::security_gate; -use std::path::PathBuf; +use hyper_suidhelper::util::safe_path::ValidationError; #[test] -fn override_path_is_consulted_when_gates_open() { - let missing = "/tmp/hyper-suidhelper-no-such-config-42.toml"; +fn override_path_is_consulted_through_the_real_lexical_gate() { + let loose = "/tmp/hyper-suidhelper/../escape.toml"; std::env::set_var("HYPER_SETUIDHELPER_IS_INSECURE_MODE", "1"); - std::env::set_var("HYPER_SETUIDHELPER_CONFIG_PATH", missing); + std::env::set_var("HYPER_SETUIDHELPER_CONFIG_PATH", loose); security_gate::init(); - let err = Config::safe_load().expect_err("absent override file must fail"); + let err = Config::safe_load().expect_err("override must be consulted"); assert!( - matches!(err, LoadingError::Unreadable(ref p) if p == &PathBuf::from(missing)), - "expected Unreadable({missing:?}), got {err:?}", + matches!(err, LoadingError::Path(ValidationError::LooseComponents)), + "expected the override path to hit the real lexical gate, got {err:?}", ); } From 9443de3a8c5884f46ececaf69eca60e064194af0 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 01:53:52 +0000 Subject: [PATCH 05/12] test(suidhelper): cover SafeBin / DmTable / ThinMessage parsers Add integration tests for the three security-critical operand parsers that were previously untested. SafeBin tests cover all five refusal axes (relative path, wrong basename, symlink, non-root owner, group/other-writable); root- only axes self-skip when running unprivileged. DmTable tests assert the full accept+round-trip contract (canonical forms, whitespace normalisation) and the reject matrix (non-whitelisted targets, arbitrary devices, bad flags, wrong arity). ThinMessage tests cover the two whitelisted verbs with normalisation and the reject set (unknown verbs, missing/extra args, non-numeric id). Also exposes DmTable and ThinMessage via pub use from the tools module so integration tests can import them without reaching into internal submodules. Adds tempfile = "3" dev-dependency for root-independent filesystem fixtures. --- native/suidhelper/Cargo.lock | 1 + native/suidhelper/Cargo.toml | 9 +++ native/suidhelper/src/tools/dmsetup/mod.rs | 4 +- native/suidhelper/src/tools/mod.rs | 2 +- .../suidhelper/tests/tools/dmsetup_parsers.rs | 70 +++++++++++++++++++ native/suidhelper/tests/util/safe_bin.rs | 66 +++++++++++++++++ 6 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 native/suidhelper/tests/tools/dmsetup_parsers.rs create mode 100644 native/suidhelper/tests/util/safe_bin.rs diff --git a/native/suidhelper/Cargo.lock b/native/suidhelper/Cargo.lock index 86d964a..18d084f 100644 --- a/native/suidhelper/Cargo.lock +++ b/native/suidhelper/Cargo.lock @@ -209,6 +209,7 @@ dependencies = [ "proptest", "serde", "serde_json", + "tempfile", "thiserror", "toml", ] diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index b6e9963..c468223 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -34,6 +34,14 @@ path = "tests/security_gate.rs" name = "config_seam" path = "tests/config_seam.rs" +[[test]] +name = "util_safe_bin" +path = "tests/util/safe_bin.rs" + +[[test]] +name = "tools_dmsetup_parsers" +path = "tests/tools/dmsetup_parsers.rs" + [dependencies] clap = { version = "4", features = ["derive"] } nix = { version = "0.29", features = ["user", "fs", "dir"] } @@ -44,6 +52,7 @@ toml = { version = "0.8", default-features = false, features = ["parse"] } [dev-dependencies] proptest = "1" +tempfile = "3" [profile.release] strip = true diff --git a/native/suidhelper/src/tools/dmsetup/mod.rs b/native/suidhelper/src/tools/dmsetup/mod.rs index 9acc069..eef016a 100644 --- a/native/suidhelper/src/tools/dmsetup/mod.rs +++ b/native/suidhelper/src/tools/dmsetup/mod.rs @@ -7,12 +7,12 @@ mod thin_pool; use super::IsTool; use crate::util::safe_dev::{self, DmName}; use clap::{Args, Subcommand}; -use message::ThinMessage; +pub use message::ThinMessage; use serde::Serialize; use std::io; use std::path::PathBuf; use std::process::{Command, Output}; -use table::DmTable; +pub use table::DmTable; use thiserror::Error as ThisError; #[derive(Debug, ThisError)] diff --git a/native/suidhelper/src/tools/mod.rs b/native/suidhelper/src/tools/mod.rs index 4ce0bb3..45f3331 100644 --- a/native/suidhelper/src/tools/mod.rs +++ b/native/suidhelper/src/tools/mod.rs @@ -10,7 +10,7 @@ mod losetup; pub use blockdev::{Blockdev, BlockdevArgs}; pub use chroot_jail::ChrootJailOp; -pub use dmsetup::{Dmsetup, DmsetupArgs}; +pub use dmsetup::{DmTable, Dmsetup, DmsetupArgs, ThinMessage}; pub use losetup::{Losetup, LosetupArgs}; use crate::util::safe_bin::SafeBin; diff --git a/native/suidhelper/tests/tools/dmsetup_parsers.rs b/native/suidhelper/tests/tools/dmsetup_parsers.rs new file mode 100644 index 0000000..0a63c6a --- /dev/null +++ b/native/suidhelper/tests/tools/dmsetup_parsers.rs @@ -0,0 +1,70 @@ +//! `DmTable`/`ThinMessage` are reconstructed from the caller's string and then +//! re-rendered, so dmsetup only ever sees a table/message we rebuilt. The accept +//! set must round-trip to a canonical single-spaced form; the reject set covers +//! non-whitelisted targets, arbitrary (non loop / non hyper-) devices, and +//! malformed arity. These are the contract that keeps dmsetup off arbitrary +//! storage. + +use hyper_suidhelper::tools::{DmTable, ThinMessage}; + +#[test] +fn accepts_canonical_tables_and_round_trips() { + for s in [ + "0 100 snapshot /dev/loop0 /dev/loop1 P 8", + "0 100 thin-pool /dev/loop0 /dev/loop1 128 1024", + "0 100 thin /dev/mapper/hyper-pool 0", + "0 100 thin /dev/mapper/hyper-pool 0 /dev/mapper/hyper-orig", + ] { + let t = s.parse::().unwrap_or_else(|_| panic!("rejected {s:?}")); + assert_eq!(t.to_string(), s, "round-trip changed {s:?}"); + } +} + +#[test] +fn normalizes_inner_whitespace_on_render() { + let weird = "0 100 snapshot /dev/loop0 /dev/loop1 P 8"; + let canonical = "0 100 snapshot /dev/loop0 /dev/loop1 P 8"; + let t = weird.parse::().expect("weird spacing must still parse"); + assert_eq!(t.to_string(), canonical); +} + +#[test] +fn rejects_non_whitelisted_targets_and_devices() { + for bad in [ + "0 100 linear /dev/loop0 0", // target not allowed + "0 100 crypt /dev/loop0 /dev/loop1", // target not allowed + "0 100 snapshot /dev/sda /dev/loop1 P 8", // origin = arbitrary storage + "0 100 snapshot /dev/loop0 /dev/sda P 8", // cow = arbitrary storage + "0 100 snapshot /dev/loop0 /dev/loop1 X 8", // bad persistence flag + "1 100 snapshot /dev/loop0 /dev/loop1 P 8", // start != 0 + "0 100 snapshot /dev/loop0 /dev/loop1 P", // too few fields + "0 100 thin-pool /dev/sda /dev/loop1 128 1024", // meta = arbitrary + "0 100 thin /dev/sda 0", // pool = arbitrary + "", // empty + "garbage", // junk + ] { + assert!(bad.parse::().is_err(), "DmTable accepted {bad:?}"); + } +} + +#[test] +fn thinmessage_accepts_whitelisted_and_normalizes() { + for (s, canon) in [ + ("create_thin 7", "create_thin 7"), + ("create_thin 7", "create_thin 7"), + ("delete 3", "delete 3"), + ] { + let m = s.parse::().unwrap_or_else(|_| panic!("rejected {s:?}")); + assert_eq!(m.to_string(), canon, "round-trip changed {s:?}"); + } +} + +#[test] +fn thinmessage_rejects_non_whitelisted() { + for bad in [ + "resize 10", "create_thin", "delete", "delete x", + "create_thin 1 2", "", "create_thin -1", + ] { + assert!(bad.parse::().is_err(), "ThinMessage accepted {bad:?}"); + } +} diff --git a/native/suidhelper/tests/util/safe_bin.rs b/native/suidhelper/tests/util/safe_bin.rs new file mode 100644 index 0000000..88ccc4e --- /dev/null +++ b/native/suidhelper/tests/util/safe_bin.rs @@ -0,0 +1,66 @@ +//! `SafeBin` is what stops `--bin` from pointing the helper at an +//! attacker-controlled binary it would then run as root. The constructor demands +//! an absolute path, exact basename, a real (non-symlink) regular file owned by +//! root and not group/other-writable. These assert the refusal axes; the symlink +//! axis is root-independent, the owner axis is asserted both ways. + +use hyper_suidhelper::util::safe_bin::SafeBin; +use std::fs; +use std::os::unix::fs::{symlink, PermissionsExt}; + +fn is_root() -> bool { + nix::unistd::geteuid().is_root() +} + +#[test] +fn rejects_relative_path() { + assert!("losetup".parse::>().is_err()); + assert!("./losetup".parse::>().is_err()); +} + +#[test] +fn rejects_wrong_basename() { + // An absolute, existing, root-owned system file with the WRONG basename fails. + assert!("/usr/bin/env".parse::>().is_err()); +} + +#[test] +fn rejects_symlink_with_correct_basename() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("target"); + fs::write(&target, b"x").unwrap(); + let link = dir.path().join("losetup"); + symlink(&target, &link).unwrap(); + // Symlink is checked before ownership, so this holds regardless of who runs it. + assert!(link.to_str().unwrap().parse::>().is_err()); +} + +#[test] +fn owner_axis_root_owned_accepted_else_rejected() { + let dir = tempfile::tempdir().unwrap(); + let f = dir.path().join("losetup"); + fs::write(&f, b"#!/bin/true\n").unwrap(); + fs::set_permissions(&f, fs::Permissions::from_mode(0o755)).unwrap(); + let got = f.to_str().unwrap().parse::>(); + if is_root() { + // root-owned, 0755, absolute, correct basename, not a symlink → valid. + assert!(got.is_ok(), "root-owned valid bin rejected: {got:?}"); + } else { + // We own it (uid != 0) → NotRoot. + assert!(got.is_err(), "non-root-owned bin accepted"); + } +} + +#[test] +fn rejects_group_or_other_writable() { + if is_root() { + let dir = tempfile::tempdir().unwrap(); + let f = dir.path().join("losetup"); + fs::write(&f, b"x").unwrap(); + // root-owned but group/other-writable → Writable rejection. + fs::set_permissions(&f, fs::Permissions::from_mode(0o757)).unwrap(); + assert!(f.to_str().unwrap().parse::>().is_err()); + } else { + eprintln!("SKIP rejects_group_or_other_writable: needs root to own a 0757 file"); + } +} From 74533739f9bbd36adc0fc4df837e0b9cb5b4d2e4 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 01:57:08 +0000 Subject: [PATCH 06/12] test(suidhelper): L2 fd-relative confinement primitives --- native/suidhelper/Cargo.toml | 4 + native/suidhelper/tests/util/confinement.rs | 124 ++++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 native/suidhelper/tests/util/confinement.rs diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index c468223..cb1ee85 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -42,6 +42,10 @@ path = "tests/util/safe_bin.rs" name = "tools_dmsetup_parsers" path = "tests/tools/dmsetup_parsers.rs" +[[test]] +name = "util_confinement" +path = "tests/util/confinement.rs" + [dependencies] clap = { version = "4", features = ["derive"] } nix = { version = "0.29", features = ["user", "fs", "dir"] } diff --git a/native/suidhelper/tests/util/confinement.rs b/native/suidhelper/tests/util/confinement.rs new file mode 100644 index 0000000..ccf311d --- /dev/null +++ b/native/suidhelper/tests/util/confinement.rs @@ -0,0 +1,124 @@ +//! L2: the fd-relative confinement primitives. The security promise is that a +//! symlinked component can never redirect a walk or a recursive delete outside +//! the anchored tree. These run unprivileged in a tempdir; the cases needing +//! root (mknod, chown to another uid) are out of scope here. + +use hyper_suidhelper::util::safe_dir::SafeDir; +use hyper_suidhelper::util::safe_file::{ + Any, IsBlockDevice, IsRegularFile, OnlyRootWritable, RootOwner, SafeFile, +}; +use hyper_suidhelper::util::safe_path::{IsAbsolute, SafePath, StrictComponents}; +use nix::fcntl::OFlag; +use std::fs; +use std::os::unix::fs::{symlink, PermissionsExt}; +use std::path::{Path, PathBuf}; + +type Strict = SafePath; + +fn safe(p: &Path) -> Strict { + p.to_path_buf().try_into().expect("test path must be strict-absolute") +} + +// remove_dir_all must unlink a symlinked entry, never follow it: a symlink +// inside the tree pointing at an external sentinel must leave the sentinel and +// its contents intact. This is the core TOCTOU guarantee. +#[test] +fn remove_dir_all_does_not_follow_symlinks_out_of_tree() { + let tmp = tempfile::tempdir().unwrap(); + + // Sentinel OUTSIDE the tree we will delete. + let sentinel = tmp.path().join("sentinel"); + fs::create_dir(&sentinel).unwrap(); + fs::write(sentinel.join("keep.txt"), b"do not delete me").unwrap(); + + // The tree we will recursively remove, containing a symlink to the sentinel. + let tree = tmp.path().join("tree"); + fs::create_dir(&tree).unwrap(); + fs::write(tree.join("a.txt"), b"x").unwrap(); + symlink(&sentinel, tree.join("escape")).unwrap(); + + let anchor = SafeDir::open(&safe(tmp.path())).unwrap(); + anchor.remove_dir_all(Path::new("tree")).unwrap(); + + assert!(!tree.exists(), "tree must be gone"); + assert!(sentinel.exists(), "sentinel dir must survive"); + assert!(sentinel.join("keep.txt").exists(), "sentinel contents must survive"); +} + +// descend refuses a symlinked path component (O_NOFOLLOW). +#[test] +fn descend_rejects_symlinked_component() { + let tmp = tempfile::tempdir().unwrap(); + let real = tmp.path().join("real"); + fs::create_dir(&real).unwrap(); + fs::create_dir(real.join("leaf")).unwrap(); + + // `link` is a symlink standing in for the `real` directory. + symlink(&real, tmp.path().join("link")).unwrap(); + + let anchor = SafeDir::open(&safe(tmp.path())).unwrap(); + let err = anchor.descend(&[PathBuf::from("link"), PathBuf::from("leaf")]); + assert!(err.is_err(), "descend followed a symlinked component"); +} + +// remove_dir_all recurses through real nested directories. +#[test] +fn remove_dir_all_clears_nested_tree() { + let tmp = tempfile::tempdir().unwrap(); + let tree = tmp.path().join("tree"); + fs::create_dir_all(tree.join("a/b/c")).unwrap(); + fs::write(tree.join("a/b/c/deep.txt"), b"x").unwrap(); + fs::write(tree.join("a/top.txt"), b"y").unwrap(); + + let anchor = SafeDir::open(&safe(tmp.path())).unwrap(); + anchor.remove_dir_all(Path::new("tree")).unwrap(); + assert!(!tree.exists()); +} + +// SafeFile file-type axis: a regular file is not a block device. +#[test] +fn safefile_type_axis_distinguishes_regular_from_block() { + let tmp = tempfile::tempdir().unwrap(); + let f = tmp.path().join("plain"); + fs::write(&f, b"data").unwrap(); + + let p = safe(&f); + assert!(SafeFile::::open(&p, OFlag::O_PATH).is_ok()); + assert!(SafeFile::::open(&p, OFlag::O_PATH).is_err()); +} + +// SafeFile owner axis: a file we (non-root) own fails RootOwner. +#[test] +fn safefile_owner_axis_rejects_non_root_file() { + let tmp = tempfile::tempdir().unwrap(); + let f = tmp.path().join("ours"); + fs::write(&f, b"data").unwrap(); + let p = safe(&f); + assert!(SafeFile::::open(&p, OFlag::O_PATH).is_err()); +} + +// SafeFile mode axis: a group/other-writable file fails OnlyRootWritable. +#[test] +fn safefile_mode_axis_rejects_group_writable() { + let tmp = tempfile::tempdir().unwrap(); + let f = tmp.path().join("loose"); + fs::write(&f, b"data").unwrap(); + fs::set_permissions(&f, fs::Permissions::from_mode(0o666)).unwrap(); + let p = safe(&f); + assert!(SafeFile::::open(&p, OFlag::O_PATH).is_err()); + + fs::set_permissions(&f, fs::Permissions::from_mode(0o644)).unwrap(); + assert!(SafeFile::::open(&p, OFlag::O_PATH).is_ok()); +} + +// SafeFile::open refuses a symlinked final component (O_NOFOLLOW). +#[test] +fn safefile_open_rejects_final_symlink() { + let tmp = tempfile::tempdir().unwrap(); + let target = tmp.path().join("target"); + fs::write(&target, b"data").unwrap(); + let link = tmp.path().join("link"); + symlink(&target, &link).unwrap(); + let p = safe(&link); + assert!(SafeFile::::open(&p, OFlag::O_PATH).is_err()); +} From 400cf89b6e9ee39934b4c5d3ede24fa4b6235845 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 02:02:53 +0000 Subject: [PATCH 07/12] test(suidhelper): L5 config-loading subprocess suite Add tests/e2e/config.rs (feature-gated on insecure_test_seams) that drives the real binary via CARGO_BIN_EXE to assert exit codes and stderr for missing, non-root-owned, malformed, relative, and valid configs. Two cases always run as non-root; three skip with a loud eprintln! and pass when not root. Register e2e_config [[test]] target. --- native/suidhelper/Cargo.toml | 4 + native/suidhelper/tests/e2e/config.rs | 113 ++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 native/suidhelper/tests/e2e/config.rs diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index cb1ee85..0dd18c0 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -46,6 +46,10 @@ path = "tests/tools/dmsetup_parsers.rs" name = "util_confinement" path = "tests/util/confinement.rs" +[[test]] +name = "e2e_config" +path = "tests/e2e/config.rs" + [dependencies] clap = { version = "4", features = ["derive"] } nix = { version = "0.29", features = ["user", "fs", "dir"] } diff --git a/native/suidhelper/tests/e2e/config.rs b/native/suidhelper/tests/e2e/config.rs new file mode 100644 index 0000000..8488785 --- /dev/null +++ b/native/suidhelper/tests/e2e/config.rs @@ -0,0 +1,113 @@ +//! L5: the binary's startup config contract, observed end to end (exit code + +//! stderr + stdout JSON). Uses the config-path seam to point at a per-test +//! tempfile instead of /etc/hyper, so tests never touch the real host. +#![cfg(feature = "insecure_test_seams")] + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; +use std::process::{Command, Output}; + +const BIN: &str = env!("CARGO_BIN_EXE_hyper-suidhelper"); + +fn is_root() -> bool { + nix::unistd::geteuid().is_root() +} + +/// Run the helper with the gate open and the config path redirected. +fn run_with_config(config_path: &Path, args: &[&str]) -> Output { + Command::new(BIN) + .args(args) + .env_clear() + .env("HYPER_SETUIDHELPER_IS_INSECURE_MODE", "1") + .env("HYPER_SETUIDHELPER_CONFIG_PATH", config_path) + .output() + .expect("spawn helper") +} + +/// Write a config file owned root:root, 0644. Requires the test run as root. +fn write_root_config(dir: &Path, body: &str) -> std::path::PathBuf { + let p = dir.join("config.toml"); + fs::write(&p, body).unwrap(); + fs::set_permissions(&p, fs::Permissions::from_mode(0o644)).unwrap(); + // Owned by root because the test process is root (checked by callers). + p +} + +/// A missing config file is refused at SafeFile::open with exit code 2. +/// The error is LoadingError::File(ValidationError::Open(ENOENT)), +/// which displays as "open failed: ". +#[test] +fn missing_config_exits_2() { + let tmp = tempfile::tempdir().unwrap(); + let missing = tmp.path().join("nope.toml"); + let out = run_with_config(&missing, &["sys-test"]); + assert_eq!(out.status.code(), Some(2), "missing config must exit 2"); + let err = String::from_utf8_lossy(&out.stderr); + assert!(err.contains("open failed"), "stderr: {err}"); +} + +#[test] +fn non_root_owned_config_is_rejected() { + // A config file owned by a non-root user must be refused. If the test runs + // as root, chown it away from root to exercise the owner check. + let tmp = tempfile::tempdir().unwrap(); + let p = tmp.path().join("config.toml"); + fs::write(&p, "work_dir = \"/srv/hyper\"\n").unwrap(); + fs::set_permissions(&p, fs::Permissions::from_mode(0o644)).unwrap(); + if is_root() { + // Drop ownership to uid/gid 65534 (nobody) so it is no longer root-owned. + let nobody = nix::unistd::Uid::from_raw(65534); + let nogrp = nix::unistd::Gid::from_raw(65534); + nix::unistd::chown(&p, Some(nobody), Some(nogrp)).unwrap(); + } + let out = run_with_config(&p, &["sys-test"]); + assert_eq!(out.status.code(), Some(2)); + let err = String::from_utf8_lossy(&out.stderr); + assert!( + err.contains("root:root") || err.contains("not owned"), + "expected an ownership rejection, stderr: {err}", + ); +} + +#[test] +fn malformed_config_exits_2_malformed() { + if !is_root() { + eprintln!("SKIP malformed_config: needs root to own the config file"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let p = write_root_config(tmp.path(), "this is not = valid = toml ==="); + let out = run_with_config(&p, &["sys-test"]); + assert_eq!(out.status.code(), Some(2)); + assert!(String::from_utf8_lossy(&out.stderr).contains("not valid TOML")); +} + +#[test] +fn relative_work_dir_exits_2_relative() { + if !is_root() { + eprintln!("SKIP relative_work_dir: needs root to own the config file"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let p = write_root_config(tmp.path(), "work_dir = \"relative/path\"\n"); + let out = run_with_config(&p, &["sys-test"]); + assert_eq!(out.status.code(), Some(2)); + assert!(String::from_utf8_lossy(&out.stderr).contains("must be an absolute path")); +} + +#[test] +fn valid_config_and_setuid_yields_sys_test_ok() { + if !is_root() { + eprintln!("SKIP valid_config sys-test: needs root to acquire privileges"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let p = write_root_config(tmp.path(), "work_dir = \"/srv/hyper\"\n"); + let out = run_with_config(&p, &["sys-test"]); + assert_eq!(out.status.code(), Some(0), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + let json: serde_json::Value = + serde_json::from_slice(&out.stdout).expect("stdout is JSON"); + assert_eq!(json["sys_test"], "ok"); + assert_eq!(json["hyper_base"], "/srv/hyper"); +} From 00a80327a9e1ff84d15734d1cf9574ea8ec3b62d Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 02:08:52 +0000 Subject: [PATCH 08/12] test(suidhelper): L4 fake-bin argv-capture subprocess suite Add tests/e2e/argv.rs (feature-gated insecure_test_seams) that points the helper's --bin at a root-owned shell fake recording argv as JSON. Asserts exact command lines for dmsetup create/remove/message and blockdev --getsz. All four tests early-skip with a loud eprintln when not root; real assertions run in the privileged CI job. --- native/suidhelper/Cargo.toml | 4 + native/suidhelper/tests/e2e/argv.rs | 156 ++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 native/suidhelper/tests/e2e/argv.rs diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index 0dd18c0..a9e4d54 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -50,6 +50,10 @@ path = "tests/util/confinement.rs" name = "e2e_config" path = "tests/e2e/config.rs" +[[test]] +name = "e2e_argv" +path = "tests/e2e/argv.rs" + [dependencies] clap = { version = "4", features = ["derive"] } nix = { version = "0.29", features = ["user", "fs", "dir"] } diff --git a/native/suidhelper/tests/e2e/argv.rs b/native/suidhelper/tests/e2e/argv.rs new file mode 100644 index 0000000..460fd65 --- /dev/null +++ b/native/suidhelper/tests/e2e/argv.rs @@ -0,0 +1,156 @@ +//! L4: prove the exact argv (and empty env) the helper hands to the child tool — +//! the one thing the design deliberately hides from the caller. We point `--bin` +//! at a root-owned fake that writes its argv+env to a file as JSON, then assert +//! on the reconstructed command line. +#![cfg(feature = "insecure_test_seams")] + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::Command; + +const HELPER: &str = env!("CARGO_BIN_EXE_hyper-suidhelper"); + +fn is_root() -> bool { + nix::unistd::geteuid().is_root() +} + +/// Install a root-owned fake tool named `basename` that records its argv (minus +/// argv[0]) into `record` as a JSON array, then exits 0 with valid stdout for +/// the helper's parser. Returns the fake's absolute path. +fn install_fake(dir: &Path, basename: &str, record: &Path, stdout_line: &str) -> PathBuf { + let path = dir.join(basename); + // A tiny shell fake. `printf '%s\n'` of a JSON array of args. stdout_line is + // what the helper's `parse` expects (e.g. a loop device path or sectors). + let script = format!( + "#!/bin/sh\nprintf '%s' \"$(\n printf '['\n sep=''\n for a in \"$@\"; do printf '%s\"%s\"' \"$sep\" \"$a\"; sep=','; done\n printf ']'\n)\" > '{record}'\nprintf '{stdout_line}\\n'\n", + record = record.display(), + ); + fs::write(&path, script).unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap(); + path // root-owned because this test runs as root +} + +fn write_root_config(dir: &Path) -> PathBuf { + let p = dir.join("config.toml"); + fs::write(&p, "work_dir = \"/srv/hyper\"\n").unwrap(); + fs::set_permissions(&p, fs::Permissions::from_mode(0o644)).unwrap(); + p +} + +fn run(config: &Path, args: &[&str]) -> std::process::Output { + Command::new(HELPER) + .args(args) + .env_clear() + .env("HYPER_SETUIDHELPER_IS_INSECURE_MODE", "1") + .env("HYPER_SETUIDHELPER_CONFIG_PATH", config) + .output() + .expect("spawn helper") +} + +fn recorded_argv(record: &Path) -> Vec { + let body = fs::read_to_string(record).expect("fake recorded argv"); + serde_json::from_str(&body).expect("argv json") +} + +#[test] +fn dmsetup_create_snapshot_reconstructs_canonical_table() { + if !is_root() { + eprintln!("SKIP dmsetup_create: needs root to acquire + own the fake bin"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let cfg = write_root_config(tmp.path()); + let rec = tmp.path().join("argv.json"); + let bin = install_fake(tmp.path(), "dmsetup", &rec, ""); + + // Deliberately weird inner spacing; the helper must re-render canonically. + let out = run( + &cfg, + &[ + "dmsetup", + "--bin", + bin.to_str().unwrap(), + "create", + "hyper-vm1", + "--readonly", + "--table", + "0 100 snapshot /dev/loop0 /dev/loop1 P 8", + ], + ); + assert_eq!(out.status.code(), Some(0), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + + let argv = recorded_argv(&rec); + assert_eq!( + argv, + vec![ + "create", + "hyper-vm1", + "--readonly", + "--table", + "0 100 snapshot /dev/loop0 /dev/loop1 P 8", // canonical single-spaced + ], + "helper did not reconstruct the canonical table", + ); +} + +#[test] +fn dmsetup_remove_retry_toggle() { + if !is_root() { + eprintln!("SKIP dmsetup_remove: needs root"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let cfg = write_root_config(tmp.path()); + let rec = tmp.path().join("argv.json"); + let bin = install_fake(tmp.path(), "dmsetup", &rec, ""); + + let out = run( + &cfg, + &["dmsetup", "--bin", bin.to_str().unwrap(), "remove", "--retry", "hyper-vm1"], + ); + assert_eq!(out.status.code(), Some(0)); + assert_eq!(recorded_argv(&rec), vec!["remove", "--retry", "hyper-vm1"]); +} + +#[test] +fn dmsetup_message_create_thin() { + if !is_root() { + eprintln!("SKIP dmsetup_message: needs root"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let cfg = write_root_config(tmp.path()); + let rec = tmp.path().join("argv.json"); + let bin = install_fake(tmp.path(), "dmsetup", &rec, ""); + + let out = run( + &cfg, + &["dmsetup", "--bin", bin.to_str().unwrap(), "message", "hyper-pool", "--message", "create_thin 7"], + ); + assert_eq!(out.status.code(), Some(0)); + // the helper passes the whole message as a single argv element (dmsetup re-joins remaining args) + assert_eq!(recorded_argv(&rec), vec!["message", "hyper-pool", "0", "create_thin 7"]); +} + +#[test] +fn blockdev_getsz_argv_and_parse() { + if !is_root() { + eprintln!("SKIP blockdev: needs root"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let cfg = write_root_config(tmp.path()); + let rec = tmp.path().join("argv.json"); + // Fake prints "2048" as the sector count for the helper to parse. + let bin = install_fake(tmp.path(), "blockdev", &rec, "2048"); + + let out = run( + &cfg, + &["blockdev", "--bin", bin.to_str().unwrap(), "--getsz", "/dev/loop0"], + ); + assert_eq!(out.status.code(), Some(0), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + assert_eq!(recorded_argv(&rec), vec!["--getsz", "/dev/loop0"]); + let json: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + assert_eq!(json["sectors"], 2048); +} From aef9b16356bc7eed8365d84b18619b50e664af92 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 02:14:54 +0000 Subject: [PATCH 09/12] ci(suidhelper): privileged job for gated E2E suites + docs Add `suidhelper-privileged` job that runs only `--test e2e_config --test e2e_argv` under `sudo -E` so the root-only cases (sys-test ok, fake-bin argv capture) actually execute in CI. The non-root `rust` job continues to run the full suite with `--all-features`; owner-axis tests that assume a non-root uid stay there. Append a Testing section to the suidhelper README documenting the three invocation tiers and the dual-gate insecure test seam. --- .github/workflows/ci.yml | 34 ++++++++++++++++++++++++++++++++++ native/suidhelper/README.md | 21 +++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbc53a1..053345f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -206,6 +206,40 @@ jobs: flags: rust fail_ci_if_error: true + suidhelper-privileged: + name: Rust suidhelper (privileged E2E) + runs-on: ubuntu-latest + defaults: + run: + working-directory: native/suidhelper + steps: + - uses: actions/checkout@v7 + + # rustup reads native/suidhelper/rust-toolchain.toml and installs the pinned + # nightly on the first cargo call. + - name: Show toolchain + run: rustup show + + - uses: Swatinem/rust-cache@v2 + with: + workspaces: native/suidhelper + + - name: Install nextest + uses: taiki-e/install-action@v2 + with: + tool: nextest + + # The feature-gated E2E suites self-skip without root. Run ONLY these two as + # root (via sudo -E so the runner's cargo/rustup home and PATH are kept) so + # the root-only cases -- sys-test ok, fake-bin argv capture -- actually run. + # The rest of the suite (owner-axis tests assume a non-root uid) stays in the + # non-root `rust` job above. + - name: Gated E2E suites as root + run: | + sudo -E env "PATH=$PATH" \ + cargo nextest run --features insecure_test_seams \ + --test e2e_config --test e2e_argv + # Uploads the triggering event payload so the workflow_run publisher # (test-results.yml) can map results back to the originating PR -- required # for PR comments on fork PRs. Tiny job, always runs. diff --git a/native/suidhelper/README.md b/native/suidhelper/README.md index cd132a8..15161a8 100644 --- a/native/suidhelper/README.md +++ b/native/suidhelper/README.md @@ -75,3 +75,24 @@ elsewhere via dm-thin), so there is no read-write attach. must be **snapshot** targets only (no `linear`/`crypt`/… that could map arbitrary devices). The device path checks reject `..` traversal. Anything else exits non-zero without running. + +## Testing + +- `cargo nextest run` — pure layers (parser matrix, confinement primitives, gate + defaults). No root, no config, no Docker. +- `cargo nextest run --features insecure_test_seams` — adds the config-seam and + subprocess suites. Cases needing root self-skip with a loud `SKIP` line. +- As root (`sudo -E env "PATH=$PATH" cargo nextest run --features + insecure_test_seams --test e2e_config --test e2e_argv`) — runs every case, + including `sys-test ok` and the fake-bin argv-capture assertions. CI does this + in the `suidhelper-privileged` job. + +### The insecure test seam + +`security_gate` is the single decision point for INSECURE TEST MODE. The insecure +code path (the `HYPER_SETUIDHELPER_CONFIG_PATH` config override) activates only +when BOTH gates are open: the `insecure_test_seams` Cargo feature (compile-time) +AND `HYPER_SETUIDHELPER_IS_INSECURE_MODE=1` (runtime). Release builds never enable +the feature — `build.rs` refuses to compile a release binary with it — so the +installed setuid binary always takes the secure arm regardless of the +environment. From 4217fb21188cd3f1d64318af8eae1efad0ecfb60 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 02:19:02 +0000 Subject: [PATCH 10/12] style(suidhelper): rustfmt the new test files --- native/suidhelper/tests/e2e/argv.rs | 46 ++++++++++++++++--- native/suidhelper/tests/e2e/config.rs | 10 ++-- .../suidhelper/tests/tools/dmsetup_parsers.rs | 26 ++++++++--- native/suidhelper/tests/util/confinement.rs | 9 +++- native/suidhelper/tests/util/safe_bin.rs | 6 ++- 5 files changed, 79 insertions(+), 18 deletions(-) diff --git a/native/suidhelper/tests/e2e/argv.rs b/native/suidhelper/tests/e2e/argv.rs index 460fd65..3d25941 100644 --- a/native/suidhelper/tests/e2e/argv.rs +++ b/native/suidhelper/tests/e2e/argv.rs @@ -78,7 +78,12 @@ fn dmsetup_create_snapshot_reconstructs_canonical_table() { "0 100 snapshot /dev/loop0 /dev/loop1 P 8", ], ); - assert_eq!(out.status.code(), Some(0), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); let argv = recorded_argv(&rec); assert_eq!( @@ -107,7 +112,14 @@ fn dmsetup_remove_retry_toggle() { let out = run( &cfg, - &["dmsetup", "--bin", bin.to_str().unwrap(), "remove", "--retry", "hyper-vm1"], + &[ + "dmsetup", + "--bin", + bin.to_str().unwrap(), + "remove", + "--retry", + "hyper-vm1", + ], ); assert_eq!(out.status.code(), Some(0)); assert_eq!(recorded_argv(&rec), vec!["remove", "--retry", "hyper-vm1"]); @@ -126,11 +138,22 @@ fn dmsetup_message_create_thin() { let out = run( &cfg, - &["dmsetup", "--bin", bin.to_str().unwrap(), "message", "hyper-pool", "--message", "create_thin 7"], + &[ + "dmsetup", + "--bin", + bin.to_str().unwrap(), + "message", + "hyper-pool", + "--message", + "create_thin 7", + ], ); assert_eq!(out.status.code(), Some(0)); // the helper passes the whole message as a single argv element (dmsetup re-joins remaining args) - assert_eq!(recorded_argv(&rec), vec!["message", "hyper-pool", "0", "create_thin 7"]); + assert_eq!( + recorded_argv(&rec), + vec!["message", "hyper-pool", "0", "create_thin 7"] + ); } #[test] @@ -147,9 +170,20 @@ fn blockdev_getsz_argv_and_parse() { let out = run( &cfg, - &["blockdev", "--bin", bin.to_str().unwrap(), "--getsz", "/dev/loop0"], + &[ + "blockdev", + "--bin", + bin.to_str().unwrap(), + "--getsz", + "/dev/loop0", + ], + ); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) ); - assert_eq!(out.status.code(), Some(0), "stderr: {}", String::from_utf8_lossy(&out.stderr)); assert_eq!(recorded_argv(&rec), vec!["--getsz", "/dev/loop0"]); let json: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); assert_eq!(json["sectors"], 2048); diff --git a/native/suidhelper/tests/e2e/config.rs b/native/suidhelper/tests/e2e/config.rs index 8488785..d3ae042 100644 --- a/native/suidhelper/tests/e2e/config.rs +++ b/native/suidhelper/tests/e2e/config.rs @@ -105,9 +105,13 @@ fn valid_config_and_setuid_yields_sys_test_ok() { let tmp = tempfile::tempdir().unwrap(); let p = write_root_config(tmp.path(), "work_dir = \"/srv/hyper\"\n"); let out = run_with_config(&p, &["sys-test"]); - assert_eq!(out.status.code(), Some(0), "stderr: {}", String::from_utf8_lossy(&out.stderr)); - let json: serde_json::Value = - serde_json::from_slice(&out.stdout).expect("stdout is JSON"); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let json: serde_json::Value = serde_json::from_slice(&out.stdout).expect("stdout is JSON"); assert_eq!(json["sys_test"], "ok"); assert_eq!(json["hyper_base"], "/srv/hyper"); } diff --git a/native/suidhelper/tests/tools/dmsetup_parsers.rs b/native/suidhelper/tests/tools/dmsetup_parsers.rs index 0a63c6a..bdb37cc 100644 --- a/native/suidhelper/tests/tools/dmsetup_parsers.rs +++ b/native/suidhelper/tests/tools/dmsetup_parsers.rs @@ -15,7 +15,9 @@ fn accepts_canonical_tables_and_round_trips() { "0 100 thin /dev/mapper/hyper-pool 0", "0 100 thin /dev/mapper/hyper-pool 0 /dev/mapper/hyper-orig", ] { - let t = s.parse::().unwrap_or_else(|_| panic!("rejected {s:?}")); + let t = s + .parse::() + .unwrap_or_else(|_| panic!("rejected {s:?}")); assert_eq!(t.to_string(), s, "round-trip changed {s:?}"); } } @@ -24,7 +26,9 @@ fn accepts_canonical_tables_and_round_trips() { fn normalizes_inner_whitespace_on_render() { let weird = "0 100 snapshot /dev/loop0 /dev/loop1 P 8"; let canonical = "0 100 snapshot /dev/loop0 /dev/loop1 P 8"; - let t = weird.parse::().expect("weird spacing must still parse"); + let t = weird + .parse::() + .expect("weird spacing must still parse"); assert_eq!(t.to_string(), canonical); } @@ -54,7 +58,9 @@ fn thinmessage_accepts_whitelisted_and_normalizes() { ("create_thin 7", "create_thin 7"), ("delete 3", "delete 3"), ] { - let m = s.parse::().unwrap_or_else(|_| panic!("rejected {s:?}")); + let m = s + .parse::() + .unwrap_or_else(|_| panic!("rejected {s:?}")); assert_eq!(m.to_string(), canon, "round-trip changed {s:?}"); } } @@ -62,9 +68,17 @@ fn thinmessage_accepts_whitelisted_and_normalizes() { #[test] fn thinmessage_rejects_non_whitelisted() { for bad in [ - "resize 10", "create_thin", "delete", "delete x", - "create_thin 1 2", "", "create_thin -1", + "resize 10", + "create_thin", + "delete", + "delete x", + "create_thin 1 2", + "", + "create_thin -1", ] { - assert!(bad.parse::().is_err(), "ThinMessage accepted {bad:?}"); + assert!( + bad.parse::().is_err(), + "ThinMessage accepted {bad:?}" + ); } } diff --git a/native/suidhelper/tests/util/confinement.rs b/native/suidhelper/tests/util/confinement.rs index ccf311d..5cffd2b 100644 --- a/native/suidhelper/tests/util/confinement.rs +++ b/native/suidhelper/tests/util/confinement.rs @@ -16,7 +16,9 @@ use std::path::{Path, PathBuf}; type Strict = SafePath; fn safe(p: &Path) -> Strict { - p.to_path_buf().try_into().expect("test path must be strict-absolute") + p.to_path_buf() + .try_into() + .expect("test path must be strict-absolute") } // remove_dir_all must unlink a symlinked entry, never follow it: a symlink @@ -42,7 +44,10 @@ fn remove_dir_all_does_not_follow_symlinks_out_of_tree() { assert!(!tree.exists(), "tree must be gone"); assert!(sentinel.exists(), "sentinel dir must survive"); - assert!(sentinel.join("keep.txt").exists(), "sentinel contents must survive"); + assert!( + sentinel.join("keep.txt").exists(), + "sentinel contents must survive" + ); } // descend refuses a symlinked path component (O_NOFOLLOW). diff --git a/native/suidhelper/tests/util/safe_bin.rs b/native/suidhelper/tests/util/safe_bin.rs index 88ccc4e..950e06b 100644 --- a/native/suidhelper/tests/util/safe_bin.rs +++ b/native/suidhelper/tests/util/safe_bin.rs @@ -32,7 +32,11 @@ fn rejects_symlink_with_correct_basename() { let link = dir.path().join("losetup"); symlink(&target, &link).unwrap(); // Symlink is checked before ownership, so this holds regardless of who runs it. - assert!(link.to_str().unwrap().parse::>().is_err()); + assert!(link + .to_str() + .unwrap() + .parse::>() + .is_err()); } #[test] From 374c6ec00af53557bda06f12d06bd7f3bbba878a Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 03:29:00 +0000 Subject: [PATCH 11/12] refactor(suidhelper): relax security_gate flag ordering to Relaxed The INSECURE flag is written once at startup before any concurrency and publishes no other memory, so SeqCst overstates the contract; Relaxed is sufficient. --- native/suidhelper/src/security_gate.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/native/suidhelper/src/security_gate.rs b/native/suidhelper/src/security_gate.rs index ad05601..8145cc3 100644 --- a/native/suidhelper/src/security_gate.rs +++ b/native/suidhelper/src/security_gate.rs @@ -30,7 +30,10 @@ pub fn init() { ({INSECURE_MODE_ENV}=1); never do this on a real host" ); } - INSECURE.store(insecure, Ordering::SeqCst); + // Relaxed is sufficient: a standalone flag, written once at startup before + // any concurrency, and it publishes no other memory — so there is no + // acquire/release (let alone total-order) relationship to enforce. + INSECURE.store(insecure, Ordering::Relaxed); } fn env_opts_in() -> bool { @@ -43,7 +46,7 @@ fn env_opts_in() -> bool { /// the feature it folds to `false`, so the whole condition is constant-false and /// the optimizer drops the `insecure` branch entirely. pub fn split(secure: impl FnOnce() -> T, insecure: impl FnOnce() -> T) -> T { - if cfg!(feature = "insecure_test_seams") && INSECURE.load(Ordering::SeqCst) { + if cfg!(feature = "insecure_test_seams") && INSECURE.load(Ordering::Relaxed) { insecure() } else { secure() From e51803e82a122275af1bb73b180ec274cbabd693 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 25 Jun 2026 03:31:03 +0000 Subject: [PATCH 12/12] more docs --- native/suidhelper/src/security_gate.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/native/suidhelper/src/security_gate.rs b/native/suidhelper/src/security_gate.rs index 8145cc3..d82caea 100644 --- a/native/suidhelper/src/security_gate.rs +++ b/native/suidhelper/src/security_gate.rs @@ -23,28 +23,18 @@ static INSECURE: AtomicBool = AtomicBool::new(false); /// the very first thing in `main`, before any seam (e.g. the config load) runs. /// Idempotent. pub fn init() { - let insecure = cfg!(feature = "insecure_test_seams") && env_opts_in(); + let insecure = cfg!(feature = "insecure_test_seams") + && std::env::var(INSECURE_MODE_ENV).as_deref() == Ok("1"); if insecure { eprintln!( "hyper-suidhelper: WARNING: running in INSECURE TEST MODE \ ({INSECURE_MODE_ENV}=1); never do this on a real host" ); } - // Relaxed is sufficient: a standalone flag, written once at startup before - // any concurrency, and it publishes no other memory — so there is no - // acquire/release (let alone total-order) relationship to enforce. INSECURE.store(insecure, Ordering::Relaxed); } -fn env_opts_in() -> bool { - std::env::var(INSECURE_MODE_ENV).as_deref() == Ok("1") -} - -/// Run `secure` in production; run `insecure` only when BOTH gates are open. -/// -/// The `cfg!(feature = ...)` is a compile-time constant: in any build without -/// the feature it folds to `false`, so the whole condition is constant-false and -/// the optimizer drops the `insecure` branch entirely. +/// Run `secure` in production; run `insecure` in testing. pub fn split(secure: impl FnOnce() -> T, insecure: impl FnOnce() -> T) -> T { if cfg!(feature = "insecure_test_seams") && INSECURE.load(Ordering::Relaxed) { insecure()