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/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 b0ac1c4..a9e4d54 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,34 @@ path = "tests/util/safe_path.rs" name = "safe_dev" path = "tests/util/safe_dev.rs" +[[test]] +name = "security_gate" +path = "tests/security_gate.rs" + +[[test]] +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" + +[[test]] +name = "util_confinement" +path = "tests/util/confinement.rs" + +[[test]] +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"] } @@ -31,6 +64,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/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. 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" + ); + } + } +} diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index d8dfbb4..425ba38 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,17 @@ 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 path = config_path(); - let safe_path: SafePath = - PathBuf::from(CONFIG_PATHSTR).try_into()?; + 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))?; - let config: Config = toml::from_str(&body).map_err(|_| LoadingError::Malformed(path))?; + .map_err(|_| LoadingError::Unreadable(path.clone()))?; + 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/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..d82caea --- /dev/null +++ b/native/suidhelper/src/security_gate.rs @@ -0,0 +1,44 @@ +// 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") + && 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" + ); + } + INSECURE.store(insecure, Ordering::Relaxed); +} + +/// 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() + } else { + secure() + } +} 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/config_seam.rs b/native/suidhelper/tests/config_seam.rs new file mode 100644 index 0000000..75b6b35 --- /dev/null +++ b/native/suidhelper/tests/config_seam.rs @@ -0,0 +1,26 @@ +//! The config-path seam routes through security_gate::split. With both gates +//! 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 hyper_suidhelper::util::safe_path::ValidationError; + +#[test] +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", loose); + security_gate::init(); + + let err = Config::safe_load().expect_err("override must be consulted"); + assert!( + matches!(err, LoadingError::Path(ValidationError::LooseComponents)), + "expected the override path to hit the real lexical gate, got {err:?}", + ); +} diff --git a/native/suidhelper/tests/e2e/argv.rs b/native/suidhelper/tests/e2e/argv.rs new file mode 100644 index 0000000..3d25941 --- /dev/null +++ b/native/suidhelper/tests/e2e/argv.rs @@ -0,0 +1,190 @@ +//! 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); +} diff --git a/native/suidhelper/tests/e2e/config.rs b/native/suidhelper/tests/e2e/config.rs new file mode 100644 index 0000000..d3ae042 --- /dev/null +++ b/native/suidhelper/tests/e2e/config.rs @@ -0,0 +1,117 @@ +//! 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"); +} 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"); +} diff --git a/native/suidhelper/tests/tools/dmsetup_parsers.rs b/native/suidhelper/tests/tools/dmsetup_parsers.rs new file mode 100644 index 0000000..bdb37cc --- /dev/null +++ b/native/suidhelper/tests/tools/dmsetup_parsers.rs @@ -0,0 +1,84 @@ +//! `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/confinement.rs b/native/suidhelper/tests/util/confinement.rs new file mode 100644 index 0000000..5cffd2b --- /dev/null +++ b/native/suidhelper/tests/util/confinement.rs @@ -0,0 +1,129 @@ +//! 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()); +} diff --git a/native/suidhelper/tests/util/safe_bin.rs b/native/suidhelper/tests/util/safe_bin.rs new file mode 100644 index 0000000..950e06b --- /dev/null +++ b/native/suidhelper/tests/util/safe_bin.rs @@ -0,0 +1,70 @@ +//! `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"); + } +}