diff --git a/browse/src/file-permissions.ts b/browse/src/file-permissions.ts index 078fcac89..6d2512502 100644 --- a/browse/src/file-permissions.ts +++ b/browse/src/file-permissions.ts @@ -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 :(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(); +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. diff --git a/browse/test/file-permissions.test.ts b/browse/test/file-permissions.test.ts index 1d772948b..6461cf075 100644 --- a/browse/test/file-permissions.test.ts +++ b/browse/test/file-permissions.test.ts @@ -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 }); + } +}); +