diff --git a/docs/trust-boundary/security.md b/docs/trust-boundary/security.md index 2db0f87f..69f28780 100644 --- a/docs/trust-boundary/security.md +++ b/docs/trust-boundary/security.md @@ -326,6 +326,16 @@ known operations; there is no arbitrary command pass-through: | `SendAgentSnapshotToFile` | `btrfs send [-p ] ` into a bare filename under `MIGRATE_STAGING_ROOT`; refuses to overwrite an existing export | | `SendAgentSnapshotToFd` | `btrfs send [-p ] ` into a **file descriptor passed with the request** (`SCM_RIGHTS`). hive-c0re connects to the peer and hands over the connected socket, so hive-priv never learns an address or protocol. hive-priv requires exactly one descriptor here, and refuses one that arrives alongside any other operation | +**Strings that reach root-loaded config** — `WriteResourceLimits` and +`WriteNspawnFlags` copy request strings into a systemd drop-in and an env +file that systemd and the nixos-containers start script read, so hive-priv +refuses, never escapes, any value outside its shape. `memory_max` is +`infinity`, a decimal percentage or a decimal size with an optional +`B`/`K`/`M`/`G`/`T`/`P`/`E`; `cpu_quota` is a decimal percentage; `bridge` is +1-15 ASCII letters, digits, `-`, `_` or `.`; `gateway_ip` is an IPv4 literal. A +bind or credential path is absolute with no whitespace, control byte, `\`, +`$`, backtick, `"` or `:`. The size limits of a value stay with systemd. + **Closures from the swarm cache** — `RealiseClosure` is how a hive's rebuild takes the closure the swarm built instead of building it. Root-owned sources alone decide what root accepts: diff --git a/hive-priv/src/container_config.rs b/hive-priv/src/container_config.rs index 95b8f9fd..f4643500 100644 --- a/hive-priv/src/container_config.rs +++ b/hive-priv/src/container_config.rs @@ -31,15 +31,16 @@ pub(crate) fn handle_write_nspawn_flags( nix_builds: Option, ) -> Result<(String, String)> { let agent = validate_container_system_name(container)?; + validate_bridge(&isolation.bridge)?; + validate_gateway_ip(&isolation.gateway_ip)?; for bind in binds { validate_bind_path(&bind.host_path)?; validate_bind_path(&bind.container_path)?; } for cred in load_credentials { validate_credential_name(&cred.name)?; - // Same path rules as binds (absolute, no colon/newline/quote/null): - // the colon ban is essential since `--load-credential=name:path` - // uses `:` as the name/path separator. + // Same path rules as binds: the colon ban is essential since + // `--load-credential=name:path` uses `:` as the name/path separator. validate_bind_path(&cred.host_path)?; } let hive_facts = render_hive_facts(hive_facts)?; @@ -157,6 +158,8 @@ pub(crate) fn write_resource_limits( io_weight: Option, ) -> Result<(String, String)> { let logical = validate_container_system_name(container)?; + validate_memory_max(memory_max)?; + validate_cpu_quota(cpu_quota)?; let runtime_dir = format!("{AGENT_RUNTIME_ROOT}/{logical}"); let dir = format!("/run/systemd/system/container@{container}.service.d"); std::fs::create_dir_all(&dir).with_context(|| format!("create {dir}"))?; @@ -317,28 +320,88 @@ fn publish_file(path: &Path, content: &[u8], mode: u32, owner: Option<(u32, u32) staged.publish() } -/// Validate a bind-mount path: must be absolute, non-empty, and contain -/// no newlines, null bytes, double-quotes, or colons. +/// Validate a bind-mount path: absolute, non-empty, and free of whitespace, +/// control bytes, `\`, `$`, backtick, `"` and `:`. /// -/// The first three would break the `EXTRA_NSPAWN_FLAGS="..."` conf line -/// format. The colon is the one that matters most and had been left out -/// of this list: `--bind=SRC:DST` is colon-separated, so a path carrying -/// one does not corrupt the line — it silently becomes a *different -/// mount* than the caller asked for. +/// The path lands inside `EXTRA_NSPAWN_FLAGS="..."` in the container conf, +/// and the nixos-containers start script expands that variable unquoted: a +/// space or tab starts another nspawn argument, `$` and backtick are read by +/// the shell, `\` and `"` break the quoting, and a newline or NUL ends the +/// line. The colon separates the two sides of `--bind=SRC:DST`, so a path +/// carrying one silently becomes a *different mount* than the caller asked +/// for. fn validate_bind_path(path: &str) -> Result<()> { if path.is_empty() || !path.starts_with('/') || path .bytes() - .any(|b| b == 0 || b == b'\n' || b == b'"' || b == b':') + .any(|b| b <= b' ' || b == 0x7f || matches!(b, b'"' | b':' | b'\\' | b'$' | b'`')) { bail!( - "invalid bind path {path:?}: must be an absolute path with no colons, newlines, null bytes, or double-quotes" + "invalid bind path {path:?}: must be an absolute path with no whitespace, control bytes, colons, backslashes, dollar signs, backticks or double-quotes" ); } Ok(()) } +/// Validate `NetworkIsolation::bridge`, written as `HOST_BRIDGE=` in the +/// container conf: an interface name of ASCII alphanumerics and `-_.`, at +/// most 15 bytes (`IFNAMSIZ` less the NUL). +fn validate_bridge(bridge: &str) -> Result<()> { + if bridge.is_empty() + || bridge.len() > 15 + || !bridge + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.')) + { + bail!("invalid bridge {bridge:?}: must be 1-15 ascii letters, digits, '-', '_' or '.'"); + } + Ok(()) +} + +/// Validate `NetworkIsolation::gateway_ip`, written as `HOST_ADDRESS=` in the +/// container conf and as the bridge-DNS marker: an IPv4 address literal. +fn validate_gateway_ip(gateway_ip: &str) -> Result<()> { + if gateway_ip.parse::().is_err() { + bail!("invalid gateway_ip {gateway_ip:?}: must be an IPv4 address literal"); + } + Ok(()) +} + +/// Digits, optionally followed by `.` and more digits. +fn is_decimal(value: &str) -> bool { + let digits = |s: &str| !s.is_empty() && s.bytes().all(|b| b.is_ascii_digit()); + match value.split_once('.') { + Some((int, frac)) => digits(int) && digits(frac), + None => digits(value), + } +} + +/// Validate `memory_max`, written as `MemoryMax=` in the limits drop-in: +/// `infinity`, a decimal percentage, or a decimal byte size with an optional +/// `B`/`K`/`M`/`G`/`T`/`P`/`E` suffix. The shape only: the range is +/// systemd's to judge. +fn validate_memory_max(value: &str) -> Result<()> { + let size = value + .strip_suffix(['B', 'K', 'M', 'G', 'T', 'P', 'E']) + .unwrap_or(value); + if value == "infinity" || value.strip_suffix('%').is_some_and(is_decimal) || is_decimal(size) { + return Ok(()); + } + bail!( + "invalid memory_max {value:?}: must be \"infinity\", a percentage such as \"50%\", or a size such as \"8G\"" + ) +} + +/// Validate `cpu_quota`, written as `CPUQuota=` in the limits drop-in: a +/// decimal percentage. The shape only, as [`validate_memory_max`]. +fn validate_cpu_quota(value: &str) -> Result<()> { + if value.strip_suffix('%').is_some_and(is_decimal) { + return Ok(()); + } + bail!("invalid cpu_quota {value:?}: must be a percentage such as \"200%\"") +} + /// `--tmpfs=/knowledge/.git`, hiding the shared docs repo's metadata from /// inside the container. /// @@ -716,11 +779,13 @@ pub(crate) fn write_container_etc_file_in(rootfs: &Path, name: &str, contents: & #[cfg(test)] mod tests { use super::{ - BindMount, HiveFacts, NIX_DAEMON_SHADOW_FLAG, NetworkIsolation, NixBuilds, - chosen_nix_builds, git_overlay_flags, handle_write_nspawn_flags, hive_priv_exe, + BindMount, CredentialMount, HiveFacts, NIX_DAEMON_SHADOW_FLAG, NetworkIsolation, NixBuilds, + Path, chosen_nix_builds, git_overlay_flags, handle_write_nspawn_flags, hive_priv_exe, is_owned_var, limits_dropin_body, nspawn_flags, previous_nix_builds, private_users_line, publish_file, remove_service_dropin, render_hive_facts, rootfs_check_command, - validate_container_system_name, write_bridge_dns_marker_in, write_resource_limits, + validate_bind_path, validate_bridge, validate_container_system_name, validate_cpu_quota, + validate_gateway_ip, validate_memory_max, write_bridge_dns_marker_in, + write_resource_limits, }; use crate::shared::test_support::{entries, scratch}; @@ -1301,4 +1366,212 @@ mod tests { } assert_eq!(validate_container_system_name("h-atlas").unwrap(), "atlas"); } + + /// A container name that exists nowhere on the host, so a refusal that + /// comes before every write leaves these paths absent. + const PROBE: &str = "h-privvalidate"; + + fn probe_paths() -> [String; 2] { + [ + format!("/etc/nixos-containers/{PROBE}.conf"), + format!("/run/systemd/system/container@{PROBE}.service.d"), + ] + } + + fn assert_nothing_written() { + for path in probe_paths() { + assert!(!Path::new(&path).exists(), "{path} was created"); + } + } + + fn isolation(bridge: &str, gateway_ip: &str) -> NetworkIsolation { + NetworkIsolation { + bridge: bridge.to_owned(), + gateway_ip: gateway_ip.to_owned(), + } + } + + /// Limits that break out of their drop-in line, or that systemd would + /// ignore, are refused by name before any file is made. Control: every + /// shape the c0re validators and the nix defaults produce passes. + #[test] + fn resource_limits_refuse_what_is_not_a_size_a_share_or_infinity() { + for bad in [ + "1G\n[Service]\nExecStartPre=/bin/true", + "1G\r\nExecStartPre=/bin/true", + "1G ExecStartPre=/bin/true", + "1G\0", + "", + "G", + "%", + ".5G", + "1.G", + "1.5.5G", + "-1G", + "+1G", + "1e3", + "8GB", + "8g", + "0x10", + "Infinity", + "50 %", + "5%%", + "5G%", + "$(id)", + "%t", + ] { + let err = write_resource_limits(PROBE, bad, "200%", None, None).unwrap_err(); + assert!( + err.to_string().contains("invalid memory_max"), + "{bad:?}: {err:#}" + ); + } + for bad in [ + "400%\nExecStartPre=/bin/true", + "400% ", + "400", + "", + "%", + "1e3%", + "+5%", + ".5%", + "1.%", + "infinity", + "4G", + "%t", + ] { + let err = write_resource_limits(PROBE, "4G", bad, None, None).unwrap_err(); + assert!( + err.to_string().contains("invalid cpu_quota"), + "{bad:?}: {err:#}" + ); + } + assert_nothing_written(); + + for ok in [ + "8G", "4G", "512M", "4096K", "1T", "1048576", "1048576B", "1.5G", "0.5K", "3P", "2E", + "50%", "12.34%", "100%", "infinity", + ] { + validate_memory_max(ok).unwrap_or_else(|e| panic!("{ok:?}: {e:#}")); + } + for ok in ["200%", "400%", "100%", "0.5%", "21474836.47%", "1500.25%"] { + validate_cpu_quota(ok).unwrap_or_else(|e| panic!("{ok:?}: {e:#}")); + } + } + + /// Paths that would add an nspawn argument, or be read by the shell that + /// expands `EXTRA_NSPAWN_FLAGS`, are refused whichever side or list + /// carries them, before the conf is touched. Control: plain paths and + /// the store-path alphabet pass. + #[test] + fn bind_paths_refuse_whitespace_control_bytes_and_shell_syntax() { + let iso = isolation("hive-br0", "10.42.0.1"); + let facts = HiveFacts::default(); + for bad in [ + "/srv/x --bind=/etc:/e", + "/srv/x\t--bind=/etc", + "/srv/x\n--bind=/etc", + "/srv/x\r", + "/srv/x\x0b", + "/srv/x\x7f", + "/srv/x\0", + "/srv/x\\ y", + "/srv/$HOME", + "/srv/${HOME}", + "/srv/`id`", + "/srv/\"x", + "/srv/a:b", + "srv/x", + "", + " /srv/x", + ] { + let on_host = [bind_from(bad, "/data")]; + let on_container = [bind_from("/srv/x", bad)]; + let credential = [CredentialMount { + name: "otel-headers".to_owned(), + host_path: bad.to_owned(), + }]; + for (binds, credentials) in [ + (&on_host[..], &[][..]), + (&on_container[..], &[][..]), + (&[][..], &credential[..]), + ] { + for nix_builds in [None, Some(NixBuilds::Host), Some(NixBuilds::Disabled)] { + let err = handle_write_nspawn_flags( + PROBE, + binds, + &iso, + credentials, + &facts, + None, + nix_builds, + ) + .unwrap_err(); + assert!( + err.to_string().contains("invalid bind path"), + "{bad:?}: {err:#}" + ); + } + } + } + assert_nothing_written(); + + for ok in [ + "/srv/x", + "/var/lib/hyperhive/agents/atlas/state", + "/nix/store/abc123-name_1.2+x=y?", + "/run/hive/secrets/otel-headers", + "/run/hive-agent/iris", + "/agents/iris/sockets", + ] { + validate_bind_path(ok).unwrap_or_else(|e| panic!("{ok:?}: {e:#}")); + } + } + + /// A bridge or gateway that adds a conf assignment, or is not an + /// interface name or an IPv4 literal, is refused before the conf is + /// touched. Control: the default bridge, the example, and the gateways + /// the c0re tests use pass. + #[test] + fn network_isolation_refuses_what_is_not_an_interface_name_and_an_ipv4_literal() { + let facts = HiveFacts::default(); + for (bridge, gateway_ip, want) in [ + ("br0\nSYSTEM_PATH=/x", "10.42.0.1", "invalid bridge"), + ("br0 x", "10.42.0.1", "invalid bridge"), + ("br0\r", "10.42.0.1", "invalid bridge"), + ("br$0", "10.42.0.1", "invalid bridge"), + ("br\"0", "10.42.0.1", "invalid bridge"), + ("br0/../x", "10.42.0.1", "invalid bridge"), + ("", "10.42.0.1", "invalid bridge"), + ("sixteen-chars-xx", "10.42.0.1", "invalid bridge"), + ( + "hive-br0", + "10.42.0.1\nSYSTEM_PATH=/x", + "invalid gateway_ip", + ), + ("hive-br0", "10.42.0.1 x", "invalid gateway_ip"), + ("hive-br0", "10.42.0.1/24", "invalid gateway_ip"), + ("hive-br0", "10.42.0", "invalid gateway_ip"), + ("hive-br0", "10.42.0.999", "invalid gateway_ip"), + ("hive-br0", "$(id)", "invalid gateway_ip"), + ("hive-br0", "fd00::1", "invalid gateway_ip"), + ("hive-br0", "", "invalid gateway_ip"), + ] { + let iso = isolation(bridge, gateway_ip); + let err = + handle_write_nspawn_flags(PROBE, &[], &iso, &[], &facts, None, None).unwrap_err(); + assert!( + err.to_string().contains(want), + "{bridge:?} {gateway_ip:?}: {err:#}" + ); + } + assert_nothing_written(); + + for bridge in ["hive-br0", "h0", "hive0", "br_0.1", "fifteen-chars-x"] { + validate_bridge(bridge).unwrap_or_else(|e| panic!("{bridge:?}: {e:#}")); + } + for gateway_ip in ["10.42.0.1", "10.42.0.254", "172.30.0.1", "192.168.1.1"] { + validate_gateway_ip(gateway_ip).unwrap_or_else(|e| panic!("{gateway_ip:?}: {e:#}")); + } + } }