From b4068209809315ab1578c78e33e73512b29749ca Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 5 Nov 2026 01:21:26 -0800 Subject: [PATCH] fix(land): merged_checks can't read land's environment through /proc (GST-1421) (#4032) * fix(land): close land's /proc environment to merged_checks (GST-1421) A merged check could read land's exec-time environment (GitHub token included) through /proc/$PPID/environ or a /proc scan. On Linux land now makes itself non-dumpable before the first check and refuses to run them when it cannot. macOS (ps -E) is documented as still readable. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): GST-1421 fragment Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(land): kill what a merged check leaves running before land's git and gh calls (GST-1421) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): GST-1421 fragment names the stray kill Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .../capy-mergedchecks-cant-read-lands.md | 1 + docs/ship-policy.md | 14 ++++- lib/land-merged-checks.ts | 45 +++++++++++++ test/ship-queue-land-direct.test.ts | 63 +++++++++++++++++++ 4 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 changelog.d/capy-mergedchecks-cant-read-lands.md diff --git a/changelog.d/capy-mergedchecks-cant-read-lands.md b/changelog.d/capy-mergedchecks-cant-read-lands.md new file mode 100644 index 000000000..c44ebd6d2 --- /dev/null +++ b/changelog.d/capy-mergedchecks-cant-read-lands.md @@ -0,0 +1 @@ +- **Land closes its own environment to merged checks on Linux.** A scrubbed environment didn't stop a `land.merged_checks` command from reading land's own through `/proc/$PPID/environ` or a `/proc` scan, GitHub token included. Land now makes itself non-dumpable before the first check, and becomes the subreaper of everything the checks start and kills whatever a check leaves running — even a process that left its group with `setsid` or a double fork — before the `git` and `gh` processes it starts next could be read. When it can't set either, the checks don't run and land refuses. docs/ship-policy.md names what is still open: on macOS `ps -E` shows land's starting environment, and a check can still read anything else the user can, so pass `--allow-repo-commands` only for policies and PRs you trust. (GST-1421) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index c63f2956a..7c1f029be 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -2127,14 +2127,24 @@ commands are repo commands: the merged tree's `package.json` decides what `--allow-repo-commands`, with `PATH` and the locale as the whole environment and a throwaway `HOME`, so no token reaches them through the environment, runs git around them with hooks and fsmonitor off, and kills a command's whole -process group with SIGKILL when it ends or times out. A check that moves HEAD +process group with SIGKILL when it ends or times out. A scrubbed environment +alone doesn't stop a check reading land's own through `/proc/$PPID/environ` +or a `/proc` scan (GST-1421), so on Linux land makes itself non-dumpable before +the first check, which closes its `/proc` environment, memory and ptrace to +processes of the same user. The `git` and `gh` processes land starts +afterward still carry its environment, so land also becomes the subreaper of +everything the checks start and kills whatever a check leaves running, even a +process that left its group with `setsid` or a double fork. When land can't +set either, the checks don't run. macOS has no `/proc`, but `ps -E` still shows +land's starting environment to a same-user process. A check that moves HEAD off the merged commit fails. Land re-reads the base policy after the locked fetch, so a `land.merged_checks` that main gained meanwhile applies. When the merged tree's lockfile or `package.json` dependencies differ from the checkout's, the linked `node_modules` can't vouch for it: land refuses with `QUEUE_NOT_READY` before any check runs, and the lane merges main and re-ships. It doesn't sandbox the filesystem: a command can still write the checkout's `.git` (the worktree -shares it) and the linked `node_modules`, so pass the flag only for a base +shares it) and the linked `node_modules`, and read anything the user can (other +processes' environments, credential files), so pass the flag only for a base policy and PRs you trust to run. A list such as typecheck plus the size ratchets takes about two minutes, against a main merge, a CI round and a new receipt per PR. gstack's own policy runs diff --git a/lib/land-merged-checks.ts b/lib/land-merged-checks.ts index ceddd186e..d6963547c 100644 --- a/lib/land-merged-checks.ts +++ b/lib/land-merged-checks.ts @@ -6,7 +6,18 @@ * (the merged tree's package.json decides what `bun run` does), so they run only under --allow-repo-commands, never * under --dry-run, with an environment that keeps PATH and locale only and a throwaway HOME. Validated here so * lib/ship-policy.ts stays one schema file. + * + * A scrubbed environment does not stop a check reading land's own: /proc//environ is the exec-time + * snapshot (deleting from process.env does not change it), and any process of the same uid can read it, through + * $PPID or a /proc scan (GST-1421). So on Linux land makes itself non-dumpable before the first check, which closes + * its /proc environ, mem and ptrace to its own uid; execve resets the flag, so the checks stay ordinary processes, and + * land never restores it. The git and gh processes land starts after the checks carry its environment and are + * dumpable, so land also becomes the subreaper of everything the checks start: a process that left the check's group + * with setsid or a double fork is reparented to land, which kills every child it has after each check. When either + * fails the checks do not run. macOS has no /proc but `ps -E` (KERN_PROCARGS2) shows a same-uid process's exec-time + * environment and has no per-process opt-out, so there the land process's environment stays readable to the checks. */ +import { dlopen, FFIType } from 'bun:ffi'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; @@ -49,6 +60,36 @@ export type MergedRun = { ok: true; tree: string } | { ok: false; conflict: bool const LOCKFILES = ['bun.lock', 'bun.lockb', 'package-lock.json', 'yarn.lock', 'pnpm-lock.yaml']; const DEP_KEYS = ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies', 'workspaces', 'overrides']; const read = (file: string): string | null => { try { return fs.readFileSync(file, 'utf8'); } catch { return null; } }; +const openLibc = () => dlopen('libc.so.6', { + prctl: { args: [FFIType.i32, FFIType.u64, FFIType.u64, FFIType.u64, FFIType.u64], returns: FFIType.i32 }, + waitpid: { args: [FFIType.i32, FFIType.ptr, FFIType.i32], returns: FFIType.i32 }, +}).symbols; +let libc: ReturnType | undefined; +/** PR_SET_DUMPABLE 0 (confirmed with PR_GET_DUMPABLE) and PR_SET_CHILD_SUBREAPER 1; null when both hold, else why not. */ +function closeLandProc(): string | null { + try { + libc ??= openLibc(); + const set = libc.prctl(4, 0, 0, 0, 0), now = libc.prctl(3, 0, 0, 0, 0), reaper = libc.prctl(36, 1, 0, 0, 0); + return set === 0 && now === 0 && reaper === 0 ? null : `prctl(PR_SET_DUMPABLE, 0) returned ${set}, dumpable is ${now}, prctl(PR_SET_CHILD_SUBREAPER, 1) returned ${reaper}`; + } catch (e) { return (e as Error).message; } +} +/** After a check, land (a subreaper) has no children of its own, so every child is something the check left behind: SIGKILL the live ones and reap the dead until none is live; false when some survive a second of passes. */ +function killStrays(): boolean { + for (let pass = 0; pass < 100; pass++) { + const kids = fs.readdirSync('/proc').filter(d => /^\d+$/.test(d)).map(d => { + const stat = read(`/proc/${d}/stat`) ?? ''; + const [state, ppid] = stat.slice(stat.lastIndexOf(')') + 2).split(' '); + return { pid: Number(d), ppid: Number(ppid), dead: state === 'Z' }; + }).filter(k => k.ppid === process.pid); + for (const k of kids) { + if (k.dead) libc!.waitpid(k.pid, null, 1); + else try { process.kill(k.pid, 'SIGKILL'); } catch {} + } + if (kids.every(k => k.dead)) return true; + Bun.sleepSync(10); + } + return false; +} const deps = (file: string): string => { try { const p = JSON.parse(read(file) ?? 'null') ?? {}; return JSON.stringify(DEP_KEYS.map(k => p[k] ?? null)); } catch { return 'unreadable'; } }; /** Run `checks` on a throwaway commit of `pin` merged onto `base` (HEAD is that commit), in a worktree of `root` that is always removed, with a scrubbed environment; a check that does not finish (its whole process group is killed), moves HEAD or changes a tracked file fails, and a merged tree whose dependencies differ from the checkout's is refused before any check runs. */ @@ -69,6 +110,9 @@ export function runMergedChecks(root: string, base: string, pin: string, checks: if (drift.length) return { ok: false, conflict: false, setup: true, tree, detail: `the merged tree's ${drift.join(', ')} differ from this checkout's, so its node_modules cannot vouch for the merged tree` }; const modules = path.join(root, 'node_modules'); if (fs.existsSync(modules) && !fs.existsSync(path.join(dir, 'node_modules'))) fs.symlinkSync(modules, path.join(dir, 'node_modules')); + const linux = process.platform === 'linux'; + const open = linux ? closeLandProc() : null; + if (open) return { ok: false, conflict: false, setup: true, tree, detail: `cannot close land's /proc environment to the checks (${open}), so they did not run` }; for (const cmd of checks) { const group = process.platform !== 'win32'; const log = path.join(scratch, 'check.log'); @@ -77,6 +121,7 @@ export function runMergedChecks(root: string, base: string, pin: string, checks: const r = spawnSync('sh', ['-c', cmd], opts); fs.closeSync(fd); if (group && r.pid) try { process.kill(-r.pid, 'SIGKILL'); } catch {} + if (linux && !killStrays()) return { ok: false, conflict: false, tree, detail: `\`${cmd}\` left processes running that land could not stop on the merged tree ${tree.slice(0, 12)}` }; if (r.status === 0 && !r.error) continue; const tail = fs.readFileSync(log, 'utf8').trim().split('\n').slice(-6).join('\n'); return { ok: false, conflict: false, tree, detail: `\`${cmd}\` ${r.error ? `did not finish (${r.error.message})` : `exited ${r.status ?? r.signal}`} on the merged tree ${tree.slice(0, 12)}${tail ? `:\n${tail}` : ''}` }; diff --git a/test/ship-queue-land-direct.test.ts b/test/ship-queue-land-direct.test.ts index cb05adee5..aef9fa465 100644 --- a/test/ship-queue-land-direct.test.ts +++ b/test/ship-queue-land-direct.test.ts @@ -233,6 +233,69 @@ describe('land --pin: direct merge at the PR\'s own green head', () => { } finally { fs.rmSync(marker, { force: true }); } }, 90_000); + test.skipIf(process.platform !== 'linux')('GST-1421: a merged check that walks its ancestors\' /proc//environ and scans every /proc/*/environ finds no secret from land\'s environment, and land still merges', () => { + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-land-environ-')); + const marker = path.join(scratch, 'found'), probe = path.join(scratch, 'probe.sh'); + const secret = `land-secret-${process.pid}-${Date.now()}`; + fs.writeFileSync(probe, [ + 'p=$PPID', + 'while [ "${p:-0}" -gt 1 ]; do', + ' if cat /proc/$p/environ >/dev/null 2>&1; then echo "ancestor $p readable"; else echo "ancestor $p denied"; fi', + ' tr "\\0" "\\n" < /proc/$p/environ 2>/dev/null | grep "^GSTACK_LAND_SENTINEL=" | sed "s/^/ancestor $p /"', + ' p=$(sed -n "s/^PPid:[[:space:]]*//p" /proc/$p/status 2>/dev/null)', + 'done', + 'for f in /proc/[0-9]*/environ; do tr "\\0" "\\n" < "$f" 2>/dev/null | grep "^GSTACK_LAND_SENTINEL=" | sed "s|^|scan $f |"; done', + 'cat /proc/self/environ >/dev/null 2>&1 && echo "self readable"', + '', + ].join('\n')); + const fx = setup({ ...POLICY, land: { merged_checks: [`sh ${probe} > ${marker}`] }, min_gstack_version: LAND_KEY_VERSION }); + try { + const pin = openPr(fx, '88'); + moveMain(fx, 'other.txt', 'other\n'); + fx.env.GSTACK_LAND_SENTINEL = secret; + const ok = land(fx, '88', pin, ['--allow-repo-commands']); + expect(ok.err).toBe(''); + expect(ok.status).toBe(EXIT.ok); + const found = fs.readFileSync(marker, 'utf8'); + expect(found).toContain('self readable'); + expect(found).toMatch(/^ancestor \d+ denied$/m); + expect(found).not.toContain(secret); + expect(fx.read().mergeArgs).toBeDefined(); + } finally { fs.rmSync(scratch, { recursive: true, force: true }); } + }, 90_000); + + test.skipIf(process.platform !== 'linux')('GST-1421: a merged check that leaves its process group with setsid, to read the environment of the git and gh processes land starts next, is killed before land starts them', () => { + const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-land-stray-')); + const marker = path.join(scratch, 'found'), pidFile = path.join(scratch, 'pid'), scan = path.join(scratch, 'scan.sh'); + const secret = `land-secret-${process.pid}-${Date.now()}`; + fs.writeFileSync(scan, [ + `echo $$ > ${pidFile}`, + 'i=0', + 'while [ $i -lt 400 ]; do', + ` for f in /proc/[0-9]*/environ; do tr "\\0" "\\n" < "$f" 2>/dev/null | grep "^GSTACK_LAND_SENTINEL=" >> ${marker}; done`, + ' i=$((i+1)); sleep 0.05', + 'done', + '', + ].join('\n')); + const fx = setup({ ...POLICY, land: { merged_checks: [`setsid sh ${scan} /dev/null 2>&1 & while [ ! -s ${pidFile} ]; do sleep 0.05; done`] }, min_gstack_version: LAND_KEY_VERSION }); + let stray = 0; + try { + const pin = openPr(fx, '89'); + moveMain(fx, 'other.txt', 'other\n'); + fx.env.GSTACK_LAND_SENTINEL = secret; + const ok = land(fx, '89', pin, ['--allow-repo-commands']); + expect(ok.err).toBe(''); + expect(ok.status).toBe(EXIT.ok); + stray = Number(fs.readFileSync(pidFile, 'utf8').trim()); + expect(stray).toBeGreaterThan(1); + expect(() => process.kill(stray, 0)).toThrow(); + expect(fs.existsSync(marker) ? fs.readFileSync(marker, 'utf8') : '').not.toContain(secret); + } finally { + if (stray > 1) try { process.kill(stray, 'SIGKILL'); } catch {} + fs.rmSync(scratch, { recursive: true, force: true }); + } + }, 90_000); + test('QUEUE_LAND_NOT_CLEAN: on a stale base the pin conflicts with main, so no merged check runs and nothing merges', () => { const fx = setup(CHECKED); const pin = openPr(fx, '83', { edit: w => fs.writeFileSync(path.join(w, 'other.txt'), 'pr\n') });