fix: every file is written under a name nothing else holds - #63
Merged
Conversation
Files go to a temporary name and are renamed into place. Three of those
temporary names were created in a way that follows a link, so a link left
at one of them sent the bytes outside the output directory while the run
reported success.
Reproduced against a build of the previous tree, in a scratch directory:
a link at <manifest>.tfg-writing the manifest landed on a file outside
the output directory, exit 0, and then
verify said "matches" and cleanup said
"2 files removed", both exit 0
a link at <recipe>.tfg-writing recipe fmt -w wrote the recipe onto
another file and left the recipe as a
link, exit 0
a link at <manifest> an empty file appeared outside the
output directory, exit 0
The first needs no race and no guessing: the name is fixed and nothing
looked at it. On Windows none of it needs a privilege, because a hard link
is enough - which is also why os.Lstat alone does not close it, since a
hard link is an ordinary file to every question but the create itself.
core.CreateNew is now the one door. It creates exclusively and believes a
refusal only when os.Lstat finds an entry, so O_EXCL closes the hard link
and Lstat closes the link that points at nothing. The fallback that exists
because O_EXCL misreports a path running through a reparse point on Windows
is unchanged, and an output directory reached through a link still works.
Measured: 23 formats byte for byte identical, 48 refusals character for
character, and no measurable cost on the write path - txt 4 kB x2000 median
2170 to 2143 ms and png 200 kB x240 866 to 849 ms, ranges overlapping with
a canary of 27% and 19%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
A run writes each file to a temporary name and renames it into place. Three of those temporary names were created in a way that follows a link, so a link left at one of them by somebody else sent the bytes outside the output directory - and the run still reported success.
SECURITY.mdputs "a way to make the tool write or delete outside the directory it was given" first in scope, and says a path leaving the directory through a symbolic link is refused. That was true of the two reading commands and of the files a run produces. It was not true of the names those files are written under first.Reproduced against a build of the previous tree
<manifest>.tfg-writingverifythen said "matches" andcleanupsaid "2 files removed", both exit 0<recipe>.tfg-writingrecipe fmt -wwrote the recipe onto another file and left the recipe itself as a link, exit 0<manifest>The first needs no race and no pid guessing: the name is fixed and nothing anywhere looked at it. The manifest name does not have to be the default one. On Windows none of this needs a privilege, because a hard link is enough - which is also why
os.Lstaton its own does not close it. A hard link is an ordinary file to every question except the create.The fix
core.CreateNewis the one door now, called byengine.writeOne,manifest.claimName,manifest.writeOverandcore.writeWhole. It creates exclusively and believes a refusal only whenos.Lstatfinds an entry, soO_EXCLcloses the hard link andLstatcloses the link that points at nothing.The fallback that exists because
O_EXCLmisreports a path running through a reparse point on Windows is unchanged, and it is now reached only whenLstatagrees nothing is there. An output directory reached through a link keeps working, which is checked.What it costs
txt 4 kB x2000median 2170 to 2143 ms,png 200 kB x240866 to 849 ms, ranges overlapping with a canary of 27% and 19%.Behaviour change worth knowing
A leftover
.tfg-writingfile from a run that was killed part way used to be written over in silence. It is now a refusal naming the file. That is inCHANGELOG.mdunder Security.Guards
internal/guard/writeescape_test.go: four shapes of a held name (plain file, hard link, symbolic link to a file, symbolic link to nothing) across three writers, plus an AST guard that keeps file creation to the one door with a named exception list. Five mutations, all caught. The symbolic link cases skip loudly where the host refuses to create one - the hard link cases run everywhere.🤖 Generated with Claude Code