hive-priv: refuse injecting strings in the limits drop-in and the container conf
memory_max and cpu_quota are checked against the shapes systemd takes before write_resource_limits makes any file. validate_bind_path also refuses whitespace, control bytes, backslash, dollar and backtick, which the start script expands unquoted in EXTRA_NSPAWN_FLAGS. bridge must be an interface name and gateway_ip an IPv4 literal. Each refusal names its field. Closes #6086
This commit is contained in:
atlas
parent
04870fbdb0
commit
18e4f195e5
2 changed files with 298 additions and 15 deletions
|
|
@ -326,6 +326,16 @@ known operations; there is no arbitrary command pass-through:
|
|||
| `SendAgentSnapshotToFile` | `btrfs send [-p <parent>] <snapshot>` into a bare filename under `MIGRATE_STAGING_ROOT`; refuses to overwrite an existing export |
|
||||
| `SendAgentSnapshotToFd` | `btrfs send [-p <parent>] <snapshot>` 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:
|
||||
|
|
|
|||
|
|
@ -31,15 +31,16 @@ pub(crate) fn handle_write_nspawn_flags(
|
|||
nix_builds: Option<NixBuilds>,
|
||||
) -> 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<u32>,
|
||||
) -> 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::<std::net::Ipv4Addr>().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:#}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue