Skip to content

dev-sandbox: write-side gaps that survive the git-config denies #87967

Open

Description

@haacked

Problem

Review on #87021 surfaced four write-side gaps in the dev-sandbox Seatbelt profile (bin/dev-sandbox, bin/dev-sandbox.sb) that are real but out of scope for that PR, which is deliberately narrow (removes access, adds no new capability). Each is confirmed below — three by reading the render logic against the profile, one by running sandbox-exec directly against minimal reproduction profiles.

None of these are regressions from #87021; the branch still improves on master, which denied nothing in a worktree git dir and nothing in bin/dev-sandbox itself. They're pre-existing (or, for the shared-cache case, inherent to the caching design) and belong to a proper redesign rather than a patch bolted onto that PR.

Findings

1. Git-directory node replacement (worktree gitdir + repo root)

gitcfg_wdeny (bin/dev-sandbox:114-126) denies <gitdir>/config, config.worktree and commondir by literal path, but not the git-dir node itself (MAINREPO/.git/worktrees/<name>), nor its parent (MAINREPO/.git/worktrees). Separately, neither PROJECT nor MAINREPO themselves carry a write-deny — only paths nested under them do.

Confirmed empirically: a literal file-write* deny on a path blocks operations that directly target that exact path (rename it away, rename something onto it, os.rename a replacement over it). It does not block renaming an undenied ancestor directory and recreating a fresh tree underneath — the descendant paths never get individually checked during a container-level rename. So denying just the leaf files, or even just $gd itself, isn't sufficient: an attacker can swap any unprotected ancestor in the chain (.git/worktrees, or PROJECT/MAINREPO at the top) and take every literal beneath it along for free.

A correct fix needs the entire ancestor chain protected (PROJECT, MAINREPO, MAINREPO/.git/worktrees, and the gitdir node itself), not a single node. This is the same class of "hand-enumerated list vs. general enumeration" problem #79300 already tracks for git-config read-allows, extended to the write side and to non-git-config paths (the repo root itself).

The one-line fix suggested against the swap ("also deny $gd itself") does not close the route on its own — it was proposed, then corrected in a follow-up comment on the same PR once the ancestor chain above it was shown to be equally exploitable.

Discovered in: #87021 (comment), #87021 (comment), #87021 (comment), #87021 (comment)

2. Non-interactive flox activation resolves git through a sandboxed-writable PATH

.flox/env/on-activate.sh:230-231 prepends $UV_PROJECT_ENVIRONMENT/bin ($FLOX_ENV_CACHE/venv/bin, inside PROJECT and therefore sandboxed-writable per bin/dev-sandbox.sb:55) onto PATH for non-interactive activation, before the git config seed calls at lines 320-330 run — unsandboxed. A git binary planted there by a prior sandboxed run would execute with full developer privileges on the next non-interactive flox activate.

Same bug class #87021 fixes for bin/dev-sandbox's own PATH resolution, just a different, earlier call site that PR doesn't touch.

Discovered in: #87021 (comment)

3. Shared TMPDIR profile cache has no cross-checkout protection

render_profile (bin/dev-sandbox:196) write-denies only its own $rendered/$stamp, keyed by this checkout's cache key. TMPDIR is shared per-user, not per-checkout, so every other posthog-dev-sandbox-*.sb in the same TMPDIR is fully writable (nothing outside the enumerated $HOME/repo/Homebrew paths is write-denied). The cache-hit check only compares mtimes against bin/dev-sandbox/bin/dev-sandbox.sb, and the preflight only confirms the profile compiles — neither validates content or provenance.

A dependency sandboxed in checkout A can overwrite checkout B's cached profile with (allow default). The next launch in checkout B loads it without re-rendering or re-running the preflight, disabling the sandbox entirely for B — a strictly larger bypass than anything the git-config route reaches.

Discovered in: #87021 (comment)

Why deferred rather than patched into #87021

Findings 1 and 3 need new SBPL logic (ancestor-chain denies, and either a cache-family regex deny or a content/hash validation step) plus their own self-tests, in an area where #87021's own history already shows adversarial review finding new escapes introduced by rushed fixes in the same session. Finding 2 is narrower but still expands scope beyond bin/dev-sandbox itself. All three deserve their own careful design-and-test cycle rather than a patch appended to an already-landed PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions