fix(browse): never chmod shared, symlinked, or foreign-owned dirs to 0700

restrictDirectoryPermissions unconditionally chmodded its target. On hosts
where the process holds CAP_FOWNER (Docker as root, CI sandboxes) that
chmod SUCCEEDS on root-owned /tmp whenever a state file is configured
there (BROWSE_STATE_FILE=/tmp/x.json derives stateDir=/tmp), and a 0700
/tmp breaks access(2)-based checks machine-wide for every other process.

The POSIX branch now refuses shared sticky dirs, world-writable mounts
under root, foreign-owned dirs, and symlinked state dirs; refusals warn
once per process instead of failing silent; owned-but-unreadable dirs
keep their chmod self-repair; and the check-then-act race is closed with
fd-anchored O_NOFOLLOW + fstat/fchmod on a single inode.

Regression tests cover the sticky-dir, foreign-uid, mkdirSecure-reapply,
and symlinked-dir shapes.
This commit is contained in:
Garry Tan
2026-08-28 04:46:59 +00:00
parent 394db326f2
commit be509bfab4
2 changed files with 154 additions and 2 deletions
+91 -2
View File
@@ -135,6 +135,21 @@ export function restrictFilePermissions(filePath: string): void {
* POSIX: `fs.chmodSync(path, 0o700)`. Idempotent if the dir was already
* created with `{ mode: 0o700 }`.
*
* Owner-only hardening is for directories gstack creates and owns. Two
* pre-existing shapes must never be chmodded, so the POSIX branch refuses
* them:
*
* - A directory owned by another user. On most hosts the chmod just
* fails EPERM, but run as root (or with CAP_FOWNER — containers, CI
* sandboxes) it SUCCEEDS and takes the directory away from its owner.
* - A shared sticky world-writable directory (`/tmp`, `/var/tmp`).
* Callers land here when a state file is configured directly inside
* the system temp dir (`BROWSE_STATE_FILE=/tmp/foo.json` makes
* `path.dirname()` derive `/tmp` as the state dir). A 0o700 `/tmp`
* locks every other process on the machine out of it: access(2)-based
* checks (`fs.existsSync`) return EACCES→false machine-wide, even
* while stat keeps working for capability-holding processes.
*
* Windows: `icacls /inheritance:r /grant:r <user>:(OI)(CI)(F)`. The
* `(OI)(CI)` flags make new files (OI = object inherit) and subdirs
* (CI = container inherit) inherit the single-user-full ACL — important
@@ -155,7 +170,78 @@ export function restrictDirectoryPermissions(dirPath: string): void {
}
return;
}
try { fs.chmodSync(dirPath, 0o700); } catch { /* best-effort */ }
try {
// fd-anchored check-then-act: statSync + chmodSync resolve the path twice,
// so a symlink swapped in between would let the chmod land on a directory
// the check never saw (exactly the CAP_FOWNER hosts this guard exists
// for). O_NOFOLLOW refuses a symlinked state dir outright; fstat + fchmod
// pin both the check and the act to the same inode.
const fd = fs.openSync(
dirPath,
fs.constants.O_RDONLY | fs.constants.O_DIRECTORY | fs.constants.O_NOFOLLOW,
);
try {
if (!shouldHardenDir(fs.fstatSync(fd))) {
warnUnhardenedDirOnce(dirPath);
return;
}
fs.fchmodSync(fd, 0o700);
} finally {
fs.closeSync(fd);
}
} catch (err: any) {
// The fd path refuses two shapes the old chmodSync handled, and a silent
// skip here would quietly lose hardening (the module's own rule is that
// refusals are never silent):
// ELOOP/ENOTDIR — dirPath is a symlink (dotfiles-managed ~/.gstack via
// stow/chezmoi). Never follow it blind; warn so the operator knows.
// EACCES — an OWNED dir stuck without the read bit (mode 0300/0000)
// can't be opened but could always be repaired by plain chmod. Keep
// that self-repair: re-check ownership via lstat (no link follow) and
// chmod only a real, owned, non-shared directory.
if (err?.code === 'EACCES') {
try {
const st = fs.lstatSync(dirPath);
if (st.isDirectory() && shouldHardenDir(st)) {
fs.chmodSync(dirPath, 0o700);
return;
}
} catch { /* fall through to the warning */ }
}
if (err?.code === 'ELOOP' || err?.code === 'ENOTDIR' || err?.code === 'EACCES') {
warnUnhardenedDirOnce(dirPath);
}
/* anything else: best-effort, matching the old behavior */
}
}
/** Owner-only hardening applies to a real dir we own that isn't shared. */
function shouldHardenDir(st: fs.Stats): boolean {
// chmod authorization is judged on the EFFECTIVE uid; fall back to the
// real uid where geteuid is unavailable (Windows — unreachable here).
const uid = process.geteuid?.() ?? process.getuid?.();
if (st.uid !== uid) return false;
// sticky + world-writable = shared temp dir (/tmp, /var/tmp) — never ours
// to restrict. Running as root, treat ANY world-writable dir as shared:
// root "owns" docker/CI volume mounts (0777, no sticky bit) that other
// users depend on, and a 0700 there locks them all out.
if ((st.mode & 0o1002) === 0o1002) return false;
if (uid === 0 && (st.mode & 0o002) === 0o002) return false;
return true;
}
// The POSIX refusal must not be silent (the Windows branch already warns when
// icacls misses its hardening target): an operator who points state at a
// shared or foreign-owned directory should learn it was left unhardened.
const unhardenedWarned = new Set<string>();
function warnUnhardenedDirOnce(dirPath: string): void {
if (unhardenedWarned.has(dirPath)) return;
unhardenedWarned.add(dirPath);
// biome-ignore lint/suspicious/noConsole: intentional user-facing warning
console.warn(
`[gstack] directory ${dirPath} is shared, symlinked, or owned by another user; ` +
'gstack left its permissions untouched — use a private, non-symlinked directory for owner-only hardening',
);
}
/**
@@ -218,7 +304,10 @@ export function repairBrokenDacl(dirPath: string): void {
/**
* `mkdir -p` with owner-only directory permissions, cross-platform.
* Replaces `fs.mkdirSync(path, { recursive: true, mode: 0o700 })` + Windows ACL.
* Safe to call on an existing directory — re-applies the ACL idempotently.
* Safe to call on an existing directory — re-applies the ACL idempotently,
* except on directories gstack doesn't own (another user's dir, or a shared
* sticky world-writable dir like `/tmp`), which are left untouched — see
* `restrictDirectoryPermissions`.
*
* Windows: after applying the restricted ACL, verifies the directory is
* still listable by this process and repairs a broken DACL (#1605) if not.
+63
View File
@@ -72,6 +72,34 @@ describe('restrictDirectoryPermissions', () => {
expect(fs.statSync(d).mode & 0o777).toBe(0o700);
});
test('on POSIX, leaves a shared sticky world-writable dir untouched', () => {
if (process.platform === 'win32') return;
// Simulates /tmp: sticky + world-writable. Hardening a shared dir to
// 0o700 locks every other user on the machine out of it, so the helper
// must refuse — even when the caller owns the dir (root / CAP_FOWNER
// hosts are where the chmod would actually succeed).
const d = path.join(tmpDir, 'shared-tmp');
fs.mkdirSync(d);
// System chmod, not fs.chmodSync: Bun masks the sticky bit off chmod/
// mkdir modes, so 0o1777 through the fs API lands as 0o777.
Bun.spawnSync(['chmod', '1777', d]);
expect(fs.statSync(d).mode & 0o7777).toBe(0o1777); // fixture took
restrictDirectoryPermissions(d);
expect(fs.statSync(d).mode & 0o7777).toBe(0o1777);
});
test('on POSIX, leaves a directory owned by another user untouched', () => {
if (process.platform === 'win32') return;
const d = path.join(tmpDir, 'foreign');
fs.mkdirSync(d, { mode: 0o755 });
// Only constructible where chown to a foreign uid succeeds (root /
// CAP_CHOWN — containers, CI sandboxes). Elsewhere the chown throws
// and there's nothing to assert; bail.
try { fs.chownSync(d, process.getuid!() + 1, fs.statSync(d).gid); } catch { return; }
restrictDirectoryPermissions(d);
expect(fs.statSync(d).mode & 0o777).toBe(0o755);
});
test('on Windows, does not throw on an existing directory', () => {
if (process.platform !== 'win32') return;
const d = path.join(tmpDir, 'subdir');
@@ -159,6 +187,23 @@ describe('mkdirSecure', () => {
expect(() => mkdirSecure(d)).not.toThrow();
});
test('does not chmod a pre-existing shared sticky dir (the /tmp state-dir case)', () => {
if (process.platform === 'win32') return;
// BROWSE_STATE_FILE=/tmp/foo.json derives stateDir=/tmp, and every
// daemon boot runs mkdirSecure(stateDir). The mkdir is a no-op on the
// existing dir; the permission re-apply must be one too — chmodding the
// real /tmp to 0o700 breaks fs.existsSync (access(2) → EACCES) for
// every process on the machine until something restores 1777.
const d = path.join(tmpDir, 'shared-tmp');
fs.mkdirSync(d);
// System chmod: Bun's fs API masks the sticky bit off modes (see the
// restrictDirectoryPermissions sticky-dir test).
Bun.spawnSync(['chmod', '1777', d]);
expect(fs.statSync(d).mode & 0o7777).toBe(0o1777); // fixture took
mkdirSecure(d);
expect(fs.statSync(d).mode & 0o7777).toBe(0o1777);
});
test('on Windows, the created directory stays usable by the caller', () => {
if (process.platform !== 'win32') return;
// The state-dir path that broke: mkdirSecure() creates .gstack/, hardens
@@ -208,3 +253,21 @@ describe('repairBrokenDacl', () => {
expect(() => repairBrokenDacl(path.join(tmpDir, 'nonexistent'))).not.toThrow();
});
});
// Symlinked state dir (dotfiles-managed ~/.gstack via stow/chezmoi): the
// fd-anchored path refuses to follow it (O_NOFOLLOW) — the refusal must warn,
// never throw, and never chmod the symlink target.
test('restrictDirectoryPermissions warns and skips a symlinked dir without throwing', () => {
const base = fs.mkdtempSync(path.join(os.tmpdir(), 'fp-symlink-'));
const target = path.join(base, 'real');
const link = path.join(base, 'link');
fs.mkdirSync(target, { mode: 0o755 });
fs.symlinkSync(target, link);
try {
expect(() => restrictDirectoryPermissions(link)).not.toThrow();
// Target permissions untouched — the link was never followed.
expect(fs.statSync(target).mode & 0o777).toBe(0o755);
} finally {
fs.rmSync(base, { recursive: true, force: true });
}
});