fix(sandbox): pin the Landlock thread so a sandboxed MCP server cannot exec unconfined

.github/workflows/sandbox-integration.yml

@@ -80,6 +80,30 @@ jobs:
             ./internal/upstream/core/... \
             ./internal/security/scanner/...
 
+      # 4b. The escape this suite exists to catch is scheduler-dependent: the
+      #     Landlock domain is per-thread, so the bug only shows when the Go
+      #     runtime moves the goroutine off the confined thread between Apply and
+      #     execve. On an idle runner that never happens and every step above
+      #     passes even with the bug present. Saturate the CPUs first so the
+      #     enforcement tests are actually discriminating.
+      - name: Sandbox enforcement under scheduler load
+        timeout-minutes: 10
+        run: |
+          set -euo pipefail
+          set +m                      # no job-control messages for the load pack
+          pids=""
+          # shellcheck disable=SC2034
+          for _ in $(seq 1 "$(nproc)"); do
+            ( while :; do :; done ) &
+            pids="$pids $!"
+          done
+          # Kill the load pack however this step exits (pass, fail, or timeout)
+          # so a stuck busy loop can never outlive the step and hang the job.
+          # shellcheck disable=SC2064
+          trap "kill $pids 2>/dev/null || true" EXIT INT TERM
+          go test -race -count=30 -run 'TestSandboxWrapper_EndToEnd' ./internal/upstream/core/
+          go test -race -count=10 -run 'TestApplyPinsThreadAcrossReschedule|TestLandlockEnforcesFilesystemAllowlist' ./internal/sandbox/
+
       # 5. Build the binary (proves sandbox code compiles on linux/amd64).
       - name: Build mcpproxy binary
         run: go build -v -o mcpproxy ./cmd/mcpproxy

internal/sandbox/runchild_unix.go

@@ -22,6 +22,9 @@ import (
 // On success it never returns (execve replaces the image). It returns a non-zero
 // exit code on any failure; diag receives human-readable confinement notes and
 // errors (os.Stderr in production, so they land in the per-server upstream log).
+// Exit codes: 2 bad invocation or missing spec, 3 confinement unavailable while
+// fail-closed, 4 the Landlock domain's thread was lost before execve, 126 execve
+// failed, 127 target not found on PATH.
 func RunChild(argv []string, diag io.Writer) int {
 	if diag == nil {
 		diag = io.Discard
@@ -55,7 +58,7 @@ func RunChild(argv []string, diag io.Writer) int {
 		target = resolved
 	}
 
-	rep, err := Apply(spec)
+	rep, err := applyConfinement(spec)
 	if err != nil {
 		// fail-closed: BestEffort was false and the primitive is unavailable.
 		fmt.Fprintf(diag, "sandbox: confinement unavailable and fail-closed: %v\n", err)
@@ -65,13 +68,45 @@ func RunChild(argv []string, diag io.Writer) int {
 
 	dropPrivilegesBestEffort(diag)
 
+	// Everything between Apply and here — the diag write above, the privilege
+	// drop, os.Environ() — can park this goroutine. Apply pins it to the thread
+	// that carries the Landlock domain, so a mismatch here means that pin failed
+	// and an execve now would run the untrusted command unconfined. Refuse
+	// instead: an MCP server that does not start is recoverable, one that starts
+	// outside its sandbox is not.
+	if now := currentTID(); threadLost(rep, now) {
+		fmt.Fprintf(diag, "sandbox: refusing to exec %q — thread changed after confinement "+
+			"(Landlock domain is on tid %d, now running on tid %d); the command would run UNCONFINED\n",
+			target, rep.LandlockTID, now)
+		return 4
+	}
+
 	if err := syscall.Exec(target, argv, os.Environ()); err != nil {
 		fmt.Fprintf(diag, "sandbox: exec %q: %v\n", target, err)
 		return 126
 	}
 	return 0 // unreachable: Exec replaced the image on success.
 }
 
+// applyConfinement indirects Apply for RunChild. Production never reassigns it;
+// it exists so the test suite can drive RunChild's fail-closed thread guard,
+// whose refusal path is otherwise untestable: Apply is irreversible for the
+// calling thread (so the test process cannot call it for real) and a genuine
+// thread migration is a race that cannot be provoked on demand. Without the
+// seam, deleting the guard from RunChild — or moving it after syscall.Exec —
+// would leave every test still green.
+var applyConfinement = Apply
+
+// threadLost reports whether the Landlock domain Apply committed lives on a
+// different thread than the one now about to execve — in which case the exec
+// would run the untrusted command outside the domain entirely. A zero
+// LandlockTID means no domain was enforced (rlimits-only, or Landlock
+// unavailable under BestEffort), so there is nothing to verify and nothing to
+// lose by exec'ing.
+func threadLost(rep Report, nowTID int) bool {
+	return rep.LandlockTID != 0 && nowTID != rep.LandlockTID
+}
+
 // describeReport renders a one-line honest summary of what Apply enforced.
 func describeReport(rep Report) string {
 	switch {

internal/sandbox/runchild_unix_test.go

@@ -0,0 +1,126 @@
+//go:build unix
+
+package sandbox
+
+import (
+	"bytes"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+// TestThreadLost pins the fail-closed guard's decision table. RunChild consults
+// it immediately before execve: a true verdict must abort the exec, because
+// exec'ing from a thread outside the Landlock domain runs the untrusted command
+// with no confinement at all.
+func TestThreadLost(t *testing.T) {
+	cases := []struct {
+		name   string
+		rep    Report
+		nowTID int
+		want   bool
+	}{
+		{
+			name:   "same thread — safe to exec",
+			rep:    Report{LandlockABI: 5, LandlockTID: 4242},
+			nowTID: 4242,
+			want:   false,
+		},
+		{
+			name:   "thread changed after confinement — must refuse",
+			rep:    Report{LandlockABI: 5, LandlockTID: 4242},
+			nowTID: 4243,
+			want:   true,
+		},
+		{
+			name:   "no domain enforced (rlimits only) — nothing to verify",
+			rep:    Report{LandlockTID: 0},
+			nowTID: 4243,
+			want:   false,
+		},
+		{
+			name:   "Landlock unavailable under BestEffort — nothing to verify",
+			rep:    Report{LandlockABI: -1, LandlockTID: 0},
+			nowTID: 99,
+			want:   false,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			if got := threadLost(c.rep, c.nowTID); got != c.want {
+				t.Errorf("threadLost(%+v, %d) = %v, want %v", c.rep, c.nowTID, got, c.want)
+			}
+		})
+	}
+}
+
+// fakeConfinement makes RunChild's Apply step return rep, so the fail-closed
+// guard can be driven from a test process that must not actually confine itself.
+func fakeConfinement(t *testing.T, rep Report) {
+	t.Helper()
+	prev := applyConfinement
+	applyConfinement = func(Spec) (Report, error) { return rep, nil }
+	t.Cleanup(func() { applyConfinement = prev })
+}
+
+// unreachableTarget is an absolute path that does not exist. Absolute so
+// RunChild skips the PATH lookup and gets as far as the guard; non-existent so
+// that a RunChild which reached syscall.Exec fails with ENOENT (126) instead of
+// replacing this test binary's process image. The two failure modes are
+// therefore distinguishable: 4 means the guard refused before exec, 126 means it
+// did not.
+func unreachableTarget(t *testing.T) string {
+	t.Helper()
+	return filepath.Join(t.TempDir(), "never-exec-me")
+}
+
+// TestRunChildRefusesExecWhenThreadLost covers the guard where it actually
+// matters — inside RunChild, before execve. TestThreadLost alone would stay
+// green if the call were deleted from RunChild or moved below syscall.Exec, and
+// either edit re-opens the escape this whole change exists to close.
+func TestRunChildRefusesExecWhenThreadLost(t *testing.T) {
+	t.Setenv(EnvSpec, `{}`)
+	// currentTID()+1 is never the running thread, on Linux (real tid) or
+	// elsewhere (currentTID is a constant 0 off Linux).
+	fakeConfinement(t, Report{LandlockABI: 5, LandlockTID: currentTID() + 1})
+
+	target := unreachableTarget(t)
+	var diag bytes.Buffer
+	code := RunChild([]string{target}, &diag)
+
+	if code != 4 {
+		t.Fatalf("RunChild with a lost Landlock thread = %d, want 4 (refuse before execve); diag:\n%s",
+			code, diag.String())
+	}
+	if strings.Contains(diag.String(), "sandbox: exec ") {
+		t.Errorf("RunChild attempted the execve despite the lost thread; diag:\n%s", diag.String())
+	}
+	if !strings.Contains(diag.String(), "refusing to exec") || !strings.Contains(diag.String(), "UNCONFINED") {
+		t.Errorf("refusal must say what went wrong and why it matters; diag:\n%s", diag.String())
+	}
+}
+
+// TestRunChildExecsWhenThreadHeld is the other half of the pin: the guard must
+// not refuse when the domain is still on this thread, or it would block every
+// legitimate launch. Without this, "return 4 unconditionally" would pass the
+// test above.
+func TestRunChildExecsWhenThreadHeld(t *testing.T) {
+	t.Setenv(EnvSpec, `{}`)
+	// Hold the thread so the tid recorded here is still the tid RunChild sees.
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+	fakeConfinement(t, Report{LandlockABI: 5, LandlockTID: currentTID()})
+
+	target := unreachableTarget(t)
+	var diag bytes.Buffer
+	code := RunChild([]string{target}, &diag)
+
+	if code != 126 {
+		t.Fatalf("RunChild on the confined thread = %d, want 126 (execve reached, target absent); diag:\n%s",
+			code, diag.String())
+	}
+	if !strings.Contains(diag.String(), "sandbox: exec ") {
+		t.Errorf("expected the diag to show execve was attempted; diag:\n%s", diag.String())
+	}
+}

internal/sandbox/sandbox.go

@@ -12,11 +12,19 @@
 // recommendation and the honest limits (no uid/gid separation without
 // privilege, filesystem-allowlist + rlimits only).
 //
-// This package is intentionally minimal: it confines the *current* process and,
-// because Landlock domains are inherited across execve, every child it then
-// execs (the npx/uvx server and its descendants). The intended integration is a
-// tiny re-exec wrapper that calls Apply and then execs the untrusted command;
-// the package test exercises exactly that shape.
+// This package is intentionally minimal: it confines the *current thread* and,
+// because a Landlock domain is inherited across execve, every process that
+// thread then execs (the npx/uvx server and its descendants).
+//
+// The per-thread scope is load-bearing, not a footnote: landlock_restrict_self(2)
+// commits the new credentials on the calling thread only. A multithreaded Go
+// process therefore CANNOT be confined by calling Apply in-process — the other
+// threads keep full filesystem access, and even the calling goroutine may be
+// rescheduled onto an unrestricted thread. The only sound shape is the one this
+// package implements: a tiny re-exec wrapper that calls Apply and immediately
+// execs the untrusted command from the very thread Apply restricted. Apply pins
+// that thread (runtime.LockOSThread) and RunChild verifies the thread identity
+// again just before execve; the package test exercises exactly that shape.
 package sandbox
 
 import "errors"
@@ -74,6 +82,11 @@ type Report struct {
 	// NoNewPrivs reports whether PR_SET_NO_NEW_PRIVS was set (always true when
 	// Landlock is enforced; Landlock requires it).
 	NoNewPrivs bool
+	// LandlockTID is the OS thread id (Linux tid) the Landlock domain was
+	// committed on, or 0 when no domain was enforced. A caller about to execve
+	// must confirm it is still running on this thread: an execve from any other
+	// thread runs the target unconfined.
+	LandlockTID int
 }
 
 // wantsLandlock reports whether the spec asks for any filesystem confinement.

internal/sandbox/sandbox_linux.go

@@ -4,6 +4,7 @@ package sandbox
 
 import (
 	"fmt"
+	"runtime"
 	"strings"
 	"unsafe"
 
@@ -63,13 +64,27 @@ func Available() bool {
 	return err == nil && abi >= 1
 }
 
-// Apply confines the current process per spec. On success the calling process
-// — and every process it subsequently execs — can only touch the filesystem
-// subtrees in the allowlist, under the supplied rlimits. The restriction is
-// irreversible for the lifetime of the process, which is why the intended
-// caller is a short-lived re-exec wrapper that calls Apply and immediately
-// execs the untrusted command.
+// Apply confines the calling THREAD per spec (rlimits, being per-process, apply
+// process-wide). On success that thread — and every process it subsequently
+// execs — can only touch the filesystem subtrees in the allowlist. The
+// restriction is irreversible for the lifetime of the thread.
+//
+// Apply cannot confine a multithreaded Go process: landlock_restrict_self(2)
+// commits credentials on the calling thread alone, so every other thread stays
+// unrestricted. The only sound caller is a short-lived re-exec wrapper that
+// calls Apply and immediately execs the untrusted command from this same
+// thread — see RunChild, which re-checks the thread identity before execve.
 func Apply(spec Spec) (Report, error) {
+	// Landlock domains are per-THREAD (landlock_restrict_self(2) enforces on the
+	// calling thread). The Go runtime is free to move this goroutine to another
+	// OS thread at any preemption point, and an execve issued from a thread that
+	// never entered the domain would run the untrusted command completely
+	// unconfined — while the caller logs "Landlock enforced". Pin the goroutine
+	// to its thread for the rest of its life; deliberately never unlocked,
+	// because the intended caller execs immediately and the confinement is
+	// irreversible anyway.
+	runtime.LockOSThread()
+
 	var rep Report
 
 	// Resource limits first — cheap, and independent of Landlock availability.
@@ -136,6 +151,9 @@ func Apply(spec Spec) (Report, error) {
 	if err := landlockRestrictSelf(rulesetFD); err != nil {
 		return rep, fmt.Errorf("sandbox: landlock_restrict_self: %w", err)
 	}
+	// Record which thread now carries the domain so the caller can fail closed
+	// if it somehow finds itself elsewhere before execve.
+	rep.LandlockTID = currentTID()
 
 	rep.LandlockABI = abi
 	if len(missing) > 0 {
@@ -167,6 +185,10 @@ func addPathRule(rulesetFD int, path string, access uint64) (bool, error) {
 	return true, nil
 }
 
+// currentTID returns the caller's OS thread id — the identity a Landlock domain
+// is attached to, and therefore the thing a caller must re-check before execve.
+func currentTID() int { return unix.Gettid() }
+
 // --- raw syscall wrappers (x/sys/unix v0.46 ships the numbers/types but not
 // high-level helpers for these three syscalls) -----------------------------
 

internal/sandbox/sandbox_linux_test.go

@@ -25,8 +25,15 @@ const (
 )
 
 func TestMain(m *testing.M) {
-	if os.Getenv(envChild) == "1" {
+	// Landlock confinement is irreversible, so every child mode below must run
+	// in a throwaway re-exec of this binary rather than the test process.
+	switch {
+	case os.Getenv(envChild) == "1":
 		os.Exit(sandboxChild())
+	case os.Getenv(envEscapeChild) == "1":
+		os.Exit(escapeChild())
+	case os.Getenv(envTIDChild) == "1":
+		os.Exit(tidChild())
 	}
 	os.Exit(m.Run())
 }

internal/sandbox/sandbox_other.go

@@ -14,6 +14,10 @@ package sandbox
 // always false off Linux: Landlock is a Linux-only LSM.
 func Available() bool { return false }
 
+// currentTID has no meaningful answer off Linux, where no thread-scoped
+// confinement domain exists. RunChild treats 0 as "nothing to verify".
+func currentTID() int { return 0 }
+
 func Apply(spec Spec) (Report, error) {
 	// No filesystem allowlist requested → nothing to enforce, same as Linux.
 	if !spec.wantsLandlock() {