diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 77e614bd..5f177d05 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -48,14 +48,28 @@ jobs: with: app-id: ${{ secrets.RELEASE_APP_ID }} private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} + # Least privilege: without these, the minted token inherits ALL of the + # App's installation permissions. semantic-release only needs to push the + # release commit/tag + create the Release (contents) and comment on the + # released issues/PRs (issues/pull-requests). Attestation/Find-PR/Comment + # steps use the default GITHUB_TOKEN, not this token. + permission-contents: write + permission-issues: write + permission-pull-requests: write - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: - # semantic-release needs full history + tags and the persisted App - # credential to push the release commit. Do not change these. + # semantic-release needs full history + tags to analyze commits. fetch-depth: 0 + # The token is still passed so checkout fetches as the App, but we do + # NOT persist it into .git/config: install/build/test run BEFORE the + # release and a compromised dependency could otherwise read the App + # credential off disk. semantic-release authenticates its push from the + # GITHUB_TOKEN env on the Release step (it builds an x-access-token URL, + # it never reads .git/config), so the push still works without it. token: ${{ steps.app-token.outputs.token }} + persist-credentials: false - name: Setup pnpm uses: pnpm/action-setup@0ebf47130e4866e96fce0953f49152a61190b271 # v6.0.9 diff --git a/scripts/obsidian-e2e-cli.mjs b/scripts/obsidian-e2e-cli.mjs index e8f75db6..b2d0beea 100644 --- a/scripts/obsidian-e2e-cli.mjs +++ b/scripts/obsidian-e2e-cli.mjs @@ -5,6 +5,7 @@ import process from "node:process"; import { promisify } from "node:util"; import { provisionVault } from "./provision-obsidian-e2e-vault.mjs"; import { + ensureSecureDir, launchObsidianInstance, parseArgs as parseInstanceArgs, prepareObsidianProfile, @@ -170,6 +171,9 @@ async function main() { } const options = resolveInstanceOptions(parseInstanceArgs(parsed.instanceArgs)); + // Validate the profile root we own before the reaper scans/removes anything in + // it, so a hijacked root aborts the run loudly instead of being trusted. + await ensureSecureDir(options.profileRoot); await reapStaleInstances(options); await ensureObsidianInstance(options); process.exitCode = await spawnObsidian(options, parsed.commandArgs); diff --git a/scripts/start-obsidian-e2e-instance.mjs b/scripts/start-obsidian-e2e-instance.mjs index f304d9cf..01e64e3b 100644 --- a/scripts/start-obsidian-e2e-instance.mjs +++ b/scripts/start-obsidian-e2e-instance.mjs @@ -134,7 +134,89 @@ function safeName(value) { .replace(/^-+|-+$/g, "") || "vault"; } +// The uid we require every profile directory to be owned by. Injectable so the +// foreign-owner branch is testable without a second account; null on a platform +// without process.getuid (none we support — the harness is macOS-only) skips the +// ownership check rather than comparing against undefined. +function resolveCurrentUid(options) { + if ("currentUid" in options) return options.currentUid; + return typeof process.getuid === "function" ? process.getuid() : null; +} + +// Reject any directory we do not exclusively own. A directory we own with no +// group/other access cannot have a foreign-planted child (only we can write into +// it), so descending into it later is safe. We REJECT a loose (group/other- +// accessible) dir rather than chmod-repairing it: an attacker who could write +// while it was loose may already have planted a `home` symlink that a parent +// chmod would not undo, and a later recursive mkdir / keychain link would follow. +function assertOwnedDir(dir, stat, currentUid) { + if (stat.isSymbolicLink() || !stat.isDirectory()) { + throw new Error( + `Refusing to use ${dir}: it is a symlink or not a regular directory.`, + ); + } + if (currentUid !== null && stat.uid !== currentUid) { + throw new Error( + `Refusing to use ${dir}: owned by uid ${stat.uid}, not ${currentUid}.`, + ); + } + if ((stat.mode & 0o077) !== 0) { + throw new Error( + `Refusing to use ${dir}: it is group/other-accessible (mode ${( + stat.mode & 0o777 + ).toString(8)}); remove it and retry.`, + ); + } +} + +// Create (when absent) and validate a private profile directory we own. The +// profile root defaults under world-writable /tmp; if a co-located actor +// pre-creates it (or symlinks it elsewhere) before our first run, +// fs.mkdir(..., {recursive}) is a no-op for ownership/mode and we would +// otherwise write the keychain-bearing HOME (and obsidian.json) through their +// directory. lstat FIRST so we never mkdir THROUGH a pre-existing symlink, then +// create only when absent, then assert ownership/mode. Callers must secure a +// parent before its children so each child is created inside an already-0o700 +// tree the attacker cannot enter (and /tmp's sticky bit then prevents swapping +// our owned dir entry). +export async function ensureSecureDir(dir, options = {}) { + const currentUid = resolveCurrentUid(options); + let stat; + try { + stat = await fs.lstat(dir); + } catch (error) { + if (error?.code !== "ENOENT") throw error; + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); + stat = await fs.lstat(dir); + } + assertOwnedDir(dir, stat, currentUid); + return dir; +} + +// Validate an EXISTING profile directory without creating or modifying it. +// Teardown paths (stop/reap) read and remove inside the root, so they must +// refuse a hijacked/symlinked root, but must not create one — a missing root +// just means there is nothing to clean up. Returns false when the path is absent. +export async function assertSecureDirIfPresent(dir, options = {}) { + const currentUid = resolveCurrentUid(options); + let stat; + try { + stat = await fs.lstat(dir); + } catch (error) { + if (error?.code === "ENOENT") return false; + throw error; + } + assertOwnedDir(dir, stat, currentUid); + return true; +} + export async function prepareObsidianProfile(options) { + // Fail closed if our profile tree is not a private directory we own + // (temp-squat / TOCTOU guard). Secure the root before the instance dir so the + // instance dir is created inside an already-validated 0o700 tree. + await ensureSecureDir(options.profileRoot); + await ensureSecureDir(options.instancePath); + const userDataPath = path.join(options.obsidianHome, "Library", "Application Support", "obsidian"); await fs.mkdir(userDataPath, { recursive: true, mode: 0o700 }); await fs.mkdir(path.join(options.obsidianHome, "Library", "Logs"), { recursive: true, mode: 0o700 }); @@ -220,7 +302,13 @@ async function writeJson(filePath, value) { } export async function launchObsidianInstance(options) { - await fs.mkdir(options.instancePath, { recursive: true }); + // Validate (and create when absent) the profile tree here too, so a future + // "relaunch existing instance" path that calls launch without the normal + // prepareObsidianProfile/main preflight cannot bypass the temp-squat guard. + // Parent-first: a loose profileRoot must not stay attacker-writable around + // the validated instance dir. + await ensureSecureDir(options.profileRoot); + await ensureSecureDir(options.instancePath); await execFileAsync("/usr/bin/open", [ "-n", "-g", @@ -352,6 +440,9 @@ async function main() { } const options = resolveInstanceOptions(rawOptions); + // Validate the profile root we own before the reaper scans/removes anything in + // it, so a hijacked root aborts the run loudly instead of being trusted. + await ensureSecureDir(options.profileRoot); await reapStaleInstances(options); const provisionResult = await provisionVault(options); const profileResult = await prepareObsidianProfile(options); diff --git a/scripts/stop-obsidian-e2e-instance.mjs b/scripts/stop-obsidian-e2e-instance.mjs index da936b7d..8818ab07 100644 --- a/scripts/stop-obsidian-e2e-instance.mjs +++ b/scripts/stop-obsidian-e2e-instance.mjs @@ -5,6 +5,7 @@ import path from "node:path"; import process from "node:process"; import { promisify } from "node:util"; import { + assertSecureDirIfPresent, INSTANCE_MARKER_FILE, resolveInstanceOptions, } from "./start-obsidian-e2e-instance.mjs"; @@ -364,6 +365,13 @@ export async function reapOrphanedInstances(options = {}) { if (!profileRoot) return { scanned: 0, reaped: [] }; + // Refuse to scan/remove through a hijacked or symlinked profile root (the same + // temp-squat guard the start path applies before writing into it). Absent root + // => nothing to reap. + if (!(await assertSecureDirIfPresent(profileRoot))) { + return { scanned: 0, reaped: [] }; + } + let entries; try { entries = await readdir(profileRoot); @@ -416,6 +424,9 @@ async function main() { } const options = resolveInstanceOptions(rawOptions); + // Refuse to operate inside a hijacked/symlinked profile root before we stop or + // remove anything under it. + await assertSecureDirIfPresent(options.profileRoot); const summary = await stopInstance(options.instancePath, { dryRun: rawOptions.dryRun, profileRoot: options.profileRoot, diff --git a/tests/release-workflow-hardening.test.ts b/tests/release-workflow-hardening.test.ts new file mode 100644 index 00000000..dee71889 --- /dev/null +++ b/tests/release-workflow-hardening.test.ts @@ -0,0 +1,75 @@ +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +// These tests pin the security hardening of the privileged Release workflow: +// the minted GitHub App token (a branch-protection bypass actor) must be scoped +// to least privilege and must NOT be persisted into .git/config while untrusted +// dependency install / build / test code runs ahead of semantic-release. They +// fail loudly if a future edit regresses either guard. No YAML dependency is +// available, so we extract the relevant step block textually and assert on it. + +const here = path.dirname(fileURLToPath(import.meta.url)); +const releaseYmlPath = path.resolve(here, "..", ".github", "workflows", "release.yml"); +const releaseYml = fs.readFileSync(releaseYmlPath, "utf8"); + +// Each step in release.yml begins with a `- name:` list item. Split the file +// into those blocks so an assertion targets exactly one step (and is immune to +// steps being reordered). Full-line comments are dropped first so a guard that +// is COMMENTED OUT (e.g. `# persist-credentials: false`) cannot satisfy a check. +function stepBlocks(content: string): string[] { + const lines = content.split("\n").filter((line) => !/^\s*#/.test(line)); + const blocks: string[] = []; + let current: string[] = []; + for (const line of lines) { + if (/^\s*- name:/.test(line)) { + if (current.length > 0) blocks.push(current.join("\n")); + current = [line]; + } else if (current.length > 0) { + current.push(line); + } + } + if (current.length > 0) blocks.push(current.join("\n")); + return blocks; +} + +function stepContaining(needle: string): string { + const matches = stepBlocks(releaseYml).filter((block) => block.includes(needle)); + expect(matches, `exactly one step should contain ${needle}`).toHaveLength(1); + return matches[0]; +} + +describe("release workflow hardening", () => { + it("does not persist the App credential to disk during install/build/test", () => { + const checkout = stepContaining("uses: actions/checkout@"); + // The token is still passed (semantic-release pushes via the GITHUB_TOKEN env + // on the Release step, but checkout fetches as the App); we just refuse to + // write it into .git/config where untrusted install/build/test code could read + // it. Anchored to real YAML lines so a commented-out guard cannot pass. + expect(checkout).toMatch( + /^\s*token:\s*\$\{\{\s*steps\.app-token\.outputs\.token\s*\}\}\s*$/m, + ); + expect(checkout).toMatch(/^\s*persist-credentials:\s*false\s*$/m); + }); + + it("scopes the minted App token to exactly the least-privilege set", () => { + const appToken = stepContaining("uses: actions/create-github-app-token@"); + // Collect every permission-* input declared on the step (comments already + // stripped by stepBlocks). + const permissions: Record = {}; + for (const line of appToken.split("\n")) { + const match = line.match(/^\s*(permission-[a-z-]+):\s*(\S+)\s*$/); + if (match) permissions[match[1]] = match[2]; + } + // Without explicit permission-* inputs the token inherits ALL of the App's + // installation permissions. Assert the list is CLOSED: exactly these three + // scopes (push commit/tag + create Release; comment on issues/PRs) and + // nothing broader - a future over-permission addition must fail this test. + expect(permissions).toEqual({ + "permission-contents": "write", + "permission-issues": "write", + "permission-pull-requests": "write", + }); + }); +}); diff --git a/tests/start-obsidian-e2e-instance.test.ts b/tests/start-obsidian-e2e-instance.test.ts index cb232276..cc777681 100644 --- a/tests/start-obsidian-e2e-instance.test.ts +++ b/tests/start-obsidian-e2e-instance.test.ts @@ -3,6 +3,8 @@ import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { + assertSecureDirIfPresent, + ensureSecureDir, INSTANCE_MARKER_FILE, parseArgs, prepareObsidianProfile, @@ -104,6 +106,97 @@ describe("start-obsidian-e2e-instance", () => { }); }); +describe("ensureSecureDir", () => { + it("creates a fresh profile dir owned by us with 0o700", async () => { + const root = await makeTempDir("quickadd-secure"); + const dir = path.join(root, "profile-root"); + + await ensureSecureDir(dir); + + const stat = await fs.lstat(dir); + expect(stat.isDirectory()).toBe(true); + expect(stat.mode & 0o777).toBe(0o700); + if (typeof process.getuid === "function") { + expect(stat.uid).toBe(process.getuid()); + } + }); + + it("rejects a pre-existing group/world-accessible dir (planted-child race)", async () => { + const root = await makeTempDir("quickadd-secure"); + const dir = path.join(root, "loose"); + await fs.mkdir(dir, { recursive: true }); + await fs.chmod(dir, 0o777); + + // A loose dir may already hold a foreign-planted child symlink that a parent + // chmod would not undo, so we refuse it outright rather than "repair" it. + await expect(ensureSecureDir(dir)).rejects.toThrow(/group\/other-accessible/); + }); + + it("refuses a symlink at the profile path (never follows it)", async () => { + const root = await makeTempDir("quickadd-secure"); + const target = path.join(root, "attacker-owned"); + await fs.mkdir(target, { recursive: true }); + const link = path.join(root, "profile-root"); + await fs.symlink(target, link); + + await expect(ensureSecureDir(link)).rejects.toThrow(/symlink or not a regular directory/); + // The symlink target must be untouched - we must not have written through it. + expect((await fs.readdir(target)).length).toBe(0); + }); + + it("refuses a dir owned by a different uid (temp-squat)", async () => { + const root = await makeTempDir("quickadd-secure"); + const dir = path.join(root, "foreign"); + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); + + const foreignUid = (process.getuid?.() ?? 0) + 4242; + await expect( + ensureSecureDir(dir, { currentUid: foreignUid }), + ).rejects.toThrow(/owned by uid/); + }); + + it("skips the owner check when getuid is unavailable (currentUid null)", async () => { + const root = await makeTempDir("quickadd-secure"); + const dir = path.join(root, "no-getuid"); + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); + + // Platforms without process.getuid resolve currentUid to null; the owner + // check is then skipped rather than throwing against an undefined uid. + await expect(ensureSecureDir(dir, { currentUid: null })).resolves.toBe(dir); + }); +}); + +describe("assertSecureDirIfPresent", () => { + it("returns false for an absent dir without creating it", async () => { + const root = await makeTempDir("quickadd-secure"); + const dir = path.join(root, "missing"); + + await expect(assertSecureDirIfPresent(dir)).resolves.toBe(false); + // Teardown must not create the root it is asked to clean up. + await expect(fs.lstat(dir)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it("accepts an existing private dir we own", async () => { + const root = await makeTempDir("quickadd-secure"); + const dir = path.join(root, "ours"); + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); + + await expect(assertSecureDirIfPresent(dir)).resolves.toBe(true); + }); + + it("refuses a symlinked profile root (teardown must not traverse it)", async () => { + const root = await makeTempDir("quickadd-secure"); + const target = path.join(root, "attacker-owned"); + await fs.mkdir(target, { recursive: true }); + const link = path.join(root, "profile-root"); + await fs.symlink(target, link); + + await expect(assertSecureDirIfPresent(link)).rejects.toThrow( + /symlink or not a regular directory/, + ); + }); +}); + async function exists(filePath: string) { try { await fs.lstat(filePath);