roothelper: pin bridge name + IP + CIDR to a banger-managed shape
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 <iface> up` against any
host interface and `ip addr add` arbitrary IP/CIDR to it.
* priv.create_tap could `ip link set <new-tap> master <iface>`,
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) <noreply@anthropic.com>
This commit is contained in:
parent
4004ce2e7e
commit
182bccf8af
3 changed files with 196 additions and 15 deletions
|
|
@ -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 <NAME> 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 <tap> 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 <path>`; without this
|
||||
// a compromised daemon could ask the helper to detach an arbitrary
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue