mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-09 18:08:31 -10:00
* security(update): stage binary replacement at an unpredictable, exclusive path Fixes #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 <[email protected]> * security(update): bind the binary swap to the staged object, not its pathname Addresses the two open findings on #751. [P1] The staged file is no longer promoted by pathname. Randomizing the name and creating it exclusively stops PRE-creation, but under #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. * security(update): close remaining updater staging races Address jatmn's review findings on PR #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 <[email protected]> * fix(update): address staging review findings * fix(update): verify promoted object identity safely Bind POSIX staging creation and both rename endpoints to directory descriptors, clean only identity-matched empty directories, and verify Windows promotion by file identity rather than path spelling. Preserve unverifiable crash leftovers and add adversarial cleanup, hard-link, and reparse-ancestor coverage. Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-27da-72ec-8e6d-5d43f127b6a1 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): preserve promotion recovery signals Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): keep the recovery copy a failed restore promised Two error-path follow-ups from the #742 staging review. When a promotion fails and restoring the original also fails, the error tells the operator their original is preserved at <binary>.old — and then the next Apply deleted it, because a present target read as proof the copy was stale. What occupies the target in exactly that case is the bytes the updater could not verify, so cleanup was erasing the one it could. A failed restore now marks the copy, cleanup honors the mark, and a successful promotion clears it. The POSIX staging flow could also leave an empty .zero-stage-* directory behind when the stat immediately after mkdirat failed. Nothing knows that name yet, so it would have stayed for good; it is now removed best-effort with rmdir semantics, which cannot touch anything but the empty directory just created. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): stop the recovery copy from being destroyed by the next attempt The marker added in d9523ba kept cleanup from deleting <target>.old, but four paths still undermined the promise it was meant to make. Promotion destroyed the copy it was preserving. Skipping the pre-rename cleanup was not enough: os.Rename uses MOVEFILE_REPLACE_EXISTING on Windows, so renaming the running binary aside overwrote the last verified copy with the unverified bytes the earlier failure left at the target — and a promotion that then failed could only move those unverified bytes back. Promotion now refuses while the tamper state is unresolved. Only the operator can say whether the file at the target is theirs, so the error names both moves that end it: restore the copy, or delete the marker to accept what is installed. The marker itself was a predictable link-following truncate write. Its path is fixed, so a writer in the install directory could pre-create it as a hard link or reparse point and have the elevated updater write through it. It is now created with the same CREATE_NEW + FILE_FLAG_OPEN_REPARSE_POINT + fresh-regular- file check as a staging file, and never truncates; an existing name is treated as already-marked rather than an object to open. A marker that could not be written left the promise unqualified while the next run's cleanup deleted exactly the file it named. That failure now rides in the error, telling the operator to copy it somewhere safe now. Helper refreshes downgraded tampering to a warning. An ordinary helper failure still warns — a stale helper is better than a failed update — but a helper whose path may hold unverified content fails the apply, because helpers are resolved from the install directory and executed by the sandbox runner, and reporting Applied: true would hand the operator a success while a sibling executable is suspect. ErrTargetPossiblyTampered moved to apply.go so callers on every platform can test for it without build tags. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): keep the recovery copy when its marker cannot be established Surfacing a failed marker write told the operator to hurry; it did not stop the next run from deleting the file they were being told to save. When no marker can be established, the copy is now moved to an unpredictable sibling name that routine cleanup never touches — CleanupStaleBinary only ever removes the exact "<target>.old" — and the error names where it went. Telling the operator to act now remains the fallback for when even that move fails. The marker check is conservative in the same direction: only a definite "not there" allows the copy to be deleted. An Lstat that fails for any other reason leaves the question open, and deleting is irreversible while keeping costs one stale file. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): preserve Windows recovery state across retries Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <[email protected]> * fix(update): watch every recovery path and never orphan a partial marker 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. * fix(update): tolerate the fail-closed outcome of a fully-unlinked substitution 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. * fix(update): surface unverified recovery path, match .old case-insensitively 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 <[email protected]> * fix(update): bound Windows recovery cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019fbdec-f8dc-71f9-abdd-ea044a902b9a Co-authored-by: Pierre Bruno <[email protected]> * fix(update): secure Windows recovery cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): terminate Windows rename paths Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * test(update): allow namespaced recovery copies Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): bind Windows cleanup to handles, drop dead cleanup API 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) <[email protected]> * fix(update): bind Windows recovery to handles and bound its state Addresses the 2026-08-02 review round on #751. Keep the recovery restore bound to the object promote moved aside instead of its pathname: promote now opens the target with delete access and no delete sharing before renaming it aside, and restoreOriginalBinary renames that same handle back (retrying through the handle, never re-resolving the source name), so a directory writer cannot swap the aside entry during the promotion gap and have the substitute installed. Run the recovery-state checks as a standalone-install precondition, under the same lock promotion uses, so "already up to date" can no longer report success while a .keep/.recovery state is unresolved. The in-promotion check stays to close the race. Make the cleanup record a queue: entries are only retired once their object is actually gone, so a copy held open by a scanner is retried by a later update rather than leaking, while records for paths that vanished or that this updater could never have created are retired instead of accumulating. Also from review of the above: - delete the pathname restore chain that lost its production callers (restoreOriginalBinary's pathname form, openIdentityFile, recordRecoveryCleanup) and point their tests at the live handle path - name the relocation path in the error when the post-move verification fails, since oldPath is already vacated by then - scope the recovery docs to Windows and describe POSIX replace semantics Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]>
471 lines
18 KiB
Go
471 lines
18 KiB
Go
//go:build windows
|
|
|
|
package update
|
|
|
|
import (
|
|
"errors"
|
|
"fmt"
|
|
"os"
|
|
"path/filepath"
|
|
"strings"
|
|
"testing"
|
|
|
|
"golang.org/x/sys/windows"
|
|
)
|
|
|
|
// The replacement path itself (rename the running binary aside, then rename the
|
|
// staged object into place through its handle) is covered by
|
|
// TestInstallBinaryInstallsVerifiedBytes and
|
|
// TestPromoteInstallsTheStagedObjectNotTheStagedPath in
|
|
// stage_promote_windows_test.go, which exercise it through the staging handle the
|
|
// production code uses rather than a loose pathname.
|
|
|
|
func TestRenameOpenFileWithRetrySucceedsImmediately(t *testing.T) {
|
|
dir := t.TempDir()
|
|
src := filepath.Join(dir, "src")
|
|
dst := filepath.Join(dir, "dst")
|
|
if err := os.WriteFile(src, []byte("data"), 0o644); err != nil {
|
|
t.Fatalf("WriteFile src: %v", err)
|
|
}
|
|
file, err := openRecoveryCopy(src)
|
|
if err != nil {
|
|
t.Fatalf("openRecoveryCopy: %v", err)
|
|
}
|
|
defer func() { _ = file.Close() }()
|
|
|
|
if err := renameOpenFileWithRetry(file, dst); err != nil {
|
|
t.Fatalf("renameOpenFileWithRetry: %v", err)
|
|
}
|
|
if _, err := os.Stat(dst); err != nil {
|
|
t.Fatalf("expected dst to exist after rename: %v", err)
|
|
}
|
|
}
|
|
|
|
// A permanently-failing rename (the destination is held by a conflicting
|
|
// exclusive handle for good) must exhaust its retries and surface the
|
|
// underlying error, rather than retrying forever.
|
|
func TestRenameOpenFileWithRetryFailsAfterExhaustingAttempts(t *testing.T) {
|
|
dir := t.TempDir()
|
|
src := filepath.Join(dir, "src")
|
|
dst := filepath.Join(dir, "dst")
|
|
for _, path := range []string{src, dst} {
|
|
if err := os.WriteFile(path, []byte("data"), 0o644); err != nil {
|
|
t.Fatalf("WriteFile %s: %v", path, err)
|
|
}
|
|
}
|
|
file, err := openRecoveryCopy(src)
|
|
if err != nil {
|
|
t.Fatalf("openRecoveryCopy: %v", err)
|
|
}
|
|
defer func() { _ = file.Close() }()
|
|
blocker, err := openWithoutSharing(dst)
|
|
if err != nil {
|
|
t.Skipf("cannot hold the destination exclusively on this filesystem: %v", err)
|
|
}
|
|
defer func() { _ = blocker.Close() }()
|
|
|
|
if err := renameOpenFileWithRetry(file, dst); err == nil {
|
|
t.Fatal("expected renameOpenFileWithRetry to fail against a permanently blocked destination")
|
|
}
|
|
}
|
|
|
|
// TestRestoreOriginalBinaryFlagsPossibleTamperingWhenRestoreFails is the
|
|
// regression test for a review finding on PR #751: when a promotion attempt
|
|
// fails AND the restore of the preserved original also cannot get past
|
|
// whatever now occupies targetPath, that combination must be reported as a
|
|
// security-relevant condition (ErrTargetPossiblyTampered), not folded into
|
|
// the same generic error a stalled download would produce — the caller needs
|
|
// to be able to tell "try the update again later" apart from "verify what is
|
|
// at this path before running it again".
|
|
func TestRestoreOriginalBinaryFlagsPossibleTamperingWhenRestoreFails(t *testing.T) {
|
|
dir := t.TempDir()
|
|
oldPath := filepath.Join(dir, "zero.exe.old")
|
|
targetPath := filepath.Join(dir, "zero.exe")
|
|
if err := os.WriteFile(oldPath, []byte("original"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile oldPath: %v", err)
|
|
}
|
|
|
|
// Simulate an attacker occupying targetPath with a lock MOVEFILE_REPLACE_EXISTING
|
|
// cannot get past: an exclusive, no-share open.
|
|
pathPtr, err := windows.UTF16PtrFromString(targetPath)
|
|
if err != nil {
|
|
t.Fatalf("UTF16PtrFromString: %v", err)
|
|
}
|
|
handle, err := windows.CreateFile(pathPtr, windows.GENERIC_WRITE, 0, nil, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL, 0)
|
|
if err != nil {
|
|
t.Fatalf("CreateFile targetPath: %v", err)
|
|
}
|
|
defer func() { _ = windows.CloseHandle(handle) }()
|
|
|
|
restoreErr := restoreOriginalBinary(openRecoveryHandle(t, oldPath), oldPath, targetPath)
|
|
if restoreErr == nil {
|
|
t.Fatal("restoreOriginalBinary succeeded despite a conflicting exclusive lock on targetPath, want an error")
|
|
}
|
|
if !errors.Is(restoreErr, ErrTargetPossiblyTampered) {
|
|
t.Fatalf("error = %v, want it to wrap ErrTargetPossiblyTampered", restoreErr)
|
|
}
|
|
}
|
|
|
|
// TestRestoreOriginalBinaryMarksPreservedCopy pins the other half: the path that
|
|
// reports "original preserved at <.old>" is the path that makes that true across
|
|
// runs.
|
|
func TestRestoreOriginalBinaryMarksPreservedCopy(t *testing.T) {
|
|
dir := t.TempDir()
|
|
targetPath := filepath.Join(dir, "zero.exe")
|
|
oldPath := targetPath + ".old"
|
|
if err := os.WriteFile(oldPath, []byte("known-good"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile old binary: %v", err)
|
|
}
|
|
if err := os.WriteFile(targetPath, []byte("unverified"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile target: %v", err)
|
|
}
|
|
// Hold the target with no sharing so the restore rename cannot replace it,
|
|
// which is the condition ErrTargetPossiblyTampered describes.
|
|
blocker, err := openWithoutSharing(targetPath)
|
|
if err != nil {
|
|
t.Skipf("cannot hold the target exclusively on this filesystem: %v", err)
|
|
}
|
|
defer func() { _ = blocker.Close() }()
|
|
|
|
recovery := openRecoveryHandle(t, oldPath)
|
|
err = restoreOriginalBinary(recovery, oldPath, targetPath)
|
|
// Release the handle promote would hold, so these assertions can read the
|
|
// files it was keeping unsubstitutable.
|
|
_ = recovery.Close()
|
|
if !errors.Is(err, ErrTargetPossiblyTampered) {
|
|
t.Fatalf("restore error = %v, want ErrTargetPossiblyTampered", err)
|
|
}
|
|
if !oldBinaryPreserved(oldPath) {
|
|
t.Fatal("a failed restore must mark the preserved copy so later cleanup keeps it")
|
|
}
|
|
if _, err := os.Stat(oldPath); err != nil {
|
|
t.Fatalf("recovery copy was removed after a failed restore: %v", err)
|
|
}
|
|
}
|
|
|
|
// TestMarkOldBinaryPreservedRefusesPreCreatedLink covers jatmn's #751 finding
|
|
// that the marker was a predictable link-following truncate write: the path is
|
|
// fixed, so a lower-privileged writer in the install directory can pre-create it
|
|
// as a hard link (or reparse point) and have the elevated updater truncate and
|
|
// write through it into a file of their choosing.
|
|
func TestMarkOldBinaryPreservedRefusesPreCreatedLink(t *testing.T) {
|
|
for _, kind := range []string{"hardlink", "symlink"} {
|
|
t.Run(kind, func(t *testing.T) {
|
|
dir := t.TempDir()
|
|
oldPath := filepath.Join(dir, "zero.exe.old")
|
|
if err := os.WriteFile(oldPath, []byte("known-good"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile old binary: %v", err)
|
|
}
|
|
victim := filepath.Join(t.TempDir(), "victim.txt")
|
|
const victimContent = "attacker-chosen target that must not be written"
|
|
if err := os.WriteFile(victim, []byte(victimContent), 0o600); err != nil {
|
|
t.Fatalf("WriteFile victim: %v", err)
|
|
}
|
|
|
|
markerPath := oldPath + oldBinaryPreservedSuffix
|
|
var linkErr error
|
|
switch kind {
|
|
case "hardlink":
|
|
linkErr = os.Link(victim, markerPath)
|
|
case "symlink":
|
|
linkErr = os.Symlink(victim, markerPath)
|
|
}
|
|
if linkErr != nil {
|
|
t.Skipf("%s unsupported here: %v", kind, linkErr)
|
|
}
|
|
|
|
// Whatever this returns, the one thing it must not do is write through
|
|
// the planted object. Reporting the marker as present is the safe
|
|
// answer: it makes the next run PRESERVE the recovery copy.
|
|
_ = markOldBinaryPreserved(oldPath)
|
|
|
|
got, err := os.ReadFile(victim)
|
|
if err != nil {
|
|
t.Fatalf("ReadFile victim: %v", err)
|
|
}
|
|
if string(got) != victimContent {
|
|
t.Fatalf("marker write followed the planted %s and wrote into %q: %q", kind, victim, got)
|
|
}
|
|
// The recovery copy itself is untouched, which is the marker's purpose.
|
|
if _, err := os.Stat(oldPath); err != nil {
|
|
t.Fatalf("recovery copy was removed while planting the marker: %v", err)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestMarkOldBinaryPreservedRemovesPartialMarkerBeforeRelocation(t *testing.T) {
|
|
dir := t.TempDir()
|
|
targetPath := filepath.Join(dir, "zero.exe")
|
|
oldPath := targetPath + ".old"
|
|
if err := os.WriteFile(targetPath, []byte("unverified"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile target: %v", err)
|
|
}
|
|
if err := os.WriteFile(oldPath, []byte("known-good"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile old binary: %v", err)
|
|
}
|
|
blocker, err := openWithoutSharing(targetPath)
|
|
if err != nil {
|
|
t.Skipf("cannot hold the target exclusively on this filesystem: %v", err)
|
|
}
|
|
defer func() { _ = blocker.Close() }()
|
|
|
|
originalWrite := writeRecoveryMarker
|
|
writeRecoveryMarker = func(marker *os.File) error {
|
|
_, _ = marker.WriteString("partial")
|
|
return errors.New("injected marker write failure")
|
|
}
|
|
t.Cleanup(func() { writeRecoveryMarker = originalWrite })
|
|
stubRandomStagingSuffix(t, "deadbeef")
|
|
|
|
recovery := openRecoveryHandle(t, oldPath)
|
|
err = restoreOriginalBinary(recovery, oldPath, targetPath)
|
|
// Release the handle promote would hold, so these assertions can read the
|
|
// files it was keeping unsubstitutable.
|
|
_ = recovery.Close()
|
|
if !errors.Is(err, ErrTargetPossiblyTampered) {
|
|
t.Fatalf("restore error = %v, want ErrTargetPossiblyTampered", err)
|
|
}
|
|
kept := oldPath + ".deadbeef.recovery"
|
|
if !strings.Contains(err.Error(), kept) {
|
|
t.Fatalf("error = %v, want relocated recovery path %s", err, kept)
|
|
}
|
|
if _, err := os.Lstat(oldPath + oldBinaryPreservedSuffix); !errors.Is(err, os.ErrNotExist) {
|
|
t.Fatalf("partial marker survived relocation: %v", err)
|
|
}
|
|
if got, err := os.ReadFile(kept); err != nil || string(got) != "known-good" {
|
|
t.Fatalf("relocated recovery = %q err=%v, want known-good", got, err)
|
|
}
|
|
}
|
|
|
|
// TestRestoreOriginalBinaryKeepsRecoveryCopyWhenMarkingFails covers the half of
|
|
// CodeRabbit's marker finding that surfacing the failure alone does not: when no
|
|
// marker can be established, nothing on disk tells the next run to keep the
|
|
// copy, so it is moved out from under routine cleanup instead of being left at
|
|
// the ordinary "<target>.old" recovery name.
|
|
func TestRestoreOriginalBinaryKeepsRecoveryCopyWhenMarkingFails(t *testing.T) {
|
|
dir := t.TempDir()
|
|
targetPath := filepath.Join(dir, "zero.exe")
|
|
oldPath := targetPath + ".old"
|
|
if err := os.WriteFile(targetPath, []byte("unverified"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile target: %v", err)
|
|
}
|
|
if err := os.WriteFile(oldPath, []byte("known-good"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile old binary: %v", err)
|
|
}
|
|
// Hold the target with no sharing so the restore rename fails.
|
|
blocker, err := openWithoutSharing(targetPath)
|
|
if err != nil {
|
|
t.Skipf("cannot hold the target exclusively on this filesystem: %v", err)
|
|
}
|
|
defer func() { _ = blocker.Close() }()
|
|
// Force the marker to be unestablishable. Doing it through the seam rather
|
|
// than by breaking the filesystem keeps oldPath itself intact, which is the
|
|
// state this behavior is about.
|
|
originalMark := markOldBinaryPreserved
|
|
markOldBinaryPreserved = func(string) error { return errors.New("injected marker failure") }
|
|
t.Cleanup(func() { markOldBinaryPreserved = originalMark })
|
|
stubRandomStagingSuffix(t, "deadbeef")
|
|
|
|
recovery := openRecoveryHandle(t, oldPath)
|
|
err = restoreOriginalBinary(recovery, oldPath, targetPath)
|
|
// Release the handle promote would hold, so these assertions can read the
|
|
// files it was keeping unsubstitutable.
|
|
_ = recovery.Close()
|
|
if !errors.Is(err, ErrTargetPossiblyTampered) {
|
|
t.Fatalf("restore error = %v, want ErrTargetPossiblyTampered", err)
|
|
}
|
|
kept := oldPath + ".deadbeef.recovery"
|
|
if !strings.Contains(err.Error(), kept) {
|
|
t.Fatalf("error = %v, want it to name the path the copy was moved to", err)
|
|
}
|
|
got, readErr := os.ReadFile(kept)
|
|
if readErr != nil {
|
|
t.Fatalf("recovery copy was not kept: %v", readErr)
|
|
}
|
|
if string(got) != "known-good" {
|
|
t.Fatalf("kept copy = %q, want the last verified binary", got)
|
|
}
|
|
if _, err := os.Lstat(oldPath); !errors.Is(err, os.ErrNotExist) {
|
|
t.Fatalf("old recovery path still exists after move: %v", err)
|
|
}
|
|
}
|
|
|
|
// TestOldBinaryPreservedTreatsAnUnreadableMarkerAsPresent pins the conservative
|
|
// side of the marker check: only a definite "not there" allows the copy to be
|
|
// deleted, because deleting it is irreversible and keeping it costs a file.
|
|
func TestOldBinaryPreservedTreatsAnUnreadableMarkerAsPresent(t *testing.T) {
|
|
dir := t.TempDir()
|
|
oldPath := filepath.Join(dir, "zero.exe.old")
|
|
if oldBinaryPreserved(oldPath) {
|
|
t.Fatal("a genuinely absent marker must report not-preserved")
|
|
}
|
|
// A directory at the marker name is an entry Lstat can see; anything other
|
|
// than a clean not-exist keeps the copy.
|
|
if err := os.Mkdir(oldPath+oldBinaryPreservedSuffix, 0o700); err != nil {
|
|
t.Fatalf("Mkdir marker: %v", err)
|
|
}
|
|
if !oldBinaryPreserved(oldPath) {
|
|
t.Fatal("an entry at the marker name must count as preserved")
|
|
}
|
|
}
|
|
|
|
// TestRestoreOriginalBinarySurfacesMarkerWriteFailure covers the #751 P3: the
|
|
// error promises the original is preserved at <target>.old, but if the marker
|
|
// cannot be written the next run's cleanup deletes exactly that file. The
|
|
// operator has to be told to act now rather than at their convenience.
|
|
func TestRestoreOriginalBinarySurfacesMarkerWriteFailure(t *testing.T) {
|
|
dir := t.TempDir()
|
|
targetPath := filepath.Join(dir, "zero.exe")
|
|
if err := os.WriteFile(targetPath, []byte("unverified"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile target: %v", err)
|
|
}
|
|
oldPath := targetPath + ".old"
|
|
if err := os.WriteFile(oldPath, []byte("known-good"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile old binary: %v", err)
|
|
}
|
|
file := openRecoveryHandle(t, oldPath)
|
|
// Neither the restore nor the marker nor the relocation can succeed, so the
|
|
// recovery copy stays at oldPath and the operator must be pointed there.
|
|
originalMark := markOldBinaryPreserved
|
|
markOldBinaryPreserved = func(string) error { return errors.New("injected marker failure") }
|
|
t.Cleanup(func() { markOldBinaryPreserved = originalMark })
|
|
originalRename := renameRecoveryFileByHandle
|
|
renameRecoveryFileByHandle = func(*os.File, string) error { return errors.New("injected rename failure") }
|
|
t.Cleanup(func() { renameRecoveryFileByHandle = originalRename })
|
|
|
|
err := restoreOriginalBinary(file, oldPath, targetPath)
|
|
_ = file.Close()
|
|
if !errors.Is(err, ErrTargetPossiblyTampered) {
|
|
t.Fatalf("restore error = %v, want ErrTargetPossiblyTampered", err)
|
|
}
|
|
if !strings.Contains(err.Error(), "recovery marker could not be written") {
|
|
t.Fatalf("error = %v, want it to disclose the failed marker", err)
|
|
}
|
|
if !strings.Contains(err.Error(), oldPath) {
|
|
t.Fatalf("error = %v, want the path the operator must copy", err)
|
|
}
|
|
if strings.Contains(err.Error(), "later update") {
|
|
t.Fatalf("error = %v, must not promise cleanup that no longer exists", err)
|
|
}
|
|
if got, readErr := os.ReadFile(oldPath); readErr != nil || string(got) != "known-good" {
|
|
t.Fatalf("recovery copy at %s = %q err=%v, want the last verified binary", oldPath, got, readErr)
|
|
}
|
|
}
|
|
|
|
func TestKeepUnmarkedRecoveryCopyMovesTheOpenedObject(t *testing.T) {
|
|
dir := t.TempDir()
|
|
oldPath := filepath.Join(dir, "zero.exe.old")
|
|
if err := os.WriteFile(oldPath, []byte("known-good"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile recovery copy: %v", err)
|
|
}
|
|
stubRandomStagingSuffix(t, "deadbeef")
|
|
|
|
originalRename := renameRecoveryFileByHandle
|
|
renameRecoveryFileByHandle = func(file *os.File, kept string) error {
|
|
// Try to substitute oldPath after keepUnmarkedRecoveryCopy has opened and
|
|
// verified it. The exclusive delete sharing normally blocks this. Even
|
|
// on a filesystem that permits it, the handle-bound rename must still
|
|
// move the verified object rather than the replacement pathname entry.
|
|
displaced := oldPath + ".attacker-moved"
|
|
if err := os.Rename(oldPath, displaced); err == nil {
|
|
if err := os.WriteFile(oldPath, []byte("attacker"), 0o755); err != nil {
|
|
return fmt.Errorf("plant substituted recovery: %w", err)
|
|
}
|
|
}
|
|
return renameOpenFile(file, kept)
|
|
}
|
|
t.Cleanup(func() { renameRecoveryFileByHandle = originalRename })
|
|
|
|
recovery := openRecoveryHandle(t, oldPath)
|
|
kept, err := keepUnmarkedRecoveryCopy(recovery, oldPath)
|
|
_ = recovery.Close()
|
|
if err != nil {
|
|
t.Fatalf("keepUnmarkedRecoveryCopy: %v", err)
|
|
}
|
|
if got, err := os.ReadFile(kept); err != nil || string(got) != "known-good" {
|
|
t.Fatalf("kept recovery = %q err=%v, want the verified object", got, err)
|
|
}
|
|
}
|
|
|
|
// TestRestoreOriginalBinaryNamesTheRelocationItCouldNotVerify covers the gap
|
|
// jatmn flagged on the pathname restore and that the handle restore inherited:
|
|
// when the relocation rename succeeds but the post-move verification does not,
|
|
// oldPath has already been vacated, so an error naming oldPath sends the
|
|
// operator to a path that no longer holds anything.
|
|
func TestRestoreOriginalBinaryNamesTheRelocationItCouldNotVerify(t *testing.T) {
|
|
dir := t.TempDir()
|
|
targetPath := filepath.Join(dir, "zero.exe")
|
|
oldPath := targetPath + ".old"
|
|
if err := os.WriteFile(targetPath, []byte("unverified"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile target: %v", err)
|
|
}
|
|
if err := os.WriteFile(oldPath, []byte("known-good"), 0o755); err != nil {
|
|
t.Fatalf("WriteFile old binary: %v", err)
|
|
}
|
|
file := openRecoveryHandle(t, oldPath)
|
|
stubRandomStagingSuffix(t, "deadbeef")
|
|
originalMark := markOldBinaryPreserved
|
|
markOldBinaryPreserved = func(string) error { return errors.New("injected marker failure") }
|
|
t.Cleanup(func() { markOldBinaryPreserved = originalMark })
|
|
|
|
kept := oldPath + ".deadbeef.recovery"
|
|
elsewhere := oldPath + ".deadbeef.elsewhere"
|
|
originalRename := renameRecoveryFileByHandle
|
|
renameRecoveryFileByHandle = func(handle *os.File, destination string) error {
|
|
if destination != kept {
|
|
return errors.New("injected restore failure")
|
|
}
|
|
// The rename reports success but lands somewhere else, so kept is
|
|
// vacated and verification there fails.
|
|
return renameOpenFile(handle, elsewhere)
|
|
}
|
|
t.Cleanup(func() { renameRecoveryFileByHandle = originalRename })
|
|
|
|
err := restoreOriginalBinary(file, oldPath, targetPath)
|
|
if !errors.Is(err, ErrTargetPossiblyTampered) {
|
|
t.Fatalf("restore error = %v, want ErrTargetPossiblyTampered", err)
|
|
}
|
|
if !strings.Contains(err.Error(), kept) {
|
|
t.Fatalf("error = %v, want it to name the relocation path %s", err, kept)
|
|
}
|
|
if _, statErr := os.Lstat(oldPath); !errors.Is(statErr, os.ErrNotExist) {
|
|
t.Fatalf("oldPath still exists after the relocation rename: %v", statErr)
|
|
}
|
|
}
|
|
|
|
// openRecoveryHandle opens path the way promote holds the binary it moved
|
|
// aside: with delete access and no delete sharing, so the object cannot be
|
|
// substituted while the restore path works with it.
|
|
func openRecoveryHandle(t *testing.T, path string) *os.File {
|
|
t.Helper()
|
|
file, err := openRecoveryCopy(path)
|
|
if err != nil {
|
|
t.Fatalf("openRecoveryCopy %s: %v", path, err)
|
|
}
|
|
t.Cleanup(func() { _ = file.Close() })
|
|
return file
|
|
}
|
|
|
|
// openWithoutSharing opens an existing file denying every share mode, so a
|
|
// rename onto it fails the way a principal squatting the executable path does.
|
|
func openWithoutSharing(path string) (*os.File, error) {
|
|
pathPtr, err := windows.UTF16PtrFromString(path)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
handle, err := windows.CreateFile(
|
|
pathPtr,
|
|
windows.GENERIC_READ,
|
|
0,
|
|
nil,
|
|
windows.OPEN_EXISTING,
|
|
windows.FILE_ATTRIBUTE_NORMAL,
|
|
0,
|
|
)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
return os.NewFile(uintptr(handle), path), nil
|
|
}
|