firecracker: drop sudo sh -c, race chown against SDK probe in Go

Replace the shell-string launcher in buildProcessRunner with a direct
exec.Command. The previous sh -c wrapper relied on shellQuote escaping
for every MachineConfig field that flowed into the launch script; any
future field that ever carried an attacker-controlled value would have
become RCE-as-root. The new path passes binary path and flags as
separate argv entries, so there is no shell to interpret anything.

The wrapper also did two things the shell can no longer do for us:

  1. umask 077 — moved to syscall.Umask in cmd/bangerd/main.go so every
     firecracker child (and any other file the daemon creates) inherits
     0600 by default. Single-user dev sandbox state should be private.

  2. chown_watcher — the SDK's HTTP probe inside Machine.Start connects
     to the API socket the moment it appears. Under sudo the socket is
     created root-owned and the daemon's connect(2) gets EACCES, so the
     post-Start EnsureSocketAccess never runs. The shell papered over
     this with a backgrounded chown loop. Replaced by
     fcproc.EnsureSocketAccessForAsync: same race-window guarantee, in
     pure Go, kicked off in LaunchFirecracker right before Start and
     awaited right after.

Tests updated: shell-substring assertions replaced with cmd-arg
assertions, plus a new fcproc test pinning the async chown sequence.
Smoke (full systemd two-service install + KVM scenarios) passes.

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()
 

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):
 		}
 	}
 }

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 {

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

internal/firecracker/client.go

@@ -201,12 +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 = exec.Command(cfg.BinaryPath, args...)
+	} else {
+		cmd = exec.Command("sudo", append([]string{"-n", "-E", cfg.BinaryPath}, args...)...)
+	}
 	cmd.Stdin = nil
 	if logFile != nil {
 		cmd.Stdout = logFile
@@ -214,58 +229,6 @@ func buildProcessRunner(cfg MachineConfig, logFile *os.File) *exec.Cmd {
 	}
 	return cmd
 }
-	// 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
-		cmd.Stderr = logFile
-	}
-	return cmd
-}
-
-func shellQuote(value string) string {
-	return "'" + strings.ReplaceAll(value, "'", `'"'"'`) + "'"
-}
 
 func newLogger(base *slog.Logger) *logrus.Entry {
 	logger := logrus.New()

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}))