From 182bccf8af6fe21b3279c975457729876bb63027 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Tue, 28 Apr 2026 16:19:28 -0300 Subject: [PATCH] roothelper: pin bridge name + IP + CIDR to a banger-managed shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit priv.ensure_bridge / priv.create_tap accepted the daemon's network config triple (BridgeName, BridgeIP, CIDR) and forwarded it straight to `ip link` / `ip addr` / `ip link set master`. Argv-style exec ruled out shell injection, but the kernel happily honours those commands against any iface a compromised owner-uid daemon names — including eth0/docker0/lo. Concretely: * priv.ensure_bridge could `ip link set up` against any host interface and `ip addr add` arbitrary IP/CIDR to it. * priv.create_tap could `ip link set master `, bridging the per-VM tap into the host's primary LAN so the guest sees host-local broadcast traffic. * priv.sync_resolver_routing / priv.clear_resolver_routing only enforced "name shaped like a Linux iface" — no banger constraint. New validators (single chokepoint via validateNetworkConfig): * validateBangerBridgeName: name must equal "br-fc" or start with "br-fc-". Stops a compromised daemon from naming any host iface in these RPCs. Users with a custom bridge keep the prefix. * validateCIDRPrefix: numeric in [8, 32]. Wider prefixes would silently widen the bridge subnet beyond what the daemon intends. * validateNetworkConfig bundles bridge-name + validateIPv4 + validateCIDRPrefix so every helper RPC that takes the triple stays in lockstep. Wired into methodEnsureBridge, methodCreateTap, and the resolver- routing pair (replacing the older validateLinuxIfaceName-only check with the stricter banger-bridge check). docs/privileges.md updated: the helper-RPC table rows now spell out the banger-managed bridge constraint, and the trust list includes the new validators. Tests: TestValidateBangerBridgeName (default + suffixed accepted, host ifaces / wrong prefix / oversized rejected), TestValidate CIDRPrefix (boundary + non-numeric + IPv6-style 64 rejected), TestValidateNetworkConfig (happy path + each-field-bad cases). Smoke at JOBS=4 still green — banger's defaults sail through the new gate. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/privileges.md | 21 +++--- internal/roothelper/roothelper.go | 94 +++++++++++++++++++++++-- internal/roothelper/roothelper_test.go | 96 ++++++++++++++++++++++++++ 3 files changed, 196 insertions(+), 15 deletions(-) diff --git a/docs/privileges.md b/docs/privileges.md index a814c2d..a991da4 100644 --- a/docs/privileges.md +++ b/docs/privileges.md @@ -71,11 +71,11 @@ validator before the helper touches the host. | Method | Effect | Validation gate | |---|---|---| -| `priv.ensure_bridge` | Create the configured Linux bridge if missing; assign the bridge IP. | Bridge name and IP come from owner config; helper does not allow caller to pick `lo` etc. | -| `priv.create_tap` | `ip link add tap NAME tuntap` and add to bridge, owned by the owner user. | Tap name must match `tap-fc-*` or `tap-pool-*`. | -| `priv.delete_tap` | `ip link del NAME`. | Same prefix check. | -| `priv.sync_resolver_routing` | `resolvectl dns/domain/default-route` on the configured bridge. | Bridge name passes the kernel iface-name rules (1–15 chars, no `/`/`:`/whitespace, not `.`/`..`). Resolver address must parse via `net.ParseIP`. | -| `priv.clear_resolver_routing` | `resolvectl revert` on the bridge. | Same iface-name check. | +| `priv.ensure_bridge` | Create the configured Linux bridge if missing; assign the bridge IP. | Bridge name must equal `br-fc` or start with `br-fc-` (so a compromised daemon can't drive `ip link` against `eth0` / `docker0` / `lo`). Bridge IP must parse as IPv4. CIDR prefix must be a number in `[8, 32]`. | +| `priv.create_tap` | `ip link add tap NAME tuntap` and add to bridge, owned by the owner user. | Tap name must match `tap-fc-*` or `tap-pool-*`. Bridge config (name + IP + CIDR) passes the same banger-managed check as `priv.ensure_bridge`, otherwise the new tap could be `master`-attached to an arbitrary host iface. | +| `priv.delete_tap` | `ip link del NAME`. | Same prefix check on the tap name. | +| `priv.sync_resolver_routing` | `resolvectl dns/domain/default-route` on the configured bridge. | Bridge name must equal `br-fc` or start with `br-fc-` (same banger-managed check). Resolver address must parse via `net.ParseIP`. | +| `priv.clear_resolver_routing` | `resolvectl revert` on the bridge. | Same banger-managed bridge-name check. | | `priv.ensure_nat` | `iptables -t nat MASQUERADE` for `(guest_ip, tap)` plus matching FORWARD rules; `enable=false` removes them. | Tap must be banger-prefixed. Guest IP must parse as IPv4. | | `priv.create_dm_snapshot` | Create a `dmsetup` device-mapper snapshot from `rootfs.ext4` with COW backing file. | Both paths must be inside `/var/lib/banger`; DM name must start with `fc-rootfs-`. | | `priv.cleanup_dm_snapshot` | `dmsetup remove` and `losetup -d` for a snapshot the helper itself just created. | Every non-empty `dmsnap.Handles` field is checked: DM name `fc-rootfs-*`, DM device `/dev/mapper/fc-rootfs-*`, loops `/dev/loopN`. | @@ -271,11 +271,12 @@ If you install banger as root, you are trusting: `validateLoopDevicePath`, `validateDMRemoveTarget`, `validateDMSnapshotHandles`, `validateRootExecutable`, `validateNotSymlink`, `validateExt4ImagePath`, - `validateLinuxIfaceName`, `validateIPv4`, `validateResolverAddr`, - and `validateFirecrackerPID`. If any of these are bypassed, the - helper would carry out a privileged op against an unmanaged - target. They are unit-tested in - `internal/roothelper/roothelper_test.go`. + `validateLinuxIfaceName`, `validateBangerBridgeName`, + `validateNetworkConfig`, `validateCIDRPrefix`, `validateIPv4`, + `validateResolverAddr`, `validateSignalName`, and + `validateFirecrackerPID`. If any of these are bypassed, the helper + would carry out a privileged op against an unmanaged target. They + are unit-tested in `internal/roothelper/roothelper_test.go`. 3. The Firecracker binary banger executes. The helper refuses to launch anything that isn't a regular, executable, root-owned, not world-writable file — but the binary's own behaviour is your diff --git a/internal/roothelper/roothelper.go b/internal/roothelper/roothelper.go index 4eac36d..1699040 100644 --- a/internal/roothelper/roothelper.go +++ b/internal/roothelper/roothelper.go @@ -436,6 +436,13 @@ func (s *Server) dispatch(ctx context.Context, req rpc.Request) rpc.Response { if err != nil { return rpc.NewError("bad_params", err.Error()) } + // Without these the helper would happily run `ip link add` + // against arbitrary names, `ip addr add` with arbitrary + // IP/CIDR, and `ip link set up` against any host + // iface a compromised daemon might pick. + if err := validateNetworkConfig(params); err != nil { + return rpc.NewError("bad_params", err.Error()) + } return marshalResultOrError(struct{}{}, s.ensureBridge(ctx, params)) case methodCreateTap: params, err := rpc.DecodeParams[struct { @@ -445,6 +452,12 @@ func (s *Server) dispatch(ctx context.Context, req rpc.Request) rpc.Response { if err != nil { return rpc.NewError("bad_params", err.Error()) } + // Pin both the bridge config (so the new TAP can't be + // attached to e.g. eth0 via `ip link set master`) and + // the tap name itself. + if err := validateNetworkConfig(params.NetworkConfig); err != nil { + return rpc.NewError("bad_params", err.Error()) + } return marshalResultOrError(struct{}{}, s.createTap(ctx, params.NetworkConfig, params.TapName)) case methodDeleteTap: params, err := rpc.DecodeParams[struct { @@ -463,11 +476,13 @@ func (s *Server) dispatch(ctx context.Context, req rpc.Request) rpc.Response { return rpc.NewError("bad_params", err.Error()) } // syncResolverRouting short-circuits on empty input; only - // validate when actually doing something. This stops a - // compromised daemon from flapping arbitrary system-managed - // links via resolvectl. + // validate when actually doing something. validateBanger + // BridgeName is stricter than the previous validateLinux + // IfaceName: it stops a compromised daemon from pointing + // resolvectl at any host interface, not just refusing + // obviously-malformed names. if strings.TrimSpace(params.BridgeName) != "" || strings.TrimSpace(params.ServerAddr) != "" { - if err := validateLinuxIfaceName(params.BridgeName); err != nil { + if err := validateBangerBridgeName(params.BridgeName); err != nil { return rpc.NewError("bad_params", err.Error()) } if err := validateResolverAddr(params.ServerAddr); err != nil { @@ -483,7 +498,7 @@ func (s *Server) dispatch(ctx context.Context, req rpc.Request) rpc.Response { return rpc.NewError("bad_params", err.Error()) } if strings.TrimSpace(params.BridgeName) != "" { - if err := validateLinuxIfaceName(params.BridgeName); err != nil { + if err := validateBangerBridgeName(params.BridgeName); err != nil { return rpc.NewError("bad_params", err.Error()) } } @@ -1105,6 +1120,75 @@ func (s *Server) validateExt4ImagePath(path string) error { return fmt.Errorf("path %q is not a banger-managed ext4 image", path) } +// bangerBridgeNamePrefix pins the only iface-name shape the helper +// will mutate via priv.ensure_bridge / priv.create_tap / the resolver +// routing RPCs. Anything that doesn't match — host primary interfaces +// like eth0/wlan0/lo, foreign managed bridges like docker0/virbr0, +// arbitrary attacker-chosen names — is refused outright. Banger's +// daemon-config default for BridgeName is "br-fc"; users wanting a +// different name must keep the "br-fc-" prefix so the helper can +// recognise it as banger-managed. +const bangerBridgeNamePrefix = "br-fc" + +// validateBangerBridgeName enforces the banger naming convention on +// any bridge name a helper RPC mutates. Without this, a compromised +// owner-uid daemon could ask the helper (which runs with +// CAP_NET_ADMIN) to bring up arbitrary host interfaces, attach +// per-VM taps to other users' bridges, or flap the host's primary +// iface — argv-style exec rules out shell injection but the kernel +// happily honours these requests against any iface the caller +// names. +func validateBangerBridgeName(name string) error { + if err := validateLinuxIfaceName(name); err != nil { + return err + } + trimmed := strings.TrimSpace(name) + if trimmed == bangerBridgeNamePrefix { + return nil + } + if strings.HasPrefix(trimmed, bangerBridgeNamePrefix+"-") { + return nil + } + return fmt.Errorf("bridge name %q is not banger-managed (must equal %q or start with %q)", name, bangerBridgeNamePrefix, bangerBridgeNamePrefix+"-") +} + +// validateCIDRPrefix accepts a numeric IPv4 prefix length in [8, 32]. +// fcproc.EnsureBridge concatenates BridgeIP + "/" + CIDR into the +// `ip addr add` argument, so anything that doesn't parse as a small +// integer in that range either errors out (helpful) or, worse, +// silently widens the bridge subnet beyond what the daemon intends. +func validateCIDRPrefix(s string) error { + trimmed := strings.TrimSpace(s) + if trimmed == "" { + return errors.New("cidr prefix is required") + } + n, err := strconv.Atoi(trimmed) + if err != nil { + return fmt.Errorf("cidr prefix %q is not numeric", s) + } + if n < 8 || n > 32 { + return fmt.Errorf("cidr prefix %d is outside [8, 32]", n) + } + return nil +} + +// validateNetworkConfig is the single chokepoint for every helper RPC +// that takes a bridge name + IP + CIDR triple. Bundling the checks +// here keeps every caller in lockstep on what counts as a +// well-formed banger network config. +func validateNetworkConfig(cfg NetworkConfig) error { + if err := validateBangerBridgeName(cfg.BridgeName); err != nil { + return err + } + if err := validateIPv4(cfg.BridgeIP); err != nil { + return fmt.Errorf("bridge ip: %w", err) + } + if err := validateCIDRPrefix(cfg.CIDR); err != nil { + return fmt.Errorf("bridge cidr: %w", err) + } + return nil +} + // validateLoopDevicePath confirms path is `/dev/loopN` for some N≥0. // dmsnap.Cleanup detaches loops via `losetup -d `; without this // a compromised daemon could ask the helper to detach an arbitrary diff --git a/internal/roothelper/roothelper_test.go b/internal/roothelper/roothelper_test.go index 24641dd..ac698c3 100644 --- a/internal/roothelper/roothelper_test.go +++ b/internal/roothelper/roothelper_test.go @@ -397,6 +397,102 @@ func TestValidateManagedPathPassesPlainSubpath(t *testing.T) { } } +func TestValidateBangerBridgeName(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + arg string + ok bool + }{ + {name: "default", arg: "br-fc", ok: true}, + {name: "suffixed", arg: "br-fc-alt", ok: true}, + {name: "with_whitespace", arg: " br-fc ", ok: true}, + {name: "wrong_prefix", arg: "br0", ok: false}, + {name: "host_iface", arg: "eth0", ok: false}, + {name: "docker", arg: "docker0", ok: false}, + {name: "loopback", arg: "lo", ok: false}, + {name: "empty", arg: "", ok: false}, + {name: "br_dash_only", arg: "br-", ok: false}, // not "br-fc" exactly + {name: "almost_match", arg: "br-fcx", ok: false}, + {name: "with_slash", arg: "br-fc/x", ok: false}, + {name: "too_long", arg: "br-fc-aaaaaaaaaa", ok: false}, // 16 chars + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := validateBangerBridgeName(tc.arg) + if tc.ok && err != nil { + t.Fatalf("validateBangerBridgeName(%q) = %v, want nil", tc.arg, err) + } + if !tc.ok && err == nil { + t.Fatalf("validateBangerBridgeName(%q) succeeded, want error", tc.arg) + } + }) + } +} + +func TestValidateCIDRPrefix(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + arg string + ok bool + }{ + {name: "default_24", arg: "24", ok: true}, + {name: "min_8", arg: "8", ok: true}, + {name: "max_32", arg: "32", ok: true}, + {name: "with_whitespace", arg: " 16 ", ok: true}, + {name: "below_min", arg: "7", ok: false}, + {name: "above_max", arg: "33", ok: false}, + {name: "non_numeric", arg: "abc", ok: false}, + {name: "ipv6_prefix", arg: "64", ok: false}, // outside [8, 32] + {name: "with_slash", arg: "/24", ok: false}, + {name: "empty", arg: "", ok: false}, + {name: "negative", arg: "-1", ok: false}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := validateCIDRPrefix(tc.arg) + if tc.ok && err != nil { + t.Fatalf("validateCIDRPrefix(%q) = %v, want nil", tc.arg, err) + } + if !tc.ok && err == nil { + t.Fatalf("validateCIDRPrefix(%q) succeeded, want error", tc.arg) + } + }) + } +} + +func TestValidateNetworkConfig(t *testing.T) { + t.Parallel() + good := NetworkConfig{ + BridgeName: "br-fc", + BridgeIP: "172.16.0.1", + CIDR: "24", + } + if err := validateNetworkConfig(good); err != nil { + t.Fatalf("validateNetworkConfig(default) = %v, want nil", err) + } + for _, tc := range []struct { + name string + mutate func(NetworkConfig) NetworkConfig + }{ + {name: "bad_bridge", mutate: func(c NetworkConfig) NetworkConfig { c.BridgeName = "eth0"; return c }}, + {name: "bad_ip", mutate: func(c NetworkConfig) NetworkConfig { c.BridgeIP = "::1"; return c }}, + {name: "bad_cidr", mutate: func(c NetworkConfig) NetworkConfig { c.CIDR = "/24"; return c }}, + {name: "missing_ip", mutate: func(c NetworkConfig) NetworkConfig { c.BridgeIP = ""; return c }}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if err := validateNetworkConfig(tc.mutate(good)); err == nil { + t.Fatalf("validateNetworkConfig(%s) succeeded, want error", tc.name) + } + }) + } +} + func TestValidateLinuxIfaceName(t *testing.T) { t.Parallel()