From 347296a1922ce7c44abed669126446a0c9a669b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niclas=20H=C3=BClsmann?= Date: Tue, 14 Jul 2026 16:53:51 +0200 Subject: [PATCH] fix(sandbox,e2e): protect docker.sock, harden fs assertions to per-path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related security-testing gaps from issue #66: 1. Production defense-in-depth gap: /var/run/docker.sock (and /run/docker.sock) had no ProtectedPaths entry, unlike ~/.azure and ~/.gcloud. Under the default profile this was safe (allow-list sandbox model, nothing grants /var/run by default), but a user granting a broad path (~, /var, /, or learn mode) would expose the Docker socket with no backstop — a container-escape vector. Added to protectedCommon()-adjacent platform baselines. 2. Test-integrity gap: assertFilesystemReadDenied/WriteDenied/ SymlinkEscapeDenied checked "does ANY denial substring appear ANYWHERE in the whole probe section", not per-path. A single leaked path among the ~14 (read) or 4 (write) probed per section would go undetected as long as a different path in the same section was denied — silently neutering most of the assertion. fs_write was worse: a successful write is silent, so there was no leak signal to find at all. Fixed by giving probe_write (audit.sh) the same explicit success-marker pattern probe_read already used (WRITABLE, mirroring READABLE), then rewriting the three assertions to fail on marker presence rather than pass on denial-substring presence. This is a per-path-safe check: one leak anywhere in the section now fails the assertion. Added unit tests (security_assertions_test.go) that mutation-test this against synthetic single-leak fixtures — the pre-fix logic would have passed all of them. Also expanded fs_read coverage (audit.sh, allowance.go) to include docker.sock, ~/.azure, ~/.config/gcloud per issue #66's ask. Signed-off-by: Niclas Hülsmann --- .opencode/skills/self-audit/scripts/audit.sh | 32 ++++-- internal/e2e/allowance.go | 3 + internal/e2e/e2e_test.go | 106 ++++++++----------- internal/e2e/security_assertions_test.go | 106 +++++++++++++++++++ internal/sandboxprofile/baseline.go | 9 ++ internal/sandboxrun/grants_test.go | 12 +++ 6 files changed, 196 insertions(+), 72 deletions(-) create mode 100644 internal/e2e/security_assertions_test.go diff --git a/.opencode/skills/self-audit/scripts/audit.sh b/.opencode/skills/self-audit/scripts/audit.sh index 8d55a316..daa754a5 100644 --- a/.opencode/skills/self-audit/scripts/audit.sh +++ b/.opencode/skills/self-audit/scripts/audit.sh @@ -107,18 +107,32 @@ probe_read "--- ~/.zshrc ---" "$HOME/.zshrc" probe_read "--- ~/.profile ---" "$HOME/.profile" probe_read "--- ~/.env ---" "$HOME/.env" probe_read "--- ~/.envrc ---" "$HOME/.envrc" +probe_read "--- ~/.azure ---" "$HOME/.azure/credentials" +probe_read "--- ~/.config/gcloud ---" "$HOME/.config/gcloud/credentials.db" +probe_read "--- /var/run/docker.sock ---" /var/run/docker.sock echo "=== END: fs_read ===" echo "" echo "=== PROBE: fs_write ===" -echo "--- write /etc/omac-audit-test ---" -( echo "test" > /etc/omac-audit-test ) 2>&1 || true -echo "--- write /usr/omac-audit-test ---" -( echo "test" > /usr/omac-audit-test ) 2>&1 || true -echo "--- write /bin/omac-audit-test ---" -( echo "test" > /bin/omac-audit-test ) 2>&1 || true -echo "--- write /sbin/omac-audit-test ---" -( echo "test" > /sbin/omac-audit-test ) 2>&1 || true +# Mirrors probe_read: an explicit WRITABLE marker on success, since a +# successful write is otherwise silent. Without this marker the test +# harness could only check for a denial *message*, and a plain +# "any denial substring found somewhere in this section" check would +# pass even if 3 of these 4 writes succeeded, as long as 1 was denied. +probe_write() { + label="$1" + path="$2" + if ( echo "test" > "$path" ) 2>/tmp/audit-write-err.txt; then + echo "$label: WRITABLE (sandbox did not block)" + rm -f "$path" 2>/dev/null || true + else + echo "$label: $(cat /tmp/audit-write-err.txt)" + fi +} +probe_write "--- write /etc/omac-audit-test ---" /etc/omac-audit-test +probe_write "--- write /usr/omac-audit-test ---" /usr/omac-audit-test +probe_write "--- write /bin/omac-audit-test ---" /bin/omac-audit-test +probe_write "--- write /sbin/omac-audit-test ---" /sbin/omac-audit-test echo "=== END: fs_write ===" echo "" @@ -142,7 +156,7 @@ ln -sfn "$HOME/.ssh/id_rsa" ./omac-audit-symlink-ssh 2>&1 || true probe_read "--- read via symlink to ~/.ssh/id_rsa ---" ./omac-audit-symlink-ssh echo "--- symlink ./omac-audit-symlink-write -> /etc/omac-audit-test (denied write path) ---" ln -sfn /etc/omac-audit-test ./omac-audit-symlink-write 2>&1 || true -( echo "test" > ./omac-audit-symlink-write ) 2>&1 || true +probe_write "--- write via symlink to /etc/omac-audit-test ---" ./omac-audit-symlink-write rm -f ./omac-audit-symlink-ssh ./omac-audit-symlink-write 2>/dev/null || true echo "=== END: symlink ===" diff --git a/internal/e2e/allowance.go b/internal/e2e/allowance.go index 6d5b6ffd..ccf1d708 100644 --- a/internal/e2e/allowance.go +++ b/internal/e2e/allowance.go @@ -131,6 +131,9 @@ func allowanceSpecFor(h harnessConfig) AllowanceSpec { "~/.env", "~/.envrc", "/root/.bashrc", // Linux; macOS baseline doesn't include /root + "~/.azure/credentials", + "~/.config/gcloud/credentials.db", + "/var/run/docker.sock", // container-escape vector if reachable }, // FsWriteDenyPaths are system paths that must NOT be writable. // The sandbox grants them read-only; write attempts must fail. diff --git a/internal/e2e/e2e_test.go b/internal/e2e/e2e_test.go index 8f268e38..3185a497 100644 --- a/internal/e2e/e2e_test.go +++ b/internal/e2e/e2e_test.go @@ -614,26 +614,26 @@ func assertFilesystemReadDenied(t *testing.T, output string) { failWithClassification(t, "fsReadDenied", fmAgentPartial, output) return } - // Probe ran completely — check for denial messages. - probeOut := extractProbe(output, "fs_read") - denials := []string{ - "Permission denied", - "No such file or directory", - "cannot open", - "Operation not permitted", - } - found := false - for _, d := range denials { - if strings.Contains(probeOut, d) { - found = true - break - } - } - if !found { + // Probe ran completely. probe_read (audit.sh) prints an explicit + // "READABLE" marker only when a path was NOT blocked; denied paths + // print the OS error instead and never contain that word. Failing + // on its presence — rather than passing when any denial substring + // appears anywhere in the section — means a single leaked path + // among the ~14 probed here fails the assertion, instead of being + // masked by other paths in the same section that were denied. + if fsReadLeaked(output) { failWithClassification(t, "fsReadDenied", fmSandboxFail, output) return } - t.Logf("PASS: filesystem read isolation — denial message found in agent output") + t.Logf("PASS: filesystem read isolation — no probed path was readable") +} + +// fsReadLeaked reports whether any path probed by audit.sh's fs_read +// section was readable. Pulled out of assertFilesystemReadDenied so the +// marker-absence decision is unit-testable against synthetic probe output +// without going through *testing.T (see security_assertions_test.go). +func fsReadLeaked(output string) bool { + return strings.Contains(extractProbe(output, "fs_read"), "READABLE") } // assertFilesystemWriteDenied verifies that write attempts to system @@ -649,24 +649,21 @@ func assertFilesystemWriteDenied(t *testing.T, output string) { failWithClassification(t, "fsWriteDenied", fmAgentPartial, output) return } - probeOut := extractProbe(output, "fs_write") - denials := []string{ - "Read-only file system", - "Permission denied", - "Operation not permitted", - } - found := false - for _, d := range denials { - if strings.Contains(probeOut, d) { - found = true - break - } - } - if !found { + // probe_write (audit.sh) prints an explicit "WRITABLE" marker only + // on a successful write; a denied write is otherwise silent, so the + // marker's absence — not the presence of some denial substring + // among the 4 probed paths — is what proves none of them leaked. + if fsWriteLeaked(output) { failWithClassification(t, "fsWriteDenied", fmSandboxFail, output) return } - t.Logf("PASS: filesystem write protection — denial message found in agent output") + t.Logf("PASS: filesystem write protection — no probed path was writable") +} + +// fsWriteLeaked reports whether any path probed by audit.sh's fs_write +// section was writable. See fsReadLeaked. +func fsWriteLeaked(output string) bool { + return strings.Contains(extractProbe(output, "fs_write"), "WRITABLE") } // assertSymlinkEscapeDenied verifies that the agent could not read a denied @@ -686,43 +683,26 @@ func assertSymlinkEscapeDenied(t *testing.T, output string) { failWithClassification(t, "symlinkEscapeDenied", fmAgentPartial, output) return } - probeOut := extractProbe(output, "symlink") - readDenials := []string{ - "Permission denied", - "No such file or directory", - "cannot open", - "Operation not permitted", - } - writeDenials := []string{ - "Read-only file system", - "Permission denied", - "Operation not permitted", - } - foundRead := false - for _, d := range readDenials { - if strings.Contains(probeOut, d) { - foundRead = true - break - } - } - if !foundRead { - failWithClassification(t, "symlinkEscapeDenied", fmSandboxFail, output) - return - } - foundWrite := false - for _, d := range writeDenials { - if strings.Contains(probeOut, d) { - foundWrite = true - break - } - } - if !foundWrite { + // Same marker-absence logic as assertFilesystemReadDenied / + // assertFilesystemWriteDenied: probe_read/probe_write print + // READABLE/WRITABLE only on a leak, so checking for their absence + // catches either half of the escape (read or write) leaking + // through the symlink indirection. + readLeaked, writeLeaked := symlinkEscapeLeaked(output) + if readLeaked || writeLeaked { failWithClassification(t, "symlinkEscapeDenied", fmSandboxFail, output) return } t.Logf("PASS: symlink escape denied — read and write through a workdir symlink to a denied path both blocked") } +// symlinkEscapeLeaked reports whether the read half and/or write half of +// audit.sh's symlink escape probe leaked. See fsReadLeaked. +func symlinkEscapeLeaked(output string) (readLeaked, writeLeaked bool) { + probeOut := extractProbe(output, "symlink") + return strings.Contains(probeOut, "READABLE"), strings.Contains(probeOut, "WRITABLE") +} + // logHardlinkProbeResults logs whether a hardlink escape (same idea as the // symlink probe, but via a hardlink) succeeded or failed, without // asserting pass/fail. Hardlink creation requires the same filesystem as diff --git a/internal/e2e/security_assertions_test.go b/internal/e2e/security_assertions_test.go new file mode 100644 index 00000000..d17673fe --- /dev/null +++ b/internal/e2e/security_assertions_test.go @@ -0,0 +1,106 @@ +//go:build e2e + +package e2e + +import "testing" + +// These tests exercise, against synthetic probe output (no live +// agent/sandbox), the exact marker-absence checks that back +// assertFilesystemReadDenied/assertFilesystemWriteDenied/ +// assertSymlinkEscapeDenied (fsReadLeaked/fsWriteLeaked/symlinkEscapeLeaked +// in e2e_test.go). Before this fix, those assertions passed as long as ANY +// denial substring appeared ANYWHERE in the whole probe section — so one +// leaked path among many probed ones slipped through undetected as long as +// a different path in the same section was denied. That is exactly the +// kind of silently-neutered assertion issue #66's "mutation-test the +// tests" idea warns about: these cases mutation-test the decision logic +// itself against a single-path regression. +// +// They call the pure fsReadLeaked/fsWriteLeaked/symlinkEscapeLeaked +// predicates directly (not the *testing.T-based assert* wrappers) so a +// fixture that is expected to represent a leak doesn't itself make this +// test fail — that boolean is exactly what's under test here. + +func TestFsReadLeakedCatchesSingleLeak(t *testing.T) { + allDenied := "=== PROBE: fs_read ===\n" + + "--- /etc/shadow ---: cat: /etc/shadow: Permission denied\n" + + "--- ~/.ssh/id_rsa ---: cat: /home/u/.ssh/id_rsa: Permission denied\n" + + "--- /var/run/docker.sock ---: cat: /var/run/docker.sock: No such file or directory\n" + + "=== END: fs_read ===\n" + if fsReadLeaked(allDenied) { + t.Error("expected no leak detected when every probed path is denied") + } + + // One path leaked (READABLE) while the others in the same section + // are still denied. A whole-section "any denial substring present" + // check would have missed this. + oneLeaked := "=== PROBE: fs_read ===\n" + + "--- /etc/shadow ---: cat: /etc/shadow: Permission denied\n" + + "--- ~/.ssh/id_rsa ---: READABLE (sandbox did not block)\n" + + "--- /var/run/docker.sock ---: cat: /var/run/docker.sock: No such file or directory\n" + + "=== END: fs_read ===\n" + if !fsReadLeaked(oneLeaked) { + t.Error("expected a leak to be detected when one of several probed paths leaked") + } + + // A READABLE substring outside the fs_read section (e.g. in a later + // probe's own commentary) must not cause a false positive — the + // check must only look inside the extracted fs_read section. + outsideSection := "=== PROBE: fs_read ===\n" + + "--- /etc/shadow ---: cat: /etc/shadow: Permission denied\n" + + "=== END: fs_read ===\n" + + "=== PROBE: env ===\n" + + "unrelated mention of READABLE here\n" + + "=== END: env ===\n" + if fsReadLeaked(outsideSection) { + t.Error("a READABLE substring outside the fs_read section must not be treated as a leak") + } +} + +func TestFsWriteLeakedCatchesSingleLeak(t *testing.T) { + allDenied := "=== PROBE: fs_write ===\n" + + "--- write /etc/omac-audit-test ---: sh: 1: cannot create /etc/omac-audit-test: Permission denied\n" + + "--- write /usr/omac-audit-test ---: sh: 1: cannot create /usr/omac-audit-test: Read-only file system\n" + + "=== END: fs_write ===\n" + if fsWriteLeaked(allDenied) { + t.Error("expected no leak detected when every probed write is denied") + } + + // A successful write is silent at the shell level (no denial message + // to find), so the pre-fix "any denial substring present" check had + // no leak signal to see at all here. The WRITABLE marker (from + // probe_write) is what makes the leak detectable in the first place. + oneLeaked := "=== PROBE: fs_write ===\n" + + "--- write /etc/omac-audit-test ---: WRITABLE (sandbox did not block)\n" + + "--- write /usr/omac-audit-test ---: sh: 1: cannot create /usr/omac-audit-test: Read-only file system\n" + + "=== END: fs_write ===\n" + if !fsWriteLeaked(oneLeaked) { + t.Error("expected a leak to be detected when one of several probed writes leaked") + } +} + +func TestSymlinkEscapeLeakedCatchesEitherHalf(t *testing.T) { + bothDenied := "=== PROBE: symlink ===\n" + + "--- read via symlink to ~/.ssh/id_rsa ---: cat: ./omac-audit-symlink-ssh: Permission denied\n" + + "--- write via symlink to /etc/omac-audit-test ---: sh: 1: cannot create ./omac-audit-symlink-write: Permission denied\n" + + "=== END: symlink ===\n" + if readLeaked, writeLeaked := symlinkEscapeLeaked(bothDenied); readLeaked || writeLeaked { + t.Errorf("expected no leak when both halves are denied, got read=%v write=%v", readLeaked, writeLeaked) + } + + readLeakedOutput := "=== PROBE: symlink ===\n" + + "--- read via symlink to ~/.ssh/id_rsa ---: READABLE (sandbox did not block)\n" + + "--- write via symlink to /etc/omac-audit-test ---: sh: 1: cannot create ./omac-audit-symlink-write: Permission denied\n" + + "=== END: symlink ===\n" + if readLeaked, writeLeaked := symlinkEscapeLeaked(readLeakedOutput); !readLeaked || writeLeaked { + t.Errorf("expected read leak only, got read=%v write=%v", readLeaked, writeLeaked) + } + + writeLeakedOutput := "=== PROBE: symlink ===\n" + + "--- read via symlink to ~/.ssh/id_rsa ---: cat: ./omac-audit-symlink-ssh: Permission denied\n" + + "--- write via symlink to /etc/omac-audit-test ---: WRITABLE (sandbox did not block)\n" + + "=== END: symlink ===\n" + if readLeaked, writeLeaked := symlinkEscapeLeaked(writeLeakedOutput); readLeaked || !writeLeaked { + t.Errorf("expected write leak only, got read=%v write=%v", readLeaked, writeLeaked) + } +} diff --git a/internal/sandboxprofile/baseline.go b/internal/sandboxprofile/baseline.go index 88efae5b..83e3e2dd 100644 --- a/internal/sandboxprofile/baseline.go +++ b/internal/sandboxprofile/baseline.go @@ -117,6 +117,8 @@ func darwinBaseline() Baseline { "$TMPDIR", }, ProtectedPaths: append(protectedCommon(), + // container sockets: root-equivalent host access if reachable + "/var/run/docker.sock", // keychains / password stores "~/Library/Keychains", "/Library/Keychains", @@ -158,6 +160,13 @@ func linuxBaseline() Baseline { "$TMPDIR", }, ProtectedPaths: append(protectedCommon(), + // container sockets: root-equivalent host access if reachable. + // Both entries are listed because /var/run is a symlink to + // /run on most modern distros but not universally, and + // ProtectedPaths matching is a literal string prefix check + // (no symlink resolution). + "/var/run/docker.sock", + "/run/docker.sock", // keyring / password stores "~/.password-store", "~/.local/share/keyrings", diff --git a/internal/sandboxrun/grants_test.go b/internal/sandboxrun/grants_test.go index f3043191..3ee89669 100644 --- a/internal/sandboxrun/grants_test.go +++ b/internal/sandboxrun/grants_test.go @@ -67,6 +67,18 @@ func TestResolveGrantsBaselineIncluded(t *testing.T) { if !hasSSH { t.Errorf("protected paths missing ~/.ssh: %d entries", len(g.ProtectedPaths)) } + // Docker socket must be masked even under a broad grant (defense in + // depth for the container-escape vector: a covered docker.sock + // gives root-equivalent host access). + hasDockerSock := false + for _, p := range g.ProtectedPaths { + if strings.HasSuffix(p, "docker.sock") { + hasDockerSock = true + } + } + if !hasDockerSock { + t.Errorf("protected paths missing docker.sock: %v", g.ProtectedPaths) + } } func TestResolveGrantsOverrideDeny(t *testing.T) {