security(update): stage binary replacement at an unpredictable, exclusive path#751
Conversation
…sive path Fixes Gitlawb#742. installBinary staged downloaded release bytes at the fixed <target>.new path and opened it with O_CREATE|O_WRONLY|O_TRUNC. In an installation directory writable by a lower-privileged process, that process could pre-create <target>.new as a hard link or reparse point to another file the (possibly elevated) updater can write; the truncating open then overwrote that unintended file with verified Zero executable bytes. stagingFilePath now generates a cryptographically random suffix so the path can't be targeted in advance, and platform-specific createStagingFile opens it exclusively without following a pre-existing link: O_CREATE|O_EXCL on POSIX (which POSIX guarantees fails on a pre-existing path, symlink or not, without resolving it), and CreateFile with CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT plus a post-open GetFileInformationByHandle check on Windows (not a reparse point, not a directory, single hard link). replace_windows.go's rename-swap sequence is unchanged: the dangerous operation was always the truncating open of the staged path, not the .old rename/removal that follows. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the standalone updater’s staging step to prevent an arbitrary-file-overwrite primitive (Issue #742) by removing the predictable <target>.new staging path and ensuring staging files are created exclusively without link/reparse-point traversal.
Changes:
- Generate an unpredictable staging filename (crypto-random suffix) instead of using a fixed
.newname. - Introduce platform-specific
createStagingFileimplementations to guarantee exclusive creation and avoid link/reparse-point following (with defense-in-depth verification on Windows). - Add regression tests covering pre-created hard links/symlinks and a concurrent creation race, and update existing tests to use a deterministic staging suffix hook.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/update/apply.go | Randomizes staging filename and switches staging writes to createStagingFile. |
| internal/update/apply_test.go | Pins staging suffix in tests to deterministically occupy the computed staging path. |
| internal/update/stage_other.go | Adds POSIX `O_CREAT |
| internal/update/stage_other_test.go | Adds non-Windows regression tests for hard link/symlink pre-creation and a concurrency race. |
| internal/update/stage_windows.go | Adds Windows `CreateFile(CREATE_NEW |
| internal/update/stage_windows_test.go | Adds Windows regression tests for hard link/symlink pre-creation and a concurrency race. |
| internal/update/stage_test_helpers_test.go | Adds test helper to stub the random staging suffix deterministically. |
Comments suppressed due to low confidence (1)
internal/update/apply.go:233
- In installBinary, the staged file is only scheduled for removal after copyFile succeeds. If copyFile creates the staging file and then fails (e.g., disk full / short write), the partially-written random staging file will be left behind in the install directory. Consider deferring the cleanup immediately after stagingFilePath succeeds so failures during copyFile are also cleaned up.
if err := copyFile(sourcePath, stagedPath); err != nil {
return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err)
}
defer func() {
_ = os.Remove(stagedPath)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const attempts = 16 | ||
| var wg sync.WaitGroup | ||
| successes := make([]bool, attempts) | ||
| for i := range attempts { |
| const attempts = 16 | ||
| var wg sync.WaitGroup | ||
| successes := make([]bool, attempts) | ||
| for i := range attempts { |
WalkthroughThe updater now generates unpredictable staging filenames and creates staging files exclusively. POSIX and Windows implementations reject pre-existing links or invalid handles, while tests cover fresh creation, link protection, concurrent races, and helper refresh failures. ChangesSecure updater staging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant installBinary
participant stagingFilePath
participant copyFile
participant createStagingFile
installBinary->>stagingFilePath: Generate random staging path
installBinary->>copyFile: Copy binary to staged path
copyFile->>createStagingFile: Create path exclusively
createStagingFile-->>copyFile: Return validated file handle
copyFile-->>installBinary: Complete staged binary
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/update/apply.go (1)
225-234: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the
deferblock beforecopyFileto prevent leaking temporary files.If
copyFilefails (e.g., due to a full disk or an interrupted read),installBinaryreturns early and the partially written staging file is never removed, causing a permanent resource leak on every failed update.Moving the
deferblock immediately afterstagedPathgeneration guarantees cleanup across all error paths. This is entirely safe: ifreplaceBinarysuccessfully renames the file later,os.Removewill return a silentos.ErrNotExistthat safely gets ignored by the blank identifier.🛠 Proposed fix
stagedPath, err := stagingFilePath(targetPath) if err != nil { return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err) } + defer func() { + _ = os.Remove(stagedPath) + }() if err := copyFile(sourcePath, stagedPath); err != nil { return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err) } - defer func() { - _ = os.Remove(stagedPath) - }() if err := replaceBinary(targetPath, stagedPath); err != nil { return fmt.Errorf("install %s: %w", filepath.Base(targetPath), err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/apply.go` around lines 225 - 234, Move the cleanup defer in installBinary to immediately after successful stagingFilePath generation and before copyFile, so partially created staging files are removed when copying fails. Keep the existing os.Remove callback and ignored error behavior unchanged.
🧹 Nitpick comments (1)
internal/update/stage_other_test.go (1)
17-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated test files into a single cross-platform
stage_test.go. Both test files contain virtually identical test suites. As per coding guidelines, prefer one cross-platform function with small conditional checks over duplicated helpers when behavior can remain unified.
internal/update/stage_other_test.go#L17-L129: merge this test suite into a new unifiedstage_test.go.internal/update/stage_windows_test.go#L17-L130: merge this identical logic into the unified file, retaining the single conditionalif runtime.GOOS == "windows"for thet.Skipf("symlink unavailable...")skip logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/stage_other_test.go` around lines 17 - 129, Merge the duplicated test suites from internal/update/stage_other_test.go lines 17-129 and internal/update/stage_windows_test.go lines 17-130 into a single cross-platform internal/update/stage_test.go. Preserve the existing tests for createStagingFile, including hard-link, symlink, fresh-path, and concurrent-race coverage; retain one runtime.GOOS == "windows" conditional for the symlink-unavailable t.Skipf behavior, and remove the duplicated platform-specific test files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/update/apply.go`:
- Around line 225-234: Move the cleanup defer in installBinary to immediately
after successful stagingFilePath generation and before copyFile, so partially
created staging files are removed when copying fails. Keep the existing
os.Remove callback and ignored error behavior unchanged.
---
Nitpick comments:
In `@internal/update/stage_other_test.go`:
- Around line 17-129: Merge the duplicated test suites from
internal/update/stage_other_test.go lines 17-129 and
internal/update/stage_windows_test.go lines 17-130 into a single cross-platform
internal/update/stage_test.go. Preserve the existing tests for
createStagingFile, including hard-link, symlink, fresh-path, and concurrent-race
coverage; retain one runtime.GOOS == "windows" conditional for the
symlink-unavailable t.Skipf behavior, and remove the duplicated
platform-specific test files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 754ad950-6559-4c37-bbdf-f873fe80c35e
📒 Files selected for processing (7)
internal/update/apply.gointernal/update/apply_test.gointernal/update/stage_other.gointernal/update/stage_other_test.gointernal/update/stage_test_helpers_test.gointernal/update/stage_windows.gointernal/update/stage_windows_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approve. I went through this on the security question and could not find a bypass: the verified binary reaches disk only through an exclusively-created, no-follow handle at an unpredictable path, then gets renamed into place.
Checked on head f62c4fc:
- POSIX (stage_other.go): createStagingFile is O_CREATE|O_EXCL|O_WRONLY, so it fails on any pre-existing name including a dangling symlink, without following it. The suffix is 16 bytes from crypto/rand, not math/rand or time-derived.
- Windows (stage_windows.go): CreateFile with CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT operates on the reparse point itself and fails on a planted symlink or junction, and verifyFreshRegularFile then rejects reparse point / directory / NumberOfLinks>1 via GetFileInformationByHandle before any bytes are written. Belt and suspenders.
- Ordering is right: the checksum is verified before the binary is moved into the live target, and copyFile writes through the exact handle from createStagingFile with no reopen-by-path in between. No residual predictable
.newname remains. - Built and ran internal/update locally: gofmt, vet and build clean, tests pass including the hardlink/symlink refusal and checksum-mismatch cases. CI green.
One non-blocking nit worth a quick follow-up: installBinary registers defer os.Remove(stagedPath) after the copyFile error check (apply.go ~229-234), and copyFile only closes dest on an io.Copy failure, never removes it. So a mid-write copy failure (short write, full disk) leaks the partial staging file in the install directory. Not a security issue (128-bit random O_EXCL name, contents are a partial copy of the already-public binary), but easy to close by moving the removal defer into copyFile right after createStagingFile succeeds, plus a test for the mid-write path. Both bots flagged the same thing.
LGTM.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — the core hardening is correct and well-tested; the only residue is orphaned staging files, which is a pre-existing cleanup gap the randomization mildly amplifies (minor), plus a test-coverage nit.
The substantive fix for #742 is solid: createStagingFile opens with O_CREATE|O_EXCL|O_WRONLY (stage_other.go), so a pre-created hard link or symlink at the staging path fails EEXIST instead of being truncated through, and the Windows path adds CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT plus the verifyFreshRegularFile link-count/reparse checks. I reproduced the base arbitrary-file-truncate primitive (fixed <target>.new + O_TRUNC opened through a pre-created hard link) and confirmed the new code refuses it. The regression tests in stage_other_test.go genuinely exercise both the hard-link and symlink variants and assert the victim is untouched. Good, well-commented work.
[Minor] Orphaned <binary>.<hex>.new staging files are never reclaimed and now accumulate — PR-introduced amplification of a pre-existing gap
internal/update/apply.go:229-234
Two paths leave a staged file behind, and no code ever removes .new files (CleanupStaleBinary only handles <binary>.old, and on non-Windows it is a no-op — replace_other.go:19 / replace_windows.go:56):
- Copy failure (pre-existing ordering, PR-introduced amplification): the
defer os.Remove(stagedPath)is registered at line 232, aftercopyFileat line 229. IfcopyFilefails (ENOSPC on the install volume, unreadable source),installBinaryreturns at line 230 before the defer is ever registered, so the partial file survives. On base this ordering existed too, but the fixed<target>.newname meant the next attempt truncated and reused the same orphan (self-healing). With the PR's random suffix, each retry on a persistently full disk writes a fresh<binary>.<hex>.new, so partial-binary-sized files pile up in the install directory. - Hard crash mid-window (PR-introduced): if the process is SIGKILLed / loses power after
copyFilesucceeds (line 229) but beforereplaceBinaryrenames (line 235), the deferred remove never runs. On base the single fixed orphan was reused by the next upgrade; now each crashed upgrade leaves a uniquely-named orphan that nothing collects.
Neither is a security or correctness bug — the swap still works and disk pressure is the only cost — hence Minor. Two independent fixes close it: (a) register the staged-file cleanup immediately after stagingFilePath succeeds (or have copyFile/createStagingFile remove its own dest on a non-nil return) so the copy-failure exit path is covered; and (b) on startup, best-effort glob and remove stale <binary>.*.new files in the target directory, analogous to CleanupStaleBinary's .old handling, to sweep crash leftovers.
[Nit] The "unpredictable path" half of the fix has no mutation-sensitive test — PR-introduced
internal/update/apply.go:260
I reverted stagingFilePath to a fixed filepath.Base(targetPath) + ".new" (dropping the random suffix) while keeping O_EXCL, and go test -count=1 ./internal/update/ still passed. The new tests call createStagingFile directly with hand-built paths, and TestApplyStandaloneUpdateWarnsWhenHelperRefreshFails computes the occupied path via stagingFilePath itself, so both pass regardless of whether the suffix is random. The randomness (the title's stated defense-in-depth against pre-creation/guessing) can therefore be removed with zero test signal. Not a security hole — removing O_EXCL instead fails 3 tests, confirming that check is the real guard — but a small unit test asserting stagingFilePath(target) places the file in filepath.Dir(target), contains the base name, and returns two different paths on successive calls with the real randomStagingSuffix would lock in the property.
Tests: gofmt, go vet (incl. GOOS=windows), and go test -race -count=1 ./internal/update/... all pass on the PR head; no PR-attributable failures, no environmental interference in this package.
Merge is kevin's call per the program gate.
Summary
Fixes #742 — the Windows updater staged verified executable bytes at the predictable path
<target>.newand opened it with truncating, link-following semantics, letting a lower-privileged process pre-create that path as a hard link or reparse point to another file the (possibly elevated) updater can write.Root cause
installBinary(internal/update/apply.go) always staged at a fixed<target>.newname viacopyFile, which opened the destination withO_CREATE|O_WRONLY|O_TRUNC. In an installation directory writable by a lower-privileged principal, that principal could pre-create<target>.newas a hard link or supported reparse-point link to another file writable by the elevated updater. The subsequent staging write then truncated and overwrote that unintended file with the verified Zero executable bytes.The
.oldrename/removal sequence inreplace_windows.gowas not itself the vulnerable primitive (per the issue) and is unchanged.Changes
stagingFilePathnow derives the staging name fromcrypto/rand(128 bits), so it can no longer be predicted or pre-created before the update runs.createStagingFile(platform-specific) opens the staging path exclusively without following any pre-existing link:internal/update/stage_other.go(POSIX):O_CREATE|O_EXCL, which POSIX guarantees fails on a pre-existing path — including a dangling symlink — without resolving it.internal/update/stage_windows.go:CreateFilewithCREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINTso a pre-existing reparse point fails creation instead of being resolved through, plus a post-openGetFileInformationByHandlecheck (not a reparse point, not a directory, single hard link) as defense in depth.randomStagingSuffixis a swappable package var so the existing helper-refresh-failure test can pin a deterministic suffix instead of relying on the old guessable name.Tests
internal/update/stage_other_test.go/stage_windows_test.goreproduce the reported primitive directly: pre-create the staging path as a hard link (and, where privileges allow, a symlink) to a "victim" file, then assertcreateStagingFilerefuses it and the victim is untouched. Also covers the fresh-path success control and a concurrent-race case (exactly one winner, no silent truncation).TestApplyStandaloneUpdateWarnsWhenHelperRefreshFailsto pin the staging suffix via the new test hook instead of relying on the removed fixed.newname.Verification
go build ./...— pass (windows, plus cross-compiled linux/darwin forinternal/update)go vet ./...— pass (same platforms)go test ./internal/update/... -count=1— pass, including all new regression tests (hard link, symlink, concurrent race, fresh path) on Windowsgofmt -l— cleanSummary by CodeRabbit