thaloco/banger
1
Fork 0
Code Pull requests Releases Packages Activity Actions

roothelper: tighten input validation across privileged RPCs

Browse source 
Defence-in-depth pass over every helper method that touches the host
as root. Each fix narrows what a compromised owner-uid daemon could
ask the helper to do; many close concrete file-ownership and DoS
primitives that the previous validators didn't reach.

Path / identifier validation:
  * priv.fsck_snapshot now requires /dev/mapper/fc-rootfs-* (was
    "is the string non-empty"). e2fsck -fy on /dev/sda1 was the
    motivating exploit.
  * priv.kill_process and priv.signal_process now read
    /proc/<pid>/cmdline and require a "firecracker" substring before
    sending the signal. Killing arbitrary host PIDs (sshd, init, …)
    is no longer a one-RPC primitive.
  * priv.read_ext4_file and priv.write_ext4_files now require the
    image path to live under StateDir or be /dev/mapper/fc-rootfs-*.
  * priv.cleanup_dm_snapshot validates every non-empty Handles field:
    DM name fc-rootfs-*, DM device /dev/mapper/fc-rootfs-*, loops
    /dev/loopN.
  * priv.remove_dm_snapshot accepts only fc-rootfs-* names or
    /dev/mapper/fc-rootfs-* paths.
  * priv.ensure_nat now requires a parsable IPv4 address and a
    banger-prefixed tap.
  * priv.sync_resolver_routing and priv.clear_resolver_routing now
    require a Linux iface-name-shaped bridge name (1–15 chars, no
    whitespace/'/'/':') and, for sync, a parsable resolver address.

Symlink defence:
  * priv.ensure_socket_access now validates the socket path is under
    RuntimeDir and not a symlink. The fcproc layer's chown/chmod
    moves to unix.Open(O_PATH|O_NOFOLLOW) + Fchownat(AT_EMPTY_PATH)
    + Fchmodat via /proc/self/fd, so even a swap of the leaf into a
    symlink between validation and the syscall is refused. The
    local-priv (non-root) fallback uses `chown -h`.
  * priv.cleanup_jailer_chroot rejects symlinks at both the leaf
    (os.Lstat) and intermediate path components (filepath.EvalSymlinks
    + clean-equality). The umount sweep was rewritten from shell
    `umount --recursive --lazy` to direct unix.Unmount(MNT_DETACH |
    UMOUNT_NOFOLLOW) per child mount, deepest-first; the findmnt
    guard remains as the rm-rf safety net. Local-priv mode falls
    back to `sudo umount --lazy`.

Binary validation:
  * validateRootExecutable now opens with O_PATH|O_NOFOLLOW and
    Fstats through the resulting fd. Rejects path-level symlinks and
    narrows the TOCTOU window between validation and the SDK's exec
    to fork+exec time on a healthy host.

Daemon socket:
  * The owner daemon now reads SO_PEERCRED on every accepted
    connection and refuses any UID that isn't 0 or the registered
    owner. Filesystem perms (0600 + ownerUID) already enforced this;
    the check is belt-and-braces in case the socket FD is ever
    leaked to a non-owner process.

Docs:
  * docs/privileges.md walked end-to-end. Each helper RPC's
    Validation gate row reflects what the code actually enforces.
    New section "Running outside the system install" calls out the
    looser dev-mode trust model (NOPASSWD sudoers, helper hardening
    bypassed) so users don't deploy that path on shared hosts.
    Trust list updated to include every new validator.

Tests added: validators (DM-loop, DM-remove-target, DM-handles,
ext4-image-path, iface-name, IPv4, resolver-addr, not-symlink,
firecracker-PID, root-executable variants), the daemon's authorize
path (non-unix conn rejection + unix conn happy path), the umount2
ordering contract (deepest-first + --lazy on the sudo branch), and
positive/negative cases for the chown-no-follow fallback.

Verified end-to-end via `make smoke JOBS=4` on a KVM host.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Thales Maciel 2026-04-28 14:39:41 -03:00
parent 6b543cb17f
commit 853249dec2
No known key found for this signature in database
GPG key ID: 33112E6833C34679
8 changed files with 1177 additions and 63 deletions
Download patch file Download diff file Expand all files Collapse all files

14
internal/daemon/vm_test.go
View file

@ -428,7 +428,7 @@ func TestHealthVMReturnsHealthyForRunningGuest(t *testing.T) {
t: t,
steps: []runnerStep{
sudoStep("", nil, "chmod", "600", vsockSock),
sudoStep("", nil, "chown", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), vsockSock),
sudoStep("", nil, "chown", "-h", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), vsockSock),
},
}
d := &Daemon{store: db, runner: runner}
@ -492,7 +492,7 @@ func TestPingVMAliasReturnsAliveForHealthyVM(t *testing.T) {
t: t,
steps: []runnerStep{
sudoStep("", nil, "chmod", "600", vsockSock),
sudoStep("", nil, "chown", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), vsockSock),
sudoStep("", nil, "chown", "-h", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), vsockSock),
},
}
d := &Daemon{store: db, runner: runner}
@ -692,7 +692,7 @@ func TestPortsVMReturnsEnrichedPortsAndWebSchemes(t *testing.T) {
t: t,
steps: []runnerStep{
sudoStep("", nil, "chmod", "600", vsockSock),
sudoStep("", nil, "chown", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), vsockSock),
sudoStep("", nil, "chown", "-h", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), vsockSock),
},
}
d := &Daemon{store: db, runner: runner}
@ -1623,7 +1623,7 @@ func TestStopVMFallsBackToForcedCleanupAfterGracefulTimeout(t *testing.T) {
t: t,
steps: []runnerStep{
sudoStep("", nil, "chmod", "600", apiSock),
sudoStep("", nil, "chown", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), apiSock),
sudoStep("", nil, "chown", "-h", fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()), apiSock),
{call: runnerCall{name: "pgrep", args: []string{"-n", "-f", apiSock}}, out: []byte(strconv.Itoa(fake.Process.Pid) + "\n")},
sudoStep("", nil, "kill", "-KILL", strconv.Itoa(fake.Process.Pid)),
},
@ -2068,14 +2068,16 @@ func (r *filesystemRunner) RunSudo(ctx context.Context, args ...string) ([]byte,
}
return nil, os.WriteFile(dst, data, os.FileMode(mode))
case "chown":
// Recognised forms, both no-op under test (we run as the test
// Recognised forms, all no-op under test (we run as the test
// user and os.Chown would need CAP_CHOWN):
// chown OWNER TARGET
// chown -R OWNER TARGET
// chown -h OWNER TARGET (symlink-no-follow; required by
// fcproc.chownChmodNoFollow)
switch {
case len(args) == 3:
return nil, nil
case len(args) == 4 && args[1] == "-R":
case len(args) == 4 && (args[1] == "-R" || args[1] == "-h"):
return nil, nil
default:
return nil, fmt.Errorf("unexpected chown args: %v", args)