From 3b8d8149bb5e283cb794ae9d70ad01715a190f59 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Thu, 27 Aug 2026 19:09:42 +0000 Subject: [PATCH 1/4] fix(update): keep Windows failed-restore state in trusted recovery records A failed restore currently records the last verified binary only as a .deletable .keep marker in the install directory. Write an unresolved identity-bound record to the existing per-user update-recovery store on that failure path too, and consult it during preflight so deleting the marker cannot silence the tamper refusal. Fixes #868 --- docs/UPDATE.md | 9 +- internal/update/replace_windows.go | 37 ++++- internal/update/stage_promote_windows_test.go | 155 ++++++++++++++++++ internal/update/stage_windows.go | 26 ++- 4 files changed, 219 insertions(+), 8 deletions(-) diff --git a/docs/UPDATE.md b/docs/UPDATE.md index d0e7eafc1..a3ee7b532 100644 --- a/docs/UPDATE.md +++ b/docs/UPDATE.md @@ -93,14 +93,17 @@ binary: | State on disk | Meaning | |---|---| -| `.old` (or `..old`) plus a `.keep` marker | A previous update could not restore the original binary. The `.old` file may be the last binary the updater verified; the installed one may be unverified. | +| `.old` (or `..old`) plus a `.keep` marker, or a per-user update-recovery record naming that copy | A previous update could not restore the original binary. The `.old` file may be the last binary the updater verified; the installed one may be unverified. The `.keep` marker is in the installation directory and can be deleted; the identity-bound record is not, so deleting the marker alone does not end the refusal. | | `.…old..recovery` | A previous update could not even write the marker, so it moved the last verified binary to that name. | | The binary is missing and one or more `*.old` files exist | The previous attempt was interrupted between moves. | The refusal names the paths involved and the two moves that end the state: either move the recovery binary back over the executable path, or — if the -installed binary is the one you want — delete the `.keep` marker (or the -`.recovery` copy, once you have verified the installed binary) and update again. +installed binary is the one you want — delete the recovery copy itself (not +only the `.keep` marker) after verifying the installed binary, then update +again. Deleting the marker alone is not enough: a failed restore also records +that copy in per-user state outside the installation directory, and the next +update consults that record even if the marker is gone. This is deliberately fail-closed and differs from older releases, which deleted `.old` and proceeded. The trade-off is that anyone who can write in the diff --git a/internal/update/replace_windows.go b/internal/update/replace_windows.go index bc405e714..1ef659f00 100644 --- a/internal/update/replace_windows.go +++ b/internal/update/replace_windows.go @@ -278,6 +278,11 @@ type recoveryCleanupRecord struct { VolumeSerial uint32 `json:"volumeSerial"` FileIndexHigh uint32 `json:"fileIndexHigh"` FileIndexLow uint32 `json:"fileIndexLow"` + // Unresolved marks a failed restore. Preflight consults these records + // alongside the sibling .keep marker so deleting that marker cannot + // silence the next run's tamper refusal (#868). Cleanup records from a + // successful promotion leave this false. + Unresolved bool `json:"unresolved,omitempty"` } type recoveryCleanupQueue struct { @@ -373,7 +378,7 @@ func prepareRecoveryCleanup(targetPath string) []recoveryCleanupCandidate { continue } retained = append(retained, record) - if oldBinaryPreserved(record.Path) { + if oldBinaryPreserved(record.Path) || record.Unresolved { continue } file, err := openRecoveryCopy(record.Path) @@ -419,6 +424,18 @@ func recoveryFileIdentity(file *os.File) (recoveryIdentity, error) { // recorded as updater-owned: the next run reopens recoveryPath and only deletes // it if it is still that same object. func appendRecoveryCleanupRecord(targetPath string, recoveryPath string, identity recoveryIdentity) error { + return appendRecoveryCleanupRecordWithState(targetPath, recoveryPath, identity, false) +} + +// appendUnresolvedRecoveryRecord records that a failed restore left recoveryPath +// as the last verified binary. Unlike the sibling .keep marker in the +// installation directory, this lives in per-user state an install-directory +// writer cannot delete. Preflight consults it alongside the marker (#868). +func appendUnresolvedRecoveryRecord(targetPath string, recoveryPath string, identity recoveryIdentity) error { + return appendRecoveryCleanupRecordWithState(targetPath, recoveryPath, identity, true) +} + +func appendRecoveryCleanupRecordWithState(targetPath string, recoveryPath string, identity recoveryIdentity, unresolved bool) error { if !validUpdaterRecoveryPath(targetPath, recoveryPath) { return fmt.Errorf("invalid updater recovery path %s", recoveryPath) } @@ -427,12 +444,30 @@ func appendRecoveryCleanupRecord(targetPath string, recoveryPath string, identit VolumeSerial: identity.VolumeSerial, FileIndexHigh: identity.FileIndexHigh, FileIndexLow: identity.FileIndexLow, + Unresolved: unresolved, } queue := loadRecoveryCleanupQueue(targetPath) queue.Records = append(queue.Records, record) return writeRecoveryCleanupQueue(targetPath, queue) } +// unresolvedRecordedRecoveryPaths returns live recovery copies vouched for by +// per-user unresolved-state records. Absence of the sibling .keep marker does +// not drop them: that marker is attacker-deletable in the install directory. +func unresolvedRecordedRecoveryPaths(targetPath string) []string { + var paths []string + for _, record := range loadRecoveryCleanupQueue(targetPath).Records { + if !record.Unresolved || !validUpdaterRecoveryPath(targetPath, record.Path) { + continue + } + if _, err := os.Lstat(record.Path); errors.Is(err, os.ErrNotExist) { + continue + } + paths = append(paths, record.Path) + } + return paths +} + func writeRecoveryCleanupQueue(targetPath string, queue recoveryCleanupQueue) error { data, err := json.Marshal(queue) if err != nil { diff --git a/internal/update/stage_promote_windows_test.go b/internal/update/stage_promote_windows_test.go index a462041ac..7b1317927 100644 --- a/internal/update/stage_promote_windows_test.go +++ b/internal/update/stage_promote_windows_test.go @@ -268,6 +268,161 @@ func TestPromoteRefusesWhileRecoveryCopyIsMarked(t *testing.T) { } } +// TestPreflightRefusesWhenRecoveryMarkerIsDeletedButTrustedRecordRemains is the +// #868 regression: after a failed restore, the sibling .keep marker is the +// only on-disk signal in the install directory that the aside copy is the last +// verified binary. A writer in that directory can delete it. The identity-bound +// per-user record written on the same failure path must still cause refusal. +func TestPreflightRefusesWhenRecoveryMarkerIsDeletedButTrustedRecordRemains(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("old-binary"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte("verified-binary"), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + + const suffix = "cafecafecafecafecafecafecafecafe" + stubRandomStagingSuffix(t, suffix) + original := renameFileByHandle + var conflicting windows.Handle + renameFileByHandle = func(_ *os.File, target string) error { + pathPtr, err := windows.UTF16PtrFromString(target) + if err != nil { + return err + } + conflicting, err = windows.CreateFile(pathPtr, windows.GENERIC_WRITE, 0, nil, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + return fmt.Errorf("create conflicting target: %w", err) + } + return errors.New("injected promotion failure") + } + t.Cleanup(func() { + renameFileByHandle = original + if conflicting != 0 { + _ = windows.CloseHandle(conflicting) + } + }) + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want ErrTargetPossiblyTampered", err) + } + if conflicting != 0 { + _ = windows.CloseHandle(conflicting) + conflicting = 0 + } + // Restore the production rename so a missed preflight refusal would + // actually install, rather than fail for the injected reason. + renameFileByHandle = original + + recoveryPath := targetPath + ".zero-update-" + suffix + ".old" + if got, readErr := os.ReadFile(recoveryPath); readErr != nil || string(got) != "old-binary" { + t.Fatalf("recovery copy = %q err=%v, want the last verified binary", got, readErr) + } + if !oldBinaryPreserved(recoveryPath) { + t.Fatal("failed restore must still write the sibling marker") + } + queue := loadRecoveryCleanupQueue(targetPath) + if len(queue.Records) != 1 || !queue.Records[0].Unresolved { + t.Fatalf("trusted records = %+v, want one unresolved entry", queue.Records) + } + + clearOldBinaryPreserved(recoveryPath) + if oldBinaryPreserved(recoveryPath) { + t.Fatal("marker deletion did not take effect") + } + + err = installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("retry after marker deletion = %v, want refusal from the trusted record", err) + } + if !strings.Contains(err.Error(), recoveryPath) { + t.Fatalf("error = %v, want it to name recovery path %s", err, recoveryPath) + } + if got, readErr := os.ReadFile(recoveryPath); readErr != nil || string(got) != "old-binary" { + t.Fatalf("recovery copy after refusal = %q err=%v, want it left intact", got, readErr) + } +} + +// TestSuccessfulPromoteCleanupRecordDoesNotBlockLaterUpdate pins the success +// path for #868: a verified promotion still writes a cleanup record, that +// record is not unresolved, and the next update proceeds. +func TestSuccessfulPromoteCleanupRecordDoesNotBlockLaterUpdate(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("old-binary"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte("verified-binary"), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + + if err := installBinary(sourcePath, targetPath); err != nil { + t.Fatalf("installBinary: %v", err) + } + queue := loadRecoveryCleanupQueue(targetPath) + if len(queue.Records) != 1 { + t.Fatalf("cleanup records after success = %d, want 1", len(queue.Records)) + } + if queue.Records[0].Unresolved { + t.Fatal("successful promotion must not record unresolved recovery state") + } + if err := preflightRecoveryState(targetPath); err != nil { + t.Fatalf("preflight after successful promotion: %v", err) + } + + nextSource := filepath.Join(t.TempDir(), "newer-binary") + if err := os.WriteFile(nextSource, []byte("next-verified"), 0o755); err != nil { + t.Fatalf("WriteFile next source: %v", err) + } + if err := installBinary(nextSource, targetPath); err != nil { + t.Fatalf("second installBinary after success: %v", err) + } + installed, err := os.ReadFile(targetPath) + if err != nil || string(installed) != "next-verified" { + t.Fatalf("installed binary = %q err=%v, want next-verified", installed, err) + } +} + +// TestPrepareRecoveryCleanupSkipsUnresolvedRecords keeps a failed-restore copy +// out of destructive cleanup even if the sibling marker is already gone. +func TestPrepareRecoveryCleanupSkipsUnresolvedRecords(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + recoveryPath := targetPath + ".zero-update-0123456789abcdef0123456789abcdef.old" + if err := os.WriteFile(recoveryPath, []byte("last-verified"), 0o755); err != nil { + t.Fatalf("WriteFile recovery: %v", err) + } + original, err := openRecoveryCopy(recoveryPath) + if err != nil { + t.Fatalf("openRecoveryCopy: %v", err) + } + identity, err := recoveryFileIdentity(original) + _ = original.Close() + if err != nil { + t.Fatalf("recoveryFileIdentity: %v", err) + } + if err := appendUnresolvedRecoveryRecord(targetPath, recoveryPath, identity); err != nil { + t.Fatalf("appendUnresolvedRecoveryRecord: %v", err) + } + + candidates := prepareRecoveryCleanup(targetPath) + if len(candidates) != 0 { + t.Fatalf("cleanup candidates = %d, want unresolved copies skipped", len(candidates)) + } + closeRecoveryCleanupCandidates(candidates) + if got, err := os.ReadFile(recoveryPath); err != nil || string(got) != "last-verified" { + t.Fatalf("unresolved recovery copy = %q err=%v, want it left untouched", got, err) + } + if got := recordedRecoveryCleanupCount(t, targetPath); got != 1 { + t.Fatalf("recorded entries = %d, want the unresolved record retained", got) + } +} + func TestPromoteRefusesRetryAfterInterruptedAside(t *testing.T) { dir := t.TempDir() targetPath := filepath.Join(dir, "zero.exe") diff --git a/internal/update/stage_windows.go b/internal/update/stage_windows.go index 738eb9c84..6aab1ce31 100644 --- a/internal/update/stage_windows.go +++ b/internal/update/stage_windows.go @@ -154,6 +154,11 @@ func (staged *stagedBinary) promote(targetPath string) error { } if renameErr != nil { if restoreErr := restoreOriginalBinary(original, asidePath, targetPath); restoreErr != nil { + // Best-effort: the restore error is what the operator must see. + // The .keep marker is still in the install directory if this write + // fails; if it succeeds, deleting that marker cannot silence the + // next preflight (#868). + _ = appendUnresolvedRecoveryRecord(targetPath, asidePath, originalIdentity) return fmt.Errorf("install new binary: %v; additionally failed to restore the original binary: %w", renameErr, restoreErr) } return fmt.Errorf("install new binary: %w", renameErr) @@ -212,7 +217,7 @@ func preflightRecoveryStateLocked(targetPath string) error { } return fmt.Errorf( "%w: a previous update could not restore the original binary. Recovery path(s) %s may hold the last binary this updater verified and %s may hold unverified content. "+ - "Move the correct recovery binary back over %s to restore it, or delete its marker (%s) to accept the installed binary, then update again", + "Move the correct recovery binary back over %s to restore it, or delete the recovery copy itself (not only its marker, %s) to accept the installed binary, then update again", ErrTargetPossiblyTampered, strings.Join(recoveryPaths, ", "), targetPath, targetPath, strings.Join(markerPaths, ", "), ) @@ -342,20 +347,33 @@ func relocatedRecoveryPaths(targetPath string) ([]string, error) { return paths, nil } -// markedRecoveryPaths finds recovery copies protected by either the canonical -// marker or a marker beside a randomized aside path. A failed second-or-later -// update commonly uses the latter because the canonical .old already exists. +// markedRecoveryPaths finds recovery copies protected by a sibling .keep +// marker or by a trusted per-user unresolved-state record. A failed +// second-or-later update commonly uses a randomized aside because the +// canonical .old already exists. The trusted record is consulted even when +// the marker is gone: that marker lives in the attacker-writable install +// directory (#868). func markedRecoveryPaths(targetPath string) ([]string, error) { recoveryPaths, err := existingRecoveryPaths(targetPath) if err != nil { return nil, err } var marked []string + seen := make(map[string]bool) for _, recoveryPath := range recoveryPaths { if oldBinaryPreserved(recoveryPath) { marked = append(marked, recoveryPath) + seen[strings.ToLower(recoveryPath)] = true } } + for _, recorded := range unresolvedRecordedRecoveryPaths(targetPath) { + key := strings.ToLower(recorded) + if seen[key] { + continue + } + seen[key] = true + marked = append(marked, recorded) + } return marked, nil } From 432ef71aa71661e18b3617167b04be7ced3e50ab Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 02:19:46 +0000 Subject: [PATCH 2/4] fix(update): keep a fail-closed signal if unresolved recovery records cannot be written Ignoring a failed appendUnresolvedRecoveryRecord left only the sibling .keep marker. An install-directory writer can delete that marker, after which preflight finds no unresolved state and a retry can overwrite the last verified recovery copy. Relocate that copy to a .recovery name when the per-user record cannot be persisted so promotion still refuses. Fixes #868 --- internal/update/replace_windows.go | 23 ++++- internal/update/stage_promote_windows_test.go | 90 +++++++++++++++++++ internal/update/stage_windows.go | 8 +- 3 files changed, 116 insertions(+), 5 deletions(-) diff --git a/internal/update/replace_windows.go b/internal/update/replace_windows.go index 1ef659f00..e981c07e8 100644 --- a/internal/update/replace_windows.go +++ b/internal/update/replace_windows.go @@ -431,10 +431,31 @@ func appendRecoveryCleanupRecord(targetPath string, recoveryPath string, identit // as the last verified binary. Unlike the sibling .keep marker in the // installation directory, this lives in per-user state an install-directory // writer cannot delete. Preflight consults it alongside the marker (#868). -func appendUnresolvedRecoveryRecord(targetPath string, recoveryPath string, identity recoveryIdentity) error { +// +// A var so a test can force the failure branch. Making the real per-user store +// unwritable also takes down cleanup-queue writes this path does not own. +var appendUnresolvedRecoveryRecord = func(targetPath string, recoveryPath string, identity recoveryIdentity) error { return appendRecoveryCleanupRecordWithState(targetPath, recoveryPath, identity, true) } +// persistFailedRestoreSignal keeps a durable fail-closed preflight signal after +// restoreOriginalBinary has already written the sibling .keep marker. The +// preferred signal is the per-user unresolved record. If that write fails, the +// marker is the only remaining protection and an install-directory writer can +// delete it; relocate the last verified copy to a .recovery name so the next +// preflight still refuses (#868). +func persistFailedRestoreSignal(file *os.File, targetPath string, asidePath string, identity recoveryIdentity, restoreErr error) error { + if err := appendUnresolvedRecoveryRecord(targetPath, asidePath, identity); err == nil { + return restoreErr + } else if kept, keepErr := keepUnmarkedRecoveryCopy(file, asidePath); keepErr == nil { + return fmt.Errorf("%w (unresolved recovery record could not be written: %v; the last binary this updater verified was moved to the distinct recovery path %s)", restoreErr, err, kept) + } else if kept != "" { + return fmt.Errorf("%w (unresolved recovery record could not be written: %v; the last binary this updater verified was moved to %s but could not be verified there: %v)", restoreErr, err, kept, keepErr) + } else { + return fmt.Errorf("%w (unresolved recovery record could not be written: %v)", restoreErr, err) + } +} + func appendRecoveryCleanupRecordWithState(targetPath string, recoveryPath string, identity recoveryIdentity, unresolved bool) error { if !validUpdaterRecoveryPath(targetPath, recoveryPath) { return fmt.Errorf("invalid updater recovery path %s", recoveryPath) diff --git a/internal/update/stage_promote_windows_test.go b/internal/update/stage_promote_windows_test.go index 7b1317927..23ea6b774 100644 --- a/internal/update/stage_promote_windows_test.go +++ b/internal/update/stage_promote_windows_test.go @@ -347,6 +347,96 @@ func TestPreflightRefusesWhenRecoveryMarkerIsDeletedButTrustedRecordRemains(t *t } } +// TestPreflightRefusesWhenRecordWriteFailsAndRecoveryMarkerIsDeleted is the +// #868 follow-up: if the per-user unresolved record cannot be written, the +// sibling .keep marker is the only remaining install-directory signal. A +// writer there can delete it. The failed-restore path must still leave a +// durable fail-closed signal so the next promotion refuses. +func TestPreflightRefusesWhenRecordWriteFailsAndRecoveryMarkerIsDeleted(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("old-binary"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte("verified-binary"), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + + const suffix = "feedfeedfeedfeedfeedfeedfeedfeed" + stubRandomStagingSuffix(t, suffix) + originalRename := renameFileByHandle + var conflicting windows.Handle + renameFileByHandle = func(_ *os.File, target string) error { + pathPtr, err := windows.UTF16PtrFromString(target) + if err != nil { + return err + } + conflicting, err = windows.CreateFile(pathPtr, windows.GENERIC_WRITE, 0, nil, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + return fmt.Errorf("create conflicting target: %w", err) + } + return errors.New("injected promotion failure") + } + originalAppend := appendUnresolvedRecoveryRecord + appendUnresolvedRecoveryRecord = func(string, string, recoveryIdentity) error { + return errors.New("injected record write failure") + } + t.Cleanup(func() { + renameFileByHandle = originalRename + appendUnresolvedRecoveryRecord = originalAppend + if conflicting != 0 { + _ = windows.CloseHandle(conflicting) + } + }) + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want ErrTargetPossiblyTampered", err) + } + if conflicting != 0 { + _ = windows.CloseHandle(conflicting) + conflicting = 0 + } + // Restore production seams so a missed preflight refusal would actually + // install, rather than fail for the injected reason. + renameFileByHandle = originalRename + appendUnresolvedRecoveryRecord = originalAppend + + asidePath := targetPath + ".zero-update-" + suffix + ".old" + expectedRecovery := asidePath + "." + suffix + ".recovery" + if got, readErr := os.ReadFile(expectedRecovery); readErr != nil || string(got) != "old-binary" { + t.Fatalf("relocated recovery copy = %q err=%v, want the last verified binary at %s", got, readErr, expectedRecovery) + } + if !strings.Contains(err.Error(), expectedRecovery) { + t.Fatalf("installBinary error = %v, want the authoritative relocated recovery path", err) + } + if _, statErr := os.Lstat(asidePath); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("aside path %s still occupied after relocation: %v", asidePath, statErr) + } + for _, record := range loadRecoveryCleanupQueue(targetPath).Records { + if record.Unresolved { + t.Fatalf("unresolved record was written despite injected failure: %+v", record) + } + } + + clearOldBinaryPreserved(asidePath) + if oldBinaryPreserved(asidePath) { + t.Fatal("marker deletion did not take effect") + } + + err = installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("retry after record-write failure and marker deletion = %v, want refusal", err) + } + if !strings.Contains(err.Error(), expectedRecovery) { + t.Fatalf("error = %v, want it to name relocated recovery path %s", err, expectedRecovery) + } + if got, readErr := os.ReadFile(expectedRecovery); readErr != nil || string(got) != "old-binary" { + t.Fatalf("recovery copy after refusal = %q err=%v, want it left intact", got, readErr) + } +} + // TestSuccessfulPromoteCleanupRecordDoesNotBlockLaterUpdate pins the success // path for #868: a verified promotion still writes a cleanup record, that // record is not unresolved, and the next update proceeds. diff --git a/internal/update/stage_windows.go b/internal/update/stage_windows.go index 6aab1ce31..4aa98277a 100644 --- a/internal/update/stage_windows.go +++ b/internal/update/stage_windows.go @@ -155,10 +155,10 @@ func (staged *stagedBinary) promote(targetPath string) error { if renameErr != nil { if restoreErr := restoreOriginalBinary(original, asidePath, targetPath); restoreErr != nil { // Best-effort: the restore error is what the operator must see. - // The .keep marker is still in the install directory if this write - // fails; if it succeeds, deleting that marker cannot silence the - // next preflight (#868). - _ = appendUnresolvedRecoveryRecord(targetPath, asidePath, originalIdentity) + // persistFailedRestoreSignal writes the per-user record when it + // can, and otherwise relocates the last verified copy so deleting + // the sibling .keep marker cannot silence the next preflight (#868). + restoreErr = persistFailedRestoreSignal(original, targetPath, asidePath, originalIdentity, restoreErr) return fmt.Errorf("install new binary: %v; additionally failed to restore the original binary: %w", renameErr, restoreErr) } return fmt.Errorf("install new binary: %w", renameErr) From ff7387a402a931affa851cf0e96b9246a4837a3c Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 18:21:43 +0000 Subject: [PATCH 3/4] fix(update): do not relocate the live binary when failed-restore compensation cannot record persistFailedRestoreSignal relocated through the handle when the per-user unresolved record could not be written, without checking that the object was still the aside copy. A restore that had already put the verified bytes back at targetPath then had that binary moved out of the executable path. Verify identity with verifyPromotedTarget before relocating. Unreadable recovery-state JSON now fails closed instead of looking like an empty queue. The per-user vs per-machine split is documented: a second account still relies on the install-directory .keep marker. Fixes #868 --- docs/UPDATE.md | 7 + internal/update/replace_windows.go | 66 +++++++-- internal/update/stage_promote_windows_test.go | 136 +++++++++++++++++- internal/update/stage_windows.go | 6 +- 4 files changed, 195 insertions(+), 20 deletions(-) diff --git a/docs/UPDATE.md b/docs/UPDATE.md index a3ee7b532..f89a39b16 100644 --- a/docs/UPDATE.md +++ b/docs/UPDATE.md @@ -105,6 +105,13 @@ again. Deleting the marker alone is not enough: a failed restore also records that copy in per-user state outside the installation directory, and the next update consults that record even if the marker is gone. +That trusted record lives under `%AppData%\zero\update-recovery` (per-user). +The recovery copy and its `.keep` marker sit in the installation directory +(per-machine). A second Windows account on the same machine does not see the +first account's unresolved record; the sibling marker is the machine-visible +signal for every account. There is no separate machine-wide store: writing one +would need a shared writable location and ACL rules the updater does not own. + This is deliberately fail-closed and differs from older releases, which deleted `.old` and proceeded. The trade-off is that anyone who can write in the installation directory can plant `.old` and `.old.keep` there diff --git a/internal/update/replace_windows.go b/internal/update/replace_windows.go index e981c07e8..f7b2fdc51 100644 --- a/internal/update/replace_windows.go +++ b/internal/update/replace_windows.go @@ -289,6 +289,13 @@ type recoveryCleanupQueue struct { Records []recoveryCleanupRecord `json:"records"` } +// recoveryCleanupStateDir is per-user (%AppData% via UserConfigDir). The +// recovery copy and its .keep marker live in the installation directory, which +// is per-machine. A second Windows account therefore does not see the first +// account's unresolved record; the sibling marker is the machine-visible +// signal. A machine-wide store would need a writable shared location and ACL +// rules this updater does not own, so the record stays in the existing +// per-user update-recovery directory. var recoveryCleanupStateDir = func() (string, error) { root, err := config.UserConfigDir() if err != nil { @@ -340,27 +347,36 @@ type recoveryCleanupCandidate struct { record recoveryCleanupRecord } -func loadRecoveryCleanupQueue(targetPath string) recoveryCleanupQueue { +func loadRecoveryCleanupQueue(targetPath string) (recoveryCleanupQueue, error) { recordPath, err := recoveryCleanupRecordPath(targetPath) if err != nil { - return recoveryCleanupQueue{} + return recoveryCleanupQueue{}, err } data, err := os.ReadFile(recordPath) if err != nil { - return recoveryCleanupQueue{} + if errors.Is(err, os.ErrNotExist) { + return recoveryCleanupQueue{}, nil + } + return recoveryCleanupQueue{}, fmt.Errorf("read recovery cleanup record %s: %w", recordPath, err) } var queue recoveryCleanupQueue - if json.Unmarshal(data, &queue) != nil || queue.Records == nil { - var legacy recoveryCleanupRecord - if json.Unmarshal(data, &legacy) == nil && legacy.Path != "" { - queue.Records = []recoveryCleanupRecord{legacy} - } + if err := json.Unmarshal(data, &queue); err == nil && queue.Records != nil { + return queue, nil + } + var legacy recoveryCleanupRecord + if json.Unmarshal(data, &legacy) == nil && legacy.Path != "" { + return recoveryCleanupQueue{Records: []recoveryCleanupRecord{legacy}}, nil } - return queue + return recoveryCleanupQueue{}, fmt.Errorf("parse recovery cleanup record %s", recordPath) } func prepareRecoveryCleanup(targetPath string) []recoveryCleanupCandidate { - queue := loadRecoveryCleanupQueue(targetPath) + queue, err := loadRecoveryCleanupQueue(targetPath) + if err != nil { + // Unreadable trusted state is not "nothing to clean": skip destructive + // cleanup rather than treat the queue as empty. + return nil + } var candidates []recoveryCleanupCandidate // Records outlive a single attempt so a temporarily locked copy is still // deleted on a later run, but a record that can never become actionable has @@ -444,9 +460,19 @@ var appendUnresolvedRecoveryRecord = func(targetPath string, recoveryPath string // marker is the only remaining protection and an install-directory writer can // delete it; relocate the last verified copy to a .recovery name so the next // preflight still refuses (#868). +// +// Relocation is bound to the object still named by asidePath. restoreOriginalBinary +// may already have moved the verified bytes back onto targetPath (the live +// binary) or to a .recovery name; keepUnmarkedRecoveryCopy renames through the +// handle without re-checking that path, so compensation must not run unless +// verifyPromotedTarget says the handle still is the aside copy. Making a failed +// record write worse by pulling the restored executable out of targetPath is +// not an acceptable fallback. func persistFailedRestoreSignal(file *os.File, targetPath string, asidePath string, identity recoveryIdentity, restoreErr error) error { if err := appendUnresolvedRecoveryRecord(targetPath, asidePath, identity); err == nil { return restoreErr + } else if verifyPromotedTarget(file, asidePath) != nil { + return fmt.Errorf("%w (unresolved recovery record could not be written: %v)", restoreErr, err) } else if kept, keepErr := keepUnmarkedRecoveryCopy(file, asidePath); keepErr == nil { return fmt.Errorf("%w (unresolved recovery record could not be written: %v; the last binary this updater verified was moved to the distinct recovery path %s)", restoreErr, err, kept) } else if kept != "" { @@ -467,7 +493,10 @@ func appendRecoveryCleanupRecordWithState(targetPath string, recoveryPath string FileIndexLow: identity.FileIndexLow, Unresolved: unresolved, } - queue := loadRecoveryCleanupQueue(targetPath) + queue, err := loadRecoveryCleanupQueue(targetPath) + if err != nil { + return err + } queue.Records = append(queue.Records, record) return writeRecoveryCleanupQueue(targetPath, queue) } @@ -475,9 +504,13 @@ func appendRecoveryCleanupRecordWithState(targetPath string, recoveryPath string // unresolvedRecordedRecoveryPaths returns live recovery copies vouched for by // per-user unresolved-state records. Absence of the sibling .keep marker does // not drop them: that marker is attacker-deletable in the install directory. -func unresolvedRecordedRecoveryPaths(targetPath string) []string { +func unresolvedRecordedRecoveryPaths(targetPath string) ([]string, error) { + queue, err := loadRecoveryCleanupQueue(targetPath) + if err != nil { + return nil, err + } var paths []string - for _, record := range loadRecoveryCleanupQueue(targetPath).Records { + for _, record := range queue.Records { if !record.Unresolved || !validUpdaterRecoveryPath(targetPath, record.Path) { continue } @@ -486,7 +519,7 @@ func unresolvedRecordedRecoveryPaths(targetPath string) []string { } paths = append(paths, record.Path) } - return paths + return paths, nil } func writeRecoveryCleanupQueue(targetPath string, queue recoveryCleanupQueue) error { @@ -559,7 +592,10 @@ func closeRecoveryCleanupCandidates(candidates []recoveryCleanupCandidate) { // full binaries. func cleanupSupersededRecoveryCopies(targetPath string, candidates []recoveryCleanupCandidate) { defer closeRecoveryCleanupCandidates(candidates) - queue := loadRecoveryCleanupQueue(targetPath) + queue, err := loadRecoveryCleanupQueue(targetPath) + if err != nil { + return + } deleted := make(map[recoveryCleanupRecord]bool) for index := range candidates { candidate := &candidates[index] diff --git a/internal/update/stage_promote_windows_test.go b/internal/update/stage_promote_windows_test.go index 23ea6b774..cd3004d3b 100644 --- a/internal/update/stage_promote_windows_test.go +++ b/internal/update/stage_promote_windows_test.go @@ -325,7 +325,10 @@ func TestPreflightRefusesWhenRecoveryMarkerIsDeletedButTrustedRecordRemains(t *t if !oldBinaryPreserved(recoveryPath) { t.Fatal("failed restore must still write the sibling marker") } - queue := loadRecoveryCleanupQueue(targetPath) + queue, loadErr := loadRecoveryCleanupQueue(targetPath) + if loadErr != nil { + t.Fatalf("loadRecoveryCleanupQueue: %v", loadErr) + } if len(queue.Records) != 1 || !queue.Records[0].Unresolved { t.Fatalf("trusted records = %+v, want one unresolved entry", queue.Records) } @@ -414,7 +417,11 @@ func TestPreflightRefusesWhenRecordWriteFailsAndRecoveryMarkerIsDeleted(t *testi if _, statErr := os.Lstat(asidePath); !errors.Is(statErr, os.ErrNotExist) { t.Fatalf("aside path %s still occupied after relocation: %v", asidePath, statErr) } - for _, record := range loadRecoveryCleanupQueue(targetPath).Records { + queue, loadErr := loadRecoveryCleanupQueue(targetPath) + if loadErr != nil { + t.Fatalf("loadRecoveryCleanupQueue: %v", loadErr) + } + for _, record := range queue.Records { if record.Unresolved { t.Fatalf("unresolved record was written despite injected failure: %+v", record) } @@ -437,6 +444,120 @@ func TestPreflightRefusesWhenRecordWriteFailsAndRecoveryMarkerIsDeleted(t *testi } } +// TestPersistFailedRestoreSignalDoesNotRelocateLiveBinary is the compensation +// identity check: when the per-user record write fails, persistFailedRestoreSignal +// must not rename through the handle unless that object is still the aside copy. +// If restore already put the verified bytes back at targetPath, relocating would +// remove the binary the user is running. +func TestPersistFailedRestoreSignalDoesNotRelocateLiveBinary(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + asidePath := targetPath + ".zero-update-0123456789abcdef0123456789abcdef.old" + if err := os.WriteFile(targetPath, []byte("restored-live-binary"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + if err := os.WriteFile(asidePath, []byte("not-the-handle"), 0o755); err != nil { + t.Fatalf("WriteFile aside: %v", err) + } + + live, err := openRecoveryCopy(targetPath) + if err != nil { + t.Fatalf("open live binary: %v", err) + } + defer func() { _ = live.Close() }() + identity, err := recoveryFileIdentity(live) + if err != nil { + t.Fatalf("recoveryFileIdentity: %v", err) + } + + originalAppend := appendUnresolvedRecoveryRecord + appendUnresolvedRecoveryRecord = func(string, string, recoveryIdentity) error { + return errors.New("injected record write failure") + } + t.Cleanup(func() { appendUnresolvedRecoveryRecord = originalAppend }) + + restoreErr := fmt.Errorf("%w: restore blocked", ErrTargetPossiblyTampered) + err = persistFailedRestoreSignal(live, targetPath, asidePath, identity, restoreErr) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("error = %v, want the wrapped restore error", err) + } + if strings.Contains(err.Error(), ".recovery") { + t.Fatalf("error = %v, compensation must not claim a relocation", err) + } + + got, readErr := os.ReadFile(targetPath) + if readErr != nil || string(got) != "restored-live-binary" { + t.Fatalf("target = %q err=%v, want the live binary left in place", got, readErr) + } + if got, readErr := os.ReadFile(asidePath); readErr != nil || string(got) != "not-the-handle" { + t.Fatalf("aside = %q err=%v, want the unrelated aside left in place", got, readErr) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, entry := range entries { + if strings.HasSuffix(strings.ToLower(entry.Name()), ".recovery") { + t.Fatalf("compensation relocated an object to %s", entry.Name()) + } + } +} + +// TestPreflightFailsClosedWhenRecoveryRecordIsUnreadable: a truncated or +// unparseable per-user state file is not "nothing to report". The file exists +// to remember that something went wrong; refusing to parse it must refuse the +// next promotion rather than silently switch the refusal off. +func TestPreflightFailsClosedWhenRecoveryRecordIsUnreadable(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("installed"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + recordPath, err := recoveryCleanupRecordPath(targetPath) + if err != nil { + t.Fatalf("recoveryCleanupRecordPath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(recordPath), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(recordPath, []byte("{truncated"), 0o600); err != nil { + t.Fatalf("WriteFile truncated record: %v", err) + } + + if _, err := loadRecoveryCleanupQueue(targetPath); err == nil { + t.Fatal("loadRecoveryCleanupQueue accepted truncated JSON, want a parse error") + } + if err := preflightRecoveryState(targetPath); !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("preflight = %v, want fail-closed ErrTargetPossiblyTampered", err) + } + + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte("verified-binary"), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + err = installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary = %v, want refusal when recovery state is unreadable", err) + } + if got, readErr := os.ReadFile(targetPath); readErr != nil || string(got) != "installed" { + t.Fatalf("target = %q err=%v, want the installed binary left in place", got, readErr) + } +} + +// TestLoadRecoveryCleanupQueueMissingIsEmpty keeps a missing state file as +// "nothing to report", distinct from an unreadable file that must fail closed. +func TestLoadRecoveryCleanupQueueMissingIsEmpty(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + queue, err := loadRecoveryCleanupQueue(targetPath) + if err != nil { + t.Fatalf("missing record: %v, want empty queue not an error", err) + } + if len(queue.Records) != 0 { + t.Fatalf("records = %+v, want none", queue.Records) + } +} + // TestSuccessfulPromoteCleanupRecordDoesNotBlockLaterUpdate pins the success // path for #868: a verified promotion still writes a cleanup record, that // record is not unresolved, and the next update proceeds. @@ -454,7 +575,10 @@ func TestSuccessfulPromoteCleanupRecordDoesNotBlockLaterUpdate(t *testing.T) { if err := installBinary(sourcePath, targetPath); err != nil { t.Fatalf("installBinary: %v", err) } - queue := loadRecoveryCleanupQueue(targetPath) + queue, loadErr := loadRecoveryCleanupQueue(targetPath) + if loadErr != nil { + t.Fatalf("loadRecoveryCleanupQueue: %v", loadErr) + } if len(queue.Records) != 1 { t.Fatalf("cleanup records after success = %d, want 1", len(queue.Records)) } @@ -860,7 +984,11 @@ func TestRecoveryCleanupRetiresRecordsForVanishedCopies(t *testing.T) { func recordedRecoveryCleanupCount(t *testing.T, targetPath string) int { t.Helper() - return len(loadRecoveryCleanupQueue(targetPath).Records) + queue, err := loadRecoveryCleanupQueue(targetPath) + if err != nil { + t.Fatalf("loadRecoveryCleanupQueue: %v", err) + } + return len(queue.Records) } func TestInstallBinaryBoundsRecoveryCopiesAcrossRepeatedUpgrades(t *testing.T) { diff --git a/internal/update/stage_windows.go b/internal/update/stage_windows.go index 4aa98277a..40c10658b 100644 --- a/internal/update/stage_windows.go +++ b/internal/update/stage_windows.go @@ -366,7 +366,11 @@ func markedRecoveryPaths(targetPath string) ([]string, error) { seen[strings.ToLower(recoveryPath)] = true } } - for _, recorded := range unresolvedRecordedRecoveryPaths(targetPath) { + recordedPaths, err := unresolvedRecordedRecoveryPaths(targetPath) + if err != nil { + return nil, err + } + for _, recorded := range recordedPaths { key := strings.ToLower(recorded) if seen[key] { continue From 8d00cccb234f9b4b4d4573f2cd1674c50d992e70 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 02:45:43 -0400 Subject: [PATCH 4/4] fix(update): durably commit recovery state on Windows --- internal/update/replace_windows.go | 23 ++++++- internal/update/stage_promote_windows_test.go | 62 +++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/internal/update/replace_windows.go b/internal/update/replace_windows.go index f7b2fdc51..2aedfd4b1 100644 --- a/internal/update/replace_windows.go +++ b/internal/update/replace_windows.go @@ -522,6 +522,19 @@ func unresolvedRecordedRecoveryPaths(targetPath string) ([]string, error) { return paths, nil } +var ( + syncRecoveryCleanupFile = (*os.File).Sync + moveRecoveryCleanupFile = windows.MoveFileEx +) + +func replaceRecoveryCleanupFile(sourcePath string, targetPath string) error { + return moveRecoveryCleanupFile( + windows.StringToUTF16Ptr(sourcePath), + windows.StringToUTF16Ptr(targetPath), + windows.MOVEFILE_REPLACE_EXISTING|windows.MOVEFILE_WRITE_THROUGH, + ) +} + func writeRecoveryCleanupQueue(targetPath string, queue recoveryCleanupQueue) error { data, err := json.Marshal(queue) if err != nil { @@ -549,10 +562,18 @@ func writeRecoveryCleanupQueue(targetPath string, queue recoveryCleanupQueue) er _ = temporary.Close() return err } + // This queue may carry an unresolved fail-closed promotion signal, not merely + // an advisory cleanup backlog. Flush its complete contents before publishing, + // then use Windows' write-through replacement so success is not reported in + // the crash window between the namespace update and durable storage. + if err := syncRecoveryCleanupFile(temporary); err != nil { + _ = temporary.Close() + return err + } if err := temporary.Close(); err != nil { return err } - return os.Rename(temporaryPath, recordPath) + return replaceRecoveryCleanupFile(temporaryPath, recordPath) } type fileDispositionInfo struct { diff --git a/internal/update/stage_promote_windows_test.go b/internal/update/stage_promote_windows_test.go index cd3004d3b..d7f454795 100644 --- a/internal/update/stage_promote_windows_test.go +++ b/internal/update/stage_promote_windows_test.go @@ -485,6 +485,9 @@ func TestPersistFailedRestoreSignalDoesNotRelocateLiveBinary(t *testing.T) { t.Fatalf("error = %v, compensation must not claim a relocation", err) } + if err := live.Close(); err != nil { + t.Fatalf("close live recovery handle: %v", err) + } got, readErr := os.ReadFile(targetPath) if readErr != nil || string(got) != "restored-live-binary" { t.Fatalf("target = %q err=%v, want the live binary left in place", got, readErr) @@ -503,6 +506,65 @@ func TestPersistFailedRestoreSignalDoesNotRelocateLiveBinary(t *testing.T) { } } +func TestWriteRecoveryCleanupQueuePropagatesSyncFailure(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + recordPath, err := recoveryCleanupRecordPath(targetPath) + if err != nil { + t.Fatalf("recoveryCleanupRecordPath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(recordPath), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(recordPath, []byte(`{"records":[]}`), 0o600); err != nil { + t.Fatalf("WriteFile existing record: %v", err) + } + + syncErr := errors.New("injected FlushFileBuffers failure") + originalSync := syncRecoveryCleanupFile + originalMove := moveRecoveryCleanupFile + syncRecoveryCleanupFile = func(*os.File) error { return syncErr } + moveCalled := false + moveRecoveryCleanupFile = func(*uint16, *uint16, uint32) error { + moveCalled = true + return nil + } + t.Cleanup(func() { + syncRecoveryCleanupFile = originalSync + moveRecoveryCleanupFile = originalMove + }) + + err = writeRecoveryCleanupQueue(targetPath, recoveryCleanupQueue{Records: []recoveryCleanupRecord{{Path: "unpublished"}}}) + if !errors.Is(err, syncErr) { + t.Fatalf("writeRecoveryCleanupQueue error = %v, want sync failure", err) + } + if moveCalled { + t.Fatal("recovery record was replaced after its content flush failed") + } + got, readErr := os.ReadFile(recordPath) + if readErr != nil || string(got) != `{"records":[]}` { + t.Fatalf("record after sync failure = %q err=%v, want prior durable state", got, readErr) + } +} + +func TestReplaceRecoveryCleanupFileUsesWriteThrough(t *testing.T) { + originalMove := moveRecoveryCleanupFile + var gotFlags uint32 + moveRecoveryCleanupFile = func(_ *uint16, _ *uint16, flags uint32) error { + gotFlags = flags + return nil + } + t.Cleanup(func() { moveRecoveryCleanupFile = originalMove }) + + if err := replaceRecoveryCleanupFile(`C:\\state\\record.tmp`, `C:\\state\\record.json`); err != nil { + t.Fatalf("replaceRecoveryCleanupFile: %v", err) + } + wantFlags := uint32(windows.MOVEFILE_REPLACE_EXISTING | windows.MOVEFILE_WRITE_THROUGH) + if gotFlags != wantFlags { + t.Fatalf("MoveFileEx flags = %#x, want %#x", gotFlags, wantFlags) + } +} + // TestPreflightFailsClosedWhenRecoveryRecordIsUnreadable: a truncated or // unparseable per-user state file is not "nothing to report". The file exists // to remember that something went wrong; refusing to parse it must refuse the