diff --git a/lib/hyper/node/fire_vmm/jailer.ex b/lib/hyper/node/fire_vmm/jailer.ex index eb15b986..b57f26a0 100644 --- a/lib/hyper/node/fire_vmm/jailer.ex +++ b/lib/hyper/node/fire_vmm/jailer.ex @@ -147,10 +147,15 @@ defmodule Hyper.Node.FireVMM.Jailer do type |> Instance.spec() |> Instance.Spec.cgroup_v2() - |> Sys.Linux.Cgroup.V2.Config.as_linux() - |> Enum.flat_map(fn {file, value} -> ["--cgroup", "#{file}=#{value}"] end) + |> Enum.flat_map(&cgroup_flag/1) end + @spec cgroup_flag({atom(), term()}) :: [String.t()] + defp cgroup_flag({:memory_max, bytes}), do: ["--memory-max", to_string(bytes)] + + defp cgroup_flag({:cpu_max, %{quota_us: quota, period_us: period}}), + do: ["--cpu-quota-us", to_string(quota), "--cpu-period-us", to_string(period)] + @doc "Host path of the VM's per-VM jail dir (`//`)." @spec chroot_dir(Hyper.Vm.Id.t()) :: Path.t() def chroot_dir(id) do diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index 92b12922..113998be 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -82,8 +82,12 @@ name = "tools_chroot_jail_grant_vsock" path = "tests/tools/chroot_jail_grant_vsock.rs" [[test]] -name = "util_chroot_jail" -path = "tests/util/chroot_jail.rs" +name = "firecracker_chroot_jail" +path = "tests/firecracker/chroot_jail.rs" + +[[test]] +name = "firecracker_id" +path = "tests/firecracker/id.rs" [[test]] name = "e2e_config" @@ -117,6 +121,10 @@ path = "tests/config_uid_gid_range.rs" name = "config_network" path = "tests/config_network.rs" +[[test]] +name = "config_paths" +path = "tests/config_paths.rs" + [[test]] name = "tools_dmsetup_backing" path = "tests/tools/dmsetup_backing.rs" diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index 493280f6..b3ed4af0 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -8,8 +8,12 @@ //! decide which UIDs it *accepts* (default `{900_000, 999_999}` when the key is //! absent). Operators narrowing the range must set **both**. +use crate::firecracker::id::UidGidRange; use crate::fs::file::{self, IsRegularFile, OpenFlags, OwnerOnlyWritable, RootOwner, SafeFile}; -use crate::fs::path::{self, IsAbsolute, SafePath, StrictComponents}; +use crate::fs::path::{ + self, IsAbsolute, SafePath, SafePathBuf, StrictComponent, StrictComponentBuf, StrictComponents, +}; +use crate::linux::cgroup; use crate::process::binary::{self, RootSafeBin, RootSafeBinPathBuf}; use serde::Deserialize; use smart_default::SmartDefault; @@ -26,12 +30,8 @@ pub enum LoadingError { File(#[from] file::ValidationError), #[error("{0:?} could not be read")] Unreadable(PathBuf), - #[error("{0:?} is not valid TOML")] - Malformed(PathBuf), - #[error("work_dir in {0:?} must be an absolute path")] - Relative(PathBuf), - #[error("uid_gid_range.min must be >= 1 and <= max (got min={min}, max={max})")] - BadUidGidRange { min: u32, max: u32 }, + #[error("{0:?} is not valid TOML: {1}")] + Malformed(PathBuf, #[source] toml::de::Error), } /// Error returned by config accessors for tool binaries derived from config. @@ -46,40 +46,6 @@ pub enum BinError { const CONFIG_PATHSTR: &str = "/etc/hyper/config.toml"; const INSECURE_CONFIG_PATH_ENV: &str = "HYPER_SETUIDHELPER_CONFIG_PATH"; -/// UID/GID allocation band, read from `[jails] uid_gid_range` in config.toml as -/// a two-element `[min, max]` array. Controls which UIDs the helper *accepts* -/// from the BEAM — see module docs. -#[derive(Debug, Clone, Copy, Deserialize)] -#[serde(from = "[u32; 2]")] -pub struct UidGidRange { - pub min: u32, - pub max: u32, -} - -impl From<[u32; 2]> for UidGidRange { - fn from([min, max]: [u32; 2]) -> Self { - Self { min, max } - } -} - -// Band defaults match Elixir's `compile_env` allocation defaults so that an -// unconfigured helper and an unconfigured node agree out of the box. -const DEFAULT_UID_GID: (u32, u32) = (900_000, 999_999); - -/// Validate a uid_gid_range value. A present range where min==0 or min>max is -/// treated as a config trust violation — fatal at load, consistent with the -/// "present but untrusted" model. Exposed so tests can verify the contract -/// without touching the file system. -pub fn validate_uid_gid_range(r: &UidGidRange) -> Result<(), LoadingError> { - if r.min == 0 || r.min > r.max { - return Err(LoadingError::BadUidGidRange { - min: r.min, - max: r.max, - }); - } - Ok(()) -} - /// 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. @@ -98,7 +64,7 @@ fn config_path() -> PathBuf { #[derive(Debug, Clone, Deserialize, SmartDefault)] pub struct Config { #[default(default_work_dir())] - work_dir: PathBuf, + work_dir: SafePathBuf, #[serde(default)] tools: Tools, #[serde(default)] @@ -117,8 +83,8 @@ pub struct Config { #[serde(default)] pub struct Jails { #[default(default_parent_cgroup())] - cgroup: String, - uid_gid_range: Option, + cgroup: StrictComponentBuf, + uid_gid_range: UidGidRange, } /// Paths to the external binaries the helper runs, the `[tools]` table. @@ -153,13 +119,15 @@ pub struct Tools { // The default data root. Must match the Elixir node's `@dev_work_dir`, which it // uses when the same config file is absent, so both sides agree (see // `Hyper.Node.check_helper_base`). -fn default_work_dir() -> PathBuf { - PathBuf::from("/srv/hyper") +fn default_work_dir() -> SafePathBuf { + SafePath::from_static::<"/srv/hyper">().to_owned() } -fn default_parent_cgroup() -> String { +const JAILS: &StrictComponent = StrictComponent::from_static("jails"); + +fn default_parent_cgroup() -> StrictComponentBuf { // Must match Elixir node's `@parent_cgroup`; operators need to keep them in sync. - "hyper".into() + StrictComponent::from_static("hyper").to_owned() } /// The `[network]` table: VM egress networking. Absent, or present without @@ -204,13 +172,13 @@ impl Config { } /// Hyper's data root. - pub fn hyper_base(&self) -> &std::path::Path { - self.work_dir.as_path() + pub fn hyper_base(&self) -> &SafePath { + &self.work_dir } /// Hyper's jail root, `/jails`. - pub fn jail_base(&self) -> PathBuf { - self.work_dir.join("jails") + pub fn jail_base(&self) -> SafePathBuf { + self.work_dir.join(JAILS) } /// The validated `dmsetup` binary the helper will run. @@ -285,19 +253,20 @@ impl Config { /// The jailer `--parent-cgroup` value, from `[jails] cgroup`. Defaults to /// `"hyper"`, matching the Elixir node's default. - pub fn parent_cgroup(&self) -> &str { + pub fn parent_cgroup(&self) -> &StrictComponent { &self.jails.cgroup } + pub fn parent_cgroup_dir(&self) -> SafePathBuf { + SafePath::from_static::<{ cgroup::ROOT }>().join(&self.jails.cgroup) + } + /// The UID/GID band the helper accepts from the BEAM. Defaults to /// `(900_000, 999_999)` when the key is absent (matching Elixir's defaults). /// A present range with min==0 or min>max is rejected at load time by /// [`Config::safe_load`], so this accessor is always total. - pub fn uid_gid_range(&self) -> (u32, u32) { - self.jails - .uid_gid_range - .map(|r| (r.min, r.max)) - .unwrap_or(DEFAULT_UID_GID) + pub fn uid_gid_range(&self) -> UidGidRange { + self.jails.uid_gid_range } /// Read, ownership-check, parse, and validate the config file. See the module @@ -324,15 +293,7 @@ impl Config { let body = std::io::read_to_string(std::fs::File::from(OwnedFd::from(file))) .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)); - } - - if let Some(r) = &config.jails.uid_gid_range { - validate_uid_gid_range(r)?; - } + toml::from_str(&body).map_err(|e| LoadingError::Malformed(path.clone(), e))?; Ok(config) } diff --git a/native/suidhelper/src/firecracker/chroot_jail.rs b/native/suidhelper/src/firecracker/chroot_jail.rs new file mode 100644 index 00000000..e94b7b88 --- /dev/null +++ b/native/suidhelper/src/firecracker/chroot_jail.rs @@ -0,0 +1,198 @@ +// SPDX-License-Identifier: MIT + +use crate::firecracker::id::JailOwner; +use crate::firecracker::jailer::VmId; +use crate::fs::dev::DeviceNode; +use crate::fs::dir::{self, SafeDir}; +use crate::fs::file::{self, Any, IsBlockDevice, IsRegularFile, IsSocket, OpenFlags, SafeFile}; +use crate::fs::path::{self, IsAbsolute, SafePath, SafePathBuf, StrictComponent, StrictComponents}; +use crate::linux::tun; +use nix::errno::Errno; +use nix::sys::stat::Mode; +use nix::unistd::{getgid, getuid}; +use serde::Serialize; +use std::io; +use std::path::PathBuf; +use thiserror::Error as ThisError; + +/// In-jail names — the contract the Elixir side (`Hyper.Node.FireVMM.ChrootJail`) +/// mirrors. +const KERNEL_NAME: &StrictComponent = StrictComponent::from_static("vmlinux"); +const ROOTFS_NAME: &StrictComponent = StrictComponent::from_static("rootfs"); +const ROOT_NAME: &StrictComponent = StrictComponent::from_static("root"); + +const KERNEL_MODE: Mode = Mode::from_bits_truncate(0o444); +const ROOTFS_MODE: Mode = Mode::from_bits_truncate(0o600); +const DIR_MODE: Mode = Mode::from_bits_truncate(0o755); + +/// Mode handed to the node: owner+group read/write, no world access. +const SOCKET_MODE: Mode = Mode::from_bits_truncate(0o660); + +/// Mode set on the jail `root` dir so the node's group can traverse it to +/// reach the socket: owner `rwx`, group `--x` (traverse, not list), other none. +const JAIL_ROOT_MODE: Mode = Mode::from_bits_truncate(0o710); + +#[derive(Debug, ThisError)] +pub enum OpenError { + #[error(transparent)] + Base(#[from] path::RelativeError), + #[error("jail root must be exactly //root below the jail base: {0:?}")] + Shape(PathBuf), + #[error("opening jail root: {0}")] + Open(#[from] file::ValidationError), +} + +#[derive(Debug, ThisError)] +pub enum KernelError { + #[error("resolving kernel source {path:?}: {source}")] + Resolve { + path: PathBuf, + #[source] + source: io::Error, + }, + #[error(transparent)] + Path(#[from] path::ValidationError), + #[error("kernel source must be under {base:?}: {path:?}")] + OutsideBase { base: PathBuf, path: PathBuf }, + #[error("opening kernel source {path:?}: {source}")] + Open { + path: PathBuf, + #[source] + source: file::ValidationError, + }, +} + +/// Result type shared by all `chroot-jail grant-*` ops. +#[derive(Debug, Serialize)] +#[serde(tag = "result", rename_all = "snake_case")] +pub enum GrantOut { + /// The socket was handed to the caller (chowned + chmoded). + Granted, + /// The socket does not exist yet; the caller should keep waiting. + Pending, +} + +#[derive(Debug, ThisError)] +pub enum GrantError { + #[error("not a socket (or is a symlink); refusing to touch it")] + NotASocket, + #[error("statting the socket: {0}")] + Stat(#[source] file::ValidationError), + #[error("chowning the socket to the caller: {0}")] + Chown(#[source] Errno), + #[error("chmoding the socket: {0}")] + Chmod(#[source] Errno), + #[error("chgrp-ing the jail root dir to the caller: {0}")] + ChgrpRoot(#[source] Errno), + #[error("chmoding the jail root dir for traversal: {0}")] + ChmodRoot(#[source] Errno), +} + +pub struct KernelImage(SafeFile); + +impl KernelImage { + pub fn open( + path: &SafePath, + hyper_base: &SafePath, + ) -> Result { + let real = + std::fs::canonicalize(path.as_path()).map_err(|source| KernelError::Resolve { + path: path.as_path().to_path_buf(), + source, + })?; + let real: SafePathBuf = real.try_into()?; + real.strip_base(hyper_base) + .map_err(|_| KernelError::OutsideBase { + base: hyper_base.as_path().to_path_buf(), + path: real.as_path().to_path_buf(), + })?; + let file = + SafeFile::open(&real, OpenFlags::RDONLY).map_err(|source| KernelError::Open { + path: real.as_path().to_path_buf(), + source, + })?; + Ok(Self(file)) + } +} + +pub struct JailRoot(SafeDir); + +impl JailRoot { + pub fn open( + jail_base: &SafePath, + path: &SafePath, + ) -> Result { + let mut segments = path.strip_base(jail_base)?.segments(); + let shape_ok = match ( + segments.next(), + segments.next(), + segments.next(), + segments.next(), + ) { + (Some(_exec), Some(id), Some(root), None) => { + root == ROOT_NAME + && id + .as_os_str() + .to_str() + .is_some_and(|id| id.parse::().is_ok()) + } + _ => false, + }; + if !shape_ok { + return Err(OpenError::Shape(path.as_path().to_path_buf())); + } + Ok(Self(SafeDir::open(path, OpenFlags::RDONLY)?)) + } + + pub fn stage_kernel(&self, kernel: KernelImage) -> Result<(), dir::Error> { + self.0.link_or_copy(kernel.0, KERNEL_NAME, KERNEL_MODE) + } + + pub fn add_rootfs( + &self, + device: &SafeFile, + owner: JailOwner, + ) -> Result<(), dir::Error> { + let node = DeviceNode::::new(device.rdev()?, ROOTFS_MODE); + self.0.make_device(ROOTFS_NAME, node, owner.into())?; + Ok(()) + } + + pub fn add_tun(&self, owner: JailOwner) -> Result<(), dir::Error> { + let net = self + .0 + .ensure_dir(tun::DEV, DIR_MODE)? + .ensure_dir(tun::NET, DIR_MODE)?; + net.make_device(tun::NAME, tun::NODE, owner.into())?; + Ok(()) + } + + /// Stat `leaf` under the jail root (`AT_SYMLINK_NOFOLLOW`), verify it is a real + /// socket (`S_IFSOCK`; a symlink reports `S_IFLNK` and is refused), then chown it + /// to the caller and chmod `0660`. Also open the jail root for the caller's + /// group to traverse (chgrp'd to the caller, chmod'd `0710`) so the node can + /// reach the socket even though the jailer left `root` at `0700`. Returns + /// `Pending` if `leaf` is not yet present (`ENOENT`). + pub fn grant_to_caller(&self, leaf: &StrictComponent) -> Result { + let sock: SafeFile = + match self.0.openat(leaf, OpenFlags::PATH, Mode::empty()) { + Ok(sock) => sock, + Err(file::ValidationError::Open(Errno::ENOENT)) => return Ok(GrantOut::Pending), + Err( + file::ValidationError::Open(Errno::ELOOP) + | file::ValidationError::WrongFileType, + ) => return Err(GrantError::NotASocket), + Err(e) => return Err(GrantError::Stat(e)), + }; + sock.chown(Some(getuid()), Some(getgid())) + .map_err(GrantError::Chown)?; + sock.chmod(SOCKET_MODE).map_err(GrantError::Chmod)?; + self.0 + .chown(None, Some(getgid())) + .map_err(GrantError::ChgrpRoot)?; + self.0 + .chmod(JAIL_ROOT_MODE) + .map_err(GrantError::ChmodRoot)?; + Ok(GrantOut::Granted) + } +} diff --git a/native/suidhelper/src/firecracker/id.rs b/native/suidhelper/src/firecracker/id.rs new file mode 100644 index 00000000..d5cb3c8a --- /dev/null +++ b/native/suidhelper/src/firecracker/id.rs @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: MIT + +use crate::fs::file::Owner; +use derive_more::{Display, Into}; +use nix::unistd::{Gid, Uid}; +use serde::Deserialize; +use thiserror::Error as ThisError; + +#[derive(Debug, ThisError, PartialEq, Eq)] +#[error("uid_gid_range.min must be >= 1 and <= max (got min={min}, max={max})")] +pub struct RangeError { + pub min: u32, + pub max: u32, +} + +#[derive(Debug, ThisError, PartialEq, Eq)] +#[error("--uid/--gid {value} is outside the configured range [{min}, {max}]")] +pub struct IdError { + pub value: u32, + pub min: u32, + pub max: u32, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(try_from = "[u32; 2]")] +pub struct UidGidRange { + min: u32, + max: u32, +} + +impl TryFrom<[u32; 2]> for UidGidRange { + type Error = RangeError; + + fn try_from([min, max]: [u32; 2]) -> Result { + if min == 0 || min > max { + return Err(RangeError { min, max }); + } + Ok(Self { min, max }) + } +} + +impl Default for UidGidRange { + fn default() -> Self { + Self { + min: 900_000, + max: 999_999, + } + } +} + +impl UidGidRange { + pub fn min(&self) -> u32 { + self.min + } + + pub fn max(&self) -> u32 { + self.max + } + + pub fn uid(&self, n: u32) -> Result { + self.check(n).map(|n| JailUid(Uid::from_raw(n))) + } + + pub fn gid(&self, n: u32) -> Result { + self.check(n).map(|n| JailGid(Gid::from_raw(n))) + } + + fn check(&self, value: u32) -> Result { + if (self.min..=self.max).contains(&value) { + Ok(value) + } else { + Err(IdError { + value, + min: self.min, + max: self.max, + }) + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Display, Into)] +pub struct JailUid(Uid); + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Display, Into)] +pub struct JailGid(Gid); + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct JailOwner { + pub uid: JailUid, + pub gid: JailGid, +} + +impl From for Owner { + fn from(owner: JailOwner) -> Self { + Owner { + uid: owner.uid.into(), + gid: owner.gid.into(), + } + } +} diff --git a/native/suidhelper/src/firecracker/jailer.rs b/native/suidhelper/src/firecracker/jailer.rs new file mode 100644 index 00000000..a61d54b1 --- /dev/null +++ b/native/suidhelper/src/firecracker/jailer.rs @@ -0,0 +1,152 @@ +// SPDX-License-Identifier: MIT +//! API into the Firecracker jailer. + +use crate::firecracker::id::{JailGid, JailUid}; +use crate::fs::path::{ + ComponentError, IsAbsolute, SafePath, StrictComponent, StrictComponentBuf, StrictComponents, +}; +use crate::process::binary::{RootSafeBin, RootSafeBinPathBuf}; +use crate::process::privilege::{RootCommandExt, Unprivileged}; +use bon::Builder; +use std::fmt; +use std::num::NonZeroU64; +use std::ops::Deref; +use std::process::Command; +use std::str::FromStr; +use thiserror::Error as ThisError; + +#[derive(Debug, ThisError)] +pub enum Error { + #[error("invalid --id {0:?}: must be 1..=64 chars of [A-Za-z0-9-], not starting with '-'")] + VmId(String), + #[error(transparent)] + Component(#[from] ComponentError), +} + +/// A VM name that can be accepted by Firecracker. +#[derive(Debug, Clone)] +pub struct VmId( + // This needs to be a path because jailer will expect to chroot into a directory of this name. + StrictComponentBuf, +); + +impl FromStr for VmId { + type Err = Error; + + fn from_str(s: &str) -> Result { + let jailer_grammar = (1..=64).contains(&s.len()) + && !s.starts_with('-') + && s.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'-'); + + if !jailer_grammar { + return Err(Error::VmId(s.to_string())); + } + + let component: &StrictComponent = s.try_into()?; + Ok(Self(component.to_owned())) + } +} + +impl Deref for VmId { + type Target = StrictComponent; + fn deref(&self) -> &StrictComponent { + &self.0 + } +} + +impl fmt::Display for VmId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0.as_os_str().to_string_lossy()) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, clap::Args)] +pub struct CpuMax { + #[arg(long = "cpu-quota-us")] + pub quota_us: NonZeroU64, + #[arg(long = "cpu-period-us")] + pub period_us: NonZeroU64, +} + +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Builder)] +pub struct CgroupLimits { + memory_max: Option, + cpu_max: Option, +} + +/// A fully-resolved jailer invocation. +#[derive(Builder)] +pub struct Jailer<'a> { + /// The binary to the jailer. + jailer: &'a RootSafeBin<"jailer">, + /// The binary to firecracker. + firecracker: &'a RootSafeBinPathBuf<"firecracker">, + /// The base directory for the jail. + jail_base: &'a SafePath, + /// The cgroup in which a new, per-vm cgroup will be created. + /// + /// ```text + /// /sys/fs/cgroup/ + /// hyper/ <- parent cgroup + /// / + /// / + /// ``` + parent_cgroup: &'a StrictComponent, + /// A unique name that is passed to the jailer. + vm: &'a VmId, + /// The user this VM runs under. + uid: JailUid, + /// The group this VM runs under. + gid: JailGid, + /// The explicit cgroup settings, given as `KEY=VALUE` pairs. + limits: CgroupLimits, + /// The path to the Firecracker API socket. + api_sock: &'a SafePath, + /// The netns the jailer must enter (`--netns`). + netns: Option<&'a SafePath>, +} + +impl Jailer<'_> { + fn command(&self) -> Command { + let mut cmd = self.jailer.command(); + cmd.arg("--id") + .arg(self.vm.as_os_str()) + .arg("--exec-file") + .arg(self.firecracker.as_path()) + .arg("--uid") + .arg(self.uid.to_string()) + .arg("--gid") + .arg(self.gid.to_string()) + .arg("--chroot-base-dir") + .arg(self.jail_base.as_path()) + .arg("--cgroup-version") + .arg("2") + .arg("--parent-cgroup") + .arg(self.parent_cgroup); + + if let Some(bytes) = self.limits.memory_max { + cmd.arg("--cgroup").arg(format!("memory.max={bytes}")); + } + + if let Some(CpuMax { + quota_us, + period_us, + }) = self.limits.cpu_max + { + cmd.arg("--cgroup") + .arg(format!("cpu.max={quota_us} {period_us}")); + } + + if let Some(netns) = self.netns { + cmd.arg("--netns").arg(netns.as_path()); + } + + cmd.arg("--").arg("--api-sock").arg(self.api_sock.as_path()); + cmd + } + + /// Permanently become root and `execve` the jailer in our place. + pub fn invoke(self, token: Unprivileged) -> ! { + self.command().exec_as_root(token) + } +} diff --git a/native/suidhelper/src/firecracker/mod.rs b/native/suidhelper/src/firecracker/mod.rs new file mode 100644 index 00000000..5108dadb --- /dev/null +++ b/native/suidhelper/src/firecracker/mod.rs @@ -0,0 +1,3 @@ +pub mod chroot_jail; +pub mod id; +pub mod jailer; diff --git a/native/suidhelper/src/fs/dev.rs b/native/suidhelper/src/fs/dev.rs index 5cd46784..8a1e19e7 100644 --- a/native/suidhelper/src/fs/dev.rs +++ b/native/suidhelper/src/fs/dev.rs @@ -1,6 +1,8 @@ +use super::file::DeviceType; use derive_more::{Constructor, Display}; use nix::libc::dev_t; -use nix::sys::stat::{major, makedev, minor}; +use nix::sys::stat::{major, makedev, minor, Mode}; +use std::marker::PhantomData; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Display, Constructor)] #[display("{major}:{minor}")] @@ -30,3 +32,27 @@ impl From for dev_t { makedev(dev.major, dev.minor) } } + +pub struct DeviceNode { + rdev: DeviceNumber, + mode: Mode, + kind: PhantomData, +} + +impl DeviceNode { + pub const fn new(rdev: DeviceNumber, mode: Mode) -> Self { + Self { + rdev, + mode, + kind: PhantomData, + } + } + + pub fn rdev(&self) -> DeviceNumber { + self.rdev + } + + pub fn mode(&self) -> Mode { + self.mode + } +} diff --git a/native/suidhelper/src/fs/dir.rs b/native/suidhelper/src/fs/dir.rs index 592b62d7..5bebccea 100644 --- a/native/suidhelper/src/fs/dir.rs +++ b/native/suidhelper/src/fs/dir.rs @@ -1,9 +1,10 @@ // SPDX-License-Identifier: MIT //! Safe directory operations. -use super::dev::DeviceNumber; +use super::dev::{DeviceNode, DeviceNumber}; use super::file::{ - Any, FileType, IsDirectory, OpenFlags, Ownership, SafeFile, ValidationError, Writability, + Any, DeviceType, FileType, IsDirectory, IsRegularFile, OpenFlags, Owner, Ownership, SafeFile, + ValidationError, Writability, }; use super::path::{StrictComponent, StrictComponentBuf}; use nix::dir::{Dir, Type}; @@ -13,6 +14,8 @@ use nix::libc; use nix::sys::stat::{fstat, mkdirat, mknodat, FileStat, Mode, SFlag}; use nix::unistd::{linkat, unlinkat, UnlinkatFlags}; use std::ffi::OsStr; +use std::fs::File; +use std::io; use std::os::unix::ffi::OsStrExt; use std::os::unix::io::{AsRawFd, FromRawFd, OwnedFd}; use std::path::Path; @@ -28,6 +31,13 @@ pub enum Error { Open(#[from] ValidationError), #[error(transparent)] Sys(#[from] Errno), + #[error(transparent)] + Io(#[from] io::Error), + #[error("existing device node is {found}, expected {expected}")] + DeviceMismatch { + expected: DeviceNumber, + found: DeviceNumber, + }, } impl Error { @@ -37,7 +47,7 @@ impl Error { match self { Error::Open(ValidationError::Open(errno) | ValidationError::Fstat(errno)) | Error::Sys(errno) => Some(*errno), - Error::Open(_) => None, + Error::Open(_) | Error::Io(_) | Error::DeviceMismatch { .. } => None, } } } @@ -111,6 +121,54 @@ impl SafeFile { ) } + pub fn ensure_dir(&self, name: &StrictComponent, mode: Mode) -> Result { + self.mkdirat(name, mode)?; + Ok(self.openat(name, OpenFlags::RDONLY, Mode::empty())?) + } + + pub fn make_device( + &self, + name: &StrictComponent, + node: DeviceNode, + owner: Owner, + ) -> Result, Error> { + match self.mknodat(name, T::KIND, node.mode(), node.rdev()) { + Ok(()) | Err(Errno::EEXIST) => {} + Err(errno) => return Err(errno.into()), + } + let device: SafeFile = self.openat(name, OpenFlags::PATH, Mode::empty())?; + let found = device.rdev()?; + if found != node.rdev() { + return Err(Error::DeviceMismatch { + expected: node.rdev(), + found, + }); + } + device.chown(Some(owner.uid), Some(owner.gid))?; + device.chmod(node.mode())?; + Ok(device) + } + + pub fn link_or_copy( + &self, + src: SafeFile, + name: &StrictComponent, + copy_mode: Mode, + ) -> Result<(), Error> { + match self.linkat(&src, name) { + Ok(()) => Ok(()), + Err(Errno::EXDEV) => { + let dest = self.mkfileat(name, copy_mode)?; + io::copy( + &mut File::from(OwnedFd::from(src)), + &mut File::from(OwnedFd::from(dest)), + )?; + Ok(()) + } + Err(errno) => Err(errno.into()), + } + } + /// Stat a file at the current directory. /// /// Note that this must remain private as this function is a footgun. It's very dangerous and diff --git a/native/suidhelper/src/fs/file.rs b/native/suidhelper/src/fs/file.rs index b9178c35..7b0fd651 100644 --- a/native/suidhelper/src/fs/file.rs +++ b/native/suidhelper/src/fs/file.rs @@ -214,10 +214,23 @@ impl SafeFile { } } -pub trait DeviceType: FileType {} +pub trait DeviceType: FileType { + const KIND: SFlag; +} + +impl DeviceType for IsBlockDevice { + const KIND: SFlag = SFlag::S_IFBLK; +} -impl DeviceType for IsBlockDevice {} -impl DeviceType for IsCharDevice {} +impl DeviceType for IsCharDevice { + const KIND: SFlag = SFlag::S_IFCHR; +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Owner { + pub uid: Uid, + pub gid: Gid, +} impl SafeFile { /// The device's `rdev` (major:minor), ready for `mknod`. diff --git a/native/suidhelper/src/fs/path.rs b/native/suidhelper/src/fs/path.rs index d4e96364..ff1b2dfa 100644 --- a/native/suidhelper/src/fs/path.rs +++ b/native/suidhelper/src/fs/path.rs @@ -3,12 +3,14 @@ use derive_more::AsRef; use ref_cast::{ref_cast_custom, RefCastCustom}; +use serde::{Deserialize, Deserializer}; use std::borrow::Borrow; use std::ffi::OsStr; use std::marker::PhantomData; use std::ops::Deref; use std::os::unix::ffi::OsStrExt; use std::path::{Path, PathBuf}; +use std::str::FromStr; use thiserror::Error as ThisError; pub const NAME_MAX: usize = 255; @@ -258,11 +260,11 @@ impl SafePath { /// Given `/b/x/y`, base `/b`, returns `x/y`. pub fn strip_base( &self, - base: &Path, + base: &SafePath, ) -> Result<&SafePath, RelativeError> { let rest = self .path - .strip_prefix(base) + .strip_prefix(base.as_path()) .map_err(|_| RelativeError::NotUnderBase)?; if rest.as_os_str().is_empty() { return Err(RelativeError::EqualsBase); @@ -343,6 +345,17 @@ impl From> for PathBuf { } } +impl FromStr for SafePathBuf +where + A: Absoluteness, + S: Components, +{ + type Err = ValidationError; + fn from_str(s: &str) -> Result { + PathBuf::from(s).try_into() + } +} + impl TryFrom for SafePathBuf where A: Absoluteness, @@ -355,3 +368,24 @@ where Ok(Self(path, PhantomData)) } } + +impl<'de, A, S> Deserialize<'de> for SafePathBuf +where + A: Absoluteness, + S: Components, +{ + fn deserialize>(deserializer: D) -> Result { + PathBuf::deserialize(deserializer)? + .try_into() + .map_err(serde::de::Error::custom) + } +} + +impl<'de> Deserialize<'de> for StrictComponentBuf { + fn deserialize>(deserializer: D) -> Result { + let raw = String::deserialize(deserializer)?; + let component: &StrictComponent = + raw.as_str().try_into().map_err(serde::de::Error::custom)?; + Ok(component.to_owned()) + } +} diff --git a/native/suidhelper/src/lib.rs b/native/suidhelper/src/lib.rs index 3f56f65f..6fe55d54 100644 --- a/native/suidhelper/src/lib.rs +++ b/native/suidhelper/src/lib.rs @@ -9,9 +9,9 @@ pub mod checksum; pub mod config; +pub mod firecracker; pub mod fs; pub mod linux; pub mod process; pub mod security_gate; pub mod tools; -pub mod util; diff --git a/native/suidhelper/src/linux/dm/mod.rs b/native/suidhelper/src/linux/dm/mod.rs index d7e099fa..2752d1ed 100644 --- a/native/suidhelper/src/linux/dm/mod.rs +++ b/native/suidhelper/src/linux/dm/mod.rs @@ -10,7 +10,7 @@ use super::Access; use crate::process::binary::RootSafeBin; use derive_more::Constructor; pub use device::Device; -pub use name::{DmName, InvalidDmName}; +pub use name::{DmName, InvalidDmName, OpenDeviceError}; use std::ffi::OsStr; use std::io; pub use table::{DmTable, LinearTable, Persistence, SnapshotTable, ThinPoolTable, ThinTable}; diff --git a/native/suidhelper/src/linux/dm/name.rs b/native/suidhelper/src/linux/dm/name.rs index 772f6852..671c5985 100644 --- a/native/suidhelper/src/linux/dm/name.rs +++ b/native/suidhelper/src/linux/dm/name.rs @@ -1,8 +1,11 @@ +use crate::fs::file::{self, Any, IsBlockDevice, OpenFlags, SafeFile}; use crate::fs::path::{ - IsAbsolute, SafePath, SafePathBuf, StrictComponent, StrictComponentBuf, StrictComponents, + self, IsAbsolute, SafePath, SafePathBuf, StrictComponent, StrictComponentBuf, StrictComponents, }; use derive_more::Display; use std::ffi::OsStr; +use std::io; +use std::path::PathBuf; use std::str::FromStr; use thiserror::Error as ThisError; @@ -12,6 +15,20 @@ const DM_NAME_MAX: usize = 127; #[error("invalid dm device name: {0:?}")] pub struct InvalidDmName(String); +#[derive(Debug, ThisError)] +pub enum OpenDeviceError { + #[error("resolving {path:?}: {source}")] + Resolve { + path: PathBuf, + #[source] + source: io::Error, + }, + #[error(transparent)] + Path(#[from] path::ValidationError), + #[error(transparent)] + Open(#[from] file::ValidationError), +} + #[derive(Debug, Clone, PartialEq, Eq, Display)] #[display("{}", _0.as_os_str().to_string_lossy())] pub struct DmName(StrictComponentBuf); @@ -34,6 +51,23 @@ impl DmName { pub fn path(&self) -> SafePathBuf { SafePath::from_static::<"/dev/mapper">().join(&self.0) } + + pub fn open(&self) -> Result, OpenDeviceError> { + // The dm form is a symlink under `/dev/mapper` — a root-owned `0755` dir an + // unprivileged caller cannot write — so resolving it to its real `/dev/dm-N` + // node cannot be redirected by the caller. We must resolve first: `SafeFile` + // refuses any symlink in the path (it must never follow an attacker-supplied + // one) and would otherwise reject the dm symlink itself. The re-opened target + // is still verified `IsBlockDevice`. + let link = self.path(); + let real = + std::fs::canonicalize(link.as_path()).map_err(|source| OpenDeviceError::Resolve { + path: link.as_path().to_path_buf(), + source, + })?; + let real: SafePathBuf = real.try_into()?; + Ok(SafeFile::open(&real, OpenFlags::PATH)?) + } } impl FromStr for DmName { diff --git a/native/suidhelper/src/linux/mod.rs b/native/suidhelper/src/linux/mod.rs index 793de6c5..f9fe7342 100644 --- a/native/suidhelper/src/linux/mod.rs +++ b/native/suidhelper/src/linux/mod.rs @@ -3,6 +3,8 @@ pub mod cgroup; pub mod dm; pub mod loopdev; +pub mod netns; +pub mod tun; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Access { diff --git a/native/suidhelper/src/linux/netns.rs b/native/suidhelper/src/linux/netns.rs new file mode 100644 index 00000000..5a443cdf --- /dev/null +++ b/native/suidhelper/src/linux/netns.rs @@ -0,0 +1 @@ +pub const DIR: &str = "/var/run/netns"; diff --git a/native/suidhelper/src/linux/tun.rs b/native/suidhelper/src/linux/tun.rs new file mode 100644 index 00000000..436d4720 --- /dev/null +++ b/native/suidhelper/src/linux/tun.rs @@ -0,0 +1,12 @@ +use crate::fs::dev::{DeviceNode, DeviceNumber}; +use crate::fs::file::IsCharDevice; +use crate::fs::path::StrictComponent; +use nix::sys::stat::Mode; + +// `/dev/net/tun`'s well-known major:minor, per Linux's `Documentation/admin-guide/devices.txt`. +pub const NODE: DeviceNode = + DeviceNode::new(DeviceNumber::new(10, 200), Mode::from_bits_truncate(0o666)); + +pub const DEV: &StrictComponent = StrictComponent::from_static("dev"); +pub const NET: &StrictComponent = StrictComponent::from_static("net"); +pub const NAME: &StrictComponent = StrictComponent::from_static("tun"); diff --git a/native/suidhelper/src/main.rs b/native/suidhelper/src/main.rs index 1ecf6367..d80543ad 100644 --- a/native/suidhelper/src/main.rs +++ b/native/suidhelper/src/main.rs @@ -83,7 +83,7 @@ impl SysTest { drop(token.elevate()?); Ok(Self { sys_test: "ok", - hyper_base: config::Config::get().hyper_base().to_path_buf(), + hyper_base: config::Config::get().hyper_base().as_path().to_path_buf(), }) } } @@ -121,13 +121,7 @@ fn run(mut token: Unprivileged) { Command::SysTest => SysTest::perform(&mut token) .map(Output::SysTest) .map_err(|e| e.to_string()), - Command::Jailer(args) => { - let e = jailer::run(args, token).expect_err("jailer::run only returns on error"); - eprintln!("{e}"); - // _exit bypasses atexit handlers; safe because we are permanently root - // at this point and must not run any cleanup registered before escalation. - unsafe { nix::libc::_exit(2) } - } + Command::Jailer(args) => jailer::run(args, token), Command::Version => unreachable!("handled above"), }; diff --git a/native/suidhelper/src/tools/chroot_jail/grant.rs b/native/suidhelper/src/tools/chroot_jail/grant.rs deleted file mode 100644 index 1d44ce61..00000000 --- a/native/suidhelper/src/tools/chroot_jail/grant.rs +++ /dev/null @@ -1,80 +0,0 @@ -// SPDX-License-Identifier: MIT -//! Shared grant-socket logic reused by `grant-api` and `grant-vsock`. -//! -//! Both ops chown a jail-confined Unix-domain socket to the helper's caller and -//! chmod it `0660`, then open the immediate parent `root` dir for group traversal. -//! This module holds the common constants, the `GrantOut` result type, the -//! `O_NOFOLLOW` walk helper, and the core grant execution so neither op duplicates -//! the logic block. - -use crate::fs::dir::SafeDir; -use crate::fs::file::{self, Any, IsSocket, OpenFlags, SafeFile}; -use crate::fs::path::StrictComponent; -use nix::errno::Errno; -use nix::sys::stat::Mode; -use nix::unistd::{getgid, getuid}; -use serde::Serialize; -use thiserror::Error as ThisError; - -/// Mode handed to the node: owner+group read/write, no world access. -pub const SOCKET_MODE: u32 = 0o660; - -/// Mode set on the jail `root` dir so the node's group can traverse it to -/// reach the socket: owner `rwx`, group `--x` (traverse, not list), other none. -pub const JAIL_ROOT_MODE: u32 = 0o710; - -/// Result type shared by all `chroot-jail grant-*` ops. -#[derive(Debug, Serialize)] -#[serde(tag = "result", rename_all = "snake_case")] -pub enum GrantOut { - /// The socket was handed to the caller (chowned + chmoded). - Granted, - /// The socket does not exist yet; the caller should keep waiting. - Pending, -} - -/// Errors that can arise in the shared grant execution (stat / chown / chmod). -/// Each `grant-*` op's `Error` wraps these transparently as its `Grant` -/// variant. -#[derive(Debug, ThisError)] -pub enum GrantError { - #[error("not a socket (or is a symlink); refusing to touch it")] - NotASocket, - #[error("statting the socket: {0}")] - Stat(#[source] file::ValidationError), - #[error("chowning the socket to the caller: {0}")] - Chown(#[source] Errno), - #[error("chmoding the socket: {0}")] - Chmod(#[source] Errno), - #[error("chgrp-ing the jail root dir to the caller: {0}")] - ChgrpRoot(#[source] Errno), - #[error("chmoding the jail root dir for traversal: {0}")] - ChmodRoot(#[source] Errno), -} - -/// Stat `leaf` under `root` (`AT_SYMLINK_NOFOLLOW`), verify it is a real socket -/// (`S_IFSOCK`; a symlink reports `S_IFLNK` and is refused), then chown it to -/// the caller and chmod `0660`. Also open the parent `root` dir for the caller's -/// group to traverse (chgrp'd to the caller, chmod'd `0710`) so the node can -/// reach the socket even though the jailer left `root` at `0700`. Returns -/// `Pending` if `leaf` is not yet present (`ENOENT`). -pub fn grant_to_caller(root: SafeDir, leaf: &StrictComponent) -> Result { - let sock: SafeFile = match root.openat(leaf, OpenFlags::PATH, Mode::empty()) - { - Ok(sock) => sock, - Err(file::ValidationError::Open(Errno::ENOENT)) => return Ok(GrantOut::Pending), - Err(file::ValidationError::Open(Errno::ELOOP) | file::ValidationError::WrongFileType) => { - return Err(GrantError::NotASocket) - } - Err(e) => return Err(GrantError::Stat(e)), - }; - sock.chown(Some(getuid()), Some(getgid())) - .map_err(GrantError::Chown)?; - sock.chmod(Mode::from_bits_truncate(SOCKET_MODE)) - .map_err(GrantError::Chmod)?; - root.chown(None, Some(getgid())) - .map_err(GrantError::ChgrpRoot)?; - root.chmod(Mode::from_bits_truncate(JAIL_ROOT_MODE)) - .map_err(GrantError::ChmodRoot)?; - Ok(GrantOut::Granted) -} diff --git a/native/suidhelper/src/tools/chroot_jail/grant_api.rs b/native/suidhelper/src/tools/chroot_jail/grant_api.rs index b3405198..7e3bf2fa 100644 --- a/native/suidhelper/src/tools/chroot_jail/grant_api.rs +++ b/native/suidhelper/src/tools/chroot_jail/grant_api.rs @@ -31,20 +31,19 @@ //! the path) is `Pending`, not an error: firecracker has not created it yet, so //! the controller keeps probing. -use super::grant; use crate::config::Config; -use crate::fs::dir::SafeDir; -use crate::fs::file::{self, OpenFlags}; -use crate::fs::path::{self, IsAbsolute, SafePathBuf, StrictComponent, StrictComponents}; +use crate::firecracker::chroot_jail::{self, JailRoot}; +use crate::fs::file; +use crate::fs::path::{self, IsAbsolute, SafePath, SafePathBuf, StrictComponent, StrictComponents}; use crate::process::privilege::{Root, Unprivileged}; use crate::tools::IsTool; use clap::Args; use nix::errno::Errno; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::str::FromStr; use thiserror::Error as ThisError; -pub use super::grant::{GrantError, GrantOut}; +pub use crate::firecracker::chroot_jail::{GrantError, GrantOut}; /// The fixed in-jail socket name firecracker opens (mirrors the Elixir /// `Hyper.Node.FireVMM.Jailer` `@jail_socket`). @@ -124,17 +123,20 @@ impl IsTool for GrantApi { /// Hand `socket` (`///root/api.socket`) to the helper's /// caller, fd-relative after an `O_NOFOLLOW` walk from `jail_base`. Returns /// `Pending` if any path component or the socket itself is not yet present. -pub fn grant_api_under(jail_base: &Path, socket: &ApiSocket) -> Result { - let rel = socket.0.strip_base(jail_base).map_err(Error::SocketBase)?; - let (4, Some((root, leaf))) = (rel.segments().count(), socket.0.split_last()) else { - return Err(Error::SocketShape(socket.0.as_ref().to_path_buf())); - }; - - let root = match SafeDir::open(root, OpenFlags::RDONLY) { +pub fn grant_api_under( + jail_base: &SafePath, + socket: &ApiSocket, +) -> Result { + let shape = || Error::SocketShape(socket.0.as_ref().to_path_buf()); + let (root, leaf) = socket.0.split_last().ok_or_else(shape)?; + let root = match JailRoot::open(jail_base, root) { Ok(root) => root, - Err(file::ValidationError::Open(Errno::ENOENT)) => return Ok(GrantOut::Pending), - Err(e) => return Err(Error::Walk(e)), + Err(chroot_jail::OpenError::Open(file::ValidationError::Open(Errno::ENOENT))) => { + return Ok(GrantOut::Pending) + } + Err(chroot_jail::OpenError::Base(e)) => return Err(Error::SocketBase(e)), + Err(chroot_jail::OpenError::Shape(_)) => return Err(shape()), + Err(chroot_jail::OpenError::Open(e)) => return Err(Error::Walk(e)), }; - - grant::grant_to_caller(root, leaf).map_err(Error::from) + Ok(root.grant_to_caller(leaf)?) } diff --git a/native/suidhelper/src/tools/chroot_jail/grant_vsock.rs b/native/suidhelper/src/tools/chroot_jail/grant_vsock.rs index 04cfe91d..79f25096 100644 --- a/native/suidhelper/src/tools/chroot_jail/grant_vsock.rs +++ b/native/suidhelper/src/tools/chroot_jail/grant_vsock.rs @@ -15,20 +15,19 @@ //! A missing socket (`ENOENT`, anywhere on the path) is `Pending`: firecracker has //! not configured vsock yet, so the controller keeps probing. -use super::grant; use crate::config::Config; -use crate::fs::dir::SafeDir; -use crate::fs::file::{self, OpenFlags}; -use crate::fs::path::{self, IsAbsolute, SafePathBuf, StrictComponent, StrictComponents}; +use crate::firecracker::chroot_jail::{self, JailRoot}; +use crate::fs::file; +use crate::fs::path::{self, IsAbsolute, SafePath, SafePathBuf, StrictComponent, StrictComponents}; use crate::process::privilege::{Root, Unprivileged}; use crate::tools::IsTool; use clap::Args; use nix::errno::Errno; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::str::FromStr; use thiserror::Error as ThisError; -pub use super::grant::{GrantError, GrantOut}; +pub use crate::firecracker::chroot_jail::{GrantError, GrantOut}; /// The fixed in-jail socket name Firecracker opens for vsock (mirrors the Elixir /// `Hyper.Node.FireVMM.Jailer` `@jail_vsock`). @@ -109,17 +108,20 @@ impl IsTool for GrantVsock { /// Hand `socket` (`///root/vsock.sock`) to the helper's /// caller, fd-relative after an `O_NOFOLLOW` walk from `jail_base`. Returns /// `Pending` if any path component or the socket itself is not yet present. -pub fn grant_vsock_under(jail_base: &Path, socket: &VsockSocket) -> Result { - let rel = socket.0.strip_base(jail_base).map_err(Error::SocketBase)?; - let (4, Some((root, leaf))) = (rel.segments().count(), socket.0.split_last()) else { - return Err(Error::SocketShape(socket.0.as_ref().to_path_buf())); - }; - - let root = match SafeDir::open(root, OpenFlags::RDONLY) { +pub fn grant_vsock_under( + jail_base: &SafePath, + socket: &VsockSocket, +) -> Result { + let shape = || Error::SocketShape(socket.0.as_ref().to_path_buf()); + let (root, leaf) = socket.0.split_last().ok_or_else(shape)?; + let root = match JailRoot::open(jail_base, root) { Ok(root) => root, - Err(file::ValidationError::Open(Errno::ENOENT)) => return Ok(GrantOut::Pending), - Err(e) => return Err(Error::Walk(e)), + Err(chroot_jail::OpenError::Open(file::ValidationError::Open(Errno::ENOENT))) => { + return Ok(GrantOut::Pending) + } + Err(chroot_jail::OpenError::Base(e)) => return Err(Error::SocketBase(e)), + Err(chroot_jail::OpenError::Shape(_)) => return Err(shape()), + Err(chroot_jail::OpenError::Open(e)) => return Err(Error::Walk(e)), }; - - grant::grant_to_caller(root, leaf).map_err(Error::from) + Ok(root.grant_to_caller(leaf)?) } diff --git a/native/suidhelper/src/tools/chroot_jail/mod.rs b/native/suidhelper/src/tools/chroot_jail/mod.rs index 08b9d50d..519477c3 100644 --- a/native/suidhelper/src/tools/chroot_jail/mod.rs +++ b/native/suidhelper/src/tools/chroot_jail/mod.rs @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT //! `chroot-jail`: per-VM chroot/jail lifecycle. -mod grant; pub mod grant_api; pub mod grant_vsock; mod prepare; diff --git a/native/suidhelper/src/tools/chroot_jail/prepare.rs b/native/suidhelper/src/tools/chroot_jail/prepare.rs index b9ce3b1a..72aa5616 100644 --- a/native/suidhelper/src/tools/chroot_jail/prepare.rs +++ b/native/suidhelper/src/tools/chroot_jail/prepare.rs @@ -2,23 +2,43 @@ //! `chroot-jail prepare`: stage the kernel file and create the rootfs device node //! inside a VM chroot, via the [`ChrootJail`] builder. -use crate::linux::dm::DmName; +use crate::config::Config; +use crate::firecracker::chroot_jail::{self, JailRoot, KernelImage}; +use crate::firecracker::id::JailOwner; +use crate::fs::dir; +use crate::fs::path::{IsAbsolute, SafePathBuf, StrictComponents}; +use crate::linux::dm::{DmName, OpenDeviceError}; use crate::process::privilege::{Root, Unprivileged}; use crate::tools::parse_dm_device; use crate::tools::IsTool; -use crate::util::chroot_jail::{self, ChrootJail}; use clap::Args; use serde::Serialize; -use std::path::PathBuf; +use thiserror::Error as ThisError; + +#[derive(Debug, ThisError)] +pub enum Error { + #[error(transparent)] + Jail(#[from] chroot_jail::OpenError), + #[error(transparent)] + Kernel(#[from] chroot_jail::KernelError), + #[error("opening rootfs device: {0}")] + Device(#[from] OpenDeviceError), + #[error("staging kernel: {0}")] + StageKernel(#[source] dir::Error), + #[error("creating rootfs node: {0}")] + Rootfs(#[source] dir::Error), + #[error("creating /dev/net/tun: {0}")] + Tun(#[source] dir::Error), +} #[derive(Args)] pub struct PrepareArgs { /// Chroot root directory (under JAIL_BASE, shape //). #[arg(long)] - chroot: PathBuf, + chroot: SafePathBuf, /// Host kernel image path (under /srv/hyper); staged as /vmlinux. #[arg(long)] - kernel: PathBuf, + kernel: SafePathBuf, /// Host block device to mirror as /rootfs (e.g. /dev/loop0 or /// /dev/mapper/hyper-vm1). Its major:minor are read by the helper. #[arg(long, value_parser = parse_dm_device)] @@ -42,24 +62,42 @@ pub fn run( args: PrepareArgs, token: &mut Unprivileged, ) -> Result { - Prepare { args }.run(token) + let config = Config::get(); + let range = config.uid_gid_range(); + let owner = JailOwner { + uid: range + .uid(args.uid) + .map_err(|e| crate::tools::Error::Tool(Box::new(e)))?, + gid: range + .gid(args.gid) + .map_err(|e| crate::tools::Error::Tool(Box::new(e)))?, + }; + let tun = config.network().is_some(); + Prepare { args, owner, tun }.run(token) } struct Prepare { args: PrepareArgs, + owner: JailOwner, + tun: bool, } impl IsTool for Prepare { type Args = PrepareArgs; type Output = PrepareOut; - type RunT = Result<(), chroot_jail::Error>; + type RunT = Result<(), Error>; fn run_privileged(&self, _root: &Root<'_>) -> Self::RunT { - let args = &self.args; - ChrootJail::new(args.chroot.clone(), args.uid, args.gid) - .with_kernel(args.kernel.clone()) - .with_rootfs(args.device.clone()) - .build() + let config = Config::get(); + let root = JailRoot::open(&config.jail_base(), &self.args.chroot)?; + let kernel = KernelImage::open(&self.args.kernel, config.hyper_base())?; + root.stage_kernel(kernel).map_err(Error::StageKernel)?; + root.add_rootfs(&self.args.device.open()?, self.owner) + .map_err(Error::Rootfs)?; + if self.tun { + root.add_tun(self.owner).map_err(Error::Tun)?; + } + Ok(()) } fn parse(&self, res: Self::RunT) -> Result> { diff --git a/native/suidhelper/src/tools/chroot_jail/remove.rs b/native/suidhelper/src/tools/chroot_jail/remove.rs index 14381375..c2b3a187 100644 --- a/native/suidhelper/src/tools/chroot_jail/remove.rs +++ b/native/suidhelper/src/tools/chroot_jail/remove.rs @@ -14,6 +14,7 @@ //! (`ENOENT`, and for the cgroup `ENOTEMPTY`) is success. use crate::config::Config; +use crate::firecracker::jailer::{self, VmId}; use crate::fs::dir::{self, SafeDir}; use crate::fs::file::{self, OpenFlags}; use crate::fs::path::{self, IsAbsolute, SafePath, StrictComponents}; @@ -24,6 +25,7 @@ use clap::Args; use nix::errno::Errno; use serde::Serialize; use std::path::{Path, PathBuf}; +use std::str::FromStr; use std::time::Duration; use thiserror::Error as ThisError; @@ -59,8 +61,10 @@ pub enum CgroupError { Path(#[source] path::ValidationError), #[error("--cgroup path: {0}")] Base(#[source] path::RelativeError), - #[error("--cgroup must be at least two components below /sys/fs/cgroup: {0:?}")] - Depth(PathBuf), + #[error("--cgroup must be exactly /: {0:?}")] + Shape(PathBuf), + #[error("--cgroup leaf: {0}")] + Leaf(#[source] jailer::Error), #[error("opening cgroup: {0}")] Open(#[source] cgroup::OpenError), #[error("killing cgroup procs: {0}")] @@ -105,7 +109,7 @@ impl IsTool for Remove { fn run_privileged(&self, _root: &Root<'_>) -> Self::RunT { // Cgroup first: it SIGKILLs any firecracker still alive in the leaf, so the // chroot teardown below is not yanking files out from under a live process. - remove_cgroup_under(Path::new(cgroup::ROOT), &self.args.cgroup)?; + remove_cgroup_under(&Config::get().parent_cgroup_dir(), &self.args.cgroup)?; remove_chroot_under(&Config::get().jail_base(), &self.args.chroot)?; Ok(()) } @@ -119,7 +123,7 @@ impl IsTool for Remove { /// Recursively remove the per-VM chroot `//`, fd-relative /// after an `O_NOFOLLOW` walk from `jail_base`. Idempotent on a missing target. /// `--chroot` must be exactly `/` (one parent component, one leaf). -pub fn remove_chroot_under(jail_base: &Path, chroot: &Path) -> Result<(), ChrootError> { +pub fn remove_chroot_under(jail_base: &LexicalPath, chroot: &Path) -> Result<(), ChrootError> { let path: &LexicalPath = chroot.try_into().map_err(ChrootError::Path)?; let rel = path.strip_base(jail_base).map_err(ChrootError::Base)?; let (2, Some((parent, id))) = (rel.segments().count(), path.split_last()) else { @@ -143,12 +147,18 @@ pub fn remove_chroot_under(jail_base: &Path, chroot: &Path) -> Result<(), Chroot /// be at least two components below `base` (one or more parents, plus the leaf). /// Idempotent: a missing leaf is success, and a leaf that has not finished /// draining after the kill (`EBUSY`/`ENOTEMPTY`) is left for a later sweep. -pub fn remove_cgroup_under(base: &Path, cgroup: &Path) -> Result<(), CgroupError> { +pub fn remove_cgroup_under(parent: &LexicalPath, cgroup: &Path) -> Result<(), CgroupError> { let path: &LexicalPath = cgroup.try_into().map_err(CgroupError::Path)?; - let rel = path.strip_base(base).map_err(CgroupError::Base)?; - if rel.segments().count() < 2 { - return Err(CgroupError::Depth(cgroup.to_path_buf())); - } + let rel = path.strip_base(parent).map_err(CgroupError::Base)?; + let mut segments = rel.segments(); + let (Some(leaf), None) = (segments.next(), segments.next()) else { + return Err(CgroupError::Shape(cgroup.to_path_buf())); + }; + let leaf = leaf + .as_os_str() + .to_str() + .ok_or_else(|| CgroupError::Shape(cgroup.to_path_buf()))?; + VmId::from_str(leaf).map_err(CgroupError::Leaf)?; // Kill any process still in the leaf cgroup before removing it. Open the leaf // O_NOFOLLOW (one step past the parent walk) and write "1" to cgroup.kill. diff --git a/native/suidhelper/src/tools/dmsetup/backing.rs b/native/suidhelper/src/tools/dmsetup/backing.rs index 8aa96972..7fcbf1a9 100644 --- a/native/suidhelper/src/tools/dmsetup/backing.rs +++ b/native/suidhelper/src/tools/dmsetup/backing.rs @@ -25,14 +25,17 @@ pub enum BackingError { pub struct BackingFile(Arc>); impl BackingFile { - pub fn open(path: &Path, base: &Path) -> Result { + pub fn open( + path: &Path, + base: &SafePath, + ) -> Result { let real = std::fs::canonicalize(path).map_err(|source| BackingError::Canonicalize { path: path.to_path_buf(), source, })?; - if !real.starts_with(base) { + if !real.starts_with(base.as_path()) { return Err(BackingError::OutsideBase { - base: base.to_path_buf(), + base: base.as_path().to_path_buf(), path: real, }); } diff --git a/native/suidhelper/src/tools/jailer.rs b/native/suidhelper/src/tools/jailer.rs index e25cb663..66a1f193 100644 --- a/native/suidhelper/src/tools/jailer.rs +++ b/native/suidhelper/src/tools/jailer.rs @@ -8,24 +8,28 @@ //! across the image replacement. There is no output and no return on success. //! //! The validated newtypes, their refusal contracts, and the `execve` mechanics -//! live in [`crate::util::jailer`]; this module is only the clap surface plus +//! live in [`crate::firecracker::jailer`]; this module is only the clap surface plus //! the trusted-config resolution that feeds them. use crate::config::{BinError, Config}; +use crate::firecracker::id::IdError; +use crate::firecracker::jailer::{CgroupLimits, CpuMax, Jailer, VmId}; +use crate::fs::path::{IsAbsolute, SafePath, SafePathBuf, StrictComponents}; +use crate::linux::netns; use crate::process::privilege::Unprivileged; -use crate::util::jailer::Jailer; use clap::Args; -use std::path::PathBuf; +use std::convert::Infallible; +use std::num::NonZeroU64; use thiserror::Error as ThisError; -pub use crate::util::jailer::{validate_id_number, CgroupSetting, JailSock, VmId}; - #[derive(Debug, ThisError)] pub enum Error { #[error(transparent)] Bin(#[from] BinError), #[error(transparent)] - Jail(#[from] crate::util::jailer::Error), + Jail(#[from] crate::firecracker::jailer::Error), + #[error(transparent)] + Id(#[from] IdError), } #[derive(Args)] @@ -39,12 +43,13 @@ pub struct JailerArgs { /// Unprivileged gid the jailer drops to (rejected if 0 or out of range). #[arg(long)] gid: u32, - /// Repeatable `KEY=VALUE` cgroup setting from the allowlist. - #[arg(long = "cgroup")] - cgroup: Vec, + #[arg(long)] + memory_max: Option, + #[command(flatten)] + cpu_max: Option, /// Absolute firecracker API socket path (single filename under `/`). #[arg(long = "api-sock")] - api_sock: JailSock, + api_sock: SafePathBuf, } /// Validate the caller's args against trusted config, then hand off to @@ -52,7 +57,13 @@ pub struct JailerArgs { /// On success this never returns (the process image is replaced); the `Ok` arm /// is therefore [`std::convert::Infallible`]. Every failure is returned as /// [`Error`] for the caller to print and exit non-zero. -pub fn run(args: JailerArgs, token: Unprivileged) -> Result { +pub fn run(args: JailerArgs, token: Unprivileged) -> ! { + let Err(e) = try_run(args, token); + eprintln!("{e}"); + std::process::exit(2) +} + +fn try_run(args: JailerArgs, token: Unprivileged) -> Result { let config = Config::get(); // Resolve everything that can fail as the REAL uid, before any privilege is @@ -62,16 +73,16 @@ pub fn run(args: JailerArgs, token: Unprivileged) -> Result` path the network tool // creates. Networking is mandatory, so in practice this is always `Some`. - let netns: Option = config + let netns = config .network() - .map(|_| PathBuf::from(format!("/var/run/netns/{}", args.id))); + .map(|_| SafePath::from_static::<{ netns::DIR }>().join(&args.id)); Jailer::builder() .jailer(&jailer) @@ -81,10 +92,14 @@ pub fn run(args: JailerArgs, token: Unprivileged) -> Result Result { - let floor = uid_range.0; + let uid = Uid::from(uid).as_raw(); + let floor = uid_range.min(); if uid < floor { return Err(AddrError::UidBelowRange { uid, floor }); } diff --git a/native/suidhelper/src/tools/network/prepare.rs b/native/suidhelper/src/tools/network/prepare.rs index 8a71749c..32fe9125 100644 --- a/native/suidhelper/src/tools/network/prepare.rs +++ b/native/suidhelper/src/tools/network/prepare.rs @@ -5,9 +5,10 @@ use super::args; use super::exec; use super::NetworkOut; use crate::config::Config; +use crate::firecracker::id::{IdError, UidGidRange}; +use crate::firecracker::jailer::VmId; use crate::process::binary::{self, RootSafeBin}; use crate::process::privilege::{Root, Unprivileged}; -use crate::tools::jailer::{self, VmId}; use crate::tools::IsTool; use clap::Args; use std::net::Ipv4Addr; @@ -31,7 +32,7 @@ pub enum Error { #[error("clone_pool {0:?} is not a valid IPv4 CIDR")] InvalidClonePool(String), #[error(transparent)] - Uid(#[from] crate::util::jailer::Error), + Uid(#[from] IdError), #[error(transparent)] Addr(#[from] AddrError), #[error(transparent)] @@ -41,17 +42,17 @@ pub enum Error { } /// Derive the [`Plan`] for `uid`, refusing before any command is ever run. -/// `uid` must satisfy [`jailer::validate_id_number`] against `range` — the +/// `uid` must satisfy [`UidGidRange::uid`] against `range` — the /// same check the jailer itself applies to `--uid`, so a uid the jailer would /// refuse is refused here too — and `clone_pool_str` must be a parseable IPv4 /// CIDR. pub fn plan_from( uid: u32, - range: (u32, u32), + range: UidGidRange, clone_pool_str: &str, vm_id: &VmId, ) -> Result { - jailer::validate_id_number(uid, range)?; + let uid = range.uid(uid)?; let clone_pool = parse_cidr_base(clone_pool_str)?; Ok(Plan::derive(uid, range, clone_pool, vm_id)?) } diff --git a/native/suidhelper/src/tools/network/teardown.rs b/native/suidhelper/src/tools/network/teardown.rs index 6305dec8..3b4e0b98 100644 --- a/native/suidhelper/src/tools/network/teardown.rs +++ b/native/suidhelper/src/tools/network/teardown.rs @@ -4,9 +4,9 @@ use super::args; use super::exec; use super::prepare; use super::NetworkOut; +use crate::firecracker::jailer::VmId; use crate::process::binary::RootSafeBin; use crate::process::privilege::{Root, Unprivileged}; -use crate::tools::jailer::VmId; use crate::tools::IsTool; use clap::Args; use thiserror::Error as ThisError; diff --git a/native/suidhelper/src/tools/network/teardown_orphan.rs b/native/suidhelper/src/tools/network/teardown_orphan.rs index 7adb2c70..c95a6965 100644 --- a/native/suidhelper/src/tools/network/teardown_orphan.rs +++ b/native/suidhelper/src/tools/network/teardown_orphan.rs @@ -18,9 +18,9 @@ use super::args::{self, Command}; use super::NetworkOut; use crate::config::Config; +use crate::firecracker::jailer::VmId; use crate::process::binary::{self, RootSafeBin}; use crate::process::privilege::{Root, Unprivileged}; -use crate::tools::jailer::VmId; use crate::tools::IsTool; use clap::Args; use thiserror::Error as ThisError; diff --git a/native/suidhelper/src/util/chroot_jail.rs b/native/suidhelper/src/util/chroot_jail.rs deleted file mode 100644 index fb7ca6fb..00000000 --- a/native/suidhelper/src/util/chroot_jail.rs +++ /dev/null @@ -1,262 +0,0 @@ -// SPDX-License-Identifier: MIT -//! Declarative builder for a VM's chroot jail contents. - -use crate::config::Config; -use crate::fs::dev::DeviceNumber; -use crate::fs::dir::{self, SafeDir}; -use crate::fs::file::{self, Any, IsBlockDevice, IsCharDevice, IsRegularFile, OpenFlags, SafeFile}; -use crate::fs::path::{self, IsAbsolute, SafePath, StrictComponent, StrictComponents}; -use crate::linux::dm::DmName; -use nix::errno::Errno; -use nix::sys::stat::{Mode, SFlag}; -use nix::unistd::{Gid, Uid}; -use std::fs::File; -use std::io; -use std::os::unix::io::OwnedFd; -use std::path::{Path, PathBuf}; -use thiserror::Error as ThisError; - -/// In-jail names — the contract the Elixir side (`Hyper.Node.FireVMM.ChrootJail`) -/// mirrors. -const KERNEL_NAME: &StrictComponent = StrictComponent::from_static("vmlinux"); -const ROOTFS_NAME: &StrictComponent = StrictComponent::from_static("rootfs"); -const DEV: &StrictComponent = StrictComponent::from_static("dev"); -const NET: &StrictComponent = StrictComponent::from_static("net"); -const TUN: &StrictComponent = StrictComponent::from_static("tun"); - -/// `/dev/net/tun`'s well-known major:minor, per Linux's `Documentation/admin-guide/devices.txt`. -const TUN_DEVICE: DeviceNumber = DeviceNumber::new(10, 200); - -#[derive(Debug, ThisError)] -pub enum Error { - #[error(transparent)] - Path(#[from] path::ValidationError), - #[error(transparent)] - Relative(#[from] path::RelativeError), - #[error("opening chroot: {0}")] - OpenChroot(#[source] file::ValidationError), - #[error("staging kernel: {0}")] - StageKernel(#[source] dir::Error), - #[error("opening rootfs device: {0}")] - Device(#[source] file::ValidationError), - #[error("creating rootfs node: {0}")] - MakeRootfs(#[source] dir::Error), - #[error("creating /dev/net/tun: {0}")] - MakeTun(#[source] dir::Error), - #[error("kernel source {path}: {source}")] - Source { - path: PathBuf, - #[source] - source: io::Error, - }, - #[error("kernel source must be under {}: {path}", .base.display())] - OutsideBase { base: PathBuf, path: PathBuf }, - #[error("open kernel source {path}: {source}")] - OpenSrc { - path: PathBuf, - #[source] - source: file::ValidationError, - }, - #[error("copy kernel: {0}")] - Copy(#[source] io::Error), - #[error("resolving block device {path}: {source}")] - ResolveDev { - path: PathBuf, - #[source] - source: io::Error, - }, -} - -/// An unfilled artifact slot. -pub struct Unset; -/// The kernel slot, carrying its host source path. -pub struct Kernel(PathBuf); -/// The rootfs slot, carrying the block device to mirror. -pub struct Rootfs(DmName); - -/// A description of a VM chroot's contents. Type parameters track which slots are -/// filled; see the module docs. -pub struct ChrootJail { - chroot: PathBuf, - uid: u32, - gid: u32, - kernel: K, - rootfs: R, -} - -impl ChrootJail { - /// A jail at `chroot` (shape `//`), everything owned by - /// `uid:gid`. - pub fn new(chroot: impl Into, uid: u32, gid: u32) -> Self { - Self { - chroot: chroot.into(), - uid, - gid, - kernel: Unset, - rootfs: Unset, - } - } -} - -impl ChrootJail { - /// Stage the guest kernel from host path `source` as `vmlinux`. - pub fn with_kernel(self, source: impl Into) -> ChrootJail { - ChrootJail { - chroot: self.chroot, - uid: self.uid, - gid: self.gid, - kernel: Kernel(source.into()), - rootfs: self.rootfs, - } - } -} - -impl ChrootJail { - /// Create the rootfs block node as `rootfs`, mirroring `device`. - pub fn with_rootfs(self, device: DmName) -> ChrootJail { - ChrootJail { - chroot: self.chroot, - uid: self.uid, - gid: self.gid, - kernel: self.kernel, - rootfs: Rootfs(device), - } - } -} - -impl ChrootJail { - /// Realize the jail on disk under the configured `JAIL_BASE`/`HYPER_BASE`. - pub fn build(self) -> Result<(), Error> { - let cfg = Config::get(); - self.build_under(&cfg.jail_base(), cfg.hyper_base()) - } - - /// Realize the jail relative to explicit base directories: confined walk to - /// the chroot dir, then stage the kernel and create the rootfs node relative - /// to that fd. The public [`build`](Self::build) wires `Config` into this. - pub fn build_under(self, jail_base: &Path, hyper_base: &Path) -> Result<(), Error> { - let chroot = open_chroot_under(jail_base, &self.chroot)?; - stage_kernel_under(&chroot, &self.kernel.0, hyper_base, self.uid, self.gid)?; - make_rootfs(&chroot, &self.rootfs.0, self.uid, self.gid)?; - if Config::get().network().is_some() { - make_tun_node(&chroot, self.uid, self.gid).map_err(Error::MakeTun)?; - } - Ok(()) - } -} - -/// Open the chroot directory by walking it from `jail_base` with `O_NOFOLLOW`, so -/// a symlinked component cannot redirect outside the jail. -pub fn open_chroot_under(jail_base: &Path, chroot: &Path) -> Result { - let path: &SafePath = chroot.try_into()?; - path.strip_base(jail_base)?; - SafeDir::open(path, OpenFlags::RDONLY).map_err(Error::OpenChroot) -} - -/// Stage host file `src` into `chroot` as `vmlinux`: confine the source under -/// `hyper_base` (after canonicalization), hard-link it (copy across filesystems), -/// then chown to `uid:gid`. -pub fn stage_kernel_under( - chroot: &SafeDir, - src: &Path, - hyper_base: &Path, - uid: u32, - gid: u32, -) -> Result<(), Error> { - let (uid, gid) = (Some(Uid::from_raw(uid)), Some(Gid::from_raw(gid))); - - let src_canon = std::fs::canonicalize(src).map_err(|source| Error::Source { - path: src.to_path_buf(), - source, - })?; - let src_path: &SafePath = src_canon.as_path().try_into()?; - src_path - .strip_base(hyper_base) - .map_err(|_| Error::OutsideBase { - base: hyper_base.to_path_buf(), - path: src_canon.clone(), - })?; - - let source = SafeFile::::open(src_path, OpenFlags::RDONLY).map_err( - |source| Error::OpenSrc { - path: src_canon.clone(), - source, - }, - )?; - - match chroot.linkat(&source, KERNEL_NAME) { - Ok(()) => source - .chown(uid, gid) - .map_err(|e| Error::StageKernel(e.into())), - Err(Errno::EXDEV) => { - let mut dest = chroot - .mkfileat(KERNEL_NAME, Mode::from_bits_truncate(0o600)) - .map_err(|e| Error::StageKernel(e.into()))?; - io::copy(&mut File::from(OwnedFd::from(source)), &mut dest).map_err(Error::Copy)?; - dest.chown(uid, gid) - .map_err(|e| Error::StageKernel(e.into())) - } - Err(e) => Err(Error::StageKernel(e.into())), - } -} - -/// Create the `rootfs` block node mirroring `device`'s major:minor, owned -/// `uid:gid`. The device is opened as a verified `SafeFile`, so the -/// number comes from a real device node, never a caller-supplied value. -fn make_rootfs(chroot: &SafeDir, device: &DmName, uid: u32, gid: u32) -> Result<(), Error> { - // `device` is lexically validated as `/dev/loopN` or `/dev/mapper/hyper-*`. - // The dm form is a symlink under `/dev/mapper` — a root-owned `0755` dir an - // unprivileged caller cannot write — so resolving it to its real `/dev/dm-N` - // node cannot be redirected by the caller. We must resolve first: `SafeFile` - // refuses any symlink in the path (it must never follow an attacker-supplied - // one) and would otherwise reject the dm symlink itself. Loop nodes resolve to - // themselves. The re-opened target is still verified `IsBlockDevice`. - let device = device.path(); - let real = std::fs::canonicalize(device.as_path()).map_err(|source| Error::ResolveDev { - path: device.as_path().to_path_buf(), - source, - })?; - let dev_path: &SafePath = real.as_path().try_into()?; - let dev = SafeFile::::open(dev_path, OpenFlags::PATH) - .map_err(Error::Device)?; - let rdev = dev.rdev().map_err(Error::Device)?; - let make = || -> Result<(), dir::Error> { - chroot.mknodat( - ROOTFS_NAME, - SFlag::S_IFBLK, - Mode::from_bits_truncate(0o600), - rdev, - )?; - let node: SafeFile = - chroot.openat(ROOTFS_NAME, OpenFlags::PATH, Mode::empty())?; - Ok(node.chown(Some(Uid::from_raw(uid)), Some(Gid::from_raw(gid)))?) - }; - make().map_err(Error::MakeRootfs) -} - -/// Create `dev/net/tun` (char `10:200`, mode `0666`) inside the jail, owned -/// `uid:gid`, so the chrooted firecracker can `open("/dev/net/tun")` and -/// `TUNSETIFF` onto the tap device waiting in its netns. Only called when VM -/// networking is enabled; a jail built without it has no `dev` tree at all -/// today (see [`ChrootJail::build_under`]). -fn make_tun_node(chroot: &SafeDir, uid: u32, gid: u32) -> Result<(), dir::Error> { - chroot.mkdirat(DEV, Mode::from_bits_truncate(0o755))?; - let dev_dir: SafeDir = chroot.openat(DEV, OpenFlags::RDONLY, Mode::empty())?; - - dev_dir.mkdirat(NET, Mode::from_bits_truncate(0o755))?; - let net_dir: SafeDir = dev_dir.openat(NET, OpenFlags::RDONLY, Mode::empty())?; - - match net_dir.mknodat( - TUN, - SFlag::S_IFCHR, - Mode::from_bits_truncate(0o666), - TUN_DEVICE, - ) { - Ok(()) | Err(Errno::EEXIST) => {} - Err(errno) => return Err(errno.into()), - } - let node: SafeFile = - net_dir.openat(TUN, OpenFlags::PATH, Mode::empty())?; - node.chown(Some(Uid::from_raw(uid)), Some(Gid::from_raw(gid)))?; - Ok(node.chmod(Mode::from_bits_truncate(0o666))?) -} diff --git a/native/suidhelper/src/util/jailer.rs b/native/suidhelper/src/util/jailer.rs deleted file mode 100644 index 61c0c831..00000000 --- a/native/suidhelper/src/util/jailer.rs +++ /dev/null @@ -1,284 +0,0 @@ -// SPDX-License-Identifier: MIT -//! Typed invocation of the firecracker jailer: the validated argument newtypes -//! and the `execve` mechanics behind the `jailer` subcommand. -//! -//! Threat model: the BEAM is untrusted. It supplies only the VM id, uid/gid, -//! cgroup settings, and the API socket path — each guarded by a newtype below. -//! Every privileged path (the jailer binary, the firecracker `--exec-file`, the -//! chroot base, the parent cgroup) comes from the root-owned config, never the -//! caller. The refusal contracts on these newtypes are the security core: a -//! compromised BEAM must not be able to name a privileged path, request uid 0, -//! traverse out of the chroot base, inject a flag, or smuggle an -//! environment/fd into root. -//! -//! ## Validator laws (property-tested in `tests/tools/jailer.rs`) -//! - [`validate_id_number`] accepts iff `n != 0 && lo <= n <= hi`; 0 is rejected -//! for *every* range (uid 0 makes the jailer skip its privilege drop). -//! - [`VmId`] round-trips exactly the allowed charset/length and rejects any -//! separator, dot, NUL, whitespace, leading dash, empty, or over-long input. -//! - [`CgroupSetting`] re-renders a valid pair to its canonical `key=value` and -//! rejects unknown keys and values outside the per-key grammar. -//! - [`JailSock`] accepts exactly `/` + one filename and rejects multi-component, -//! relative, `..`, and NUL/whitespace inputs. - -use crate::process::binary::{RootSafeBin, RootSafeBinPathBuf}; -use crate::process::privilege::{RootCommandExt, Unprivileged}; -use bon::Builder; -use derive_more::{AsRef, Display}; -use std::ffi::{CString, NulError, OsStr}; -use std::os::unix::ffi::OsStrExt; -use std::path::Path; -use std::str::FromStr; -use thiserror::Error as ThisError; - -/// The jailer protects at most a handful of controllers; an unbounded `--cgroup` -/// list is a caller trying something. Cap it well above any legitimate need. -pub const MAX_CGROUPS: usize = 16; - -/// `sun_path` in `sockaddr_un` is 108 bytes on Linux; a socket path longer than -/// that can never be bound, so reject it up front. -const MAX_SOCK_LEN: usize = 108; - -#[derive(Debug, ThisError)] -pub enum Error { - #[error("invalid --id {0:?}: must be 1..=64 chars of [A-Za-z0-9_-], not starting with '-'")] - VmId(String), - #[error("invalid --cgroup {0:?}: unknown key or value outside its grammar")] - Cgroup(String), - #[error("invalid --api-sock {0:?}: must be / with name in [A-Za-z0-9_.-]")] - Sock(String), - #[error("--uid/--gid {value} is zero or outside the configured range [{lo}, {hi}]")] - IdNumber { value: u32, lo: u32, hi: u32 }, - #[error("too many --cgroup settings: {0} (max {MAX_CGROUPS})")] - TooManyCgroups(usize), - #[error("argument contains an interior NUL byte")] - NulArgument(#[from] NulError), -} - -/// `n != 0 && lo <= n <= hi`. uid/gid 0 is rejected unconditionally: a jailer run -/// with uid 0 skips its privilege drop and leaves firecracker running as root. -pub fn validate_id_number(n: u32, range: (u32, u32)) -> Result { - let (lo, hi) = range; - if n != 0 && lo <= n && n <= hi { - Ok(n) - } else { - Err(Error::IdNumber { value: n, lo, hi }) - } -} - -/// A VM id used as a chroot subdirectory name: `[A-Za-z0-9_-]`, length `1..=64`, -/// first character not `-` (so it can never be read as a flag). No `/`, `.`, NUL, -/// or whitespace can appear, so it can never traverse out of the chroot base. -#[derive(Debug, Clone, Display, AsRef)] -#[as_ref(str)] -pub struct VmId(String); - -impl FromStr for VmId { - type Err = Error; - - fn from_str(s: &str) -> Result { - let reject = || Error::VmId(s.to_string()); - let bytes = s.as_bytes(); - if !(1..=64).contains(&bytes.len()) { - return Err(reject()); - } - if bytes[0] == b'-' { - return Err(reject()); - } - if !bytes - .iter() - .all(|&b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-') - { - return Err(reject()); - } - Ok(Self(s.to_string())) - } -} - -/// `1..=20` ASCII digits — bounds a numeric cgroup limit without pulling in a -/// regex engine. The upper bound comfortably exceeds `u64::MAX`'s 20 digits. -fn is_digits_1_20(s: &str) -> bool { - !s.is_empty() && s.len() <= 20 && s.bytes().all(|b| b.is_ascii_digit()) -} - -/// A single `KEY=VALUE` cgroup setting from an allowlist. The helper re-emits -/// `key=value` itself from the canonical key, so the jailer never sees the -/// caller's raw bytes. Per-key value grammar: -/// - `memory.max` : `[0-9]{1,20}` or the literal `max` -/// - `cpu.max` : `[0-9]{1,20} [0-9]{1,20}` or `max [0-9]{1,20}` -#[derive(Debug, Clone, Display)] -#[display("{key}={value}")] -pub struct CgroupSetting { - key: &'static str, - value: String, -} - -impl FromStr for CgroupSetting { - type Err = Error; - - fn from_str(s: &str) -> Result { - let reject = || Error::Cgroup(s.to_string()); - // Split on the FIRST `=`. None of the value grammars contains a `=`, so a - // second `=` lands in `value` and is rejected by the grammar check below. - let (raw_key, value) = s.split_once('=').ok_or_else(reject)?; - - let key: &'static str = match raw_key { - "memory.max" => "memory.max", - "cpu.max" => "cpu.max", - _ => return Err(reject()), - }; - - let valid = match key { - "memory.max" => value == "max" || is_digits_1_20(value), - "cpu.max" => match value.split_once(' ') { - Some((quota, period)) => { - (quota == "max" || is_digits_1_20(quota)) && is_digits_1_20(period) - } - None => false, - }, - _ => false, - }; - - if valid { - Ok(Self { - key, - value: value.to_string(), - }) - } else { - Err(reject()) - } - } -} - -/// The firecracker API socket path: an absolute path that is exactly `/` plus one -/// filename in `[A-Za-z0-9_.-]`. The charset excludes `/`, so the value is always -/// a direct child of `/` with no extra components and no traversal; `.`/`..` as -/// the whole filename are rejected explicitly. -#[derive(Debug, Clone, Display, AsRef)] -#[as_ref(str)] -pub struct JailSock(String); - -impl FromStr for JailSock { - type Err = Error; - - fn from_str(s: &str) -> Result { - let reject = || Error::Sock(s.to_string()); - if s.len() > MAX_SOCK_LEN { - return Err(reject()); - } - let name = s.strip_prefix('/').ok_or_else(reject)?; - if name.is_empty() || name == "." || name == ".." { - return Err(reject()); - } - if !name - .bytes() - .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'.' || b == b'-') - { - return Err(reject()); - } - Ok(Self(s.to_string())) - } -} - -/// A fully-resolved jailer invocation. The privileged fields (`jailer`, -/// `firecracker`, `jail_base`, `parent_cgroup`) must come from trusted config; -/// the caller-supplied fields (`vm`, `uid`, `gid`, `cgroups`, `api_sock`) must -/// already have passed their newtype validation. -#[derive(Builder)] -pub struct Jailer<'a> { - jailer: &'a RootSafeBin<"jailer">, - firecracker: &'a RootSafeBinPathBuf<"firecracker">, - jail_base: &'a Path, - parent_cgroup: &'a str, - vm: &'a VmId, - uid: u32, - gid: u32, - cgroups: &'a [CgroupSetting], - api_sock: &'a JailSock, - /// The netns the jailer must enter (`--netns`), `Some` when VM networking is - /// enabled. Derived by the caller from trusted config + the validated `vm` - /// id — never from caller input — and must match the `ip netns add ` - /// path the `network` tool creates. - netns: Option<&'a Path>, -} - -impl Jailer<'_> { - /// Build the exact argv handed to the jailer. argv[0] is the jailer path. The - /// caller never names the jailer, the `--exec-file`, the chroot base, the - /// cgroup version, or the parent cgroup — those come from trusted config. - fn argv(&self) -> Result, Error> { - let vm: &str = self.vm.as_ref(); - let api_sock: &str = self.api_sock.as_ref(); - let mut argv = vec![ - "jailer".to_cstring()?, - "--id".to_cstring()?, - vm.to_cstring()?, - "--exec-file".to_cstring()?, - self.firecracker.as_path().to_cstring()?, - "--uid".to_cstring()?, - self.uid.to_string().to_cstring()?, - "--gid".to_cstring()?, - self.gid.to_string().to_cstring()?, - "--chroot-base-dir".to_cstring()?, - self.jail_base.to_cstring()?, - "--cgroup-version".to_cstring()?, - "2".to_cstring()?, - "--parent-cgroup".to_cstring()?, - self.parent_cgroup.to_cstring()?, - ]; - - for cg in self.cgroups { - argv.push("--cgroup".to_cstring()?); - argv.push(cg.to_string().to_cstring()?); - } - - if let Some(netns) = self.netns { - argv.push("--netns".to_cstring()?); - argv.push(netns.to_cstring()?); - } - - argv.push("--".to_cstring()?); - argv.push("--api-sock".to_cstring()?); - argv.push(api_sock.to_cstring()?); - - Ok(argv) - } - - /// Permanently become root and `execve` the jailer in our place. On success - /// this never returns (the process image is replaced); the `Ok` arm is - /// therefore [`std::convert::Infallible`]. Every failure is returned as - /// [`Error`] for the caller to print and exit non-zero. - pub fn invoke(self, token: Unprivileged) -> Result { - if self.cgroups.len() > MAX_CGROUPS { - return Err(Error::TooManyCgroups(self.cgroups.len())); - } - - // Resolve everything that can fail before any privilege is raised. - let argv = self.argv()?; - - self.jailer - .command() - .args( - argv[1..] - .iter() - .map(|arg| OsStr::from_bytes(arg.as_bytes())), - ) - .exec_as_root(token) - } -} - -trait ToCStringExt { - fn to_cstring(&self) -> Result; -} - -impl ToCStringExt for Path { - fn to_cstring(&self) -> Result { - CString::new(self.as_os_str().as_bytes()) - } -} - -impl ToCStringExt for str { - fn to_cstring(&self) -> Result { - CString::new(self.as_bytes()) - } -} diff --git a/native/suidhelper/src/util/mod.rs b/native/suidhelper/src/util/mod.rs deleted file mode 100644 index 711ecb66..00000000 --- a/native/suidhelper/src/util/mod.rs +++ /dev/null @@ -1,4 +0,0 @@ -//! Internal utilities shared across the helper. - -pub mod chroot_jail; -pub mod jailer; diff --git a/native/suidhelper/tests/config_paths.rs b/native/suidhelper/tests/config_paths.rs new file mode 100644 index 00000000..a01d1cdc --- /dev/null +++ b/native/suidhelper/tests/config_paths.rs @@ -0,0 +1,48 @@ +use hyper_suidhelper::config::Config; +use std::path::Path; + +fn parse(work_dir: &str) -> Result { + toml::from_str(&format!("work_dir = {work_dir:?}\n")) +} + +#[test] +fn non_strict_work_dir_is_refused_at_load() { + for work_dir in [ + "relative/path", + "/srv/../etc", + "/srv/./hyper", + "/srv//hyper", + "/srv/hyper/", + "", + ] { + assert!(parse(work_dir).is_err(), "{work_dir:?} accepted"); + } +} + +#[test] +fn bases_derive_from_work_dir() { + let config = parse("/data/hyper").unwrap(); + assert_eq!(config.hyper_base().as_path(), Path::new("/data/hyper")); + assert_eq!(config.jail_base().as_path(), Path::new("/data/hyper/jails")); +} + +#[test] +fn non_component_parent_cgroup_is_refused_at_load() { + for cgroup in ["", ".", "..", "a/b", "/hyper", "hyper/"] { + let body = format!("work_dir = \"/srv/hyper\"\n[jails]\ncgroup = {cgroup:?}\n"); + assert!( + toml::from_str::(&body).is_err(), + "{cgroup:?} accepted" + ); + } +} + +#[test] +fn parent_cgroup_dir_is_under_the_cgroup_root() { + let config: Config = + toml::from_str("work_dir = \"/srv/hyper\"\n[jails]\ncgroup = \"vms\"\n").unwrap(); + assert_eq!( + config.parent_cgroup_dir().as_path(), + Path::new("/sys/fs/cgroup/vms") + ); +} diff --git a/native/suidhelper/tests/config_uid_gid_range.rs b/native/suidhelper/tests/config_uid_gid_range.rs index 1a6f4647..c2c9ca12 100644 --- a/native/suidhelper/tests/config_uid_gid_range.rs +++ b/native/suidhelper/tests/config_uid_gid_range.rs @@ -8,52 +8,38 @@ //! receive it — it skips its privilege drop when uid == 0). //! - min > max is always rejected (incoherent range; likely a config typo). -use hyper_suidhelper::config::{validate_uid_gid_range, Config, LoadingError, UidGidRange}; +use hyper_suidhelper::config::Config; use proptest::prelude::*; +fn parse(min: u32, max: u32) -> Result { + toml::from_str(&format!( + "work_dir = \"/srv/hyper\"\n[jails]\nuid_gid_range = [{min}, {max}]\n" + )) +} + #[test] fn absent_range_yields_default() { - assert_eq!(Config::default().uid_gid_range(), (900_000, 999_999)); + let r = Config::default().uid_gid_range(); + assert_eq!((r.min(), r.max()), (900_000, 999_999)); } proptest! { - #[test] - fn valid_range_accepted(min in 1u32.., delta in 0u32..) { - // max = min + delta, saturating so it never wraps past u32::MAX. - let max = min.saturating_add(delta); - let r = UidGidRange { min, max }; - prop_assert!(validate_uid_gid_range(&r).is_ok()); - } - #[test] fn valid_range_round_trips_via_toml(min in 1u32.., delta in 0u32..) { let max = min.saturating_add(delta); - let body = format!( - "work_dir = \"/srv/hyper\"\n[jails]\nuid_gid_range = [{min}, {max}]\n" - ); - let config: Config = toml::from_str(&body).expect("valid TOML"); - prop_assert_eq!(config.uid_gid_range(), (min, max)); + let r = parse(min, max).expect("valid TOML").uid_gid_range(); + prop_assert_eq!((r.min(), r.max()), (min, max)); } #[test] fn zero_min_always_rejected(max in 0u32..) { - let r = UidGidRange { min: 0, max }; - let rejected = matches!( - validate_uid_gid_range(&r), - Err(LoadingError::BadUidGidRange { min: 0, .. }) - ); - prop_assert!(rejected); + let err = parse(0, max).unwrap_err(); + prop_assert!(err.to_string().contains("uid_gid_range")); } #[test] fn min_exceeds_max_always_rejected(max in 0u32..u32::MAX) { - // min = max + 1 is always strictly greater than max and always >= 1. - let min = max + 1; - let r = UidGidRange { min, max }; - let rejected = matches!( - validate_uid_gid_range(&r), - Err(LoadingError::BadUidGidRange { .. }) - ); - prop_assert!(rejected); + let err = parse(max + 1, max).unwrap_err(); + prop_assert!(err.to_string().contains("uid_gid_range")); } } diff --git a/native/suidhelper/tests/e2e/chroot_jail.rs b/native/suidhelper/tests/e2e/chroot_jail.rs index c3a617a8..817ff22e 100644 --- a/native/suidhelper/tests/e2e/chroot_jail.rs +++ b/native/suidhelper/tests/e2e/chroot_jail.rs @@ -29,6 +29,20 @@ fn write_config(dir: &Path, work_dir: &Path) -> PathBuf { /// Same as [`write_config`], plus a `[network]` table — the gate that makes /// `chroot-jail prepare` also mknod `dev/net/tun` into the jail. +fn write_config_with_parent_cgroup(dir: &Path, work_dir: &Path, parent: &str) -> PathBuf { + let p = dir.join("config.toml"); + fs::write( + &p, + format!( + "work_dir = \"{}\"\n[jails]\ncgroup = \"{parent}\"\n", + work_dir.display() + ), + ) + .unwrap(); + fs::set_permissions(&p, fs::Permissions::from_mode(0o644)).unwrap(); + p +} + fn write_config_with_network(dir: &Path, work_dir: &Path) -> PathBuf { let p = dir.join("config.toml"); fs::write( @@ -112,7 +126,7 @@ fn prepare_succeeds_and_builds_jail_as_root() { } let tmp = tempfile::tempdir().unwrap(); let work = tmp.path().join("srv"); - let chroot = work.join("jails").join("exec").join("id"); + let chroot = work.join("jails").join("exec").join("id").join("root"); fs::create_dir_all(&chroot).unwrap(); let kernel = work.join("vmlinux-src"); fs::write(&kernel, b"kernel image").unwrap(); @@ -135,9 +149,9 @@ fn prepare_succeeds_and_builds_jail_as_root() { "--device", &dev.path(), "--uid", - "0", + "900000", "--gid", - "0", + "900000", ], ); let dev_rdev = fs::metadata(dev.path()).map(|m| m.rdev()); @@ -162,7 +176,7 @@ fn prepare_creates_tun_node_when_networking_enabled_as_root() { } let tmp = tempfile::tempdir().unwrap(); let work = tmp.path().join("srv"); - let chroot = work.join("jails").join("exec").join("id"); + let chroot = work.join("jails").join("exec").join("id").join("root"); fs::create_dir_all(&chroot).unwrap(); let kernel = work.join("vmlinux-src"); fs::write(&kernel, b"kernel image").unwrap(); @@ -173,8 +187,8 @@ fn prepare_creates_tun_node_when_networking_enabled_as_root() { return; }; - let uid = 1234; - let gid = 5678; + let uid = 900_001; + let gid = 900_002; let out = run( &cfg, &[ @@ -205,6 +219,52 @@ fn prepare_creates_tun_node_when_networking_enabled_as_root() { assert_eq!(meta.permissions().mode() & 0o777, 0o666); } +#[test] +fn prepare_refuses_ids_outside_range_as_root() { + if !is_root() { + eprintln!("SKIP prepare_refuses_ids_outside_range: needs root"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let work = tmp.path().join("srv"); + let chroot = work.join("jails").join("exec").join("id").join("root"); + fs::create_dir_all(&chroot).unwrap(); + let kernel = work.join("vmlinux-src"); + fs::write(&kernel, b"kernel image").unwrap(); + let cfg = write_config(tmp.path(), &work); + + for (uid, gid) in [ + ("0", "900000"), + ("900000", "0"), + ("1000", "900000"), + ("900000", "1000000"), + ] { + run( + &cfg, + &[ + "chroot-jail", + "prepare", + "--chroot", + chroot.to_str().unwrap(), + "--kernel", + kernel.to_str().unwrap(), + "--device", + "/dev/loop0", + "--uid", + uid, + "--gid", + gid, + ], + ) + .code(2) + .stderr(predicates::str::contains("outside the configured range")); + assert!( + !chroot.join("vmlinux").exists(), + "uid={uid} gid={gid}: nothing may be staged" + ); + } +} + // `chroot-jail prepare` with a system device (not a loop / hyper-* dm) is // rejected by BlockDev parsing at clap parse time — exit 2, nothing built. This // case needs no root (clap rejects before the privilege boundary) but also no @@ -225,9 +285,9 @@ fn prepare_rejects_system_device_operand() { "--device", "/dev/sda", "--uid", - "0", + "900000", "--gid", - "0", + "900000", ], ) .failure(); @@ -246,7 +306,7 @@ fn remove_succeeds_and_is_idempotent_as_root() { let chroot = work.join("jails").join("exec").join("id"); fs::create_dir_all(chroot.join("inner")).unwrap(); fs::write(chroot.join("vmlinux"), b"x").unwrap(); - let cfg = write_config(tmp.path(), &work); + let cfg = write_config_with_parent_cgroup(tmp.path(), &work, "hyper-e2e-does-not-exist"); // A cgroup leaf that does not exist under the real /sys/fs/cgroup: removal of // a missing target is success, and nothing on the host is modified. @@ -268,3 +328,40 @@ fn remove_succeeds_and_is_idempotent_as_root() { assert_eq!(json["result"], "removed"); assert!(!chroot.exists(), "chroot must be gone"); } + +#[test] +fn remove_refuses_cgroup_outside_parent_as_root() { + if !is_root() { + eprintln!("SKIP remove_refuses_cgroup_outside_parent: needs root"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let work = tmp.path().join("srv"); + let chroot = work.join("jails").join("exec").join("id"); + fs::create_dir_all(&chroot).unwrap(); + let cfg = write_config_with_parent_cgroup(tmp.path(), &work, "hyper-e2e-does-not-exist"); + + for cgroup in [ + "/sys/fs/cgroup/hyper-e2e-other/leaf", + "/sys/fs/cgroup/hyper-e2e-does-not-exist", + "/sys/fs/cgroup/hyper-e2e-does-not-exist/leaf/deeper", + "/sys/fs/cgroup/hyper-e2e-does-not-exist/not.a.vm.id", + ] { + run( + &cfg, + &[ + "chroot-jail", + "remove", + "--chroot", + chroot.to_str().unwrap(), + "--cgroup", + cgroup, + ], + ) + .code(2); + assert!( + chroot.exists(), + "{cgroup}: chroot must survive a refused remove" + ); + } +} diff --git a/native/suidhelper/tests/e2e/config.rs b/native/suidhelper/tests/e2e/config.rs index 98b0fc57..556cf207 100644 --- a/native/suidhelper/tests/e2e/config.rs +++ b/native/suidhelper/tests/e2e/config.rs @@ -83,7 +83,7 @@ fn relative_work_dir_exits_2_relative_as_root() { let p = write_root_config(tmp.path(), "work_dir = \"relative/path\"\n"); run(&p, &["sys-test"]) .code(2) - .stderr(contains("must be an absolute path")); + .stderr(contains("path is not absolute")); } #[test] diff --git a/native/suidhelper/tests/e2e/jailer.rs b/native/suidhelper/tests/e2e/jailer.rs index 2651921c..6d321a58 100644 --- a/native/suidhelper/tests/e2e/jailer.rs +++ b/native/suidhelper/tests/e2e/jailer.rs @@ -79,10 +79,12 @@ fn execs_jailer_with_canonical_argv_and_empty_env_as_root() { "900001", "--gid", "900002", - "--cgroup", - "memory.max=1048576", - "--cgroup", - "cpu.max=100000 100000", + "--memory-max", + "1048576", + "--cpu-quota-us", + "100000", + "--cpu-period-us", + "100000", "--api-sock", "/api.sock", ], diff --git a/native/suidhelper/tests/firecracker/chroot_jail.rs b/native/suidhelper/tests/firecracker/chroot_jail.rs new file mode 100644 index 00000000..6a1e71f9 --- /dev/null +++ b/native/suidhelper/tests/firecracker/chroot_jail.rs @@ -0,0 +1,308 @@ +use hyper_suidhelper::firecracker::chroot_jail::{JailRoot, KernelError, KernelImage, OpenError}; +use hyper_suidhelper::firecracker::id::{JailOwner, UidGidRange}; +use hyper_suidhelper::fs::dir; +use hyper_suidhelper::fs::path::{IsAbsolute, RelativeError, SafePathBuf, StrictComponents}; +use hyper_suidhelper::linux::dm::DmName; +use nix::errno::Errno; +use nix::sys::stat::{makedev, mknod, Mode, SFlag}; +use std::fs; +use std::os::unix::fs::{symlink, FileTypeExt, MetadataExt}; +use std::path::{Path, PathBuf}; +use std::process::Command; + +type Strict = SafePathBuf; + +fn strict(p: impl AsRef) -> Strict { + p.as_ref().to_path_buf().try_into().unwrap() +} + +fn is_root() -> bool { + nix::unistd::geteuid().is_root() +} + +fn owner(uid: u32, gid: u32) -> JailOwner { + let range = UidGidRange::try_from([uid.min(gid), uid.max(gid)]).unwrap(); + JailOwner { + uid: range.uid(uid).unwrap(), + gid: range.gid(gid).unwrap(), + } +} + +fn jail_root(jail_base: &Path) -> PathBuf { + let root = jail_base.join("exec").join("id").join("root"); + fs::create_dir_all(&root).unwrap(); + root +} + +#[test] +fn jail_root_opens_exec_id_root() { + let tmp = tempfile::tempdir().unwrap(); + let root = jail_root(tmp.path()); + assert!(JailRoot::open(&strict(tmp.path()), &strict(root)).is_ok()); +} + +#[test] +fn jail_root_refuses_other_shapes() { + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path(); + for rel in [ + "exec", + "exec/id", + "exec/id/root/extra", + "exec/id/notroot", + "exec/a_b/root", + "exec/-id/root", + ] { + let err = JailRoot::open(&strict(base), &strict(base.join(rel))).err(); + assert!( + matches!(err, Some(OpenError::Shape(_))), + "{rel}: got {err:?}" + ); + } +} + +#[test] +fn jail_root_refuses_path_outside_base() { + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path().join("jail"); + let outside = tmp.path().join("elsewhere/exec/id/root"); + let err = JailRoot::open(&strict(&base), &strict(outside)).err(); + assert!( + matches!(err, Some(OpenError::Base(RelativeError::NotUnderBase))), + "got {err:?}" + ); +} + +// A symlinked component is not followed (O_NOFOLLOW): open must fail. +#[test] +fn jail_root_refuses_symlinked_component() { + let tmp = tempfile::tempdir().unwrap(); + let jail = tmp.path().join("jail"); + fs::create_dir(&jail).unwrap(); + let real_exec = tmp.path().join("real-exec"); + fs::create_dir_all(real_exec.join("id").join("root")).unwrap(); + symlink(&real_exec, jail.join("exec")).unwrap(); + + let err = JailRoot::open(&strict(&jail), &strict(jail.join("exec/id/root"))).err(); + assert!(matches!(err, Some(OpenError::Open(_))), "got {err:?}"); +} + +// A source that lives entirely outside hyper_base is refused. +#[test] +fn kernel_outside_base_is_refused() { + let tmp = tempfile::tempdir().unwrap(); + let hyper_base = tmp.path().join("hyper"); + fs::create_dir(&hyper_base).unwrap(); + let outside = tmp.path().join("outside-kernel"); + fs::write(&outside, b"kernel").unwrap(); + + let err = KernelImage::open(&strict(&outside), &strict(&hyper_base)).err(); + assert!( + matches!(err, Some(KernelError::OutsideBase { .. })), + "got {err:?}" + ); +} + +// A source UNDER hyper_base but which is a symlink RESOLVING outside it is +// refused: confinement is enforced after canonicalization, not on the lexical +// path. This is the key source-confinement invariant. +#[test] +fn kernel_symlink_escaping_base_is_refused() { + let tmp = tempfile::tempdir().unwrap(); + let hyper_base = tmp.path().join("hyper"); + fs::create_dir(&hyper_base).unwrap(); + let real_target = tmp.path().join("secret"); + fs::write(&real_target, b"secret kernel").unwrap(); + let link = hyper_base.join("kernel-link"); + symlink(&real_target, &link).unwrap(); + + let err = KernelImage::open(&strict(&link), &strict(&hyper_base)).err(); + assert!( + matches!(err, Some(KernelError::OutsideBase { .. })), + "got {err:?}" + ); +} + +#[test] +fn missing_kernel_is_a_resolve_error() { + let tmp = tempfile::tempdir().unwrap(); + let missing = tmp.path().join("nope"); + let err = KernelImage::open(&strict(&missing), &strict(tmp.path())).err(); + assert!( + matches!(err, Some(KernelError::Resolve { .. })), + "got {err:?}" + ); +} + +#[test] +fn stage_kernel_materializes_then_refuses_overwrite() { + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path(); + let root_dir = jail_root(base); + let src = base.join("vmlinux-src"); + fs::write(&src, b"\x7fELF fake kernel bytes").unwrap(); + let root = JailRoot::open(&strict(base), &strict(&root_dir)).unwrap(); + + let kernel = KernelImage::open(&strict(&src), &strict(base)).unwrap(); + root.stage_kernel(kernel).expect("first stage"); + assert_eq!( + fs::read(root_dir.join("vmlinux")).unwrap(), + b"\x7fELF fake kernel bytes" + ); + + let kernel = KernelImage::open(&strict(&src), &strict(base)).unwrap(); + let err = root.stage_kernel(kernel).unwrap_err(); + assert_eq!(err.errno(), Some(Errno::EEXIST), "got {err:?}"); +} + +#[test] +fn stage_kernel_never_changes_the_source() { + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path(); + let root_dir = jail_root(base); + let src = base.join("vmlinux-src"); + fs::write(&src, b"kernel").unwrap(); + let before = fs::metadata(&src).unwrap(); + + let root = JailRoot::open(&strict(base), &strict(&root_dir)).unwrap(); + let kernel = KernelImage::open(&strict(&src), &strict(base)).unwrap(); + root.stage_kernel(kernel).unwrap(); + + let after = fs::metadata(&src).unwrap(); + assert_eq!( + (after.uid(), after.gid(), after.mode()), + (before.uid(), before.gid(), before.mode()) + ); +} + +fn setup_loop(tmp: &Path) -> Option { + let backing = tmp.join("backing.img"); + let f = fs::File::create(&backing).ok()?; + f.set_len(1024 * 1024).ok()?; + let out = Command::new("losetup") + .args(["--find", "--show"]) + .arg(&backing) + .output() + .ok()?; + if !out.status.success() { + return None; + } + let dev = String::from_utf8(out.stdout).ok()?.trim().to_string(); + if dev.is_empty() { + return None; + } + Some(PathBuf::from(dev)) +} + +fn teardown_loop(dev: &Path) { + let _ = Command::new("losetup").arg("-d").arg(dev).output(); +} + +fn setup_dm(loop_dev: &Path) -> Option { + let name = format!("hyper-test-chroot-{}", std::process::id()); + let table = format!("0 2048 linear {} 0", loop_dev.display()); + let created = Command::new("dmsetup") + .args(["create", &name, "--table", &table]) + .status() + .ok()? + .success(); + if !created { + return None; + } + name.parse().ok() +} + +fn teardown_dm(name: &DmName) { + let _ = Command::new("dmsetup") + .args(["remove", "--retry"]) + .arg(name.as_os_str()) + .output(); +} + +#[test] +fn add_rootfs_mirrors_the_device_as_root() { + if !is_root() { + eprintln!("SKIP add_rootfs_mirrors_the_device: needs root for mknod/chown"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path(); + let root_dir = jail_root(base); + + let Some(dev) = setup_loop(base) else { + eprintln!("SKIP add_rootfs: losetup unavailable"); + return; + }; + let Some(block) = setup_dm(&dev) else { + teardown_loop(&dev); + eprintln!("SKIP add_rootfs: dmsetup unavailable"); + return; + }; + + let root = JailRoot::open(&strict(base), &strict(&root_dir)).unwrap(); + let res = block.open().map_err(|e| e.to_string()).and_then(|device| { + root.add_rootfs(&device, owner(12345, 12346)) + .map_err(|e| e.to_string()) + }); + let dev_rdev = fs::metadata(block.path().as_path()).map(|m| m.rdev()); + teardown_dm(&block); + teardown_loop(&dev); + res.expect("add_rootfs must succeed as root against a real device"); + + let meta = fs::metadata(root_dir.join("rootfs")).unwrap(); + assert!(meta.file_type().is_block_device()); + assert_eq!(meta.rdev(), dev_rdev.unwrap()); + assert_eq!((meta.uid(), meta.gid()), (12345, 12346)); + assert_eq!(meta.mode() & 0o777, 0o600); +} + +#[test] +fn add_tun_is_idempotent_as_root() { + if !is_root() { + eprintln!("SKIP add_tun_is_idempotent: needs root for mknod/chown"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let root_dir = jail_root(tmp.path()); + let root = JailRoot::open(&strict(tmp.path()), &strict(&root_dir)).unwrap(); + + root.add_tun(owner(12345, 12346)).unwrap(); + root.add_tun(owner(12345, 12346)).unwrap(); + + let meta = fs::metadata(root_dir.join("dev/net/tun")).unwrap(); + assert!(meta.file_type().is_char_device()); + assert_eq!(meta.rdev(), makedev(10, 200)); + assert_eq!((meta.uid(), meta.gid()), (12345, 12346)); + assert_eq!(meta.mode() & 0o777, 0o666); +} + +#[test] +fn add_tun_refuses_a_preexisting_node_with_another_number_as_root() { + if !is_root() { + eprintln!("SKIP add_tun_refuses_a_preexisting_node: needs root for mknod"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let root_dir = jail_root(tmp.path()); + fs::create_dir_all(root_dir.join("dev/net")).unwrap(); + let planted = root_dir.join("dev/net/tun"); + mknod( + &planted, + SFlag::S_IFCHR, + Mode::from_bits_truncate(0o666), + makedev(1, 3), + ) + .unwrap(); + let root = JailRoot::open(&strict(tmp.path()), &strict(&root_dir)).unwrap(); + + let err = root.add_tun(owner(12345, 12346)).unwrap_err(); + assert!( + matches!(err, dir::Error::DeviceMismatch { .. }), + "got {err:?}" + ); + assert_eq!( + fs::metadata(&planted).unwrap().uid(), + 0, + "planted node must not be chowned" + ); +} diff --git a/native/suidhelper/tests/firecracker/id.rs b/native/suidhelper/tests/firecracker/id.rs new file mode 100644 index 00000000..8204898c --- /dev/null +++ b/native/suidhelper/tests/firecracker/id.rs @@ -0,0 +1,58 @@ +use hyper_suidhelper::firecracker::id::{IdError, RangeError, UidGidRange}; +use nix::unistd::{Gid, Uid}; +use proptest::prelude::*; + +fn range() -> impl Strategy { + (1u32.., any::()) + .prop_map(|(min, span)| UidGidRange::try_from([min, min.saturating_add(span)]).unwrap()) +} + +proptest! { + #[test] + fn ordered_nonzero_pair_is_a_range(min in 1u32.., span in any::()) { + let max = min.saturating_add(span); + let r = UidGidRange::try_from([min, max]).unwrap(); + prop_assert_eq!((r.min(), r.max()), (min, max)); + } + + #[test] + fn zero_min_is_never_a_range(max in any::()) { + prop_assert_eq!( + UidGidRange::try_from([0, max]), + Err(RangeError { min: 0, max }) + ); + } + + #[test] + fn inverted_pair_is_never_a_range(max in 0u32..u32::MAX, gap in 1u32..) { + let min = max.saturating_add(gap); + prop_assert_eq!( + UidGidRange::try_from([min, max]), + Err(RangeError { min, max }) + ); + } + + #[test] + fn zero_is_never_minted(r in range()) { + prop_assert!(r.uid(0).is_err()); + prop_assert!(r.gid(0).is_err()); + } + + #[test] + fn in_range_id_is_minted_unchanged(r in range(), off in any::()) { + let n = r.min() + off % (r.max() - r.min() + 1); + prop_assert_eq!(Uid::from(r.uid(n).unwrap()), Uid::from_raw(n)); + prop_assert_eq!(Gid::from(r.gid(n).unwrap()), Gid::from_raw(n)); + } + + #[test] + fn out_of_range_id_is_refused(r in range()) { + let expected = |value| IdError { value, min: r.min(), max: r.max() }; + prop_assert_eq!(r.uid(r.min() - 1), Err(expected(r.min() - 1))); + prop_assert_eq!(r.gid(r.min() - 1), Err(expected(r.min() - 1))); + if let Some(above) = r.max().checked_add(1) { + prop_assert_eq!(r.uid(above), Err(expected(above))); + prop_assert_eq!(r.gid(above), Err(expected(above))); + } + } +} diff --git a/native/suidhelper/tests/tools/chroot_jail_grant_api.rs b/native/suidhelper/tests/tools/chroot_jail_grant_api.rs index 9865812c..623489d3 100644 --- a/native/suidhelper/tests/tools/chroot_jail_grant_api.rs +++ b/native/suidhelper/tests/tools/chroot_jail_grant_api.rs @@ -21,7 +21,9 @@ //! its parent `root` dir is opened for the caller's group to traverse //! (chgrp'd to the caller, chmod'd 0710) so the node can reach the socket. -use hyper_suidhelper::fs::path::{ComponentError, RelativeError, ValidationError}; +use hyper_suidhelper::fs::path::{ + ComponentError, IsAbsolute, RelativeError, SafePathBuf, StrictComponents, ValidationError, +}; use hyper_suidhelper::tools::chroot_jail::grant_api::{ grant_api_under, ApiSocket, Error, GrantError, GrantOut, }; @@ -32,6 +34,10 @@ use std::path::{Path, PathBuf}; use std::str::FromStr; use std::{fs, os::unix::fs::MetadataExt}; +fn strict(p: impl AsRef) -> SafePathBuf { + p.as_ref().to_path_buf().try_into().unwrap() +} + /// Parse an absolute socket path into a validated `ApiSocket`, panicking if the /// lexical gate rejects it — for tests whose subject is the base-relative or /// filesystem behavior, not the parse. @@ -83,7 +89,7 @@ fn socket_outside_jail_base_is_rejected() { let jail = tmp.path().join("jail"); fs::create_dir(&jail).unwrap(); let outside = tmp.path().join("elsewhere/exec/id/root/api.socket"); - let err = grant_api_under(&jail, &sock(&outside)).unwrap_err(); + let err = grant_api_under(&strict(&jail), &sock(&outside)).unwrap_err(); assert!( matches!(err, Error::SocketBase(RelativeError::NotUnderBase)), "got {err:?}", @@ -95,7 +101,7 @@ fn too_shallow_is_shape_error() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let bad = jail.join("exec").join("id").join("api.socket"); // missing root/ - let err = grant_api_under(jail, &sock(&bad)).unwrap_err(); + let err = grant_api_under(&strict(jail), &sock(&bad)).unwrap_err(); assert!(matches!(err, Error::SocketShape(_)), "got {err:?}"); } @@ -109,7 +115,7 @@ fn too_deep_is_shape_error() { .join("root") .join("extra") .join("api.socket"); - let err = grant_api_under(jail, &sock(&bad)).unwrap_err(); + let err = grant_api_under(&strict(jail), &sock(&bad)).unwrap_err(); assert!(matches!(err, Error::SocketShape(_)), "got {err:?}"); } @@ -119,7 +125,8 @@ fn missing_socket_is_pending() { let jail = tmp.path(); let root = make_root(jail); let socket = root.join("api.socket"); // never created - let out = grant_api_under(jail, &sock(&socket)).expect("missing socket must be Ok(Pending)"); + let out = + grant_api_under(&strict(jail), &sock(&socket)).expect("missing socket must be Ok(Pending)"); assert!(matches!(out, GrantOut::Pending), "got {out:?}"); } @@ -128,7 +135,8 @@ fn missing_jail_tree_is_pending() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let socket = jail.join("exec").join("id").join("root").join("api.socket"); // nothing created - let out = grant_api_under(jail, &sock(&socket)).expect("half-built jail must be Ok(Pending)"); + let out = grant_api_under(&strict(jail), &sock(&socket)) + .expect("half-built jail must be Ok(Pending)"); assert!(matches!(out, GrantOut::Pending), "got {out:?}"); } @@ -143,7 +151,7 @@ fn real_socket_is_granted_and_chmod_0660() { let _listener = UnixListener::bind(&socket).unwrap(); fs::set_permissions(&socket, fs::Permissions::from_mode(0o755)).unwrap(); - let out = grant_api_under(jail, &sock(&socket)).expect("real socket must grant"); + let out = grant_api_under(&strict(jail), &sock(&socket)).expect("real socket must grant"); assert!(matches!(out, GrantOut::Granted), "got {out:?}"); let meta = fs::symlink_metadata(&socket).unwrap(); @@ -177,7 +185,7 @@ fn regular_file_at_leaf_is_refused_and_untouched() { fs::write(&imposter, b"not a socket").unwrap(); fs::set_permissions(&imposter, fs::Permissions::from_mode(0o600)).unwrap(); - let err = grant_api_under(jail, &sock(&imposter)).unwrap_err(); + let err = grant_api_under(&strict(jail), &sock(&imposter)).unwrap_err(); assert!( matches!(err, Error::Grant(GrantError::NotASocket)), "got {err:?}" @@ -201,7 +209,7 @@ fn symlink_at_leaf_is_refused() { let link = root.join("api.socket"); symlink(&target, &link).unwrap(); - let err = grant_api_under(jail, &sock(&link)).unwrap_err(); + let err = grant_api_under(&strict(jail), &sock(&link)).unwrap_err(); assert!( matches!(err, Error::Grant(GrantError::NotASocket)), "got {err:?}" @@ -226,7 +234,7 @@ fn symlinked_component_does_not_escape() { symlink(&sentinel, jail.join("exec")).unwrap(); let socket = jail.join("exec").join("id").join("root").join("api.socket"); - let _ = grant_api_under(&jail, &sock(&socket)); // O_NOFOLLOW makes the walk refuse + let _ = grant_api_under(&strict(&jail), &sock(&socket)); // O_NOFOLLOW makes the walk refuse assert_eq!( fs::symlink_metadata(&outside_socket).unwrap().mode() & 0o777, @@ -244,6 +252,10 @@ proptest! { fn shape_classification( parents in prop::collection::vec("[a-z][a-z0-9]{0,5}", 1..6) ) { + let mut parents = parents; + if parents.len() == 3 { + parents[2] = "root".to_string(); + } let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let mut socket = jail.to_path_buf(); @@ -251,7 +263,7 @@ proptest! { socket.push(c); } socket.push("api.socket"); - let res = grant_api_under(jail, &sock(&socket)); + let res = grant_api_under(&strict(jail), &sock(&socket)); if parents.len() == 3 { prop_assert!(matches!(res, Ok(GrantOut::Pending)), "depth 3 must be Pending, got {res:?}"); } else { diff --git a/native/suidhelper/tests/tools/chroot_jail_grant_vsock.rs b/native/suidhelper/tests/tools/chroot_jail_grant_vsock.rs index 66d85bf3..dad46efb 100644 --- a/native/suidhelper/tests/tools/chroot_jail_grant_vsock.rs +++ b/native/suidhelper/tests/tools/chroot_jail_grant_vsock.rs @@ -22,7 +22,9 @@ //! its parent `root` dir is opened for the caller's group to traverse //! (chgrp'd to the caller, chmod'd 0710) so the node can reach the socket. -use hyper_suidhelper::fs::path::{ComponentError, RelativeError, ValidationError}; +use hyper_suidhelper::fs::path::{ + ComponentError, IsAbsolute, RelativeError, SafePathBuf, StrictComponents, ValidationError, +}; use hyper_suidhelper::tools::chroot_jail::grant_vsock::{ grant_vsock_under, Error, GrantError, GrantOut, VsockSocket, }; @@ -33,6 +35,10 @@ use std::path::{Path, PathBuf}; use std::str::FromStr; use std::{fs, os::unix::fs::MetadataExt}; +fn strict(p: impl AsRef) -> SafePathBuf { + p.as_ref().to_path_buf().try_into().unwrap() +} + /// Parse an absolute socket path into a validated `VsockSocket`, panicking if /// the lexical gate rejects it — for tests whose subject is the base-relative /// or filesystem behavior, not the parse. @@ -94,7 +100,7 @@ fn socket_outside_jail_base_is_rejected() { let jail = tmp.path().join("jail"); fs::create_dir(&jail).unwrap(); let outside = tmp.path().join("elsewhere/exec/id/root/vsock.sock"); - let err = grant_vsock_under(&jail, &sock(&outside)).unwrap_err(); + let err = grant_vsock_under(&strict(&jail), &sock(&outside)).unwrap_err(); assert!( matches!(err, Error::SocketBase(RelativeError::NotUnderBase)), "got {err:?}", @@ -106,7 +112,7 @@ fn too_shallow_is_shape_error() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let bad = jail.join("exec").join("id").join("vsock.sock"); // missing root/ - let err = grant_vsock_under(jail, &sock(&bad)).unwrap_err(); + let err = grant_vsock_under(&strict(jail), &sock(&bad)).unwrap_err(); assert!(matches!(err, Error::SocketShape(_)), "got {err:?}"); } @@ -120,7 +126,7 @@ fn too_deep_is_shape_error() { .join("root") .join("extra") .join("vsock.sock"); - let err = grant_vsock_under(jail, &sock(&bad)).unwrap_err(); + let err = grant_vsock_under(&strict(jail), &sock(&bad)).unwrap_err(); assert!(matches!(err, Error::SocketShape(_)), "got {err:?}"); } @@ -130,7 +136,8 @@ fn missing_socket_is_pending() { let jail = tmp.path(); let root = make_root(jail); let socket = root.join("vsock.sock"); // never created - let out = grant_vsock_under(jail, &sock(&socket)).expect("missing socket must be Ok(Pending)"); + let out = grant_vsock_under(&strict(jail), &sock(&socket)) + .expect("missing socket must be Ok(Pending)"); assert!(matches!(out, GrantOut::Pending), "got {out:?}"); } @@ -139,7 +146,8 @@ fn missing_jail_tree_is_pending() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let socket = jail.join("exec").join("id").join("root").join("vsock.sock"); - let out = grant_vsock_under(jail, &sock(&socket)).expect("half-built jail must be Ok(Pending)"); + let out = grant_vsock_under(&strict(jail), &sock(&socket)) + .expect("half-built jail must be Ok(Pending)"); assert!(matches!(out, GrantOut::Pending), "got {out:?}"); } @@ -152,7 +160,7 @@ fn real_socket_is_granted_and_chmod_0660() { let _listener = UnixListener::bind(&socket).unwrap(); fs::set_permissions(&socket, fs::Permissions::from_mode(0o755)).unwrap(); - let out = grant_vsock_under(jail, &sock(&socket)).expect("real socket must grant"); + let out = grant_vsock_under(&strict(jail), &sock(&socket)).expect("real socket must grant"); assert!(matches!(out, GrantOut::Granted), "got {out:?}"); let meta = fs::symlink_metadata(&socket).unwrap(); @@ -183,7 +191,7 @@ fn regular_file_at_leaf_is_refused_and_untouched() { fs::write(&imposter, b"not a socket").unwrap(); fs::set_permissions(&imposter, fs::Permissions::from_mode(0o600)).unwrap(); - let err = grant_vsock_under(jail, &sock(&imposter)).unwrap_err(); + let err = grant_vsock_under(&strict(jail), &sock(&imposter)).unwrap_err(); assert!( matches!(err, Error::Grant(GrantError::NotASocket)), "got {err:?}" @@ -206,7 +214,7 @@ fn symlink_at_leaf_is_refused() { let link = root.join("vsock.sock"); symlink(&target, &link).unwrap(); - let err = grant_vsock_under(jail, &sock(&link)).unwrap_err(); + let err = grant_vsock_under(&strict(jail), &sock(&link)).unwrap_err(); assert!( matches!(err, Error::Grant(GrantError::NotASocket)), "got {err:?}" @@ -229,7 +237,7 @@ fn symlinked_component_does_not_escape() { symlink(&sentinel, jail.join("exec")).unwrap(); let socket = jail.join("exec").join("id").join("root").join("vsock.sock"); - let _ = grant_vsock_under(&jail, &sock(&socket)); // O_NOFOLLOW makes the walk refuse + let _ = grant_vsock_under(&strict(&jail), &sock(&socket)); // O_NOFOLLOW makes the walk refuse assert_eq!( fs::symlink_metadata(&outside_socket).unwrap().mode() & 0o777, @@ -252,7 +260,7 @@ fn symlink_to_real_socket_at_leaf_is_refused_and_target_untouched() { let link = root.join("vsock.sock"); symlink(&outside, &link).unwrap(); - let err = grant_vsock_under(jail, &sock(&link)).unwrap_err(); + let err = grant_vsock_under(&strict(jail), &sock(&link)).unwrap_err(); assert!( matches!(err, Error::Grant(GrantError::NotASocket)), "got {err:?}" @@ -276,7 +284,7 @@ fn unreadable_jail_base_is_a_hard_walk_error_not_pending() { let socket = jail.join("exec").join("id").join("root").join("vsock.sock"); fs::set_permissions(&jail, fs::Permissions::from_mode(0o000)).unwrap(); - let res = grant_vsock_under(&jail, &sock(&socket)); + let res = grant_vsock_under(&strict(&jail), &sock(&socket)); // Restore before asserting so the tempdir can clean up even on failure. fs::set_permissions(&jail, fs::Permissions::from_mode(0o755)).unwrap(); @@ -295,6 +303,10 @@ proptest! { fn shape_classification( parents in prop::collection::vec("[a-z][a-z0-9]{0,5}", 1..6) ) { + let mut parents = parents; + if parents.len() == 3 { + parents[2] = "root".to_string(); + } let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let mut socket = jail.to_path_buf(); @@ -302,7 +314,7 @@ proptest! { socket.push(c); } socket.push("vsock.sock"); - let res = grant_vsock_under(jail, &sock(&socket)); + let res = grant_vsock_under(&strict(jail), &sock(&socket)); if parents.len() == 3 { prop_assert!(matches!(res, Ok(GrantOut::Pending)), "depth 3 must be Pending, got {res:?}"); } else { diff --git a/native/suidhelper/tests/tools/chroot_jail_remove.rs b/native/suidhelper/tests/tools/chroot_jail_remove.rs index 21699534..2f24793c 100644 --- a/native/suidhelper/tests/tools/chroot_jail_remove.rs +++ b/native/suidhelper/tests/tools/chroot_jail_remove.rs @@ -11,7 +11,9 @@ //! * removal — a real `/` tree is gone afterward, and its siblings //! and the cgroup parent survive. -use hyper_suidhelper::fs::path::{ComponentError, ValidationError}; +use hyper_suidhelper::fs::path::{ + ComponentError, IsAbsolute, RelativeError, SafePathBuf, StrictComponents, ValidationError, +}; use hyper_suidhelper::tools::chroot_jail::remove::{ remove_cgroup_under, remove_chroot_under, CgroupError, ChrootError, }; @@ -20,6 +22,10 @@ use std::fs; use std::os::unix::fs::symlink; use std::path::{Path, PathBuf}; +fn strict(p: impl AsRef) -> SafePathBuf { + p.as_ref().to_path_buf().try_into().unwrap() +} + // `--chroot` must be exactly /: a single component below the base is // too shallow. #[test] @@ -27,7 +33,7 @@ fn chroot_too_shallow_is_depth_error() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let chroot = jail.join("only-one"); - let err = remove_chroot_under(jail, &chroot).unwrap_err(); + let err = remove_chroot_under(&strict(jail), &chroot).unwrap_err(); assert!(matches!(err, ChrootError::Depth(_)), "got {err:?}"); } @@ -37,19 +43,29 @@ fn chroot_too_deep_is_depth_error() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let chroot = jail.join("exec").join("id").join("extra"); - let err = remove_chroot_under(jail, &chroot).unwrap_err(); + let err = remove_chroot_under(&strict(jail), &chroot).unwrap_err(); assert!(matches!(err, ChrootError::Depth(_)), "got {err:?}"); } -// `--cgroup` must be at least two components below its base: a direct child is -// too shallow. #[test] -fn cgroup_too_shallow_is_depth_error() { +fn cgroup_parent_itself_is_refused() { let tmp = tempfile::tempdir().unwrap(); - let base = tmp.path(); - let cgroup = base.join("leaf"); - let err = remove_cgroup_under(base, &cgroup).unwrap_err(); - assert!(matches!(err, CgroupError::Depth(_)), "got {err:?}"); + let parent = tmp.path().join("hyper"); + let err = remove_cgroup_under(&strict(&parent), &parent).unwrap_err(); + assert!( + matches!(err, CgroupError::Base(RelativeError::EqualsBase)), + "got {err:?}" + ); +} + +#[test] +fn cgroup_leaf_that_is_not_a_vm_id_is_refused() { + let tmp = tempfile::tempdir().unwrap(); + let parent = tmp.path().join("hyper"); + for leaf in ["-leaf", "a.b", "sshd.service", &"a".repeat(65)] { + let err = remove_cgroup_under(&strict(&parent), &parent.join(leaf)).unwrap_err(); + assert!(matches!(err, CgroupError::Leaf(_)), "{leaf:?}: got {err:?}"); + } } // A `..` component is rejected by the real SafePath gate, before any FS access. @@ -58,7 +74,7 @@ fn chroot_with_dotdot_is_rejected() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let chroot = PathBuf::from(format!("{}/exec/../escape", jail.display())); - let err = remove_chroot_under(jail, &chroot).unwrap_err(); + let err = remove_chroot_under(&strict(jail), &chroot).unwrap_err(); assert!( matches!( err, @@ -72,7 +88,7 @@ fn chroot_with_dotdot_is_rejected() { #[test] fn relative_chroot_is_rejected() { let tmp = tempfile::tempdir().unwrap(); - let err = remove_chroot_under(tmp.path(), Path::new("exec/id")).unwrap_err(); + let err = remove_chroot_under(&strict(tmp.path()), Path::new("exec/id")).unwrap_err(); assert!( matches!(err, ChrootError::Path(ValidationError::NotAbsolute)), "got {err:?}", @@ -85,16 +101,16 @@ fn missing_chroot_is_ok() { let tmp = tempfile::tempdir().unwrap(); let jail = tmp.path(); let chroot = jail.join("exec").join("id"); // never created - remove_chroot_under(jail, &chroot).expect("missing chroot must be Ok"); + remove_chroot_under(&strict(jail), &chroot).expect("missing chroot must be Ok"); } -// Removing a cgroup leaf whose ancestor is missing is success. +// Removing a cgroup leaf whose parent is missing is success. #[test] fn missing_cgroup_is_ok() { let tmp = tempfile::tempdir().unwrap(); - let base = tmp.path(); - let cgroup = base.join("slice").join("leaf"); // slice/ never created - remove_cgroup_under(base, &cgroup).expect("missing cgroup must be Ok"); + let parent = tmp.path().join("hyper"); // never created + let cgroup = parent.join("leaf"); + remove_cgroup_under(&strict(&parent), &cgroup).expect("missing cgroup must be Ok"); } // A real / tree with contents is fully removed; a sibling id survives. @@ -108,7 +124,7 @@ fn removes_chroot_tree_and_spares_siblings() { let sibling = jail.join("exec").join("other"); fs::create_dir_all(&sibling).unwrap(); - remove_chroot_under(jail, &victim).unwrap(); + remove_chroot_under(&strict(jail), &victim).unwrap(); assert!(!victim.exists(), "victim chroot must be gone"); assert!(sibling.exists(), "sibling chroot must survive"); } @@ -132,7 +148,7 @@ fn symlinked_chroot_component_does_not_escape() { let chroot = jail.join("exec").join("id"); // O_NOFOLLOW on the `exec` component makes the walk fail rather than follow. - let _ = remove_chroot_under(&jail, &chroot); + let _ = remove_chroot_under(&strict(&jail), &chroot); assert!( sentinel.join("id").join("keep.txt").exists(), @@ -145,12 +161,11 @@ fn symlinked_chroot_component_does_not_escape() { #[test] fn missing_leaf_with_present_parent_is_ok() { let tmp = tempfile::tempdir().unwrap(); - let base = tmp.path(); - let parent = base.join("slice"); + let parent = tmp.path().join("hyper"); fs::create_dir(&parent).unwrap(); let leaf = parent.join("leaf"); // never created - remove_cgroup_under(base, &leaf).expect("missing leaf must be Ok"); + remove_cgroup_under(&strict(&parent), &leaf).expect("missing leaf must be Ok"); assert!(parent.exists(), "parent must survive"); } @@ -168,7 +183,7 @@ proptest! { for c in &comps { chroot.push(c); } - let res = remove_chroot_under(jail, &chroot); + let res = remove_chroot_under(&strict(jail), &chroot); if comps.len() == 2 { prop_assert!(res.is_ok(), "depth 2 must be Ok, got {res:?}"); } else { @@ -176,23 +191,40 @@ proptest! { } } - // For a cgroup `depth` components below its base, remove_cgroup_under returns - // CgroupDepth iff depth == 1, else Ok (target missing -> idempotent). #[test] - fn cgroup_depth_classification( + fn cgroup_shape_classification( comps in prop::collection::vec("[a-z][a-z0-9]{0,5}", 1..6) ) { let tmp = tempfile::tempdir().unwrap(); - let base = tmp.path(); - let mut cgroup = base.to_path_buf(); + let parent = tmp.path().join("hyper"); + let mut cgroup = parent.clone(); for c in &comps { cgroup.push(c); } - let res = remove_cgroup_under(base, &cgroup); + let res = remove_cgroup_under(&strict(&parent), &cgroup); if comps.len() == 1 { - prop_assert!(matches!(res, Err(CgroupError::Depth(_))), "got {res:?}"); + prop_assert!(res.is_ok(), "parent/ must be Ok, got {res:?}"); } else { - prop_assert!(res.is_ok(), "depth >= 2 must be Ok, got {res:?}"); + prop_assert!(matches!(res, Err(CgroupError::Shape(_))), "got {res:?}"); + } + } + + #[test] + fn cgroup_outside_parent_is_refused( + other in "[a-z][a-z0-9]{0,5}", + comps in prop::collection::vec("[a-z][a-z0-9]{0,5}", 0..4), + ) { + prop_assume!(other != "hyper"); + let tmp = tempfile::tempdir().unwrap(); + let parent = tmp.path().join("hyper"); + let mut cgroup = tmp.path().join(&other); + for c in &comps { + cgroup.push(c); } + let res = remove_cgroup_under(&strict(&parent), &cgroup); + prop_assert!( + matches!(res, Err(CgroupError::Base(RelativeError::NotUnderBase))), + "got {res:?}" + ); } } diff --git a/native/suidhelper/tests/tools/dmsetup_backing.rs b/native/suidhelper/tests/tools/dmsetup_backing.rs index f1e13cb0..55487024 100644 --- a/native/suidhelper/tests/tools/dmsetup_backing.rs +++ b/native/suidhelper/tests/tools/dmsetup_backing.rs @@ -8,14 +8,19 @@ //! * an in-base regular file is accepted as a handle to exactly that inode; //! * anything in-base that is not a regular file is refused. +use hyper_suidhelper::fs::path::{IsAbsolute, SafePathBuf, StrictComponents}; use hyper_suidhelper::tools::{BackingError, BackingFile}; use nix::sys::stat::fstat; use proptest::prelude::*; use std::os::unix::fs::MetadataExt; use std::os::unix::io::AsRawFd; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use tempfile::TempDir; +fn strict(p: impl AsRef) -> SafePathBuf { + p.as_ref().to_path_buf().try_into().unwrap() +} + fn real_tempdir() -> (TempDir, PathBuf) { let tmp = tempfile::tempdir().unwrap(); let real = std::fs::canonicalize(tmp.path()).unwrap(); @@ -32,7 +37,7 @@ proptest! { let p = outside.join(&name); std::fs::write(&p, b"x").unwrap(); - let err = BackingFile::open(&p, &base).expect_err("out-of-base backing file accepted"); + let err = BackingFile::open(&p, &strict(&base)).expect_err("out-of-base backing file accepted"); prop_assert!(matches!(err, BackingError::OutsideBase { .. }), "got {err:?}"); } } @@ -41,7 +46,8 @@ proptest! { fn absent_path_is_refused_as_canonicalize() { let (_base_dir, base) = real_tempdir(); - let err = BackingFile::open(&base.join("nope.img"), &base).expect_err("absent file accepted"); + let err = BackingFile::open(&base.join("nope.img"), &strict(&base)) + .expect_err("absent file accepted"); assert!( matches!(err, BackingError::Canonicalize { .. }), "got {err:?}" @@ -66,7 +72,8 @@ fn refusal_is_decided_on_the_resolved_path() { .join("target.img"); for spelling in [outer_link, inner_link, dotdot] { - let err = BackingFile::open(&spelling, &base).expect_err("out-of-base spelling accepted"); + let err = BackingFile::open(&spelling, &strict(&base)) + .expect_err("out-of-base spelling accepted"); assert!( matches!(err, BackingError::OutsideBase { .. }), "{}: got {err:?}", @@ -81,7 +88,7 @@ fn in_base_file_is_accepted_as_that_inode() { let backing = base.join("disk.img"); std::fs::write(&backing, b"data").unwrap(); - let got = BackingFile::open(&backing, &base).expect("in-base regular file refused"); + let got = BackingFile::open(&backing, &strict(&base)).expect("in-base regular file refused"); let held = fstat(got.file().as_raw_fd()).unwrap(); let on_disk = std::fs::metadata(&backing).unwrap(); @@ -94,6 +101,7 @@ fn directory_under_base_is_refused() { let dir = base.join("subdir"); std::fs::create_dir(&dir).unwrap(); - let err = BackingFile::open(&dir, &base).expect_err("directory accepted as backing file"); + let err = + BackingFile::open(&dir, &strict(&base)).expect_err("directory accepted as backing file"); assert!(matches!(err, BackingError::Open(_)), "got {err:?}"); } diff --git a/native/suidhelper/tests/tools/jailer.rs b/native/suidhelper/tests/tools/jailer.rs index 9ab75447..9adc29af 100644 --- a/native/suidhelper/tests/tools/jailer.rs +++ b/native/suidhelper/tests/tools/jailer.rs @@ -4,51 +4,43 @@ //! a compromised BEAM from naming uid 0, a privileged path, a traversal, or a //! flag through the jailer subcommand. -use hyper_suidhelper::tools::jailer::{validate_id_number, CgroupSetting, JailSock, VmId}; +use clap::{Args, FromArgMatches}; +use hyper_suidhelper::firecracker::jailer::VmId; +use hyper_suidhelper::tools::jailer::JailerArgs; use proptest::prelude::*; use std::str::FromStr; -proptest! { - /// uid/gid 0 is rejected for EVERY range — a jailer run with uid 0 skips its - /// privilege drop and leaves firecracker running as root. - #[test] - fn id_zero_always_rejected(lo in any::(), span in any::()) { - let hi = lo.saturating_add(span); - prop_assert!(validate_id_number(0, (lo, hi)).is_err()); - } - - /// Any nonzero value inside the (nonempty) range is accepted unchanged. - #[test] - fn id_in_range_nonzero_accepted( - lo in 1u32..=1_000_000, - span in 0u32..=1_000_000, - off in 0u32..=1_000_000, - ) { - let hi = lo + span; - let n = lo + (off % (span + 1)); // off bounded into [0, span] => n in [lo, hi] - prop_assert_eq!(validate_id_number(n, (lo, hi)).ok(), Some(n)); - } +fn parse(limits: &[&str]) -> Result { + parse_with_sock("/api.sock", limits) +} - /// Values just below `lo` (still nonzero) and just above `hi` are rejected. - #[test] - fn id_out_of_range_rejected(lo in 2u32..=1_000_000, span in 0u32..=1_000_000) { - let hi = lo + span; - prop_assert!(validate_id_number(lo - 1, (lo, hi)).is_err()); - if hi < u32::MAX { - prop_assert!(validate_id_number(hi + 1, (lo, hi)).is_err()); - } - } +fn parse_with_sock(api_sock: &str, limits: &[&str]) -> Result { + let base = [ + "--id", + "vm1", + "--uid", + "900001", + "--gid", + "900001", + "--api-sock", + api_sock, + ]; + let matches = JailerArgs::augment_args(clap::Command::new("jailer").no_binary_name(true)) + .try_get_matches_from(base.iter().chain(limits))?; + JailerArgs::from_arg_matches(&matches) +} +proptest! { /// Every string over the allowed charset/length with a non-dash leader parses /// and renders back to itself. #[test] - fn vmid_valid_round_trips(s in "[A-Za-z0-9_][A-Za-z0-9_-]{0,63}") { + fn vmid_valid_round_trips(s in "[A-Za-z0-9][A-Za-z0-9-]{0,63}") { prop_assert_eq!(VmId::from_str(&s).unwrap().to_string(), s); } /// A leading dash is always rejected (no flag injection via `--id`). #[test] - fn vmid_leading_dash_rejected(s in "-[A-Za-z0-9_-]{0,63}") { + fn vmid_leading_dash_rejected(s in "-[A-Za-z0-9-]{0,63}") { prop_assert!(VmId::from_str(&s).is_err()); } @@ -60,45 +52,10 @@ proptest! { /// Over-length ids (>64) are rejected. #[test] - fn vmid_too_long_rejected(s in "[A-Za-z][A-Za-z0-9_-]{64,90}") { + fn vmid_too_long_rejected(s in "[A-Za-z][A-Za-z0-9-]{64,90}") { prop_assert!(VmId::from_str(&s).is_err()); } - /// A valid `memory.max` setting re-renders to exactly `key=value`. - #[test] - fn cgroup_memory_round_trips(s in "memory[.]max=([0-9]{1,20}|max)") { - prop_assert_eq!(CgroupSetting::from_str(&s).unwrap().to_string(), s); - } - - /// A valid `cpu.max` setting re-renders to exactly `key=value`. - #[test] - fn cgroup_cpu_round_trips(s in "cpu[.]max=([0-9]{1,20} [0-9]{1,20}|max [0-9]{1,20})") { - prop_assert_eq!(CgroupSetting::from_str(&s).unwrap().to_string(), s); - } - - /// A single-filename absolute socket path round-trips; `.`/`..` filenames are - /// rejected even though they are within the charset. - #[test] - fn jailsock_single_filename(name in "[A-Za-z0-9_.-]{1,40}") { - let s = format!("/{name}"); - let res = JailSock::from_str(&s); - if name == "." || name == ".." { - prop_assert!(res.is_err()); - } else { - prop_assert_eq!(res.unwrap().to_string(), s); - } - } - - /// A second path component is always rejected (the socket must be a direct - /// child of `/`). - #[test] - fn jailsock_multi_component_rejected( - a in "[A-Za-z0-9_]{1,10}", - b in "[A-Za-z0-9_]{1,10}", - ) { - let s = format!("/{a}/{b}"); - prop_assert!(JailSock::from_str(&s).is_err()); - } } #[test] @@ -108,6 +65,9 @@ fn vmid_rejects_known_bad_shapes() { "-leading", // leading dash "a/b", // separator "a.b", // dot + ".", // current dir + "..", // parent dir + "a_b", // underscore (firecracker rejects it) "a b", // whitespace "a\tb", // tab "a\0b", // NUL @@ -119,44 +79,57 @@ fn vmid_rejects_known_bad_shapes() { } #[test] -fn cgroup_rejects_known_bad_shapes() { - for bad in [ - "linear=10", // unknown key - "memory.high=10", // unknown key - "memory.max=", // empty value - "memory.max=12x", // non-digit - "memory.max=1=2", // second '=' - "memory.max", // no '=' - "cpu.max=100000", // missing period field - "cpu.max=100000 100000 5", // extra field - "cpu.max=x 100000", // bad quota - "cpu.max=max max", // bad period - "cpu.max=max", // missing period +fn cgroup_limits_accepted() { + for limits in [ + &[][..], + &["--memory-max", "1048576"], + &["--cpu-quota-us", "50000", "--cpu-period-us", "100000"], + &[ + "--memory-max", + "1", + "--cpu-quota-us", + "1", + "--cpu-period-us", + "1", + ], ] { - assert!( - CgroupSetting::from_str(bad).is_err(), - "CgroupSetting accepted {bad:?}" - ); + assert!(parse(limits).is_ok(), "{limits:?} refused"); } } #[test] -fn jailsock_rejects_known_bad_shapes() { - for bad in [ - "", // empty - "relative", // not absolute - "/", // no filename - "/a/b", // multi-component - "/../etc", // traversal - "/..", // traversal as whole filename - "/.", // current dir - "/a b", // whitespace - "/a\0b", // NUL - "//x", // empty leading component +fn cgroup_limits_refused() { + for limits in [ + &["--memory-max", "0"][..], + &["--memory-max", "max"], + &["--memory-max", "-1"], + &["--memory-max", "12x"], + &["--memory-max", "18446744073709551616"], + &["--memory-max", "1", "--memory-max", "2"], + &["--cpu-quota-us", "50000"], + &["--cpu-period-us", "100000"], + &["--cpu-quota-us", "0", "--cpu-period-us", "100000"], + &["--cpu-quota-us", "50000", "--cpu-period-us", "0"], + &[ + "--cpu-quota-us", + "1", + "--cpu-quota-us", + "2", + "--cpu-period-us", + "1", + ], + &["--cgroup", "memory.max=1"], ] { - assert!( - JailSock::from_str(bad).is_err(), - "JailSock accepted {bad:?}" - ); + assert!(parse(limits).is_err(), "{limits:?} accepted"); + } +} + +#[test] +fn api_sock_must_be_a_strict_absolute_path() { + for ok in ["/api.socket", "/run/fc/api.socket", "/-dash", "/a b"] { + assert!(parse_with_sock(ok, &[]).is_ok(), "{ok:?} refused"); + } + for bad in ["", "api.socket", "/../etc/x", "/a/./b", "//x", "/a/"] { + assert!(parse_with_sock(bad, &[]).is_err(), "{bad:?} accepted"); } } diff --git a/native/suidhelper/tests/tools/network_addr.rs b/native/suidhelper/tests/tools/network_addr.rs index 96ec7d93..a933646d 100644 --- a/native/suidhelper/tests/tools/network_addr.rs +++ b/native/suidhelper/tests/tools/network_addr.rs @@ -1,11 +1,20 @@ -use hyper_suidhelper::tools::jailer::VmId; +use hyper_suidhelper::firecracker::id::{JailUid, UidGidRange}; +use hyper_suidhelper::firecracker::jailer::VmId; use hyper_suidhelper::tools::network::addr::{self, AddrError, Plan}; use proptest::prelude::*; use std::net::Ipv4Addr; use std::str::FromStr; const POOL: Ipv4Addr = Ipv4Addr::new(172, 31, 0, 0); -const RANGE: (u32, u32) = (900_000, 999_999); +const FLOOR: u32 = 900_000; + +fn range() -> UidGidRange { + UidGidRange::try_from([FLOOR, 999_999]).unwrap() +} + +fn jail_uid(n: u32) -> JailUid { + range().uid(n).unwrap() +} fn vm_id() -> VmId { VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap() @@ -13,7 +22,7 @@ fn vm_id() -> VmId { #[test] fn slot_zero_is_first_block() { - let p = Plan::derive(900_000, RANGE, POOL, &vm_id()).unwrap(); + let p = Plan::derive(jail_uid(FLOOR), range(), POOL, &vm_id()).unwrap(); assert_eq!(p.slot, 0); assert_eq!(p.veth_host_ip, Ipv4Addr::new(172, 31, 0, 1)); assert_eq!(p.veth_ns_ip, Ipv4Addr::new(172, 31, 0, 2)); @@ -24,7 +33,12 @@ fn slot_zero_is_first_block() { #[test] fn uid_below_range_is_refused() { assert!(matches!( - Plan::derive(42, RANGE, POOL, &vm_id()), + Plan::derive( + UidGidRange::try_from([1, 100]).unwrap().uid(42).unwrap(), + range(), + POOL, + &vm_id() + ), Err(AddrError::UidBelowRange { .. }) )); } @@ -34,8 +48,8 @@ proptest! { // IFNAMSIZ, for every uid the helper would ever accept. #[test] fn derived_plan_is_well_formed(offset in 0u32..16_384) { - let uid = RANGE.0 + offset; - let p = Plan::derive(uid, RANGE, POOL, &vm_id()).unwrap(); + let uid = FLOOR + offset; + let p = Plan::derive(jail_uid(uid), range(), POOL, &vm_id()).unwrap(); prop_assert!(p.veth_host.len() <= 15 && p.veth_ns.len() <= 15); // clone addresses live in 172.31/16, never in the inner 172.30/30 prop_assert_eq!(p.veth_host_ip.octets()[1], 31); @@ -47,8 +61,8 @@ proptest! { // A slot past the /16's capacity is always refused, never wrapped. #[test] fn slot_past_pool_is_refused(offset in 16_384u32..40_000) { - let uid = RANGE.0 + offset; - let result = Plan::derive(uid, RANGE, POOL, &vm_id()); + let uid = FLOOR + offset; + let result = Plan::derive(jail_uid(uid), range(), POOL, &vm_id()); let is_slot_out_of_pool = matches!(result, Err(AddrError::SlotOutOfPool { .. })); prop_assert!(is_slot_out_of_pool); } @@ -61,10 +75,10 @@ proptest! { offset_a in 0u32..16_384, offset_b in 0u32..16_384, ) { - let uid_a = RANGE.0 + offset_a; - let uid_b = RANGE.0 + offset_b; - let plan_a = Plan::derive(uid_a, RANGE, POOL, &vm_id()).unwrap(); - let plan_b = Plan::derive(uid_b, RANGE, POOL, &vm_id()).unwrap(); + let uid_a = FLOOR + offset_a; + let uid_b = FLOOR + offset_b; + let plan_a = Plan::derive(jail_uid(uid_a), range(), POOL, &vm_id()).unwrap(); + let plan_b = Plan::derive(jail_uid(uid_b), range(), POOL, &vm_id()).unwrap(); let ip_a = u32::from(plan_a.veth_host_ip); let ip_b = u32::from(plan_b.veth_host_ip); diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index 98e6226f..2b2d027f 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -1,5 +1,6 @@ use hyper_suidhelper::config::Network; -use hyper_suidhelper::tools::jailer::VmId; +use hyper_suidhelper::firecracker::id::UidGidRange; +use hyper_suidhelper::firecracker::jailer::VmId; use hyper_suidhelper::tools::network::addr::Plan; use hyper_suidhelper::tools::network::args::{self, Which}; use hyper_suidhelper::tools::network::{prepare, resolve_network, NetworkingDisabled}; @@ -7,9 +8,10 @@ use std::net::Ipv4Addr; use std::str::FromStr; fn plan0() -> Plan { + let range = UidGidRange::default(); Plan::derive( - 900_000, - (900_000, 999_999), + range.uid(900_000).unwrap(), + range, Ipv4Addr::new(172, 31, 0, 0), &VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(), ) @@ -392,7 +394,7 @@ fn prepare_refuses_uid_outside_range() { // itself fails. let err = prepare::plan_from( 42, - (900_000, 999_999), + UidGidRange::default(), "172.31.0.0/16", &VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(), ); @@ -405,7 +407,7 @@ fn prepare_refuses_malformed_clone_pool_cidr() { // uid-range refusal. let err = prepare::plan_from( 900_000, - (900_000, 999_999), + UidGidRange::default(), "not-a-cidr", &VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(), ); diff --git a/native/suidhelper/tests/util/chroot_jail.rs b/native/suidhelper/tests/util/chroot_jail.rs deleted file mode 100644 index b803ad32..00000000 --- a/native/suidhelper/tests/util/chroot_jail.rs +++ /dev/null @@ -1,277 +0,0 @@ -//! Contracts of the VM jail builder (`util::chroot_jail`), driven through the -//! base-injected seams. Non-root, in a tempdir, the promises under test: -//! * confinement — `open_chroot_under` follows no symlinked component and -//! refuses a chroot that is not under the jail base; -//! * source confinement — `stage_kernel_under` canonicalizes the source and -//! refuses anything resolving outside `hyper_base` (a symlink that escapes -//! is caught after resolution, not before); -//! * staging — a confined source is materialized as `vmlinux`, byte-identical, -//! and re-staging into an occupied slot is refused (O_EXCL / EEXIST). -//! -//! Root-gated (self-skip without root): a full `build_under` against a real loop -//! device stages the kernel AND creates the `rootfs` block node whose major:minor -//! mirrors the source device, owned by the requested uid:gid. - -use hyper_suidhelper::fs::dir::SafeDir; -use hyper_suidhelper::fs::file::OpenFlags; -use hyper_suidhelper::fs::path::{IsAbsolute, RelativeError, SafePathBuf, StrictComponents}; -use hyper_suidhelper::linux::dm::DmName; -use hyper_suidhelper::util::chroot_jail::{ - open_chroot_under, stage_kernel_under, ChrootJail, Error, -}; -use nix::errno::Errno; -use std::fs; -use std::os::unix::fs::{symlink, MetadataExt}; -use std::path::{Path, PathBuf}; -use std::process::Command; - -type Strict = SafePathBuf; - -fn safe(p: &Path) -> Strict { - p.to_path_buf() - .try_into() - .expect("test path is strict-absolute") -} - -fn is_root() -> bool { - nix::unistd::geteuid().is_root() -} - -fn own_ids() -> (u32, u32) { - ( - nix::unistd::getuid().as_raw(), - nix::unistd::getgid().as_raw(), - ) -} - -// A valid / tree under the jail base opens successfully. -#[test] -fn open_chroot_descends_valid_tree() { - let tmp = tempfile::tempdir().unwrap(); - let jail = tmp.path(); - let chroot = jail.join("exec").join("id"); - fs::create_dir_all(&chroot).unwrap(); - assert!(open_chroot_under(jail, &chroot).is_ok()); -} - -// A symlinked component is not followed (O_NOFOLLOW): open must fail. -#[test] -fn open_chroot_rejects_symlinked_component() { - let tmp = tempfile::tempdir().unwrap(); - let jail = tmp.path(); - let real_exec = tmp.path().join("real-exec"); - fs::create_dir_all(real_exec.join("id")).unwrap(); - symlink(&real_exec, jail.join("exec")).unwrap(); - - let chroot = jail.join("exec").join("id"); - assert!( - open_chroot_under(jail, &chroot).is_err(), - "open_chroot followed a symlinked component", - ); -} - -// A chroot that is not under the jail base is rejected. -#[test] -fn open_chroot_rejects_path_outside_base() { - let tmp = tempfile::tempdir().unwrap(); - let jail = tmp.path().join("jail"); - fs::create_dir(&jail).unwrap(); - let outside = tmp.path().join("elsewhere").join("id"); - let result = open_chroot_under(&jail, &outside); - let err = result.expect_err("expected an error for path outside base"); - assert!( - matches!(err, Error::Relative(RelativeError::NotUnderBase)), - "got {err:?}", - ); -} - -// A source that lives entirely outside hyper_base is refused. -#[test] -fn stage_kernel_rejects_source_outside_base() { - let tmp = tempfile::tempdir().unwrap(); - let hyper_base = tmp.path().join("hyper"); - let chroot_dir = hyper_base.join("jail"); - fs::create_dir_all(&chroot_dir).unwrap(); - let outside = tmp.path().join("outside-kernel"); - fs::write(&outside, b"kernel").unwrap(); - - let chroot = SafeDir::open(&safe(&chroot_dir), OpenFlags::RDONLY).unwrap(); - let (uid, gid) = own_ids(); - let err = stage_kernel_under(&chroot, &outside, &hyper_base, uid, gid).unwrap_err(); - assert!(matches!(err, Error::OutsideBase { .. }), "got {err:?}"); -} - -// A source UNDER hyper_base but which is a symlink RESOLVING outside it is -// refused: confinement is enforced after canonicalization, not on the lexical -// path. This is the key source-confinement invariant. -#[test] -fn stage_kernel_rejects_symlink_escaping_base() { - let tmp = tempfile::tempdir().unwrap(); - let hyper_base = tmp.path().join("hyper"); - let chroot_dir = hyper_base.join("jail"); - fs::create_dir_all(&chroot_dir).unwrap(); - - let real_target = tmp.path().join("secret"); // OUTSIDE hyper_base - fs::write(&real_target, b"secret kernel").unwrap(); - let link = hyper_base.join("kernel-link"); // INSIDE hyper_base, escapes - symlink(&real_target, &link).unwrap(); - - let chroot = SafeDir::open(&safe(&chroot_dir), OpenFlags::RDONLY).unwrap(); - let (uid, gid) = own_ids(); - let err = stage_kernel_under(&chroot, &link, &hyper_base, uid, gid).unwrap_err(); - assert!( - matches!(err, Error::OutsideBase { .. }), - "symlink escaping the base must be refused, got {err:?}", - ); -} - -// A missing source path is reported as a source error, not a confinement one. -#[test] -fn stage_kernel_missing_source_errors() { - let tmp = tempfile::tempdir().unwrap(); - let hyper_base = tmp.path(); - let chroot_dir = hyper_base.join("jail"); - fs::create_dir_all(&chroot_dir).unwrap(); - let missing = hyper_base.join("nope"); - - let chroot = SafeDir::open(&safe(&chroot_dir), OpenFlags::RDONLY).unwrap(); - let (uid, gid) = own_ids(); - let err = stage_kernel_under(&chroot, &missing, hyper_base, uid, gid).unwrap_err(); - assert!(matches!(err, Error::Source { .. }), "got {err:?}"); -} - -// A confined source is staged byte-identically as `vmlinux`; re-staging into the -// now-occupied slot is refused (O_EXCL / link EEXIST). -#[test] -fn stage_kernel_materializes_then_refuses_overwrite() { - let tmp = tempfile::tempdir().unwrap(); - let hyper_base = tmp.path(); - let chroot_dir = hyper_base.join("jail"); - fs::create_dir_all(&chroot_dir).unwrap(); - let src = hyper_base.join("vmlinux-src"); - fs::write(&src, b"\x7fELF fake kernel bytes").unwrap(); - - let chroot = SafeDir::open(&safe(&chroot_dir), OpenFlags::RDONLY).unwrap(); - let (uid, gid) = own_ids(); - stage_kernel_under(&chroot, &src, hyper_base, uid, gid).expect("first stage"); - - let staged = chroot_dir.join("vmlinux"); - assert_eq!( - fs::read(&staged).unwrap(), - b"\x7fELF fake kernel bytes", - "staged kernel bytes must match the source", - ); - - // Re-staging must not silently clobber: the slot is occupied. - let chroot2 = SafeDir::open(&safe(&chroot_dir), OpenFlags::RDONLY).unwrap(); - let err = stage_kernel_under(&chroot2, &src, hyper_base, uid, gid).unwrap_err(); - assert!( - matches!(&err, Error::StageKernel(e) if e.errno() == Some(Errno::EEXIST)), - "re-stage must be refused, got {err:?}" - ); -} - -/// Attach a fresh loop device backed by a 1 MiB temp file. Returns the device -/// path, or None if losetup is unavailable / fails (test self-skips). -fn setup_loop(tmp: &Path) -> Option { - let backing = tmp.join("backing.img"); - let f = fs::File::create(&backing).ok()?; - f.set_len(1024 * 1024).ok()?; - let out = Command::new("losetup") - .args(["--find", "--show"]) - .arg(&backing) - .output() - .ok()?; - if !out.status.success() { - return None; - } - let dev = String::from_utf8(out.stdout).ok()?.trim().to_string(); - if dev.is_empty() { - return None; - } - Some(PathBuf::from(dev)) -} - -fn teardown_loop(dev: &Path) { - let _ = Command::new("losetup").arg("-d").arg(dev).output(); -} - -fn setup_dm(loop_dev: &Path) -> Option { - let name = format!("hyper-test-chroot-{}", std::process::id()); - let table = format!("0 2048 linear {} 0", loop_dev.display()); - let created = Command::new("dmsetup") - .args(["create", &name, "--table", &table]) - .status() - .ok()? - .success(); - if !created { - return None; - } - name.parse().ok() -} - -fn teardown_dm(name: &DmName) { - let _ = Command::new("dmsetup") - .args(["remove", "--retry"]) - .arg(name.as_os_str()) - .output(); -} - -// A full build stages the kernel AND creates a `rootfs` block node mirroring the -// loop device's major:minor, both owned by the requested uid:gid. -#[test] -fn build_under_stages_kernel_and_mirrors_device_as_root() { - if !is_root() { - eprintln!("SKIP build_under_stages_kernel_and_mirrors_device: needs root for mknod/chown"); - return; - } - let tmp = tempfile::tempdir().unwrap(); - let hyper_base = tmp.path(); - let chroot_dir = hyper_base.join("jails").join("exec").join("id"); - fs::create_dir_all(&chroot_dir).unwrap(); - let jail_base = hyper_base.join("jails"); - let kernel_src = hyper_base.join("vmlinux-src"); - fs::write(&kernel_src, b"kernel image").unwrap(); - - let Some(dev) = setup_loop(hyper_base) else { - eprintln!("SKIP build_under: losetup unavailable"); - return; - }; - let Some(block) = setup_dm(&dev) else { - teardown_loop(&dev); - eprintln!("SKIP build_under: dmsetup unavailable"); - return; - }; - let block_path = block.path(); - - let (uid, gid) = (12345u32, 12345u32); - let res = ChrootJail::new(chroot_dir.clone(), uid, gid) - .with_kernel(kernel_src.clone()) - .with_rootfs(block.clone()) - .build_under(&jail_base, hyper_base); - - let dev_rdev = fs::metadata(block_path.as_path()).map(|m| m.rdev()); - teardown_dm(&block); - teardown_loop(&dev); - res.expect("build_under must succeed as root against a real device"); - - let vmlinux = chroot_dir.join("vmlinux"); - let rootfs = chroot_dir.join("rootfs"); - assert_eq!(fs::read(&vmlinux).unwrap(), b"kernel image"); - - let kmeta = fs::metadata(&vmlinux).unwrap(); - assert_eq!((kmeta.uid(), kmeta.gid()), (uid, gid), "kernel ownership"); - - let rmeta = fs::metadata(&rootfs).unwrap(); - assert_eq!((rmeta.uid(), rmeta.gid()), (uid, gid), "rootfs ownership"); - assert_eq!( - rmeta.mode() & 0o170000, - 0o060000, - "rootfs must be a block device" - ); - assert_eq!( - rmeta.rdev(), - dev_rdev.unwrap(), - "rootfs node must mirror the source device major:minor", - ); -} diff --git a/native/suidhelper/tests/util/safe_path.rs b/native/suidhelper/tests/util/safe_path.rs index b0b68309..7a5869d6 100644 --- a/native/suidhelper/tests/util/safe_path.rs +++ b/native/suidhelper/tests/util/safe_path.rs @@ -11,6 +11,10 @@ use std::path::{Path, PathBuf}; // The fully-enforced flavor used at every real call site. type Strict = SafePathBuf; +fn strict(p: impl AsRef) -> Strict { + p.as_ref().to_path_buf().try_into().unwrap() +} + // A plain, safe path component: no `.`/`..`/`/`/empty. fn safe_component() -> impl Strategy { "[a-z][a-z0-9_]{0,7}" @@ -87,7 +91,7 @@ proptest! { let safe = Strict::try_from(full.clone()).unwrap(); let segments: Vec = safe - .strip_base(&base) + .strip_base(&strict(&base)) .unwrap() .segments() .map(|s| s.as_os_str().to_string_lossy().into_owned()) @@ -113,7 +117,7 @@ proptest! { let safe = Strict::try_from(path).unwrap(); prop_assert!(matches!( - safe.strip_base(&base), + safe.strip_base(&strict(&base)), Err(RelativeError::NotUnderBase) )); } @@ -138,7 +142,7 @@ proptest! { let full_parts: Vec = base_parts.iter().chain(rel_parts.iter()).cloned().collect(); let safe = Strict::try_from(join_abs(&full_parts)).unwrap(); - let rel = safe.strip_base(&base).unwrap(); + let rel = safe.strip_base(&strict(&base)).unwrap(); prop_assert_eq!(rel.file_name(), rel.segments().last()); } @@ -149,7 +153,7 @@ proptest! { let base = join_abs(&base_parts); let safe = Strict::try_from(base.clone()).unwrap(); prop_assert!(matches!( - safe.strip_base(&base), + safe.strip_base(&strict(&base)), Err(RelativeError::EqualsBase) )); } diff --git a/test/hyper/node/fire_vmm/jailer_test.exs b/test/hyper/node/fire_vmm/jailer_test.exs index 3b1d9f14..34030618 100644 --- a/test/hyper/node/fire_vmm/jailer_test.exs +++ b/test/hyper/node/fire_vmm/jailer_test.exs @@ -74,11 +74,18 @@ defmodule Hyper.Node.FireVMM.JailerTest do refute "--" in args end - test "args include --cgroup cpu.max and memory.max for :micro type" do + test "args carry the :micro cgroup limits as typed flags" do %{args: args} = Jailer.command(micro_opts()) - assert "--cgroup" in args - assert Enum.any?(args, &String.starts_with?(&1, "cpu.max=")) - assert Enum.any?(args, &String.starts_with?(&1, "memory.max=")) + cgroup = Hyper.Vm.Instance.Spec.cgroup_v2(Hyper.Vm.Instance.spec(:micro)) + + assert flag_value(args, "--memory-max") == to_string(cgroup.memory_max) + assert flag_value(args, "--cpu-quota-us") == to_string(cgroup.cpu_max.quota_us) + assert flag_value(args, "--cpu-period-us") == to_string(cgroup.cpu_max.period_us) + refute "--cgroup" in args + end + + defp flag_value(args, flag) do + args |> Enum.drop_while(&(&1 != flag)) |> Enum.at(1) end test "host_vsock/1 returns the chroot-rooted host path for vsock.sock" do