diff --git a/cmd/bangerd/main.go b/cmd/bangerd/main.go index 0cf8ab1..ee4826b 100644 --- a/cmd/bangerd/main.go +++ b/cmd/bangerd/main.go @@ -11,6 +11,12 @@ import ( ) func main() { + // 0o077 ensures the firecracker API/vsock sockets (and any other files + // the daemon or its children create) are user-private by default. The + // previous shell wrapper around firecracker exec did this inline; with + // the wrapper gone, the daemon process owns the umask. + syscall.Umask(0o077) + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() diff --git a/internal/daemon/fcproc/fcproc.go b/internal/daemon/fcproc/fcproc.go index eda6b27..ae2a0c9 100644 --- a/internal/daemon/fcproc/fcproc.go +++ b/internal/daemon/fcproc/fcproc.go @@ -13,6 +13,7 @@ import ( "os" "strconv" "strings" + "sync" "time" "banger/internal/firecracker" @@ -137,7 +138,64 @@ func (m *Manager) EnsureSocketAccess(ctx context.Context, socketPath, label stri // EnsureSocketAccessFor waits for the socket to appear then chowns/chmods it // to uid:gid, mode 0600. func (m *Manager) EnsureSocketAccessFor(ctx context.Context, socketPath, label string, uid, gid int) error { - if err := waitForPath(ctx, socketPath, 5*time.Second, label); err != nil { + return m.ensureSocketAccessFor(ctx, socketPath, label, uid, gid, 5*time.Second, 100*time.Millisecond) +} + +// EnsureSocketAccessForAsync runs EnsureSocketAccessFor concurrently for each +// non-empty path and returns a channel that receives a single error (nil on +// full success) once all per-path operations complete. Caller MUST receive on +// the channel to unblock the goroutine. +// +// Used during firecracker boot: the SDK's HTTP probe inside Machine.Start +// connects to the API socket the moment it appears. When firecracker is +// launched under sudo the socket is created root-owned, and the daemon's +// connect(2) gets EACCES until something chowns it. Running the chown +// concurrently with Start (instead of after Start returns, which deadlocks) +// closes the race without a shell-level chown_watcher. +// +// Uses a 25ms poll cadence (vs 100ms for the synchronous variant) to win +// against the SDK's tight HTTP retry loop. +func (m *Manager) EnsureSocketAccessForAsync(ctx context.Context, socketPaths []string, uid, gid int) <-chan error { + var clean []string + for _, p := range socketPaths { + if strings.TrimSpace(p) != "" { + clean = append(clean, p) + } + } + done := make(chan error, 1) + if len(clean) == 0 { + done <- nil + close(done) + return done + } + go func() { + defer close(done) + var wg sync.WaitGroup + errCh := make(chan error, len(clean)) + for _, p := range clean { + wg.Add(1) + go func(path string) { + defer wg.Done() + if err := m.ensureSocketAccessFor(ctx, path, "firecracker socket", uid, gid, 3*time.Second, 25*time.Millisecond); err != nil { + errCh <- err + } + }(p) + } + wg.Wait() + close(errCh) + for err := range errCh { + if err != nil { + done <- err + return + } + } + done <- nil + }() + return done +} + +func (m *Manager) ensureSocketAccessFor(ctx context.Context, socketPath, label string, uid, gid int, timeout, interval time.Duration) error { + if err := pollPath(ctx, socketPath, timeout, interval, label); err != nil { return err } if os.Geteuid() == 0 { @@ -214,6 +272,10 @@ func (m *Manager) Kill(ctx context.Context, pid int) error { } func waitForPath(ctx context.Context, path string, timeout time.Duration, label string) error { + return pollPath(ctx, path, timeout, 100*time.Millisecond, label) +} + +func pollPath(ctx context.Context, path string, timeout, interval time.Duration, label string) error { deadline := time.Now().Add(timeout) for { if _, err := os.Stat(path); err == nil { @@ -227,7 +289,7 @@ func waitForPath(ctx context.Context, path string, timeout time.Duration, label select { case <-ctx.Done(): return ctx.Err() - case <-time.After(100 * time.Millisecond): + case <-time.After(interval): } } } diff --git a/internal/daemon/fcproc/fcproc_test.go b/internal/daemon/fcproc/fcproc_test.go index 57b3573..d013c7b 100644 --- a/internal/daemon/fcproc/fcproc_test.go +++ b/internal/daemon/fcproc/fcproc_test.go @@ -180,6 +180,58 @@ func TestEnsureSocketAccessTimesOutBeforeTouchingRunner(t *testing.T) { } } +// TestEnsureSocketAccessForAsyncReturnsImmediatelyWhenNoPaths pins the +// fast-path: callers can hand the helper an empty list (e.g. when VSockPath +// is unset) and get a no-op channel back without spinning a goroutine. +func TestEnsureSocketAccessForAsyncReturnsImmediatelyWhenNoPaths(t *testing.T) { + runner := &scriptedRunner{t: t} // any runner call would fail the test + mgr := New(runner, Config{}, slog.Default()) + + done := mgr.EnsureSocketAccessForAsync(context.Background(), []string{"", " "}, 1000, 1000) + select { + case err := <-done: + if err != nil { + t.Fatalf("got %v, want nil for empty input", err) + } + case <-time.After(time.Second): + t.Fatal("EnsureSocketAccessForAsync did not signal completion") + } +} + +// TestEnsureSocketAccessForAsyncWaitsForSocketThenChowns pins the boot-time +// race fix: while Machine.Start spins up firecracker, the helper polls for the +// socket and runs chmod + chown the moment it appears. If this drifts, the +// SDK's HTTP probe gets EACCES on a root-owned socket and Start times out. +func TestEnsureSocketAccessForAsyncWaitsForSocketThenChowns(t *testing.T) { + socketPath := filepath.Join(t.TempDir(), "delayed.sock") + go func() { + time.Sleep(50 * time.Millisecond) + _ = os.WriteFile(socketPath, []byte{}, 0o600) + }() + + runner := &scriptedRunner{ + t: t, + sudos: []scriptedCall{ + {}, // chmod 600 + {}, // chown uid:gid + }, + } + mgr := New(runner, Config{}, slog.Default()) + + done := mgr.EnsureSocketAccessForAsync(context.Background(), []string{socketPath}, 4242, 4242) + select { + case err := <-done: + if err != nil { + t.Fatalf("EnsureSocketAccessForAsync: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("EnsureSocketAccessForAsync did not signal completion") + } + if len(runner.sudos) != 0 { + t.Fatalf("expected both chmod and chown to run, %d sudo calls remaining", len(runner.sudos)) + } +} + func contains(s, sub string) bool { for i := 0; i+len(sub) <= len(s); i++ { if s[i:i+len(sub)] == sub { diff --git a/internal/daemon/privileged_ops.go b/internal/daemon/privileged_ops.go index 5e2f8b1..11d2411 100644 --- a/internal/daemon/privileged_ops.go +++ b/internal/daemon/privileged_ops.go @@ -190,11 +190,22 @@ func (o *localPrivilegedOps) LaunchFirecracker(ctx context.Context, req roothelp if err != nil { return 0, err } - if err := machine.Start(ctx); err != nil { + // Race the chown against the SDK's HTTP probe inside Start: when the + // daemon is non-root, firecracker is launched under sudo and the API + // socket appears root-owned. Without a concurrent chown the SDK's + // connect(2) gets EACCES and Start times out before our post-Start + // EnsureSocketAccess can ever run. + chownDone := o.fc().EnsureSocketAccessForAsync(ctx, []string{req.SocketPath, req.VSockPath}, o.clientUID, o.clientGID) + startErr := machine.Start(ctx) + chownErr := <-chownDone + if startErr != nil { if pid := o.fc().ResolvePID(context.Background(), machine, req.SocketPath); pid > 0 { _ = o.KillProcess(context.Background(), pid) } - return 0, err + return 0, startErr + } + if chownErr != nil { + return 0, chownErr } if err := o.EnsureSocketAccess(ctx, req.SocketPath, "firecracker api socket"); err != nil { return 0, err diff --git a/internal/firecracker/client.go b/internal/firecracker/client.go index f54fd9f..eacb50b 100644 --- a/internal/firecracker/client.go +++ b/internal/firecracker/client.go @@ -201,60 +201,27 @@ func defaultDriveID(drive DriveConfig, fallback string) string { return fallback } +// buildProcessRunner constructs the *exec.Cmd the SDK will start. Args are +// passed directly — no shell, no string interpolation — so any future change +// to MachineConfig fields can't smuggle shell metacharacters into the launch. +// +// The daemon and root-helper processes set umask 077 at startup, so the +// API/vsock sockets firecracker creates inherit 0600 mode without needing a +// shell-level `umask` wrapper. +// +// When firecracker has to be launched under sudo (non-root daemon), the +// resulting sockets are root-owned. The caller (LaunchFirecracker) kicks off +// fcproc.EnsureSocketAccessForAsync immediately *before* Machine.Start so the +// chown wins the race against the SDK's HTTP probe over the API socket. That +// replaces the previous in-shell chown_watcher. func buildProcessRunner(cfg MachineConfig, logFile *os.File) *exec.Cmd { + args := []string{"--api-sock", cfg.SocketPath, "--id", cfg.VMID} + var cmd *exec.Cmd if os.Geteuid() == 0 { - script := "umask 077 && exec " + shellQuote(cfg.BinaryPath) + - " --api-sock " + shellQuote(cfg.SocketPath) + - " --id " + shellQuote(cfg.VMID) - cmd := exec.Command("sh", "-c", script) - cmd.Stdin = nil - if logFile != nil { - cmd.Stdout = logFile - cmd.Stderr = logFile - } - return cmd + cmd = exec.Command(cfg.BinaryPath, args...) + } else { + cmd = exec.Command("sudo", append([]string{"-n", "-E", cfg.BinaryPath}, args...)...) } - // Two moving parts, run inside a single sudo'd shell: - // - // 1. umask 077 + exec firecracker → the API and vsock sockets - // firecracker creates are born 0600 owned by root (sudo user), - // not 0755. Without the umask there's a real window where a - // local attacker could hit the control plane. - // - // 2. A background subshell polls for each expected socket and - // chowns it to $SUDO_UID:$SUDO_GID as soon as it appears. - // - // The chown is required *before* the firecracker-go-sdk's - // waitForSocket returns from Machine.Start — the SDK does both an - // os.Stat and an HTTP GET over the socket, and AF_UNIX connect(2) - // needs write permission on the socket file. With the socket at - // 0600 root:root, the daemon process (running as the invoking - // user) gets EACCES on connect and the SDK loops until its 3s - // timeout. The daemon's post-Start EnsureSocketAccess chown would - // fix it, but Start never returns to hand control back. - // - // Racing the chown inside sudo's shell closes the gap: by the - // time the SDK's HTTP probe fires, the socket is already owned by - // the invoking user. - chownWatcher := func(path string) string { - // Bounded poll: 20 × 50ms = 1s. Matches the SDK's 3s wait - // budget with headroom and bails quietly if firecracker - // never creates the socket (e.g. bad args — the error - // surfaces through firecracker's non-zero exit). - return `for _ in $(seq 1 20); do [ -S ` + shellQuote(path) + ` ] && break; sleep 0.05; done; ` + - `[ -S ` + shellQuote(path) + ` ] && chown "$SUDO_UID:$SUDO_GID" ` + shellQuote(path) + ` || true` - } - watchers := chownWatcher(cfg.SocketPath) - if strings.TrimSpace(cfg.VSockPath) != "" { - watchers += "; " + chownWatcher(cfg.VSockPath) - } - script := "umask 077 && (" + watchers + ") & exec " + shellQuote(cfg.BinaryPath) + - " --api-sock " + shellQuote(cfg.SocketPath) + - " --id " + shellQuote(cfg.VMID) - // sudo -E preserves SUDO_UID / SUDO_GID (sudo sets them itself - // regardless, but -E is already the convention in this codebase - // and the background subshell needs them). - cmd := exec.Command("sudo", "-n", "-E", "sh", "-c", script) cmd.Stdin = nil if logFile != nil { cmd.Stdout = logFile @@ -263,10 +230,6 @@ func buildProcessRunner(cfg MachineConfig, logFile *os.File) *exec.Cmd { return cmd } -func shellQuote(value string) string { - return "'" + strings.ReplaceAll(value, "'", `'"'"'`) + "'" -} - func newLogger(base *slog.Logger) *logrus.Entry { logger := logrus.New() logger.SetOutput(io.Discard) diff --git a/internal/firecracker/client_test.go b/internal/firecracker/client_test.go index 9c5b300..cf22d5c 100644 --- a/internal/firecracker/client_test.go +++ b/internal/firecracker/client_test.go @@ -5,6 +5,7 @@ import ( "context" "log/slog" "net" + "os" "path/filepath" "strings" "testing" @@ -72,7 +73,7 @@ func TestBuildConfig(t *testing.T) { } } -func TestBuildProcessRunnerUsesSudoShellWrapper(t *testing.T) { +func TestBuildProcessRunnerInvokesSudoWithDirectArgs(t *testing.T) { cmd := buildProcessRunner(MachineConfig{ BinaryPath: "/repo/firecracker", SocketPath: "/tmp/fc.sock", @@ -80,53 +81,48 @@ func TestBuildProcessRunnerUsesSudoShellWrapper(t *testing.T) { VMID: "vm-1", }, nil) + // No shell, no string interpolation: the binary path and every flag + // are independent argv entries. Even if MachineConfig ever carried an + // attacker-controlled value, there's no shell to interpret it. + wantArgs := []string{"sudo", "-n", "-E", "/repo/firecracker", "--api-sock", "/tmp/fc.sock", "--id", "vm-1"} + if !equalStrings(cmd.Args, wantArgs) { + t.Fatalf("args = %v, want %v", cmd.Args, wantArgs) + } if cmd.Path != "/usr/bin/sudo" && cmd.Path != "sudo" { t.Fatalf("command path = %q", cmd.Path) } - if len(cmd.Args) != 6 { - t.Fatalf("args = %v", cmd.Args) - } - if cmd.Args[1] != "-n" || cmd.Args[2] != "-E" || cmd.Args[3] != "sh" || cmd.Args[4] != "-c" { - t.Fatalf("args = %v", cmd.Args) - } - script := cmd.Args[5] - - // The firecracker exec must run in the foreground so its exit - // status propagates through sh back to the SDK. - if !strings.Contains(script, "exec '/repo/firecracker' --api-sock '/tmp/fc.sock' --id 'vm-1'") { - t.Fatalf("script missing firecracker exec: %q", script) - } - // umask stays — the security intent is unchanged. - if !strings.Contains(script, "umask 077") { - t.Fatalf("script dropped umask 077: %q", script) - } - // Background watcher chowns both the API socket and the vsock - // socket to the invoking user as soon as they appear, so - // firecracker-go-sdk's waitForSocket HTTP probe (which needs - // connect access) isn't blocked on root-owned sockets. - if !strings.Contains(script, `chown "$SUDO_UID:$SUDO_GID" '/tmp/fc.sock'`) { - t.Fatalf("script missing API-socket chown: %q", script) - } - if !strings.Contains(script, `chown "$SUDO_UID:$SUDO_GID" '/tmp/vsock.sock'`) { - t.Fatalf("script missing vsock-socket chown: %q", script) - } if cmd.Cancel != nil { t.Fatal("process runner should not be tied to a request context") } } -func TestBuildProcessRunnerOmitsVSockChownWhenUnset(t *testing.T) { +func TestBuildProcessRunnerOmitsSudoWhenAlreadyRoot(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("requires root to exercise the no-sudo branch") + } cmd := buildProcessRunner(MachineConfig{ BinaryPath: "/repo/firecracker", SocketPath: "/tmp/fc.sock", VMID: "vm-1", }, nil) - script := cmd.Args[5] - if strings.Contains(script, "vsock") { - t.Fatalf("script should not mention vsock when VSockPath is empty: %q", script) + wantArgs := []string{"/repo/firecracker", "--api-sock", "/tmp/fc.sock", "--id", "vm-1"} + if !equalStrings(cmd.Args, wantArgs) { + t.Fatalf("args = %v, want %v", cmd.Args, wantArgs) } } +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + func TestSDKLoggerBridgeEmitsStructuredDebugLogs(t *testing.T) { var buf bytes.Buffer logger := slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))