security: refuse to write credentials through pre-existing symlinks#94
Open
garagon wants to merge 1 commit intostripe:mainfrom
Open
security: refuse to write credentials through pre-existing symlinks#94garagon wants to merge 1 commit intostripe:mainfrom
garagon wants to merge 1 commit intostripe:mainfrom
Conversation
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 <id> --output-file <path>
--force` and an attacker on the same filesystem (CI runner, multi-tenant
host, container with shared /tmp) pre-plants a symlink at <path> pointing
at a file the attacker can already read, the attacker:
1. Plants <path> as a symbolic link to /tmp/<readable>.
2. Opens a read fd on /tmp/<readable> 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.
Contributor
raubrey-stripe
left a comment
There was a problem hiding this comment.
Thank you for this, some quick feedback + windows support!
| const resolved = path.resolve(filePath); | ||
|
|
||
| if (!force) { | ||
| // Atomically create the output file with O_EXCL | O_NOFOLLOW so we never |
Contributor
There was a problem hiding this comment.
Could we cut down on the comment size here to make this more concise?
| 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, |
Contributor
There was a problem hiding this comment.
It looks like O_NOFOLLOW is not supported on windows.
You can see in a similar vulnerability in a filelock python package here took a Windows specific patch in the "Patches" section.
If you're able we'd very much appreciate an investigation into the windows specific fix as well! But in the meantime, could you document this fix only applies to Posix? We can do a check for constants.O_NOFOLLOW !== 0 and make a decision about whether we support --auth-file as well here 🤔 cc @danhill-stripe
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
writeCredentialFile(packages/cli/src/utils/credential-output.ts, added in #67) is vulnerable to a TOCTOU exfiltration on shared filesystems. The previous flow was:If the operator runs
spend-request retrieve <id> --output-file <path> --forceand an attacker on the same filesystem (CI runner, multi-tenant host, container with shared/tmp) pre-plants a symbolic link at<path>pointing at a file the attacker can already read, the attacker can:<path>as a symbolic link to/tmp/<readable>./tmp/<readable>while it is still world-readable.writeFilefollows the symlink and writes the full card credential (PAN, CVC, billing address,valid_until) to the symlink target.fs.chmodthen locks the target down to0o600— too late, the attacker's fd was opened before the chmod and survives it (open fds keep their access mode regardless of subsequent permission changes).Verified end-to-end by a Node reproduction. Pre-fix, the attacker fd reads the JSON-encoded credential immediately after the operator's
writeCredentialFilecall returns:Fix
Replace
fs.access+fs.writeFile+fs.chmodwith a single atomic open:chmodfollow-up step is unnecessary (and the symlink-following bug it introduced is gone).O_NOFOLLOWmakesopenfail withELOOPif the final path component is a symbolic link.O_EXCLmakesopenfail withEEXISTif the file already exists.fs.unlink(operating on the symlink itself rather than the target viafs.writeFile), then takes the same atomic-create path. A re-planted symlink that races into place between the unlink and the open causesopento fail-closed instead of writing through.Post-fix verification:
Tests
packages/cli/src/utils/__tests__/credential-output.test.ts— four new cases:refuses to write through a symbolic link without force— symlink at the target path causesOUTPUT_FILE_EXISTS, and the link target is unchanged.refuses to write through a symbolic link with force—--forceunlinks the symlink and atomically creates a fresh regular file at the path. The original link target is unchanged. The new file is a regular file (not a symlink) with mode0o600.refuses to write through a symlink that races into place between unlink and create— sanity check on the--forcerace-shape. The fail-closed semantics are guaranteed byO_EXCL | O_NOFOLLOW.produces a 0o600 file even when force is set— regression guard for the removedfs.chmodstep. Mode is now set atopen()time via the third argument.Negative control: with the implementation reverted, two of the new tests fail because the symlink target is overwritten by the credential JSON (the existing
it('overwrites existing file with force', …)test still passes — the previous behavior was technically "compatible" with that assertion).Test plan
pnpm vitest run src/utils/__tests__/credential-output.test.tsgreen (8/8)pnpm testgreenpnpm typecheckgreen--force, the call refuses up front. With--force, the symlink is replaced by a fresh regular file at the target path; the original symlink target is unmodified and the attacker fd reads the pre-existing contents.Files