diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5f320add..70cf73c6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,6 +113,11 @@ jobs: - name: Check local gate discovery and required-check warnings run: bash tests/release/ci-local.test.sh + - name: Check bounded actions and removal of shell grants + # Root is used only in an isolated fixture subprocess to prove that the + # helper irreversibly drops credentials before touching user files. + run: sudo -n bash tests/release/action-steps.test.sh + - name: Check the audit export states its confidentiality class run: bash tests/release/audit-export-confidentiality.test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ec44bfe..5e6fc2b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ Releases before `0.2.5` predate the public launch; their notes live in the ## [Unreleased] +### Changed + +- Remove whole-binary shell and runuser sudo grants. Firewall, group, Snap, + SSH-key and user-scoped Flatpak/Podman/Toolbox operations now use a bounded + helper with fixed command grammars. User operations refuse UID 0 and drop + groups/GID/UID before file access or execution; SSH edits reject symlinks. + Install the new helper together with the daemon and sudoers fragment (#417). + ### Fixed - Align planner action descriptions with the Ubuntu execution fence, including diff --git a/Makefile b/Makefile index 3824d78f..a1870ecd 100644 --- a/Makefile +++ b/Makefile @@ -118,6 +118,7 @@ daemon-install: daemon-install-preflight build # sysknife-daemon --test helper_install_coverage` derives the required set # from the daemon source and fails if one is missing. install -Dm 755 packaging/sysknife-apt-pin-edit $(HELPERS)/apt-pin-edit + install -Dm 755 packaging/sysknife-action-steps $(HELPERS)/action-steps install -Dm 755 packaging/sysknife-audit-edit $(HELPERS)/audit-edit install -Dm 755 packaging/sysknife-fail2ban-jail-edit $(HELPERS)/fail2ban-jail-edit install -Dm 755 packaging/sysknife-grub-kargs-edit $(HELPERS)/grub-kargs-edit @@ -148,6 +149,7 @@ daemon-uninstall: rm -f $(SYSUSERS)/sysknife.conf rm -f $(TMPFILES)/sysknife.conf rm -f $(HELPERS)/apt-pin-edit + rm -f $(HELPERS)/action-steps rm -f $(HELPERS)/audit-edit rm -f $(HELPERS)/fail2ban-jail-edit rm -f $(HELPERS)/grub-kargs-edit diff --git a/crates/sysknife-daemon/src/actions/containers.rs b/crates/sysknife-daemon/src/actions/containers.rs index 0f2993d7..877cf4e3 100644 --- a/crates/sysknife-daemon/src/actions/containers.rs +++ b/crates/sysknife-daemon/src/actions/containers.rs @@ -1,4 +1,4 @@ -use super::{command_mechanism, ActionMechanism, ActionSpec}; +use super::{ActionMechanism, ActionSpec}; use sysknife_types::RiskLevel; pub fn specs() -> Vec { @@ -16,52 +16,32 @@ pub fn specs() -> Vec { ] } -/// Run a Podman command as the target user via `sudo runuser -l`. +/// Run fixed Podman argv after dropping to the target user. /// /// Rootless Podman operates against the user's container storage, not a shared /// system store. The daemon runs as the `sysknife` system user whose container /// namespace is empty; we must switch to `username` to reach their containers. -/// `runuser -l` establishes a full login context including XDG_RUNTIME_DIR and -/// the sub-UID/GID ranges required by the kernel namespace code — those env -/// vars are populated by `pam_systemd` which only fires for a login shell. -/// -/// **Shell-injection safety:** the `-c ""` form passes a string to -/// `/bin/sh`, so any metacharacter in an interpolated value would be expanded -/// by the shell. Defence-in-depth: -/// 1. `username` flows through `validated_username` (`[A-Za-z0-9._-]`, no -/// leading dash, ≤32 bytes). -/// 2. Container `name` and `image` flow through `validated_safe_arg`, which -/// enforces `[A-Za-z0-9._:/+@-]`, no leading dash, ≤254 bytes — every -/// shell metacharacter (`;`, `&`, `|`, `$`, backtick, quotes, whitespace, -/// glob, brace, `\`, etc.) is rejected at the boundary. -/// 3. The format!-interpolated values are wrapped in single quotes so even -/// a future validator regression cannot escape the surrounding quotes. -fn podman_as(username: &str, podman_cmd: &str) -> ActionMechanism { +/// The helper sets HOME and XDG_RUNTIME_DIR for the resolved UID, resets +/// supplementary groups, and drops GID/UID before invoking the fixed tool. +/// Rootless Podman still requires the account's configured sub-UID/GID ranges +/// and runtime directory. No login shell or user startup file runs as root. +fn podman_as(username: &str, parameters: &[&str]) -> ActionMechanism { + let mut args = vec![ + "/usr/lib/sysknife/action-steps".to_string(), + "podman".to_string(), + username.to_string(), + ]; + args.extend(parameters.iter().map(|value| value.to_string())); ActionMechanism::Command { program: "sudo", - args: vec![ - "runuser".to_string(), - "-l".to_string(), - username.to_string(), - "-c".to_string(), - podman_cmd.to_string(), - ], + args, } } pub fn list_containers(username: &str) -> ActionSpec { ActionSpec { action_name: "ListContainers", - mechanism: command_mechanism( - "sudo", - [ - "runuser", - "-l", - username, - "-c", - "podman ps --all --format json", - ], - ), + mechanism: podman_as(username, &["ps", "--all", "--format", "json"]), risk_level: RiskLevel::Low, reboot_required: false, rollback_available: false, @@ -69,10 +49,9 @@ pub fn list_containers(username: &str) -> ActionSpec { } pub fn create_container(username: &str, name: &str, image: &str) -> ActionSpec { - let cmd = format!("podman create --name '{}' '{}'", name, image); ActionSpec { action_name: "CreateContainer", - mechanism: podman_as(username, &cmd), + mechanism: podman_as(username, &["create", "--name", name, image]), risk_level: RiskLevel::Medium, reboot_required: false, rollback_available: false, @@ -80,10 +59,9 @@ pub fn create_container(username: &str, name: &str, image: &str) -> ActionSpec { } pub fn start_container(username: &str, name: &str) -> ActionSpec { - let cmd = format!("podman start '{}'", name); ActionSpec { action_name: "StartContainer", - mechanism: podman_as(username, &cmd), + mechanism: podman_as(username, &["start", name]), risk_level: RiskLevel::Medium, reboot_required: false, rollback_available: false, @@ -91,10 +69,9 @@ pub fn start_container(username: &str, name: &str) -> ActionSpec { } pub fn stop_container(username: &str, name: &str) -> ActionSpec { - let cmd = format!("podman stop '{}'", name); ActionSpec { action_name: "StopContainer", - mechanism: podman_as(username, &cmd), + mechanism: podman_as(username, &["stop", name]), risk_level: RiskLevel::Medium, reboot_required: false, rollback_available: false, @@ -102,10 +79,9 @@ pub fn stop_container(username: &str, name: &str) -> ActionSpec { } pub fn remove_container(username: &str, name: &str) -> ActionSpec { - let cmd = format!("podman rm '{}'", name); ActionSpec { action_name: "RemoveContainer", - mechanism: podman_as(username, &cmd), + mechanism: podman_as(username, &["rm", name]), risk_level: RiskLevel::Medium, reboot_required: false, rollback_available: false, @@ -113,10 +89,9 @@ pub fn remove_container(username: &str, name: &str) -> ActionSpec { } pub fn get_container_info(username: &str, name: &str) -> ActionSpec { - let cmd = format!("podman inspect '{}'", name); ActionSpec { action_name: "GetContainerInfo", - mechanism: podman_as(username, &cmd), + mechanism: podman_as(username, &["inspect", name]), risk_level: RiskLevel::Low, reboot_required: false, rollback_available: false, diff --git a/crates/sysknife-daemon/src/actions/flatpak.rs b/crates/sysknife-daemon/src/actions/flatpak.rs index 81146a29..fb294d78 100644 --- a/crates/sysknife-daemon/src/actions/flatpak.rs +++ b/crates/sysknife-daemon/src/actions/flatpak.rs @@ -19,29 +19,22 @@ pub fn specs() -> Vec { ] } -/// Run a Flatpak command as the target user via `sudo runuser -u user -- flatpak `. +/// Run a fixed Flatpak operation after the helper drops to the target user. /// /// Flatpak user installations live under `~/.local/share/flatpak/` and are /// accessed through the user's D-Bus session. The daemon runs as `sysknife` -/// (a system user) with no user installation; `runuser -u` switches to the +/// (a system user) with no user installation; the helper switches to the /// correct user UID without spawning a login shell, so each argv element is /// passed to `flatpak` verbatim. /// -/// **Shell-injection safety:** unlike `runuser -l user -c ""`, -/// the `-u user -- argv` form bypasses the shell entirely. There is no string -/// interpolation, no metacharacter expansion, and no quoting concern — every -/// argument reaches `flatpak(1)` exactly as supplied. Callers must still pass -/// arguments through `validated_safe_arg`/`validated_username` upstream so a -/// hostile value cannot impersonate a flag (`-X`) or break out of the -/// command's own option parser, but they no longer have to defend against -/// shell metacharacters. +/// The helper independently allowlists the complete Flatpak argv grammar and +/// rejects option-shaped values before dropping credentials and executing the +/// fixed binary. Callers also validate values upstream; no shell parses them. fn flatpak_as(username: &str, args: &[&str]) -> ActionMechanism { let mut argv: Vec = vec![ - "runuser".to_string(), - "-u".to_string(), - username.to_string(), - "--".to_string(), + "/usr/lib/sysknife/action-steps".to_string(), "flatpak".to_string(), + username.to_string(), ]; argv.extend(args.iter().map(|s| s.to_string())); ActionMechanism::Command { @@ -168,7 +161,7 @@ pub fn get_flatpak_app_info(username: &str, app_id: &str) -> ActionSpec { // every Ubuntu wrapper delegates directly to the shared `flatpak_as` helper. // --------------------------------------------------------------------------- -/// Install a Flatpak app on Ubuntu (`sudo runuser -u -- flatpak install --user -y `). +/// Install a Flatpak app on Ubuntu through the bounded user-operation helper. /// /// Identical argv to `InstallFlatpak` on Fedora. Distinct action name for /// Ubuntu-specific routing in the daemon and LLM prompt. @@ -184,7 +177,7 @@ pub fn ubuntu_install_flatpak(username: &str, app_id: &str, remote: &str) -> Act } } -/// Remove a Flatpak app on Ubuntu (`sudo runuser -u -- flatpak uninstall --user -y `). +/// Remove a Flatpak app on Ubuntu through the bounded user-operation helper. /// /// Risk: Medium. Uninstalls a sandboxed Flatpak application. pub fn ubuntu_remove_flatpak(username: &str, app_id: &str) -> ActionSpec { diff --git a/crates/sysknife-daemon/src/actions/mod.rs b/crates/sysknife-daemon/src/actions/mod.rs index 1fbb282b..8872ed8c 100644 --- a/crates/sysknife-daemon/src/actions/mod.rs +++ b/crates/sysknife-daemon/src/actions/mod.rs @@ -152,7 +152,7 @@ pub fn exclusive_resource(spec: &ActionSpec) -> Option { "apt-get" | "apt" | "apt-mark" | "aptitude" | "dpkg" | "dpkg-reconfigure" | "add-apt-repository" | "do-release-upgrade" | "unattended-upgrade" | "apt-pin-edit" => Some(ExclusiveResource::Dpkg), - "snap" => Some(ExclusiveResource::Snap), + "snap" | "snap-install-hold" => Some(ExclusiveResource::Snap), "rpm-ostree" | "ostree" => Some(ExclusiveResource::RpmOstree), "flatpak" => Some(ExclusiveResource::Flatpak), _ => None, diff --git a/crates/sysknife-daemon/src/actions/network.rs b/crates/sysknife-daemon/src/actions/network.rs index b4b63d67..ae36b169 100644 --- a/crates/sysknife-daemon/src/actions/network.rs +++ b/crates/sysknife-daemon/src/actions/network.rs @@ -55,37 +55,27 @@ pub fn set_dns_servers(interface: &str, servers: &[&str]) -> ActionSpec { /// Configure a firewalld rule and reload so it takes effect. /// -/// Uses `sh -c` to chain `firewall-cmd --permanent ... && firewall-cmd --reload` -/// atomically; firewalld has no single-call equivalent that updates the -/// permanent rule and reloads runtime in one shot. -/// -/// **Shell-injection safety:** `zone` and `service` are interpolated into the -/// script via `format!`, so any shell metacharacter in either would be -/// expanded by `/bin/sh`. Defence-in-depth: -/// 1. Both flow through `validated_safe_arg` upstream, which enforces a -/// strict ASCII allowlist (`[A-Za-z0-9._:/+@-]`, no leading dash, ≤254 -/// bytes) and rejects every shell metacharacter at the boundary. -/// 2. The interpolated values are wrapped in single quotes so a future -/// validator regression cannot escape the surrounding quotes. -/// 3. `verb` is selected from a fixed pair of literals (`add-service` / -/// `remove-service`); it is never attacker-influenced. +/// The bounded helper runs the permanent mutation then reloads only on success. +/// These are sequential commands, not an atomic firewalld transaction. Arguments +/// are validated independently by the helper and never interpreted as a shell. pub fn configure_firewall(zone: &str, service: &str, enabled: bool) -> ActionSpec { let verb = if enabled { "add-service" } else { "remove-service" }; - let script = format!( - "firewall-cmd --permanent --zone='{}' --{}='{}' && firewall-cmd --reload", - zone, verb, service - ); - ActionSpec { action_name: "ConfigureFirewall", - mechanism: super::ActionMechanism::Command { - program: "sudo", - args: vec!["sh".to_string(), "-c".to_string(), script], - }, + mechanism: command_mechanism( + "sudo", + [ + "/usr/lib/sysknife/action-steps", + "firewall", + zone, + service, + verb, + ], + ), risk_level: RiskLevel::High, reboot_required: false, rollback_available: false, diff --git a/crates/sysknife-daemon/src/actions/snap.rs b/crates/sysknife-daemon/src/actions/snap.rs index d82af700..900388fd 100644 --- a/crates/sysknife-daemon/src/actions/snap.rs +++ b/crates/sysknife-daemon/src/actions/snap.rs @@ -10,9 +10,8 @@ //! `snap refresh --hold ` to pin the snap at the installed version. //! Set `auto_update: true` in the plan params to skip the hold. //! -//! The hold is applied by building a two-command spec using a shell fragment -//! via `sh -c "snap install … && snap refresh --hold …"`. `name` and -//! `channel` are validated by [`snap_install`] itself before interpolation +//! The bounded helper installs and then holds, stopping on either failure. +//! `name` and `channel` are validated by [`snap_install`] and the helper //! (in addition to, not instead of, the executor's own `validated_safe_arg` //! check) — see the `SnapInstallError` doc below. @@ -31,10 +30,8 @@ pub enum SnapInstallError { /// /// Defense in depth: the executor already validates both via /// `validated_safe_arg` before calling this constructor, but the - /// `auto_update: false` path interpolates `name`/`channel` into a - /// `sh -c "snap install … && snap refresh --hold …"` fragment — a future - /// internal Rust caller (fleet plan/execute path) that skipped the - /// executor could not otherwise be blocked from injecting through it. + /// helper also revalidates them. Keep constructor validation so internal + /// callers cannot produce an invalid spec by skipping the executor. InvalidArg { param: &'static str, value: String }, } @@ -132,16 +129,19 @@ pub fn snap_install( rollback_available: false, }) } else { - // Install + hold in one shell fragment to avoid a window where the snap - // can be auto-refreshed between install and hold. + // Sequential install and hold: the helper does not hold a failed install. let channel_arg = channel.unwrap_or("stable"); - let cmd = format!( - "snap install --channel={} {} && snap refresh --hold {}", - channel_arg, name, name - ); Ok(ActionSpec { action_name: "SnapInstall", - mechanism: super::command_mechanism("sudo", ["sh", "-c", &cmd]), + mechanism: super::command_mechanism( + "sudo", + [ + "/usr/lib/sysknife/action-steps", + "snap-install-hold", + name, + channel_arg, + ], + ), risk_level: RiskLevel::Medium, reboot_required: false, rollback_available: false, @@ -299,15 +299,18 @@ mod tests { let spec = snap_install("firefox", None, false).unwrap(); let (prog, args) = extract_args(&spec); assert_eq!(prog, "sudo"); - // When auto_update=false the hold is embedded in a sh -c fragment. - let full = args.join(" "); - assert!( - full.contains("snap install"), - "missing 'snap install': {full}" + assert_eq!( + args, + [ + "/usr/lib/sysknife/action-steps", + "snap-install-hold", + "firefox", + "stable" + ] ); - assert!( - full.contains("snap refresh --hold firefox"), - "missing hold: {full}" + assert_eq!( + crate::actions::exclusive_resource(&spec), + Some(crate::actions::ExclusiveResource::Snap) ); } diff --git a/crates/sysknife-daemon/src/actions/ssh.rs b/crates/sysknife-daemon/src/actions/ssh.rs index 9c41325f..e67330a8 100644 --- a/crates/sysknife-daemon/src/actions/ssh.rs +++ b/crates/sysknife-daemon/src/actions/ssh.rs @@ -1,4 +1,4 @@ -use super::{command_mechanism, ActionMechanism, ActionSpec}; +use super::{command_mechanism, ActionSpec}; use sysknife_types::RiskLevel; pub fn specs() -> Vec { @@ -10,19 +10,10 @@ pub fn specs() -> Vec { ] } -/// Installed path of the privileged sshd-option helper script. -/// See `packaging/sysknife-sshd-option-edit` and the matching NOPASSWD grant in -/// `packaging/sysknife-sudoers`. +/// Installed root-owned helper, with an independent option/value allowlist. const SSHD_OPTION_HELPER: &str = "/usr/lib/sysknife/sshd-option-edit"; -/// Set an allowlisted sshd option via a drop-in under -/// `/etc/ssh/sshd_config.d/`, validated with `sshd -t` and applied by reloading -/// the ssh service. -/// -/// Risk: High. A misconfigured sshd can lock out remote access; the helper -/// gates every change on `sshd -t` and rolls back on failure. `option` and -/// `value` are checked against a fixed allowlist by both the daemon and the -/// helper — this is deliberately NOT an arbitrary `sshd_config` editor. +/// Validate a drop-in with sshd -t before reloading; roll back on rejection. pub fn set_sshd_option(option: &str, value: &str) -> ActionSpec { ActionSpec { action_name: "SetSshdOption", @@ -46,54 +37,27 @@ pub fn get_authorized_keys(username: &str) -> ActionSpec { } } -/// Shell body for `AddAuthorizedKey`. -/// -/// The key and path arrive as positional arguments (`$1`, `$2`) rather than -/// being interpolated into the script text, so no value the caller supplies is -/// ever parsed as shell syntax. `printf '%s\n'` is used instead of `echo` -/// because `echo` mangles values beginning with `-` and interprets backslash -/// escapes on some shells. -const ADD_KEY_SCRIPT: &str = - "key=$1; path=$2; grep -Fxq -- \"$key\" \"$path\" 2>/dev/null || printf '%s\\n' \"$key\" >> \"$path\""; - -/// `sudo runuser -u -- sh -c