security(update): stage binary replacement at an unpredictable, exclusive path - #751
security(update): stage binary replacement at an unpredictable, exclusive path#751PierrunoYT wants to merge 21 commits into
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe updater replaces predictable pathname-based staging with exclusive, handle-bound staging objects. POSIX and Windows promotion paths verify object identity, preserve recovery state, classify possible tampering, and add coverage for link, race, substitution, and failure scenarios. ChangesSecure updater staging
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant installBinary
participant PlatformStaging
participant TargetBinary
participant Recovery
installBinary->>PlatformStaging: Create and populate verified staged object
PlatformStaging->>TargetBinary: Promote using handle-bound operation
PlatformStaging->>TargetBinary: Verify promoted object identity
PlatformStaging->>Recovery: Restore or preserve original after failure
Recovery-->>installBinary: Return recovery or tampering status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep the staged file bound to its verified handle through replacement
internal/update/apply.go:229-235
copyFilecloses the exclusively-created staging handle beforereplaceBinaryconsumesstagedPathby name. Under #742's documented model—where the lower-privileged principal can create or replace sibling entries—it can observe the new randomized filename, replace that entry after the close, and have the updater promote its replacement instead of the verified bytes. On POSIX,replaceBinaryfirst follows the substituted path inos.Chmodand then renames it into the executable path; Windows likewise performs the final rename by pathname after the handle has been closed. Randomizing the name and usingO_EXCLonly prevent pre-creation, not this live handoff race, so the updater can still install attacker-controlled code. Retain and validate object identity through the final swap (or use a handle-bound replacement primitive), and add an end-to-end substitution-race regression test. -
[P2] Clean up a staged file when copying it fails
internal/update/apply.go:229-234
The removal defer is registered only aftercopyFilereturns successfully. Ifio.CopyorClosefails aftercreateStagingFilehas created the destination (for example, on ENOSPC), that partial file is left behind. This change makes each retry use a fresh randomized*.newname, whereas the prior fixed name was reused, and startup cleanup only removes.oldfiles. Repeated failed upgrades can therefore accumulate release-sized artifacts until the installation volume is exhausted. Register cleanup before copying (and clean crash leftovers) so an unsuccessful update does not consume space permanently.
…pathname Addresses the two open findings on Gitlawb#751. [P1] The staged file is no longer promoted by pathname. Randomizing the name and creating it exclusively stops PRE-creation, but under Gitlawb#742's model — a lower-privileged principal that can write in the installation directory — the name can be observed after the fact and the entry replaced between the write and the swap, so the updater would install the substituted file. installBinary now keeps the creating handle open and promotes through it: - Windows renames through the handle itself (SetFileInformationByHandle/FileRenameInfo, hence the added DELETE access), so there is no second pathname lookup to win. replaceBinary is gone; promote owns the aside-rename of the running binary and its restore-on-failure retry. - POSIX has no rename-by-descriptor, so staging moves into a private directory created next to the target by os.MkdirTemp (mode 0700, random name, created exclusively). An attacker who can write in the installation directory cannot create, replace, or list entries inside it. promote additionally sets the executable bit through the handle (os.Chmod would re-resolve the path) and fails closed if the entry ever stops naming the object it wrote. [P2] Staging cleanup now covers every failure path: the removal defer is registered before the copy, and stageBinary discards the object if the copy fails, so a mid-write error (ENOSPC, unreadable source) no longer leaks a release-sized file that the next attempt will not reuse. CleanupStaleBinary also sweeps leftovers from a hard crash — staging files on Windows, staging directories on POSIX, where it used to be a no-op — skipping anything younger than an hour so a concurrent update is never disturbed. Tests: substitution-race regression per platform (Windows asserts the verified bytes are installed despite a replaced staging entry; POSIX asserts the private 0700 directory and that promote refuses a substituted entry), success controls, cleanup-on-failure, and the crash-leftover sweep. The forced-staging-failure test for helper refresh now uses a stageBinary seam, because the staging location can no longer be occupied from outside — which is the point of the fix.
fa02593
|
Pushed [P1] Keep the staged file bound to its verified handle through replacement — this was a real hole and the fix is structural:
[P2] Clean up a staged file when copying it fails (also raised by @Vasanthdev2004, @gnanam1990, CodeRabbit and Copilot) — the removal defer is registered before the copy, and Tests
Validation — honest about the platform limits of this host (Windows, Go 1.26.5):
@jatmn @Vasanthdev2004 @gnanam1990 — this is a bigger change than the previous round (the promotion primitive itself changed per platform), so a fresh look at @coderabbitai review |
|
✅ Action performedReview finished.
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Fix the Windows handle-promotion path before merging
internal/update/stage_windows.go:106-118
The required Windows job is red on the current head:TestPromoteInstallsTheStagedObjectNotTheStagedPathgets a nil result fromstaged.promote, but after closing the handlezero.exedoes not exist. At that point the old executable has already been moved to.oldandpromotedsuppresses deferred removal, so an update can report success while stranding the user without the executable. Correct theFILE_RENAME_INFOconstruction/handle rename behavior and keep an end-to-end Windows assertion that the verified binary is reachable attargetPath. -
[P1] Keep the POSIX staging directory bound through the final rename
internal/update/stage_other.go:65-69
A0700directory protects its contents, but not its directory entry: under the issue's writable-install-directory threat model, an attacker can rename the.zero-stage-*directory from its writable parent afterverifyStagedIdentityreturns, recreate that directory with an attacker-controlled file at the same basename, and win the gap beforeos.Rename(staged.path, targetPath). The final path-based rename then installs attacker bytes. The current test only substitutes the child entry before the check, so it misses the ancestor replacement race; use a promotion primitive that remains safe when the parent namespace can change and cover this race end to end. -
[P1] Do not leave a raced target executable in place on Windows
internal/update/stage_windows.go:101-114
After the updater renames the running binary to.old, a writer of the installation directory can create a malicioustargetPath.renameFileByHandledeliberately usesReplaceIfExists=false, so promotion fails; the restore also fails because that attacker file occupiestargetPath. The function returns an error but leaves the attacker-controlled file at the executable path for the next launch, with the original only at.old. Restore/replace must safely handle this namespace race rather than treating it as a harmless failed update. -
[P2] Restrict stale cleanup to artifacts the updater can prove it owns
internal/update/stage_other.go:114-122
internal/update/replace_windows.go:55-64
Cleanup runs on everyApply, including the no-update path, but recognizes arbitrary old sibling entries by loose names: every.zero-stage-*directory is recursively removed on POSIX and every<binary>.*.newfile is removed on Windows. Those are broader than the generated artifact formats and can match legitimate user data such as.zero-stage-backuporzero.exe.release-notes.new. The pathname is also re-resolved after the age check, allowing a writable-parent actor to swap a checked directory beforeRemoveAll. Validate exact owned artifacts and avoid recursive deletion of untrusted pathnames before deleting anything. -
[P3] Do not age out an update that is still copying
internal/update/stage_other.go:118-122
The POSIX cleanup uses the staging directory's mtime as its liveness signal, butcopyFromonly writes the child file and never refreshes that timestamp. If a copy is slow or stalled for over an hour, a secondzero upgraderemoves the live staging directory; the first updater then fails identity verification or promotion even though it is still active. Track live staging independently, refresh a liveness marker, or avoid age-based removal of active directories.
Address jatmn's review findings on PR Gitlawb#751: - Windows promote() now verifies targetPath is actually reachable after a reported-successful handle rename before trusting it. Some Windows versions have been observed accepting SetFileInformationByHandle against a handle whose directory entry was substituted out from under it without the object actually moving, which let promote report success while targetPath was left missing entirely. renameFileByHandle is now a package var so this is covered by a deterministic regression test rather than relying on reproducing the exact trigger condition. - When a promotion failure's restore-to-.old also fails (a writable-parent attacker occupying targetPath with a lock MOVEFILE_REPLACE_EXISTING can't get past), that combination is now wrapped in ErrTargetPossiblyTampered instead of reading like an ordinary failed update, with a best-effort MOVEFILE_DELAY_UNTIL_REBOOT fallback so an admin-context updater can still recover the original at next boot. - POSIX promote() now binds its final rename to a directory descriptor opened when the staging directory is created (unix.Renameat), not a pathname. The 0700 staging directory protects its contents from a writable-parent principal, but not its own directory entry — that principal could rename the staging directory aside and recreate a look-alike with an attacker file at the same basename in the gap between the identity check and the rename, which a plain os.Rename would silently follow. - Stale-cleanup on both platforms now matches the exact generated artifact shape (POSIX: prefix + os.MkdirTemp's all-digit suffix; Windows: prefix + 32 lowercase hex chars + ".new") instead of a loose prefix/suffix, so a user's own similarly-named file or directory is never swept up. Both also re-check identity immediately before deleting to shrink the window a writable-parent principal could swap the checked path in. - copyFrom now copies in chunks and refreshes the POSIX staging directory's mtime between them, since writing into an already-created file never touches the parent directory's own mtime — a large or slow copy could previously look abandoned to a concurrent update's cleanup sweep while still in progress. Verified on real Windows and Linux (via WSL), plus cross-compiled build/vet checks for darwin/linux-arm64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
internal/update/replace_windows.go (1)
98-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup-sweep logic duplicates the POSIX implementation's structure.
Same scan→age-filter→identity-recheck→delete skeleton as
removeStaleStagingLeftoversininternal/update/stage_other.go, only the name predicate (isGeneratedStagingFileNamevs.isGeneratedStagingDirName) and the deletion call (os.Removevs.os.RemoveAll) differ. See the consolidated comment.🤖 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/replace_windows.go` around lines 98 - 129, Consolidate the duplicated cleanup-sweep logic shared by removeStaleStagingLeftovers and the POSIX implementation in stage_other.go into a reusable helper. Parameterize the name predicate and deletion operation so file cleanup uses isGeneratedStagingFileName with os.Remove, while directory cleanup uses isGeneratedStagingDirName with os.RemoveAll; preserve the existing age and identity-recheck behavior.Source: Coding guidelines
internal/update/stage_other.go (1)
152-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup-sweep logic is nearly identical to the Windows counterpart.
This scan→age-filter→identity-recheck→delete loop mirrors
removeStaleStagingLeftoversininternal/update/replace_windows.goalmost line for line, differing only in the name-matching predicate and whether the target is a directory (RemoveAll) or a file (Remove). See the consolidated comment for a suggested shared helper.🤖 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.go` around lines 152 - 184, Consolidate the duplicated scan, age-filter, identity-recheck, and deletion logic shared by removeStaleStagingLeftovers and its Windows counterpart into a reusable helper. Parameterize the helper for the existing name predicate and directory/file deletion behavior, then update both callers while preserving their current cleanup semantics.Source: Coding guidelines
internal/update/stage_promote_windows_test.go (1)
172-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
assertNoStagingLeftovershelper vs. the POSIX test file.This Windows-only helper (matching
.newsuffix) duplicates the intent ofassertNoStagingLeftoversinstage_promote_other_test.go(which also matches the generated staging-directory prefix). See consolidated comment for the proposed unification. As per coding guidelines, "Prefer one cross-platform function with small conditional checks over duplicated platform-specific helpers when behavior can remain unified."🤖 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_promote_windows_test.go` around lines 172 - 184, Unify the Windows-only assertNoStagingLeftovers helper with the cross-platform helper in stage_promote_other_test.go, removing the duplicate definition from the Windows test file. Preserve checks for both the generated staging-directory prefix and the Windows “.new” suffix using small platform-appropriate conditions within the shared helper.Source: Coding guidelines
internal/update/stage_promote_other_test.go (2)
134-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only checks the end-state, not "during copy" behavior it claims to regress-test.
The test forces small chunks and asserts the staging directory's mtime is fresh after
copyFromreturns. An implementation that only refreshes the mtime once at the very end of the copy (rather than periodically "as it goes," per the doc comment's stated intent) would pass this assertion too — yet that implementation would not actually fix the concurrent-sweep-sees-stale-mtime-mid-copy race this test is meant to guard against. Consider asserting freshness partway through the copy (e.g., wrap the source reader to pause/check after the first chunk) to actually exercise the "during copy" guarantee.🤖 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_promote_other_test.go` around lines 134 - 176, Strengthen TestCopyFromRefreshesStagingLivenessDuringCopy to verify the staging directory mtime during copy, not only after staged.copyFrom returns. Wrap or control the source read so the test pauses after the first chunk, checks that staged.dir has a mtime newer than stale, then allows copying to finish; retain the final completion assertion and cleanup behavior.
282-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
assertNoStagingLeftovershelper vs. the Windows test file.This helper (matching generated staging prefix or
.newsuffix) duplicatesassertNoStagingLeftoversinstage_promote_windows_test.go(which matches.newsuffix only). Since checking both patterns is harmless on either platform (the POSIX-only prefix pattern will simply never match on Windows and vice versa), this looks like a good candidate to unify into a single helper in the sharedstage_test_helpers_test.gorather than maintaining two near-identical, platform-forked copies that can drift. See consolidated comment for details. As per coding guidelines, "Prefer one cross-platform function with small conditional checks over duplicated platform-specific helpers when behavior can remain unified."🤖 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_promote_other_test.go` around lines 282 - 294, Move the shared assertNoStagingLeftovers helper into stage_test_helpers_test.go and remove the duplicate definitions from the POSIX and Windows test files. Preserve both checks in the unified helper: stagingDirPrefix matches and the .new suffix matches, so leftover detection remains consistent across platforms.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.
Nitpick comments:
In `@internal/update/replace_windows.go`:
- Around line 98-129: Consolidate the duplicated cleanup-sweep logic shared by
removeStaleStagingLeftovers and the POSIX implementation in stage_other.go into
a reusable helper. Parameterize the name predicate and deletion operation so
file cleanup uses isGeneratedStagingFileName with os.Remove, while directory
cleanup uses isGeneratedStagingDirName with os.RemoveAll; preserve the existing
age and identity-recheck behavior.
In `@internal/update/stage_other.go`:
- Around line 152-184: Consolidate the duplicated scan, age-filter,
identity-recheck, and deletion logic shared by removeStaleStagingLeftovers and
its Windows counterpart into a reusable helper. Parameterize the helper for the
existing name predicate and directory/file deletion behavior, then update both
callers while preserving their current cleanup semantics.
In `@internal/update/stage_promote_other_test.go`:
- Around line 134-176: Strengthen TestCopyFromRefreshesStagingLivenessDuringCopy
to verify the staging directory mtime during copy, not only after
staged.copyFrom returns. Wrap or control the source read so the test pauses
after the first chunk, checks that staged.dir has a mtime newer than stale, then
allows copying to finish; retain the final completion assertion and cleanup
behavior.
- Around line 282-294: Move the shared assertNoStagingLeftovers helper into
stage_test_helpers_test.go and remove the duplicate definitions from the POSIX
and Windows test files. Preserve both checks in the unified helper:
stagingDirPrefix matches and the .new suffix matches, so leftover detection
remains consistent across platforms.
In `@internal/update/stage_promote_windows_test.go`:
- Around line 172-184: Unify the Windows-only assertNoStagingLeftovers helper
with the cross-platform helper in stage_promote_other_test.go, removing the
duplicate definition from the Windows test file. Preserve checks for both the
generated staging-directory prefix and the Windows “.new” suffix using small
platform-appropriate conditions within the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c330dc9-d61a-49c6-9d09-7c211cf38cc5
📒 Files selected for processing (7)
internal/update/apply.gointernal/update/replace_windows.gointernal/update/replace_windows_test.gointernal/update/stage_other.gointernal/update/stage_promote_other_test.gointernal/update/stage_promote_windows_test.gointernal/update/stage_windows.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/update/apply.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore gofmt cleanliness before merging
internal/update/replace_windows_test.go:90
The required Ubuntu smoke job currently fails its formatting step on this file, so vet, tests, build, and smoke are all skipped. Run gofmt and commit the resulting alignment change. -
[P1] Bind the POSIX staging directory before an attacker can replace it
internal/update/stage_other.go:41
MkdirTempcreates the private directory, but the code subsequently opens it again by pathname. A principal that can write the installation directory can rename that entry and recreate it beforeos.Open, makingdirHandlerefer to an attacker-writable directory. The attacker can then replace the child afterverifyStagedIdentityand beforeRenameat, causing the verified updater to install attacker bytes. Acquire/validate the created directory without this pathname handoff and cover that interleaving. -
[P1] Do not recursively remove an attacker-substituted staging directory
internal/update/apply.go:337
The POSIX promotion deliberately tolerates the staging directory being renamed aside, but deferreddiscardlater callsRemoveAll(staged.dir)through the old pathname. An attacker can replace that pathname with a chosen directory; the elevated updater then recursively deletes the replacement. The stale-cleanup path instage_other.go:177-182has the same Lstat-to-RemoveAll race. Keep deletion bound to the original object or fail closed rather than recursively deleting a mutable pathname; the ancestor-replacement test should assert the impostor survives deferred cleanup. -
[P1] Verify that the promoted Windows target is the staged object
internal/update/stage_windows.go:149
This check accepts any non-directory attargetPath. In the anomalous success case it is meant to handle—handle rename reports success but does not link the staged file—an attacker with write access to the installation directory can create a regular malicious target beforeos.Stat. The check then passes and the update is reported successful with attacker bytes. Verify object identity, not only target existence. -
[P1] Do not schedule reboot recovery from a mutable source pathname
internal/update/replace_windows.go:66
After an immediate restore failure, this queues<target>.oldto replace the executable at reboot. Under the stated writable-directory threat model, an attacker can replace that source entry before reboot, turning the recovery action into a delayed installation of attacker-selected bytes. Surface the recovery failure or use a recovery source outside the attacker-controlled directory. -
[P2] Do not infer cleanup ownership from a public filename shape
internal/update/stage_other.go:163
Any old directory named.zero-stage-<1..10 digits>is treated as an updater artifact and recursively removed; no provenance distinguishes it from user data. Windows makes the same assumption for any exact<binary>.<32 hex>.newfile inreplace_windows.go:109. Exact pattern matching reduces accidental collisions but does not establish ownership, so preserve non-verifiable entries or introduce durable, unforgeable ownership metadata with an appropriate migration story.
|
Addressed the requested changes in
Validation completed:
Environment limitations: the repository-wide test run reaches an unrelated CLI completion test that requires an installed WSL distribution, and |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Recovery markers on random aside paths are not consulted before promotion retries
internal/update/stage_windows.go:115-121,internal/update/replace_windows.go:61
After the first successful Windows upgrade,<target>.oldis always left behind (CleanupStaleBinaryis now a no-op), so every laterpromoteuses a random aside path when.oldalready exists (stage_windows.go:134-140). If that promotion fails andrestoreOriginalBinarycannot put the aside copy back,markOldBinaryPreservedwrites<target>.<suffix>.old.keep. The nextpromoteonly callsoldBinaryPreservedon the canonical<target>.oldpath, so it never sees the marker on the aside file. Because the stale canonical.oldhas no.keep, a retry proceeds over an unverifiedtargetPathwhile the marked recovery copy sits on the aside path the refusal logic does not watch. This is systematic on second and later failed updates, not an edge case. Please gate promotion on markers for whichever aside path was actually used (or scan for any*.keepbeside a sibling*.old), and add a Windows regression that leaves a stale<target>.old, fails promotion with aside at<target>.<suffix>.oldmarked, then asserts the nextinstallBinaryrefuses instead of proceeding. -
[P2] Missing-target recovery guidance can point at the wrong
.oldfile
internal/update/stage_windows.go:123-128,internal/update/stage_windows.go:134-140
After an interrupted aside-rename on a second or later upgrade, disk state can be:targetPathmissing, a stale<target>.oldfrom an earlier successful upgrade, and the actual last running binary at<target>.<suffix>.oldfrom the interrupted attempt. The refusal at lines 123-128 only names<target>.oldas the recoverable copy. An operator following that message can restore the wrong (older) binary.TestPromoteRefusesRetryAfterInterruptedAsidecovers only the single-.oldlayout. Please name the aside path that actually holds the last moved-aside binary, or refuse until the operator resolves ambiguous multi-.oldlayout, and add a Windows test with both a stale canonical.oldand a fresh random aside present whiletargetPathis absent. -
[P2] A partial marker write can orphan
.keepand cite the wrong recovery path
internal/update/replace_windows.go:135-145,internal/update/replace_windows.go:61-69
markOldBinaryPreservedcreates the marker exclusively, but ifWriteStringorClosefails it returns an error without removing the newly created file. WhenkeepUnmarkedRecoveryCopythen succeeds, the recovery bytes move to*.<suffix>.recoverywhile a.keepmarker remains at the old path.oldBinaryPreservedstill returns true for that path, so laterpromoterefuses fail-closed (good) but names the wrong location as the verified copy. This needs a disk-full-style failure and is narrower than the P1 aside-path gap, but the wrong-path error text is still misleading during incident response. Please roll back or finalize the marker on write/close failure before relocating the copy, or consult the relocated path inoldBinaryPreserved, and add regression coverage for marker-write failure followed by relocation. -
[P3] Operator error text still promises cleanup that no longer exists
internal/update/replace_windows.go:71-73
When marker creation and recovery relocation both fail, the error tells the operator that a later update will otherwise remove<target>.old.CleanupStaleBinaryis now a no-op on all platforms and is no longer called fromApply, so that removal never happens. This is stale wording left over from the old cleanup contract, not a logic bug, but it can mislead incident response. Please update the message to say manual copy is required.
A failed second-or-later update marks the randomized aside it used, but promotion only consulted the canonical <target>.old marker, so a retry proceeded over an unverified target while the verified copy sat marked on a path the refusal logic did not watch. Promotion now enumerates every recovery candidate — canonical and randomized asides — and refuses while any of them is marked, naming each recovery path and its marker. The missing-target refusal had the same single-path blind spot: with a stale canonical .old from an earlier successful update plus the aside from an interrupted attempt, it named only the canonical path, which can be the wrong binary. It now names the single actual candidate or refuses the ambiguous layout outright until the operator resolves it. A failed marker write could also leave a partial .keep behind while the recovery copy was relocated elsewhere; later refusals then named a location that no longer held the verified bytes. markOldBinaryPreserved now removes the entry it created before reporting a write/close failure, and conservatively reports success when that removal (or the state check) fails so oldPath stays authoritative. keepUnmarkedRecoveryCopy is handle-bound end to end: it pins the recovery copy with a no-delete-sharing, no-reparse open, verifies it is a single-link regular file, renames through the handle with ReplaceIfExists false, and confirms identity at the destination, failing closed with ErrTargetPossiblyTampered on any race. Finally, the error for an unestablishable marker still promised that a later update would otherwise remove the copy, but CleanupStaleBinary is now a no-op; the operator is told that a manual copy is required.
|
Thanks for the review — all four findings are addressed in [P1] Recovery markers on random aside paths not consulted: [P2] Missing-target guidance could name the wrong [P2] Partial marker write can orphan [P3] Stale cleanup promise: the error now tells the operator a manual copy is required; Both open CodeRabbit comments are covered by the same commit: the partial-marker rollback above, and Validation: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/update/replace_windows.go (1)
144-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoolean logic is correct but fragile — mixing
&&/||across three sub-conditions on one line.I traced all realistic
(removeErr, statErr)combinations and the current precedence does resolve correctly (returns nil/"conservatively preserved" exactly when removal/absence can't be positively confirmed), but it's an easy trap for a future edit to silently break.♻️ Suggested clarification (no behavior change)
removeErr := os.Remove(markerPath) _, statErr := os.Lstat(markerPath) - if removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) || - statErr == nil || !errors.Is(statErr, os.ErrNotExist) { + removed := (removeErr == nil || errors.Is(removeErr, os.ErrNotExist)) && + errors.Is(statErr, os.ErrNotExist) + if !removed { + // Could not positively confirm the partial marker was cleaned up; + // conservatively keep treating oldPath as marked/preserved. return nil }🤖 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/replace_windows.go` around lines 144 - 153, Clarify the compound condition in the recovery-marker cleanup flow around removeErr and statErr by adding explicit grouping or splitting it into named boolean checks, without changing its current behavior. Preserve the existing conservative return-nil cases and the subsequent writeErr/closeErr reporting.
🤖 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.
Inline comments:
In `@internal/update/replace_windows.go`:
- Around line 172-196: Update keepUnmarkedRecoveryCopy to return the attempted
kept path alongside the verification error when renameRecoveryFileByHandle
succeeds but verifyPromotedTarget fails; retain the empty path for failures
before the rename. Update restoreOriginalBinary’s caller handling to check kept
!= "" and report that unverified recovery path, falling back to oldPath only
when no move was attempted.
In `@internal/update/stage_windows.go`:
- Around line 197-215: Update existingRecoveryPaths to match recovery filenames
case-insensitively, including the base-name prefix and .old suffix checks, so
files such as zero.exe.OLD are included. Preserve the existing minimum-length
guard and path construction behavior.
---
Nitpick comments:
In `@internal/update/replace_windows.go`:
- Around line 144-153: Clarify the compound condition in the recovery-marker
cleanup flow around removeErr and statErr by adding explicit grouping or
splitting it into named boolean checks, without changing its current behavior.
Preserve the existing conservative return-nil cases and the subsequent
writeErr/closeErr reporting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4118db2f-21db-4e83-8689-13ae6fefb3d6
📒 Files selected for processing (4)
internal/update/replace_windows.gointernal/update/replace_windows_test.gointernal/update/stage_promote_windows_test.gointernal/update/stage_windows.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/update/replace_windows_test.go
…stitution TestPromoteInstallsTheStagedObjectNotTheStagedPath deletes the staging file's only directory entry and recreates it as an "attacker" file, then required promote to succeed with the verified bytes installed. CI's windows-latest runner permits that delete (this workstation's exclusive share mode blocks it, so the path went unexercised here); on that runner, renaming the now fully-unlinked staging handle back into existence is not something every Windows build honors, so verifyPromotedTarget's post- rename identity check finds nothing at targetPath, promote fails, and restoreOriginalBinary moves the pre-update binary back — a safe, fail- closed outcome, not a security regression. The test now accepts either outcome after a real substitution: promote succeeding with the verified bytes, or promote failing as long as the attacker's substituted bytes were never installed. Any promote failure that occurs without substitution having actually happened still fails the test outright.
|
CI failure on Root cause: That delete is supposed to fail everywhere (share mode The test asserted "promote must always succeed" and didn't allow for that legitimate failure mode. It now accepts either outcome once real substitution occurred: promote succeeding with the verified bytes, or promote failing as long as the attacker's bytes were never installed. A promote failure that happens without substitution having actually occurred still fails the test outright, so this doesn't mask a real regression. Validation: |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
The #742 staging hardening on head eeb44e2 looks sound: unpredictable exclusive staging, handle-bound Windows promotion with post-rename identity verification, descriptor-bound POSIX promotion/cleanup, and the adversarial regression coverage address the reported arbitrary-file-overwrite primitive. Your commits since fd383fd3 also resolve my earlier blockers around marked-recovery overwrite on retry, startup cleanup deleting preserved .old copies, helper ErrTargetPossiblyTampered propagation, and relocating the recovery copy when the marker cannot be written.
The remaining blockers below are real gaps in the Windows recovery/cleanup contract this PR added after that core staging work. They are not regressions in the #742 fix itself, and they are not documentation drift — findings 1 and 2 are omissions in new recovery-path logic; finding 3 is incomplete follow-through on the same writable-install-directory threat model on a failure-path cleanup that still uses pathname removal after the exclusive handle is released.
Findings
-
[P2] Propagate the authoritative recovery path when relocation verification fails
internal/update/replace_windows.go:65-74
internal/update/replace_windows.go:189-194
WhenmarkOldBinaryPreservedfails,keepUnmarkedRecoveryCopyhandle-renames the verified recovery bytes away from<target>.oldand then callsverifyPromotedTarget. The success branch at lines 65–69 correctly names the new*.recoverypath in the outer error. ThekeepErr != nilbranch at lines 71–74 always tells the operator tomanually copy <target>.old somewhere safe nowand dropskeepErr, even when the handle-bound rename already succeeded andoldPathis empty. In that case the only verified copy lives at the*.recoverypath named insidekeepErr, not atoldPath. The bytes are not lost, but the surfaced guidance points at a path that no longer exists. Please propagatekeepErr(or extract and include thekeptpath) in the fallback branch, and add a Windows regression that stubs a failingverifyPromotedTargetafter a successfulrenameRecoveryFileByHandle—TestRestoreOriginalBinaryKeepsRecoveryCopyWhenMarkingFailsonly covers the success path today. -
[P2] Include
*.recoverycopies in the recovery state machine
internal/update/stage_windows.go:197-232
internal/update/replace_windows.go:183-195
keepUnmarkedRecoveryCopyrelocates the last verified binary to<base>.<suffix>.recoverywhen marker creation fails.existingRecoveryPathsandmarkedRecoveryPathsonly scan*.oldand*.<suffix>.old(lines 207–211), so a surviving*.recoverysibling is invisible to the pre-promotion refusal logic. After a successful relocation, a retry can callpromotewithout failing closed on that orphaned copy — unlike marked.oldrecovery, whichTestPromoteRefusesWhileRecoveryCopyIsMarkedalready guards. Please extend recovery inspection to treat*.<suffix>.recoveryas authoritative recovery state (or otherwise refuse promotion while one exists) and add coverage for retry after a successful relocation. -
[P2] Bind Windows staging cleanup to the staged object, not its pathname
internal/update/apply.go:328-335
internal/update/stage_windows.go:290-297
This PR hardened staging creation and promotion under the writable-install-directory threat model but left failure-path cleanup on a pathname re-resolve.installBinary’s deferreddiscardcloses the exclusive staging handle and thendiscardPathscallsos.Remove(staged.path)(POSIX counterpart unlinks through the bound directory descriptor after an identity check instage_other.go). A principal who can write the install directory and observes the random*.newpath can substitute that directory entry in the gap betweenCloseandRemove, so cleanup may unlink an attacker-chosen entry instead of the staging object this updater created. This is not the original #742 overwrite primitive — it affects failed-attempt cleanup, not verified-byte installation — but it is inconsistent with the handle-bound hardening applied elsewhere in this change. Please delete the staging object through the handle (for exampleFileDispositionInfo) before releasing it, or otherwise avoid pathname-based removal after the exclusive handle is dropped, and add a regression patterned after the POSIX impostor-survival tests.
…itively keepUnmarkedRecoveryCopy discarded the destination path when the rename to it succeeded but the post-rename identity verification failed, leaving restoreOriginalBinary's error telling the operator to save oldPath after it had already been vacated. Return the attempted kept path alongside the verification error so the caller can still name it. existingRecoveryPaths compared "<target>.old" filenames case-sensitively, but NTFS is case-insensitive/case-preserving, so a recovery or marker file spelled e.g. "zero.exe.OLD" could be silently skipped by every caller's fail-closed check. Fold both sides before matching. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes — two things, both narrow, then this is good to go.
First, credit where it's due: the core #742 fix is solid. I mutation-tested the three guards it rests on and all three are genuinely pinned, not just asserted:
- swapping
CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINTforCREATE_ALWAYSincreateStagingFilemakesTestCreateStagingFileRefusesPrecreatedHardLinkfail withvictim content = ""(the truncate lands), and the concurrent test drops to 9 winners - deleting the
verifyPromotedTargetcall inpromotemakesTestPromoteRejectsALyingRenameByHandlefail - disabling the marked-recovery refusal makes both
TestPromoteRefusesWhileRecoveryCopyIsMarkedandTestPromoteRefusesMarkedRandomAsideRecoveryfail
go build ./..., go vet (windows/linux/darwin), gofmt -l and go test ./internal/update/... -count=1 are clean on 76735375.
1. CleanupStaleBinary became a no-op with no production callers, so every upgrade leaks a full binary forever
internal/update/replace_windows.go:260, internal/update/stage_other.go:197, internal/update/apply.go:60-71
On main, Apply called CleanupStaleBinary(executablePath) (apply.go:62) and the Windows implementation removed <target>.old. This PR drops the call from Apply and empties both implementations — grep -rn CleanupStaleBinary finds no non-test caller left anywhere in the tree. The exported symbol is dead API whose only remaining "coverage" is four tests that pass vacuously against an empty function.
I drove the real installBinary four times against one target. The install directory afterwards:
zero.exe
zero.exe.old
zero.exe.453803e71c0448d5e801e488b92c5b55.old
zero.exe.b3e041ea951fed2707d8c1b21012e2ab.old
zero.exe.cd68e3e86d1d42ddba78947a8c418a2d.old
bin/zero.exe is 23.9 MB, and the two Windows helpers leak their own .old copies through the same path. Nothing in the product ever removes any of them, and there's no zero doctor-style cleanup to point an operator at.
There's a second-order effect too. Because <target>.old now survives forever, existingRecoveryPaths returns ≥2 entries on every install past its second upgrade. I removed the target after two upgrades and retried:
install zero.exe: target executable path may hold unverified content after a failed update:
...\zero.exe is missing and multiple recovery binaries exist
(...\zero.exe.4c20eb97db6e3ffc8fed777a97760aa5.old, ...\zero.exe.old);
resolve the ambiguous recovery state before updating again
So the case 1: branch at stage_windows.go:141-145 — the one that names the single recoverable binary, which is the good guidance — is unreachable on any real install, and TestPromoteRefusesRetryAfterInterruptedAside only exercises the first-ever-upgrade layout that produces it.
I understand why you removed the cleanup: a public pathname can't prove an .old is obsolete when the directory is attacker-writable, and I don't want that reasoning reverted. But "never clean up anything" isn't the only fail-safe answer. The updater does know, at promotion time, exactly which path it just renamed aside and what was in it. Recording that in updater-owned state outside the install directory (config dir), and deleting only paths that state vouches for on the next successful promotion, gets you bounded growth without trusting a filename. Deleting the aside copy in-process isn't an option for the main binary since it's the running image, but it is for the helpers, which aren't running.
Whatever shape you pick, please leave the install directory bounded and add a regression that runs installBinary three or four times and asserts the leftover count doesn't grow without limit.
2. A relocated *.recovery copy is invisible to the refusal logic, so the next update walks over an unverified target
internal/update/stage_windows.go:197-220, internal/update/replace_windows.go:181-209
This is the "Include *.recovery copies in the recovery state machine" item from jatmn's 2026-07-30 review; 76735375 fixed the sibling finding but not this one.
existingRecoveryPaths matches <base>.old and <base>.<suffix>.old only. keepUnmarkedRecoveryCopy relocates the verified copy to <base>.old.<suffix>.recovery (replace_windows.go:192), which nothing scans — so after a successful relocation, promote's pre-flight sees a clean directory.
Proven through the real entry point. First installBinary with renameFileByHandle injected to fail after creating an exclusively-held file at the target, and markOldBinaryPreserved stubbed to fail:
- returns
ErrTargetPossiblyTampered— correct zero.exeholds"attacker"(unverified)- verified bytes relocated to
zero.exe.old.deadbeef.recovery zero.exe.oldvacated
Then, with every seam restored, the operator just runs the upgrade again — nothing has acknowledged the tampering. Second installBinary returned nil. It renamed the unverified "attacker" bytes aside into zero.exe.old and installed over them, so existingRecoveryPaths now reports [zero.exe.old] — the unverified bytes — as the recovery copy, while the actual last-verified binary sits at a .recovery name nothing looks at.
That's the exact loss the .keep refusal exists to prevent, reached through the relocation path instead. Please treat <base>.<suffix>.recovery as authoritative recovery state in existingRecoveryPaths/markedRecoveryPaths (or refuse promotion outright while one exists), with a regression for retry-after-successful-relocation.
Smaller things, not blocking
stage_windows.go:301—discardPathsstill doesos.Remove(staged.path)afterdiscard()closes the exclusive handle. I confirmed it deletes whatever entry occupies that path: close the handle, plant a different file at the staging name, calldiscard(), and the planted file is gone. It's the one place left that re-resolves a pathname in a change whose whole thesis is handle-binding. jatmn raised this on 2026-07-30 as well.stage_other.go:138—createStagingFilehas no production caller on POSIX;createStagedBinarygoes throughcreateStagingFileAt. The four tests instage_other_test.gothat the PR body describes as reproducing the reported primitive only call the dead one, so they'd stay green ifcreateStagingFileAtlostO_EXCL|O_NOFOLLOW. The real POSIX path is stronger, so nothing is broken — but either delete the dead helper and point those tests atcreateStagingFileAt, or say in the comment that they cover the primitive rather than the production path.replace_windows_test.go:88-196andstage_promote_other_test.go:237— theTestCleanupStaleBinaryPreserves*tests can't fail against an empty function. If cleanup comes back in some form, they'll need real assertions; if it doesn't, they're noise.stage_promote_windows_test.go:448— "the replaced binary must be preserved for later cleanup" no longer describes anything.- Worth a line in the upgrade docs: under this PR's own threat model, someone who can write the install directory can plant
<target>.old+<target>.old.keepand make every future update refuse. Fail-closed is the right call and the error text tells the operator how to clear it, but it's a behaviour change frommain(which would have removed the.oldand proceeded) and operators should hear about it before they hit it.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbdec-f8dc-71f9-abdd-ea044a902b9a Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
e1abb74
|
PierrunoYT addressed the two requested Windows recovery changes in e1abb74:
Validation:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Fix the normal Windows handle-rename path
internal/update/stage_windows.go:432
The normal (non-racing) promotion path is failing in the current required Windows smoke job:TestPromoteInstallsTheStagedObjectNotTheStagedPathgetsThe system cannot find the path specifiedfromSetFileInformationByHandle. That means a standalone Windows update moves the current executable to its aside path, cannot promote the verified staged object, and rolls back instead of installing. Please correct theFILE_RENAME_INFO/rename invocation and add coverage for the ordinary promotion path on the supported Windows runner. -
[P2] Do not delete arbitrary
*.oldfiles during recovery cleanup
internal/update/replace_windows.go:260
prepareRecoveryCleanuppasses every unmarked name accepted byexistingRecoveryPathsto the destructive handle-based cleanup. That matcher accepts any<target>.<anything>.old, so a user backup such aszero.exe.before-manual-patch.oldis silently deleted on the next successful update. The broad pattern is appropriate for failing closed on possible recovery state, but cleanup needs exact updater-owned provenance before deleting a file.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Addressed the latest Windows review blockers in
Validation on final head:
@jatmn @Vasanthdev2004, please re-review when convenient. |
Failure-path staging cleanup on Windows re-resolved the staging pathname after the exclusive handle was closed, so a principal who can write in the installation directory could substitute that entry in the gap and have the updater delete a file of their choosing. Removal is now requested through the handle (FileDispositionInfo) before it is released, and nothing is removed by name afterwards; the same change covers the staging-file verification failure path and the partial recovery marker. CleanupStaleBinary has had no production caller since bounded, identity-bound cleanup moved into promote, so the exported no-op and the tests that could only pass vacuously against it are removed. The POSIX link-regression tests now drive createStagingFileAt, the primitive createStagedBinary actually uses, instead of a path-taking helper kept alive only for them. Also documents the fail-closed Windows recovery states in docs/UPDATE.md, including the refusal a planted .old/.keep pair can cause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed @jatmn (2026-08-01 19:13)
CodeRabbit — the Verification on this head: |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep recovery restore bound to the original object
internal/update/stage_windows.go:194
This correctly captures the pre-update file's identity, but it does not preserve that identity across the recovery path:openIdentityFilepermits delete sharing, the value is used only for successful-cleanup bookkeeping, andrestoreOriginalBinarylater movesasidePathby pathname. Afteros.Rename(targetPath, asidePath), a directory writer can rename the genuine aside entry away, place attacker bytes at that name, then temporarily occupytargetPathto makerenameFileByHandlefail. If it releases that target handle before the retrying restore,os.Rename(asidePath, targetPath)installs the substitute and returns success; the update reports only an ordinary install failure, while the next Zero invocation executes attacker-controlled bytes. The root cause is treating a writable-directory pathname as recovery provenance after the promotion gap. Keep a recovery handle that denies substitution and rename that exact object back by handle, or verify its identity immediately before restore and fail closed on any mismatch. Add a regression that replaces the aside entry and releases a target squatting lock between promotion failure and restore. -
[P1] Check unresolved recovery state before returning “already up to date”
internal/update/apply.go:67
The fail-closed checks for marked/relocated recovery copies and a missing target are embedded instagedBinary.promote, so they are reached only after a new release has been found and staged. When a failed promotion was already attempting the current latest release, the nextzero upgrade(which defaults to apply) takes this early success return instead. It tells the operator “already up to date” while a.keep/.recoverystate can remain andtargetPathmay still hold unverified bytes—the opposite of the recovery contract documented by this PR. The root cause is coupling recovery-state validation to the replacement operation rather than treating it as a standalone-install precondition. Extract a Windows recovery preflight and run it before the no-update return for standalone installs, under the same target lock used by promotion; retain the in-promotion check to close the race. Add an Apply-level regression for an already-current release with each unresolved recovery-state form. -
[P3] Retain cleanup provenance when an old recovery copy is temporarily locked
internal/update/replace_windows.go:322
The design intentionally deletes only an identity-recorded recovery object, butprepareRecoveryCleanupsilently drops the previous record when that object cannot be opened for deletion (for example, during a transient AV or user lock), andcleanupSupersededRecoveryCopiesalso ignores a failed delete. A later successful promotion nevertheless atomically replaces the single per-target record with the new aside, permanently losing trusted provenance for the old updater-owned binary once the temporary lock clears. Repeated transient locks therefore leak full recovery binaries indefinitely. The root cause is representing a potentially multi-item cleanup backlog with one record and advancing it without a successful deletion acknowledgement. Preserve pending identity records until their objects are actually deleted (or make the record an atomic queue), and test a blocked old copy, a succeeding next promotion, and a later cleanup retry. -
[P2] Scope the recovery documentation to Windows
docs/UPDATE.md:46
The heading and text present.zero-update-*.old,.keep, and.recoverybehavior as a property of standalone installs. It is Windows-only:stage_other.gopromotes withRenameatdirectly over the target and never creates an aside copy, marker, or per-user recovery record. Linux and macOS users are therefore told to inspect and restore paths that cannot be produced. The root cause is documenting the sharedinstallBinaryabstraction rather than the platform-specific promotion implementations. Mark this recovery workflow explicitly Windows-only, and either describe POSIX's replace semantics separately or omit recovery guidance for platforms that do not expose it.
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