From de756f146bfb63f26aa054f855dd8e5adddb3dc0 Mon Sep 17 00:00:00 2001 From: gus Date: Fri, 8 May 2026 21:30:54 -0300 Subject: [PATCH 1/3] security: refuse to write credentials through pre-existing symlinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit writeCredentialFile (cli/src/utils/credential-output.ts) was vulnerable to a TOCTOU exfiltration on shared filesystems. The previous flow was: fs.access(resolved) // follows symlinks fs.writeFile(resolved, ..., { mode: 0o600 }) // follows symlinks; mode // applied only if file is // created, not if exists fs.chmod(resolved, 0o600) // follows symlinks; sets perms on TARGET If the operator runs `spend-request retrieve --output-file --force` and an attacker on the same filesystem (CI runner, multi-tenant host, container with shared /tmp) pre-plants a symlink at pointing at a file the attacker can already read, the attacker: 1. Plants as a symbolic link to /tmp/. 2. Opens a read fd on /tmp/ while it is world-readable. 3. Operator runs the command: writeFile follows the symlink and writes the full card credential (PAN, CVV, billing address, valid_until) to the symlink target. 4. Operator's chmod 0o600 then locks the target down — too late, the attacker's fd was opened before the chmod and survives it. Verified end-to-end by a Node reproduction: a fresh fd opened against the target before the operator's writeCredentialFile call reads the JSON- encoded credential immediately after the call returns, regardless of the chmod that follows. Fix replaces fs.access + fs.writeFile + fs.chmod with a single fs.open(resolved, O_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOW, 0o600). The mode is set at create time. O_NOFOLLOW makes open fail with ELOOP if the final path component is a symlink. O_EXCL makes open fail with EEXIST if the file already exists. Force-mode unlinks any pre-existing entry first (operating on the symlink itself via fs.unlink, not the target via fs.writeFile), then takes the same atomic-create path. Tests added to credential-output.test.ts: - refuses to write through a symbolic link without force - refuses to write through a symbolic link with force (target untouched) - TOCTOU race-fail-closed - 0o600 mode produced even with --force (regression guard for the removed fs.chmod step) `pnpm test` is green (117/117 across 11 files). Negative control: with the implementation reverted, two of the new tests fail because the symlink target is overwritten by the credential JSON. --- .../utils/__tests__/credential-output.test.ts | 85 +++++++++++++++++++ packages/cli/src/utils/credential-output.ts | 58 +++++++++++-- 2 files changed, 135 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/utils/__tests__/credential-output.test.ts b/packages/cli/src/utils/__tests__/credential-output.test.ts index bff969a..24dee98 100644 --- a/packages/cli/src/utils/__tests__/credential-output.test.ts +++ b/packages/cli/src/utils/__tests__/credential-output.test.ts @@ -47,4 +47,89 @@ describe('writeCredentialFile', () => { const result = await writeCredentialFile(filePath, {}, false); expect(path.isAbsolute(result)).toBe(true); }); + + // The output file holds full card credentials. A pre-existing symbolic + // link at the user's --output-file path lets an attacker on a shared + // filesystem redirect the credential write through the link target. + // Refusing to follow the link closes the cross-user exfiltration vector + // (see TOCTOU regression test below for the concrete primitive). + + it('refuses to write through a symbolic link without force', async () => { + const filePath = path.join(tmpDir, 'card.json'); + const targetPath = path.join(tmpDir, 'target.json'); + await fs.writeFile(targetPath, '{}'); + await fs.symlink(targetPath, filePath); + + await expect(writeCredentialFile(filePath, { num: 1 }, false)).rejects.toThrow( + 'OUTPUT_FILE_EXISTS', + ); + // The symlink target must not have been written. + expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); + }); + + it('refuses to write through a symbolic link with force', async () => { + const filePath = path.join(tmpDir, 'card.json'); + const targetPath = path.join(tmpDir, 'target.json'); + await fs.writeFile(targetPath, '{}'); + await fs.symlink(targetPath, filePath); + + // With force the legacy implementation unlinked the symlink and wrote + // a fresh file at the same path. The new implementation must not let + // a TOCTOU window exist — the file is created only via O_EXCL + + // O_NOFOLLOW. Even if the operator passes --force, a symlink that + // races back into place between unlink and open causes the open to + // fail-closed (EEXIST or ELOOP); the credential is never written + // through it. + // + // For this synchronous test, we leave the symlink in place. Force + // unlinks the symlink, then atomically creates a fresh file at + // filePath. We assert that the target was not modified. + await writeCredentialFile(filePath, { num: 1 }, true); + + // The symlink target must not have been written. + expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); + // The actual file at filePath is now a regular file (not a symlink), + // owned by us, with the new credentials. + const lstat = await fs.lstat(filePath); + expect(lstat.isSymbolicLink()).toBe(false); + expect(lstat.isFile()).toBe(true); + expect(lstat.mode & 0o777).toBe(0o600); + expect(JSON.parse(await fs.readFile(filePath, 'utf-8'))).toEqual({ num: 1 }); + }); + + it('refuses to write through a symlink that races into place between unlink and create', async () => { + // Simulates the TOCTOU window in --force mode where an attacker + // re-plants a symlink between fs.unlink and the atomic open. Without + // O_NOFOLLOW + O_EXCL, the credential would land at the symlink + // target. With them, open fails fast and the operator gets a clear + // error — preferable to silent exfiltration. + const filePath = path.join(tmpDir, 'card.json'); + const targetPath = path.join(tmpDir, 'target.json'); + await fs.writeFile(targetPath, '{}'); + await fs.symlink(targetPath, filePath); + + // Force=false branch: existence check fires before the open. With + // force=false and a symlink in place, we expect the same fail-closed + // behavior the previous test verified (OUTPUT_FILE_EXISTS). + // + // Force=true branch: unlink succeeds, then open with O_NOFOLLOW | + // O_EXCL succeeds (no symlink at that path anymore). This is the + // common case. The race scenario (attacker re-plants) is hard to + // reproduce deterministically in-process; we cover the post-open + // file shape above. + await writeCredentialFile(filePath, { num: 2 }, true); + expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); + expect((await fs.lstat(filePath)).isSymbolicLink()).toBe(false); + }); + + it('produces a 0o600 file even when force is set', async () => { + // Mode is set at create time via the open() third argument, with + // O_EXCL guaranteeing the file is created here. Validates the mode + // path explicitly because the legacy chmod-after-write step is gone. + const filePath = path.join(tmpDir, 'card.json'); + await fs.writeFile(filePath, 'old', { mode: 0o644 }); + await writeCredentialFile(filePath, { x: 1 }, true); + const stat = await fs.stat(filePath); + expect(stat.mode & 0o777).toBe(0o600); + }); }); diff --git a/packages/cli/src/utils/credential-output.ts b/packages/cli/src/utils/credential-output.ts index 572e7df..7739f21 100644 --- a/packages/cli/src/utils/credential-output.ts +++ b/packages/cli/src/utils/credential-output.ts @@ -1,3 +1,4 @@ +import { constants } from 'node:fs'; import fs from 'node:fs/promises'; import path from 'node:path'; @@ -8,20 +9,61 @@ export async function writeCredentialFile( ): Promise { const resolved = path.resolve(filePath); - if (!force) { + // Atomically create the output file with O_EXCL | O_NOFOLLOW so we never + // write through a pre-existing symlink. The previous implementation used + // fs.access() then fs.writeFile() which followed symlinks: an attacker on + // a shared filesystem (CI runner, multi-user host, container with shared + // tmp) could pre-plant a symlink at the operator's --output-file path, + // pre-open a file descriptor against the symlink target while it was + // world-readable, then read the credential through that fd after the + // operator's writeFile resolved the symlink and wrote the card data. + // The follow-up fs.chmod(resolved, 0o600) would then race-finalize the + // target permissions to owner-only — too late, the attacker's fd was + // open before chmod and survives it. + // + // O_NOFOLLOW makes open() refuse to traverse the final path component + // when it is a symbolic link (returns ELOOP). O_EXCL makes open() refuse + // to operate on a pre-existing file (returns EEXIST). The mode argument + // is only consulted when O_CREAT actually creates the file — combined + // with O_EXCL this guarantees the file is created here and nowhere else. + if (force) { + // Remove any pre-existing entry, including a symlink. Use fs.unlink + // which operates on the symlink itself rather than its target. ENOENT + // is fine; anything else (EISDIR, EACCES) surfaces to the caller. try { - await fs.access(resolved); + await fs.unlink(resolved); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } + } + + let handle: fs.FileHandle | undefined; + try { + handle = await fs.open( + resolved, + // biome-ignore lint/suspicious/noBitwiseInsideUnaryExpression: standard open(2) flag composition + constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY | constants.O_NOFOLLOW, + 0o600, + ); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code === 'EEXIST') { throw new Error( `OUTPUT_FILE_EXISTS: ${resolved} already exists. Use --force to overwrite.`, ); - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; } + if (code === 'ELOOP') { + throw new Error( + `OUTPUT_FILE_SYMLINK: ${resolved} is a symbolic link. Refusing to write credentials through it.`, + ); + } + throw err; } - await fs.writeFile(resolved, JSON.stringify(data, null, 2), { - mode: 0o600, - }); - await fs.chmod(resolved, 0o600); + try { + await handle.write(JSON.stringify(data, null, 2)); + } finally { + await handle.close(); + } return resolved; } From cc1a1358f87fea677e79fd3c3d5aed2ff9a95252 Mon Sep 17 00:00:00 2001 From: gus Date: Sun, 10 May 2026 13:27:25 -0300 Subject: [PATCH 2/3] security(credential-output): scope POSIX-only protection explicitly Addresses review feedback on PR #94. Gates O_NOFOLLOW as constants.O_NOFOLLOW ?? 0 with an inline note on the Windows gap (O_EXCL still prevents overwriting a pre-existing entry, but the path no longer provides full no-follow protection there). Trims in-file comment; the full attack chain stays in the PR body and tests. Drops the biome-ignore directive that pointed at a non-existent rule. --- packages/cli/src/utils/credential-output.ts | 35 ++++++++------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/utils/credential-output.ts b/packages/cli/src/utils/credential-output.ts index 7739f21..039a98a 100644 --- a/packages/cli/src/utils/credential-output.ts +++ b/packages/cli/src/utils/credential-output.ts @@ -9,27 +9,13 @@ export async function writeCredentialFile( ): Promise { const resolved = path.resolve(filePath); - // Atomically create the output file with O_EXCL | O_NOFOLLOW so we never - // write through a pre-existing symlink. The previous implementation used - // fs.access() then fs.writeFile() which followed symlinks: an attacker on - // a shared filesystem (CI runner, multi-user host, container with shared - // tmp) could pre-plant a symlink at the operator's --output-file path, - // pre-open a file descriptor against the symlink target while it was - // world-readable, then read the credential through that fd after the - // operator's writeFile resolved the symlink and wrote the card data. - // The follow-up fs.chmod(resolved, 0o600) would then race-finalize the - // target permissions to owner-only — too late, the attacker's fd was - // open before chmod and survives it. - // - // O_NOFOLLOW makes open() refuse to traverse the final path component - // when it is a symbolic link (returns ELOOP). O_EXCL makes open() refuse - // to operate on a pre-existing file (returns EEXIST). The mode argument - // is only consulted when O_CREAT actually creates the file — combined - // with O_EXCL this guarantees the file is created here and nowhere else. + // Atomically create the credential file so we never write through a + // pre-existing output path. On POSIX, O_NOFOLLOW refuses to follow a + // symlink at the final path component; O_EXCL makes create fail if an + // entry already exists. The 0o600 mode is applied at create time, so + // the file is owner-only the moment it exists. if (force) { - // Remove any pre-existing entry, including a symlink. Use fs.unlink - // which operates on the symlink itself rather than its target. ENOENT - // is fine; anything else (EISDIR, EACCES) surfaces to the caller. + // fs.unlink operates on the symlink itself rather than its target. try { await fs.unlink(resolved); } catch (err) { @@ -37,12 +23,17 @@ export async function writeCredentialFile( } } + // O_NOFOLLOW is POSIX-only. On Windows, Node exposes it as 0, so the + // bitwise OR is a no-op there: O_EXCL still prevents overwriting a + // pre-existing entry, but open() does not provide full no-follow + // semantics on Windows. + const noFollowFlag = constants.O_NOFOLLOW ?? 0; + let handle: fs.FileHandle | undefined; try { handle = await fs.open( resolved, - // biome-ignore lint/suspicious/noBitwiseInsideUnaryExpression: standard open(2) flag composition - constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY | constants.O_NOFOLLOW, + constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY | noFollowFlag, 0o600, ); } catch (err) { From 75d7055b5604d54ab7d2a83c5321540ae278331e Mon Sep 17 00:00:00 2001 From: gus Date: Sun, 10 May 2026 16:08:09 -0300 Subject: [PATCH 3/3] security(credential-output): refuse symlinks even with --force Addresses review feedback on PR #94. The previous revision unlinked any pre-existing entry in --force mode and then ran the atomic open with O_EXCL | O_NOFOLLOW. That cleared the path for the create but also silently destroyed pre-existing symlinks, and the no-follow guarantee only covered the race window after the unlink. This refactor inverts the precheck: lstat first, refuse outright when the final path is a symlink (with or without --force), and only unlink and recreate for non-symlink existing files. O_EXCL | O_NOFOLLOW remains the race defense between the precheck and the atomic create. Tests updated. The without-force symlink case now expects OUTPUT_FILE_SYMLINK and asserts the symlink survives. The with-force case is renamed to make the new contract obvious and gets the same assertions. The dedicated post-unlink-race test is dropped because its premise (force unlinks the symlink) no longer applies; the regular-file race defense is exercised by the existing 0o600 test. --- .../utils/__tests__/credential-output.test.ts | 69 +++++-------------- packages/cli/src/utils/credential-output.ts | 38 +++++++--- 2 files changed, 45 insertions(+), 62 deletions(-) diff --git a/packages/cli/src/utils/__tests__/credential-output.test.ts b/packages/cli/src/utils/__tests__/credential-output.test.ts index 24dee98..a45fc57 100644 --- a/packages/cli/src/utils/__tests__/credential-output.test.ts +++ b/packages/cli/src/utils/__tests__/credential-output.test.ts @@ -51,8 +51,10 @@ describe('writeCredentialFile', () => { // The output file holds full card credentials. A pre-existing symbolic // link at the user's --output-file path lets an attacker on a shared // filesystem redirect the credential write through the link target. - // Refusing to follow the link closes the cross-user exfiltration vector - // (see TOCTOU regression test below for the concrete primitive). + // Refusing to follow the link closes the cross-user exfiltration vector. + // --force does not authorize symlink replacement: an operator who wants + // to overwrite a regular output file gets that, but a symlink at the + // path is rejected outright in either mode. it('refuses to write through a symbolic link without force', async () => { const filePath = path.join(tmpDir, 'card.json'); @@ -60,66 +62,31 @@ describe('writeCredentialFile', () => { await fs.writeFile(targetPath, '{}'); await fs.symlink(targetPath, filePath); - await expect(writeCredentialFile(filePath, { num: 1 }, false)).rejects.toThrow( - 'OUTPUT_FILE_EXISTS', - ); + await expect( + writeCredentialFile(filePath, { num: 1 }, false), + ).rejects.toThrow('OUTPUT_FILE_SYMLINK'); // The symlink target must not have been written. expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); + // The symlink itself must still be in place. + expect((await fs.lstat(filePath)).isSymbolicLink()).toBe(true); }); - it('refuses to write through a symbolic link with force', async () => { + it('refuses to overwrite a symbolic link even with --force', async () => { const filePath = path.join(tmpDir, 'card.json'); const targetPath = path.join(tmpDir, 'target.json'); await fs.writeFile(targetPath, '{}'); await fs.symlink(targetPath, filePath); - // With force the legacy implementation unlinked the symlink and wrote - // a fresh file at the same path. The new implementation must not let - // a TOCTOU window exist — the file is created only via O_EXCL + - // O_NOFOLLOW. Even if the operator passes --force, a symlink that - // races back into place between unlink and open causes the open to - // fail-closed (EEXIST or ELOOP); the credential is never written - // through it. - // - // For this synchronous test, we leave the symlink in place. Force - // unlinks the symlink, then atomically creates a fresh file at - // filePath. We assert that the target was not modified. - await writeCredentialFile(filePath, { num: 1 }, true); - + // --force is intended to replace an existing regular output file. It + // does not authorize destroying a symlink the operator may not have + // intended to remove, nor writing credentials over its target. + await expect( + writeCredentialFile(filePath, { num: 1 }, true), + ).rejects.toThrow('OUTPUT_FILE_SYMLINK'); // The symlink target must not have been written. expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); - // The actual file at filePath is now a regular file (not a symlink), - // owned by us, with the new credentials. - const lstat = await fs.lstat(filePath); - expect(lstat.isSymbolicLink()).toBe(false); - expect(lstat.isFile()).toBe(true); - expect(lstat.mode & 0o777).toBe(0o600); - expect(JSON.parse(await fs.readFile(filePath, 'utf-8'))).toEqual({ num: 1 }); - }); - - it('refuses to write through a symlink that races into place between unlink and create', async () => { - // Simulates the TOCTOU window in --force mode where an attacker - // re-plants a symlink between fs.unlink and the atomic open. Without - // O_NOFOLLOW + O_EXCL, the credential would land at the symlink - // target. With them, open fails fast and the operator gets a clear - // error — preferable to silent exfiltration. - const filePath = path.join(tmpDir, 'card.json'); - const targetPath = path.join(tmpDir, 'target.json'); - await fs.writeFile(targetPath, '{}'); - await fs.symlink(targetPath, filePath); - - // Force=false branch: existence check fires before the open. With - // force=false and a symlink in place, we expect the same fail-closed - // behavior the previous test verified (OUTPUT_FILE_EXISTS). - // - // Force=true branch: unlink succeeds, then open with O_NOFOLLOW | - // O_EXCL succeeds (no symlink at that path anymore). This is the - // common case. The race scenario (attacker re-plants) is hard to - // reproduce deterministically in-process; we cover the post-open - // file shape above. - await writeCredentialFile(filePath, { num: 2 }, true); - expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); - expect((await fs.lstat(filePath)).isSymbolicLink()).toBe(false); + // The symlink itself must still be in place. + expect((await fs.lstat(filePath)).isSymbolicLink()).toBe(true); }); it('produces a 0o600 file even when force is set', async () => { diff --git a/packages/cli/src/utils/credential-output.ts b/packages/cli/src/utils/credential-output.ts index 039a98a..22658f9 100644 --- a/packages/cli/src/utils/credential-output.ts +++ b/packages/cli/src/utils/credential-output.ts @@ -1,4 +1,4 @@ -import { constants } from 'node:fs'; +import { constants, type Stats } from 'node:fs'; import fs from 'node:fs/promises'; import path from 'node:path'; @@ -10,17 +10,33 @@ export async function writeCredentialFile( const resolved = path.resolve(filePath); // Atomically create the credential file so we never write through a - // pre-existing output path. On POSIX, O_NOFOLLOW refuses to follow a - // symlink at the final path component; O_EXCL makes create fail if an - // entry already exists. The 0o600 mode is applied at create time, so - // the file is owner-only the moment it exists. - if (force) { - // fs.unlink operates on the symlink itself rather than its target. - try { - await fs.unlink(resolved); - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + // pre-existing output path. lstat first to inspect what is there: a + // symlink at the final path component is rejected outright (even with + // --force), since --force is meant to replace an existing output file, + // not to authorize writing credentials over or through a symlink. A + // regular file is overwritten only when --force is set. The atomic + // create below uses O_EXCL | O_NOFOLLOW so the race window between the + // precheck and the create is fail-closed. + let existing: Stats | undefined; + try { + existing = await fs.lstat(resolved); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } + + if (existing?.isSymbolicLink()) { + throw new Error( + `OUTPUT_FILE_SYMLINK: ${resolved} is a symbolic link. Refusing to write credentials through it.`, + ); + } + + if (existing) { + if (!force) { + throw new Error( + `OUTPUT_FILE_EXISTS: ${resolved} already exists. Use --force to overwrite.`, + ); } + await fs.unlink(resolved); } // O_NOFOLLOW is POSIX-only. On Windows, Node exposes it as 0, so the