mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-15 21:07:20 +01:00
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>
This commit is contained in:
1 parent
f37042c0d3
commit
b406820980
4 files changed
+121
-2
No files matched your search
@@ -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)
|
||||
+12
-2
@@ -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
|
||||
|
||||
@@ -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/<land pid>/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<typeof openLibc> | 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}` : ''}` };
|
||||
|
||||
@@ -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/<pid>/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 >/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') });
|
||||
|
||||
Reference in new issue
Block a user