fix(sandbox): make greyproxy's credential files unreadable inside the Linux sandbox#103
Open
kennedyjosh wants to merge 6 commits into
Open
Conversation
… deny-write) On Linux, greyproxy's private state was left READABLE inside the sandbox, defeating credential protection. Three defects combined: 1. denyRead clobber. getMandatoryDenyPaths() routed the greyproxy secrets (session.key, ca-key.pem) through the deny-WRITE idiom `--ro-bind <realfile> <realfile>`, which keeps a path read-only but still READABLE. Emitted after the denyRead `/dev/null` mask, it clobbered the mask (in bubblewrap the later mount at a path wins), so the real key bytes were re-exposed for reading. 2. Wholesale data-dir bind. The generic ~/.local home-cache bind exposed the entire ~/.local/share/greyproxy directory, including greyproxy.db (the encrypted placeholder->real-value mappings) and the CA private key. 3. --watch bypass. WrapCommandLinuxWithOptions skips the whole deny/mask block under --watch, but credential substitution stays active in watch mode (disabled only in --learning), so the secrets remained readable there too. Together an untrusted agent could read session.key (AES-256) + greyproxy.db and decrypt the real upstream credential entirely offline, in-sandbox. Fix: - Stop routing the greyproxy secrets through getMandatoryDenyPaths() (the deny-WRITE set). Secrets need deny-READ treatment. - Add greyproxyDenyReadArgs(): replace each greyproxy data dir with an empty tmpfs, mask each sensitive file with an empty /dev/null bind, and re-expose ONLY the public ca-cert.pem read-only for TLS trust. Emit these mounts LAST so bubblewrap's "last mount wins" keeps the mask from being clobbered. - Apply the mask whenever substitution can be active (gate on `!opts.Learning`, outside the `!Learning && !Watch` block), so --watch is covered too. This mirrors the macOS Seatbelt backend, which already emits `(deny file-read-data ...)` for these paths (macos.go) and does not wholesale-expose the data directory. Credential injection is unaffected: the sandbox still sees only the placeholder, trusts the CA cert, and greyproxy injects the real key on egress. Adds a regression test that drives the full wrapper in BOTH enforcing and watch mode, asserting the sensitive files/dir/db are masked and never re-exposed (no clobbering bind after the mask) while ca-cert.pem stays readable, plus that the secrets are excluded from the mandatory deny-write set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the multi-model review of 5af4b3c so the credential-leak fix is sound, not just directionally right: - linux.go: emit greyproxyDenyReadArgs() as the FINAL filesystem-mount block (before the "--" separator), so "last mount wins" is guaranteed rather than incidental. A bridge socket path placed inside the greyproxy data dir could otherwise re-clobber the mask via a later --bind. Invariant documented. - credentials.go: resolve XDG_DATA_HOME independently of HOME (no fail-open when HOME is unset but XDG is set), and derive SensitiveGreyproxyFiles from SensitiveGreyproxyDirs so per-file and directory masks can't drift. - dangerous.go/macos.go: drop the GetSensitiveSystemPaths alias (the conflated deny-set shape that caused the original bug); macos.go calls SensitiveGreyproxyFiles directly. - linux.go: document the known symlink limitation (masks act on logical paths). - test: parse the generated command into structured mounts; add hostile-bridge cases (reverse bridge socket dir == greyproxy data dir) in enforcing and --watch; replace the hardcoded ancestor allowlist with a generic ancestor scan; cover XDG_DATA_HOME; add a skip-gated real-bwrap runtime test. The hostile-bridge assertions are mutation-verified (reverting the mask to its non-last position fails them). Validated on a real Linux/bubblewrap toolchain: go build, go vet, gofmt clean; full greyproxy deny-read suite passes; runtime test skips unprivileged and passes privileged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cut ~half the comment lines added in the hardening pass — keep the non-obvious "why" (last-mount-wins ordering, XDG fail-open, symlink limitation) and let the readable code speak for the rest. No behavior change; go build/vet/gofmt clean, greyproxy deny-read suite still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per a codex convention review: - use the existing skipIfAlreadySandboxed / skipIfCommandNotFound helpers (integration_test.go) instead of a hand-rolled skip block; - use slices.Contains (as dangerous_test.go does) instead of a local contains helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…deny-read CI's make fmt-check (gofumpt) and make lint (golangci-lint v2 w/ gosec): - gofumpt: split the multi-line append() first arg onto its own line; - gosec G301: tighten test data dir perms 0o755 -> 0o750; - gosec G204: //nolint on the intentional subprocess that runs the greywall-generated sandbox command in the runtime test (matches the existing benchmark_test.go convention). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Consolidate the two standalone XDG tests into a 5th case of the main table (enforcing_xdg_data_home); the wrapper case already guards the path-helper regression, so the separate unit test was redundant. - Trim the deny-read doc + call-site comments toward the file's median comment density, keeping the load-bearing "why" (deny-read vs deny-write, mask must be emitted last, symlink limitation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Linux, the files greyproxy uses to protect credentials were readable from
inside the sandbox. A process running in the sandbox could read the session key
and the encrypted credential store and reconstruct the real upstream credential
entirely offline — without ever talking to greyproxy. That undermines the point
of running untrusted code in the sandbox in the first place.
The sandbox did try to hide those files, but it mounted them read-only instead
of unreadable — and that mount was applied after the step meant to hide them, so
it silently undid the protection. macOS was unaffected; it already blocks reads
on these paths.
Fix
Hide greyproxy's private data properly: replace its data directory with an empty
one inside the sandbox and block each secret file, leaving only the public CA
certificate the sandbox legitimately needs for TLS. This masking is now the last
filesystem step, so nothing applied afterward can re-expose the files, and it
also covers
--watchmode. Credential injection and the network egress rules areunchanged.
Testing
Added tests that confirm the secrets are unreadable — including one that stands
up a real sandbox and tries to read each file from inside it — and that
specifically guard the ordering, so a future change that re-exposed the files
would fail.
make fmtandmake lintare clean.