diff --git a/.github/workflows/sandbox-integration.yml b/.github/workflows/sandbox-integration.yml index 5bbe367f..367df9bb 100644 --- a/.github/workflows/sandbox-integration.yml +++ b/.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 diff --git a/internal/sandbox/runchild_unix.go b/internal/sandbox/runchild_unix.go index 5e98b9fd..3c282786 100644 --- a/internal/sandbox/runchild_unix.go +++ b/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,6 +68,19 @@ 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 @@ -72,6 +88,25 @@ func RunChild(argv []string, diag io.Writer) int { 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 { diff --git a/internal/sandbox/runchild_unix_test.go b/internal/sandbox/runchild_unix_test.go new file mode 100644 index 00000000..1f736acf --- /dev/null +++ b/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()) + } +} diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index a353dd90..c4f5b719 100644 --- a/internal/sandbox/sandbox.go +++ b/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. diff --git a/internal/sandbox/sandbox_linux.go b/internal/sandbox/sandbox_linux.go index 897fa5ff..c68705a7 100644 --- a/internal/sandbox/sandbox_linux.go +++ b/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) ----------------------------- diff --git a/internal/sandbox/sandbox_linux_test.go b/internal/sandbox/sandbox_linux_test.go index 3ebfaab2..046d3c4d 100644 --- a/internal/sandbox/sandbox_linux_test.go +++ b/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()) } diff --git a/internal/sandbox/sandbox_other.go b/internal/sandbox/sandbox_other.go index 8865040c..b0492c1b 100644 --- a/internal/sandbox/sandbox_other.go +++ b/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() { diff --git a/internal/sandbox/threadpin_linux_test.go b/internal/sandbox/threadpin_linux_test.go new file mode 100644 index 00000000..24f270d6 --- /dev/null +++ b/internal/sandbox/threadpin_linux_test.go @@ -0,0 +1,235 @@ +//go:build linux + +package sandbox + +import ( + "bytes" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync/atomic" + "syscall" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +// Child-process protocol for the thread-pinning regression (see escapeChild). +const ( + envEscapeChild = "MCPPROXY_SANDBOX_TEST_ESCAPE_CHILD" + envTIDChild = "MCPPROXY_SANDBOX_TEST_TID_CHILD" + envOutsideDir = "MCPPROXY_SANDBOX_TEST_OUTSIDE" +) + +// escapeChild reproduces, deliberately, the shape that let an untrusted command +// run UNCONFINED even though Apply reported "Landlock enforced": +// +// Apply → (goroutine parks and may resume on a different OS thread) → execve +// +// Landlock domains are per-THREAD: landlock_restrict_self(2) commits the new +// credentials on `current` only. The production wrapper does a blocking diag +// write, a privilege drop and os.Environ() between Apply and syscall.Exec, each +// of which can park the goroutine; on a loaded machine the Go scheduler then +// resumes it on an OS thread that never entered the Landlock domain, and the +// execve from that thread runs the untrusted command with no confinement at all. +// +// Here we reproduce that window deliberately (see provokeThreadMigration), +// turning a load-dependent race into a near-certain one. Apply must pin the +// thread so the exec is unconditionally confined. +func escapeChild() int { + rwDir := os.Getenv(envRWDir) + outside := os.Getenv(envOutsideDir) + + spec := Spec{ + // RO "/" so /bin/sh and the loader stay reachable; the only writable + // subtrees are rwDir and /dev. outside is under neither. + ReadOnlyPaths: []string{"/"}, + ReadWritePaths: []string{rwDir, "/dev"}, + } + rep, err := Apply(spec) + if err != nil { + os.Stderr.WriteString("escape-child: Apply failed: " + err.Error() + "\n") + return 12 + } + if rep.LandlockABI < 1 { + os.Stderr.WriteString("escape-child: Landlock not enforced: " + rep.LandlockNote + "\n") + return 12 + } + + provokeThreadMigration() + + // Exec directly rather than through RunChild: this test must exercise the + // thread pin itself, not the fail-closed tid guard that backstops it. + // + // The exec'd shell brackets the escape attempt with two markers written to + // rwDir, which IS inside the allowlist and so is writable whether or not the + // pin held. reachedMarker proves the shell actually started; doneMarker + // proves it got all the way past the outside-write attempt. Without them the + // parent could only observe the absence of escaped.txt, which any unrelated + // failure (exec error, missing shell, child killed) would also produce — a + // broken pin and a broken test environment would look identical. + script := "echo ran > " + shellQuoteTest(filepath.Join(rwDir, reachedMarker)) + "\n" + + "echo escaped > " + shellQuoteTest(filepath.Join(outside, "escaped.txt")) + "\n" + + "echo done > " + shellQuoteTest(filepath.Join(rwDir, doneMarker)) + "\n" + err = syscall.Exec("/bin/sh", []string{"/bin/sh", "-c", script}, os.Environ()) + os.Stderr.WriteString("escape-child: exec failed: " + err.Error() + "\n") + return 126 +} + +// Markers the exec'd shell writes inside the read-write allowlist, so the parent +// can tell "confined, as intended" apart from "never ran". +const ( + reachedMarker = "child-ran.txt" + doneMarker = "child-done.txt" +) + +// TestApplyPinsThreadAcrossReschedule is the regression guard for the per-thread +// Landlock escape. Each iteration re-execs this test binary into escapeChild, +// which confines itself, deliberately invites a thread migration, and then execs +// a shell that writes OUTSIDE the allowlist. A single surviving write is a +// security failure, so the tolerance is zero. +// +// Every iteration also demands the two in-allowlist markers the shell writes +// around that attempt, so an iteration cannot pass by simply failing early. +func TestApplyPinsThreadAcrossReschedule(t *testing.T) { + if !Available() { + t.Skip("Landlock unavailable on this kernel (needs 5.13+ with Landlock LSM enabled)") + } + + const iterations = 20 + escapes := 0 + for i := 0; i < iterations; i++ { + rwDir := t.TempDir() + outside := t.TempDir() + + cmd := exec.Command(os.Args[0]) //nolint:gosec // re-exec of this test binary by design + cmd.Env = append(os.Environ(), + envEscapeChild+"=1", + envRWDir+"="+rwDir, + envOutsideDir+"="+outside, + ) + var errb bytes.Buffer + cmd.Stderr = &errb + runErr := cmd.Run() + + // exit 12 means the child could not confine itself at all — that is a + // broken test environment, not a pass. + var exitErr *exec.ExitError + if errors.As(runErr, &exitErr) && exitErr.ExitCode() == 12 { + t.Fatalf("iteration %d: child could not enforce Landlock:\n%s", i, errb.String()) + } + + // Positive proof first: the absence of escaped.txt below only means + // "Landlock denied the write" if the shell really ran and really tried. + // Both markers live inside the allowlist, so a working pin does not stop + // them; only a child that never got there does. + for _, marker := range []string{reachedMarker, doneMarker} { + if _, err := os.Stat(filepath.Join(rwDir, marker)); err != nil { + t.Fatalf("iteration %d: exec'd shell never wrote %s inside the allowlist (%v) — "+ + "the child did not reach the escape attempt, so this iteration proves nothing; "+ + "child exit: %v; child stderr:\n%s", i, marker, err, runErr, errb.String()) + } + } + + escaped := filepath.Join(outside, "escaped.txt") + if _, err := os.Stat(escaped); err == nil { + escapes++ + t.Errorf("iteration %d: command exec'd after Apply wrote OUTSIDE the allowlist (%s) — "+ + "it ran unconfined; child stderr:\n%s", i, escaped, errb.String()) + } + } + if escapes > 0 { + t.Fatalf("%d/%d exec'd commands escaped the Landlock domain; want 0", escapes, iterations) + } +} + +// TestApplyReportsRestrictedThread pins the fail-closed guard's input: Apply +// must report the tid it actually confined, and must still be on that thread +// when it returns. +func TestApplyReportsRestrictedThread(t *testing.T) { + if !Available() { + t.Skip("Landlock unavailable on this kernel (needs 5.13+ with Landlock LSM enabled)") + } + // Apply is irreversible for the calling thread, so this assertion has to run + // in the same throwaway child machinery as the enforcement tests. + rwDir := t.TempDir() + outside := t.TempDir() + cmd := exec.Command(os.Args[0]) //nolint:gosec // re-exec of this test binary by design + cmd.Env = append(os.Environ(), + envTIDChild+"=1", + envRWDir+"="+rwDir, + envOutsideDir+"="+outside, + ) + var errb bytes.Buffer + cmd.Stderr = &errb + if err := cmd.Run(); err != nil { + t.Fatalf("tid child failed: %v\nchild stderr:\n%s", err, errb.String()) + } +} + +// tidChild asserts Report.LandlockTID is populated and still matches the running +// thread after a deliberate reschedule window. +func tidChild() int { + rwDir := os.Getenv(envRWDir) + rep, err := Apply(Spec{ReadOnlyPaths: []string{"/"}, ReadWritePaths: []string{rwDir}}) + if err != nil { + os.Stderr.WriteString("tid-child: Apply failed: " + err.Error() + "\n") + return 12 + } + if rep.LandlockTID == 0 { + os.Stderr.WriteString("tid-child: Report.LandlockTID not populated after enforcement\n") + return 11 + } + provokeThreadMigration() + + if now := currentTID(); now != rep.LandlockTID { + os.Stderr.WriteString("tid-child: thread moved after Apply: confined tid differs from current\n") + return 10 + } + return 0 +} + +func shellQuoteTest(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } + +// provokeThreadMigration parks the calling goroutine in a blocking syscall while +// every P is saturated. That is precisely the mechanism by which the production +// wrapper used to lose its Landlock-restricted thread: sysmon retakes the P +// during the blocking call, and on exitsyscall the M cannot reacquire one, so +// the goroutine is handed to a different M — a different OS thread, outside the +// domain. +// +// The syscall shape is load-bearing. Measured on a 14-CPU Linux 6.12 kernel over +// 50 unpinned runs each: a blocking pipe read moved the goroutine 50/50 times, +// while a plain time.Sleep of the same duration moved it 0/50 (the runtime hands +// a sleeping goroutine back to the M it parked on). A sleep-based version of +// this test would be a vacuous guard. +func provokeThreadMigration() { + var fds [2]int + if err := unix.Pipe(fds[:]); err != nil { + return + } + defer unix.Close(fds[0]) + defer unix.Close(fds[1]) + + // A genuine spin (not a yielding loop) is what keeps every P occupied, so + // the blocking read below cannot get its P back on exitsyscall. + var stop atomic.Bool + for i := 0; i < runtime.NumCPU()*4; i++ { + go func() { + for !stop.Load() { //nolint:revive // deliberate busy-wait: saturating every P is the point + } + }() + } + defer stop.Store(true) + + go func() { + time.Sleep(2 * time.Millisecond) + _, _ = unix.Write(fds[1], []byte{'x'}) + }() + buf := make([]byte, 1) + _, _ = syscall.Read(fds[0], buf) +} diff --git a/internal/sandbox/wrap.go b/internal/sandbox/wrap.go index b99f9e20..3755abc2 100644 --- a/internal/sandbox/wrap.go +++ b/internal/sandbox/wrap.go @@ -8,10 +8,11 @@ import ( // Re-exec wrapper protocol (MCP-34.3). // -// Landlock confines the *current* process and every process it then execs, and -// the confinement is irreversible — so it cannot be applied in-process before -// mcp-go spawns an upstream stdio server. The integration is therefore a tiny -// re-exec wrapper: mcpproxy launches itself as +// Landlock confines the *calling thread* and every process that thread then +// execs, and the confinement is irreversible — so it cannot be applied +// in-process before mcp-go spawns an upstream stdio server (mcpproxy is +// multithreaded; the other threads would stay unrestricted). The integration is +// therefore a tiny re-exec wrapper: mcpproxy launches itself as // // mcpproxy __sandbox_exec -- [args...] // diff --git a/internal/upstream/core/sandbox_linux_test.go b/internal/upstream/core/sandbox_linux_test.go index 7b10ce4c..1e23fd10 100644 --- a/internal/upstream/core/sandbox_linux_test.go +++ b/internal/upstream/core/sandbox_linux_test.go @@ -41,6 +41,16 @@ func TestSandboxWrapper_EndToEnd(t *testing.T) { BestEffort: false, // fail-closed: this test requires real enforcement } + // Assertion (3b) below is only meaningful if `outside` really is outside the + // write allowlist. t.TempDir() layouts are not contractual, so pin it here: + // a future change that put both dirs under one parent would silently turn + // the denial check into a vacuous pass. + for _, allowed := range spec.ReadWritePaths { + if isUnder(outside, allowed) { + t.Fatalf("test setup is vacuous: outside dir %q lies under the read-write allowlist entry %q", outside, allowed) + } + } + // Script: echo stdin back (passthrough), report the fd limit, write inside // the allowlist, then try to write OUTSIDE it (must fail). script := fmt.Sprintf(` @@ -119,6 +129,27 @@ func TestSandboxWrapper_FailClosed(t *testing.T) { func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } +// isUnder reports whether path is parent itself or lives beneath it, comparing +// symlink-resolved forms — Landlock rules apply to the resolved subtree, so a +// purely lexical comparison could miss an overlap (e.g. /tmp → /private/tmp). +func isUnder(path, parent string) bool { + resolve := func(p string) string { + if r, err := filepath.EvalSymlinks(p); err == nil { + return filepath.Clean(r) + } + return filepath.Clean(p) + } + path, parent = resolve(path), resolve(parent) + if path == parent { + return true + } + rel, err := filepath.Rel(parent, path) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + func scrubEnv(env []string, key string) []string { out := env[:0:0] for _, e := range env {