fix(runner): bound the agent socket path under the AF_UNIX sun_path cap

go/internal/runner/gateway/socket.go

@@ -46,6 +46,31 @@ const socketDirMode os.FileMode = 0o700
 // teardown (design SocketListener.Close).
 const shutdownGrace = 5 * time.Second
 
+// sunPathMax is the longest AF_UNIX path the kernel accepts: sockaddr_un's
+// sun_path holds the path plus a NUL terminator. Exceeded, bind(2) returns a
+// bare EINVAL naming neither the limit nor the length, so the path is checked
+// here instead (see listenAgentSocket).
+//
+// Derived from the platform's own sockaddr_un rather than hard-coded, because
+// the array is not one size across the platforms //go:build unix admits, and
+// the spread is wider than the two sizes anyone reaches for: measured over all
+// nine, it is 108 bytes on linux, solaris and illumos, 104 on darwin and the
+// BSDs including dragonfly, and 1023 on aix. No pair of hand-written numbers is
+// correct here, and an author who enumerates the ones they thought of ships the
+// bug on the rest — an under-sized constant merely refuses a path that would
+// have worked, but an over-sized one passes a path the kernel rejects at bind,
+// which is the exact failure this check exists to prevent.
+//
+// The -1 is the NUL terminator, which is justification enough on its own — no
+// appeal to Go's internals is needed, and citing them by line would pin this
+// comment to a toolchain version nothing here asserts. Worth knowing that the
+// bound is deliberately conservative rather than exact at two edges: AIX's
+// wrapper permits a name of len(Path), and a Linux ABSTRACT name carries no
+// terminator, so both admit one byte more than this constant. Neither reaches
+// the Runner, which only ever binds a filesystem path, and refusing a byte that
+// would have worked is the safe direction.
+const sunPathMax = len(syscall.RawSockaddrUnix{}.Path) - 1
+
 // runnerUID reports the uid a reclaimable stale socket must be owned by. It is a
 // package var over os.Getuid so a hermetic test can drive the wrong-owner
 // fail-closed branch (which cannot be forged on disk without root).
@@ -71,7 +96,41 @@ type SocketListener struct {
 // (regular file, dir, symlink, or a socket owned by another uid — a path
 // collision or partial op, never an abandoned Runner socket) is rejected fail-
 // closed and never deleted.
+//
+// Path length is checked FIRST, before any directory is created. Not only for
+// the diagnostic: a bind that fails EINVAL has already run MkdirAll, and the
+// dir survives — measured, 0700 left on disk. Nothing reclaims it on any path,
+// because nothing in this socket's lifecycle removes the directory at all:
+// Close removes the socket file and never filepath.Dir(path), so the
+// Launch-failure teardown Provision does run (host.go:120-125) leaks it too.
+// Refusing before the mkdir is what makes the question moot, rather than a
+// teardown that would have to be added.
 func listenAgentSocket(ctx context.Context, path string, h http.Handler) (*SocketListener, error) {
+	// The socket path is RuntimeDir + /containers/<container>/agent.sock
+	// (host.go), where <container> is NamePrefix + the agent account id
+	// (spec.go). RuntimeDir is the operator-supplied variable: --runtime-dir is
+	// unbounded on every hop and inflates the path for every agent at once.
+	//
+	// The account id's SHAPE is validated where it enters, not here — see
+	// validAccountID in spec.go, which refuses an id that is not a single safe
+	// path element. That is a separate property from length and cannot be
+	// checked here: a traversing id SHORTENS the path, so it would sail past
+	// this guard.
+	//
+	// With the minted id the tail is 69 bytes, so the default /run/compass lands
+	// at 81 and the --runtime-dir budget is sunPathMax-69: 38 on Linux, 34 on
+	// darwin/BSD. That is the figure a startup precheck should assert, so an
+	// operator learns it at boot rather than at first provision (SEA-1443).
+	//
+	// The bind's own error is "bind: invalid argument", an EINVAL naming neither
+	// the limit nor the actual length, which reads as a permissions or path
+	// problem. Check first, before any directory is created, so a misconfigured
+	// deployment is self-diagnosing at Provision and leaves nothing behind. The
+	// message names both knobs: the path alone does not say which one to shrink.
+	if len(path) > sunPathMax {
+		return nil, fmt.Errorf("agent socket path %q is %d bytes, over the %d-byte AF_UNIX limit: shorten the Runner's --runtime-dir or the agent account id", path, len(path), sunPathMax)
+	}
+
 	dir := filepath.Dir(path)
 	if err := os.MkdirAll(dir, socketDirMode); err != nil {
 		return nil, fmt.Errorf("creating agent socket dir %q: %w", dir, err)

go/internal/runner/gateway/socket_test.go

@@ -19,10 +19,12 @@ package gateway
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -361,6 +363,112 @@ func TestRejectsWrongOwnerSocketNeverDeletes(t *testing.T) {
 	}
 }
 
+// Case 7b. A path over the AF_UNIX sun_path limit is refused with an error that
+// names the limit and the actual length, and nothing is created on disk; a path
+// exactly at the limit still binds. The kernel enforces the cap at bind(2) with
+// a bare EINVAL ("invalid argument"), which names neither number and reads as a
+// permissions or path fault; the Runner's socket path is RuntimeDir plus a
+// 69-byte tail at the store-minted 32-hex account id — a width fixed at its
+// minting site, not validated at this hop — and RuntimeDir is operator-supplied
+// with no bound anywhere, so this is a reachable misconfiguration, not a
+// defensive check.
+//
+// Mutations, both RED: dropping the pre-check lets the over-long path reach bind,
+// failing the message assertions and the never-created-dir assertion; tightening
+// the bound to >= refuses the at-cap path the kernel accepts, failing the first
+// subtest.
+func TestRejectsPathOverSunPathLimit(t *testing.T) {
+	// padTo returns a path under a fresh temp root of exactly n bytes, or fails
+	// the subtest when the root alone already exceeds the budget (a very deep
+	// TMPDIR).
+	padTo := func(t *testing.T, n int) (dir, path string) {
+		t.Helper()
+		// A short fixed root, not t.TempDir(): the latter embeds the test name,
+		// which lands these subtests at ~135 bytes under a darwin-shaped TMPDIR
+		// (/var/folders/<2>/<hash>/T, ~49 bytes) and skips them — on the one
+		// platform where sunPathMax differs from Linux's, and so the one platform
+		// this test most needs to run. MkdirTemp("", "g") is bounded instead: the
+		// random suffix is a uint32 in decimal, so at most 10 digits, giving <=16
+		// bytes on Linux and <=61 under that darwin TMPDIR — both well under the
+		// cap, and a bound rather than a measurement that could drift.
+		root, err := os.MkdirTemp("", "g") //nolint:usetesting // t.TempDir embeds the test name and blows this test's own byte budget on darwin — see above
+		if err != nil {
+			t.Fatalf("MkdirTemp: %v", err)
+		}
+		t.Cleanup(func() {
+			if err := os.RemoveAll(root); err != nil {
+				t.Errorf("removing %q: %v", root, err)
+			}
+		})
+		dir = filepath.Join(root, "run")
+		base := len(dir) + 1 // + separator
+		// These two subtests are the only coverage of the boundary the guard
+		// exists to enforce, so a deep TMPDIR must redden the run rather than
+		// skip it — a skip here reports `ok` for a package that asserted
+		// nothing, which is the failure mode this whole change is about.
+		if base >= n {
+			t.Fatalf("temp root %q is %d bytes, leaving no room for a %d-byte path; set TMPDIR to a short path", dir, base, n)
+		}
+		path = filepath.Join(dir, strings.Repeat("s", n-base))
+		if len(path) != n {
+			t.Fatalf("constructed path is %d bytes, want exactly %d", len(path), n)
+		}
+		return dir, path
+	}
+
+	t.Run("exactly at the cap binds", func(t *testing.T) {
+		// The boundary case: the check must reject only what the kernel would.
+		// An off-by-one (>= instead of >) refuses a path that binds fine, so this
+		// is what keeps the guard from being over-tight.
+		_, path := padTo(t, sunPathMax)
+		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
+		defer cancel()
+		l, err := listenAgentSocket(ctx, path, stubHandler(t))
+		if err != nil {
+			t.Fatalf("listen on a %d-byte path (exactly the cap) = %v, want success", len(path), err)
+		}
+		if err := l.Close(context.Background()); err != nil {
+			t.Errorf("Close: %v", err)
+		}
+	})
+
+	t.Run("one byte over is refused with a diagnostic", func(t *testing.T) {
+		dir, path := padTo(t, sunPathMax+1)
+		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
+		defer cancel()
+		l, err := listenAgentSocket(ctx, path, stubHandler(t))
+		if err == nil {
+			_ = l.Close(context.Background())
+			t.Fatal("listen on an over-long path must fail, got nil error")
+		}
+		// The diagnostic is the whole point: the message must carry both numbers
+		// and the flag to change, none of which the kernel's EINVAL does. Match
+		// the surrounding phrases, not the bare digits — a temp path carries
+		// random digits that could contain the cap by chance and pass a
+		// substring check against a message that never mentioned it.
+		msg := err.Error()
+		for _, want := range []string{
+			fmt.Sprintf("is %d bytes", len(path)),
+			fmt.Sprintf("over the %d-byte", sunPathMax),
+			"--runtime-dir",
+		} {
+			if !strings.Contains(msg, want) {
+				t.Errorf("error %q does not contain %q", msg, want)
+			}
+		}
+		// A bare EINVAL is exactly what the check exists to replace, so the
+		// message must not be one: that would mean the path reached bind.
+		if strings.Contains(msg, "invalid argument") {
+			t.Errorf("error %q reached bind; the pre-check must refuse it first", msg)
+		}
+
+		// Refused before anything was created: no directory left behind.
+		if _, err := os.Lstat(dir); !os.IsNotExist(err) {
+			t.Errorf("Lstat(%q) = %v, want the dir never to have been created", dir, err)
+		}
+	})
+}
+
 // Case 8. Close is idempotent and tolerates the socket file already being gone:
 // a second Close returns nil (the os.Remove ErrNotExist is swallowed), and a
 // Close after the socket was removed out from under the listener does not error

go/internal/runner/spec.go

@@ -11,8 +11,10 @@ package runner
 import (
 	"errors"
 	"fmt"
+	"io/fs"
 	"net/url"
 	"strings"
+	"unicode"
 
 	compassv1 "github.com/sealedsecurity/compass/go/gen/compass/v1"
 	"github.com/sealedsecurity/compass/go/internal/runtime"
@@ -51,6 +53,13 @@ func NewConfigSpecBuilder(defaults SpecDefaults) (SpecBuilder, error) {
 	if defaults.CheckoutDir == "" || defaults.HomeDir == "" {
 		return nil, errors.New("spec defaults require checkout and home dirs")
 	}
+	// The other operand of the container name. validAccountID constrains the
+	// request-derived half; this constrains the operator-derived half, so both
+	// inputs to a path segment are checked and a separator here cannot escape
+	// RuntimeDir through the same filepath.Join clean.
+	if strings.Contains(defaults.NamePrefix, "/") {
+		return nil, errors.New("spec defaults name prefix must not contain a path separator")
+	}
 	return &configSpecBuilder{defaults: defaults}, nil
 }
 
@@ -67,8 +76,8 @@ func (b *configSpecBuilder) BuildSpec(req *compassv1.ProvisionAgentWorkspaceRequ
 
 	d := b.defaults
 	accountID := req.GetAgentAccountId()
-	if accountID == "" {
-		return runtime.AgentSpec{}, errors.New("provision request requires an agent account id")
+	if err := validAccountID(accountID); err != nil {
+		return runtime.AgentSpec{}, err
 	}
 	name := d.NamePrefix + accountID
 	return runtime.AgentSpec{
@@ -86,6 +95,49 @@ func (b *configSpecBuilder) BuildSpec(req *compassv1.ProvisionAgentWorkspaceRequ
 	}, nil
 }
 
+// validAccountID refuses an agent account id that is not a single safe path
+// element. The id is not merely a label: it is concatenated into the container
+// name (below) and that name becomes a path segment of the agent socket,
+// RuntimeDir/containers/<container>/agent.sock (host.go). filepath.Join CLEANS,
+// so a "../" in the id escapes RuntimeDir entirely — "../../../../tmp/pwned"
+// resolves to /run/tmp/pwned/agent.sock — and the socket's own length guard
+// cannot catch it, because traversal SHORTENS the path.
+//
+// Nothing upstream makes this check redundant. The id is minted 32-hex, but the
+// width is a property of the minting site and is never re-checked here; the
+// foreign key that ties the id to a real account is enforced by
+// RecordAgentContainer, which runs after hub.Provision has already created the
+// 0700 directory and bound the socket; and an admin-only RPC narrows who calls
+// it, not what they may pass. So this is the hop that has to reject the shape.
+func validAccountID(id string) error {
+	if id == "" {
+		return errors.New("provision request requires an agent account id")
+	}
+	// fs.ValidPath rejects "..", any empty segment, a leading or trailing "/",
+	// and invalid UTF-8, but returns true for "." (documented) and for a legal
+	// multi-element path like "abc/def" — hence the other two conjuncts. (The
+	// empty id returned above, with its own message.)
+	if !fs.ValidPath(id) || id == "." || strings.Contains(id, "/") {
+		return fmt.Errorf("agent account id %q is not a valid path element", id)
+	}
+	// A control or format character clears every check above — it is valid UTF-8
+	// and carries no separator — and nothing downstream stops it either.
+	// Measured on Linux against listenAgentSocket's own ordering: a newline, a
+	// DEL, a C1 control and a bidi override all pass MkdirAll AND bind, so the
+	// id reaches the container name and every log line that quotes it intact.
+	// (A NUL is the lone exception, failing at MkdirAll — one hop before the
+	// listener — as a bare EINVAL naming nothing.) The name is what an operator
+	// reads back out of `podman ps` and the Runner's logs to identify an agent,
+	// so a character that can forge a line break or reorder the display makes
+	// that identification unreliable. unicode.IsControl covers C0, DEL and C1;
+	// the format class (Cf) is checked with it because a bidi override is not a
+	// control character but spoofs a rendered name just as effectively.
+	if strings.ContainsFunc(id, func(r rune) bool { return unicode.IsControl(r) || unicode.Is(unicode.Cf, r) }) {
+		return fmt.Errorf("agent account id %q contains a control or format character", id)
+	}
+	return nil
+}
+
 // repoSourceFromRequest maps the request's repo oneof to a runtime.RepoSource,
 // enforcing that exactly one variant is set.
 func repoSourceFromRequest(req *compassv1.ProvisionAgentWorkspaceRequest) (runtime.RepoSource, error) {

go/internal/runner/spec_test.go

@@ -41,6 +41,11 @@ func TestNewConfigSpecBuilderRejectsIncompleteDefaults(t *testing.T) {
 		{"no image", func(d *SpecDefaults) { d.Image = "" }},
 		{"no checkout dir", func(d *SpecDefaults) { d.CheckoutDir = "" }},
 		{"no home dir", func(d *SpecDefaults) { d.HomeDir = "" }},
+		// NamePrefix is the operator-derived half of the container name, which
+		// becomes a path segment of the agent socket. A separator here escapes
+		// RuntimeDir through the same filepath.Join clean that validAccountID
+		// guards on the request-derived half, so both operands are checked.
+		{"a name prefix containing a path separator", func(d *SpecDefaults) { d.NamePrefix = "a/../../" }},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -170,6 +175,74 @@ func TestBuildSpecRejectsEmptyAgentAccountID(t *testing.T) {
 	}
 }
 
+// The agent account id becomes a path segment of the agent socket
+// (RuntimeDir/containers/<prefix><id>/agent.sock), and filepath.Join cleans, so
+// a "../" in the id escapes RuntimeDir: "../../../../tmp/pwned" resolves to
+// /run/tmp/pwned/agent.sock, where the Runner would MkdirAll a 0700 directory
+// and bind. The socket's length guard cannot catch this, because traversal
+// SHORTENS the path — every row here is well under the AF_UNIX cap. Each reject
+// must surface a non-nil error AND the zero AgentSpec, so no half-built spec
+// carries an escaping name to Launch. Uses a valid repo so the id is the only
+// failing dimension.
+func TestBuildSpecRejectsAgentAccountIDThatEscapesItsPathElement(t *testing.T) {
+	builder, err := NewConfigSpecBuilder(goodDefaults())
+	if err != nil {
+		t.Fatalf("NewConfigSpecBuilder: %v", err)
+	}
+	rejected := []struct {
+		name      string
+		accountID string
+	}{
+		{"parent traversal escaping the runtime dir", "../../../../tmp/pwned"},
+		{"shallow parent traversal", "../../etc"},
+		{"a single parent segment", ".."},
+		{"the current directory", "."},
+		{"an embedded separator", "abc/def"},
+		{"a leading separator (absolute)", "/etc/passwd"},
+		{"a trailing separator", "abc/"},
+		// Control and format characters clear every check above and, measured,
+		// pass MkdirAll and bind too (a NUL is the exception, failing at
+		// MkdirAll) — so without this guard they reach the container name and
+		// the logs that quote it. The C1 and bidi rows are the ones a predicate
+		// written as `r < 0x20 || r == 0x7f` would silently admit.
+		{"an embedded NUL", "abc\x00def"},
+		{"an embedded newline", "abc\ndef"},
+		{"an embedded DEL", "abc\x7fdef"},
+		{"an embedded C1 control", "abc\u0085def"},
+		{"an embedded bidi override", "abc\u202edef"},
+	}
+	for _, tc := range rejected {
+		t.Run(tc.name, func(t *testing.T) {
+			spec, err := builder.BuildSpec(&compassv1.ProvisionAgentWorkspaceRequest{
+				AgentAccountId: tc.accountID,
+				Repo:           &compassv1.ProvisionAgentWorkspaceRequest_RemoteUrl{RemoteUrl: "https://example.com/r.git"},
+			})
+			if err == nil {
+				t.Fatalf("BuildSpec with agent_account_id %q = nil error, want a path-element rejection", tc.accountID)
+			}
+			if !strings.Contains(err.Error(), "agent account id") {
+				t.Fatalf("BuildSpec error = %v, want the agent-account-id validation error (not a repo failure)", err)
+			}
+			if spec.Name != "" {
+				t.Fatalf("BuildSpec on rejection returned spec with name %q, want the zero AgentSpec", spec.Name)
+			}
+		})
+	}
+
+	// The ordinary minted shape still builds, so the guard refuses traversal
+	// rather than every id.
+	spec, err := builder.BuildSpec(&compassv1.ProvisionAgentWorkspaceRequest{
+		AgentAccountId: strings.Repeat("f", 32),
+		Repo:           &compassv1.ProvisionAgentWorkspaceRequest_RemoteUrl{RemoteUrl: "https://example.com/r.git"},
+	})
+	if err != nil {
+		t.Fatalf("BuildSpec with a 32-hex account id: %v", err)
+	}
+	if want := "compass-agent-" + strings.Repeat("f", 32); spec.Name != want {
+		t.Fatalf("BuildSpec name = %q, want %q", spec.Name, want)
+	}
+}
+
 // BuildSpec must constrain the caller-supplied repo value before it is forwarded
 // verbatim as the `git clone` target: an unconstrained remote_url reaches git's
 // `ext::<cmd>` (arbitrary command) / `file://` (read-outside-boundary) transports,