diff --git a/docs/UPDATE.md b/docs/UPDATE.md index 9447d6433..d20f8928a 100644 --- a/docs/UPDATE.md +++ b/docs/UPDATE.md @@ -42,3 +42,43 @@ Endpoint resolution order: Installer scripts download the matching release asset for the local platform and verify its `.sha256` file. If Zero is already installed, run `zero update --check` before reinstalling. + +## Windows recovery state (standalone installs) + +This section describes Windows only. On Linux and macOS a standalone update +renames the staged file directly over the executable path through the +installation directory's file descriptor, so the replacement is atomic and no +aside copy, marker, or recovery record is ever created — a failed update leaves +the previous binary in place and nothing to resolve. + +On Windows, a running executable cannot be replaced in place, so the previous +binary is moved aside to `.zero-update-.old` first. The updater +records that exact file — bound to its filesystem identity, in per-user state +outside the installation directory — and deletes only that recorded copy after +the next update is verified in place. Backups it did not create are never +removed, so a file such as `zero.exe.before-manual-patch.old` is left alone. A +recorded copy that something else holds open (an on-access scanner, an editor) +stays recorded and is removed by a later update instead. + +An update **refuses to run** while unresolved recovery state exists beside the +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..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. + +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 +and make every subsequent update refuse until an operator clears them. That is +the safe direction: the alternative is an update that overwrites the only +verified copy of the previous binary. If updates start refusing, inspect those +files before removing them — an installation directory that a lower-privileged +account can write to is itself worth fixing. diff --git a/internal/update/apply.go b/internal/update/apply.go index 4f97dd458..62ec95eee 100644 --- a/internal/update/apply.go +++ b/internal/update/apply.go @@ -2,6 +2,7 @@ package update import ( "context" + "errors" "fmt" "io" "net/http" @@ -15,6 +16,15 @@ import ( "github.com/Gitlawb/zero/internal/release" ) +// ErrTargetPossiblyTampered reports that an executable path may hold content +// this updater could not verify, because a promotion failed and the original +// could not be restored over it. Only the Windows promotion path produces it +// today (see replace_windows.go for why that platform has the gap and the POSIX +// descriptor-bound path does not), but it is declared here so callers on every +// platform can test for it without build tags — the helper-refresh loop below +// must fail the apply on it rather than downgrade it to a warning. +var ErrTargetPossiblyTampered = errors.New("target executable path may hold unverified content after a failed update") + // DefaultDownloadTimeout bounds the archive/checksum download phase of a // standalone Apply, separately from Options.Timeout (which only covers the // small release-metadata check), so a stalled connection can't hang forever. @@ -36,6 +46,7 @@ type ApplyResult struct { var ( windowsOptionalBinaries = []string{"zero-windows-command-runner.exe", "zero-windows-sandbox-setup.exe"} linuxOptionalBinaries = []string{"zero-linux-sandbox", "zero-seccomp"} + currentExecutable = os.Executable ) // Apply checks for an update and, if one is available, installs it: via @@ -47,25 +58,23 @@ func Apply(ctx context.Context, options Options) (ApplyResult, error) { return ApplyResult{}, err } - executablePath, err := os.Executable() + executablePath, err := currentExecutable() if err != nil { return ApplyResult{}, fmt.Errorf("resolve current executable: %w", err) } if resolved, err := filepath.EvalSymlinks(executablePath); err == nil { executablePath = resolved } - // Best-effort: remove a ".old" left behind by a previous Windows - // replaceBinary call now that enough time (a whole separate invocation) - // has passed for the old process to have released the file. Runs - // regardless of whether an update is available, so it isn't stuck waiting - // on a future upgrade that may never come. - CleanupStaleBinary(executablePath) - + method := DetectInstallMethod(executablePath) + if method != InstallMethodNpm { + if err := preflightRecoveryState(executablePath); err != nil { + return ApplyResult{}, err + } + } if !checkResult.UpdateAvailable { return ApplyResult{Result: checkResult, Message: "already up to date"}, nil } - method := DetectInstallMethod(executablePath) switch method { case InstallMethodNpm: if err := applyNpmUpdate(ctx); err != nil { @@ -179,10 +188,6 @@ func applyStandaloneUpdate(ctx context.Context, result Result, executablePath st } targetDir := filepath.Dir(executablePath) - if err := installBinary(newBinaryPath, executablePath); err != nil { - return nil, err - } - var warnings []string for _, name := range optionalBinaries { source, err := findByBasename(extractDir, name) @@ -194,9 +199,29 @@ func applyStandaloneUpdate(ctx context.Context, result Result, executablePath st continue // only refresh helpers this install already has } if err := installBinary(source, destPath); err != nil { + // An optional helper that simply fails to refresh is a warning: the + // sandbox degrades gracefully without a newer one, and aborting the + // whole update over it would be worse than the stale helper. + // + // Possible tampering is not that. It means this helper's path may now + // hold content the updater could not verify, and helpers are resolved + // from the install directory and EXECUTED by the sandbox runner — so + // reporting Applied: true and exiting 0 would hand the operator a + // success while a sibling executable is suspect. Fail the apply and + // keep errors.Is intact, exactly as the main binary does. + if errors.Is(err, ErrTargetPossiblyTampered) { + return nil, fmt.Errorf("update helper %s: %w", name, err) + } warnings = append(warnings, fmt.Sprintf("failed to update helper %s: %v", name, err)) } } + // Refresh helpers before the main executable. If a helper path may have + // been tampered with, the running main binary must remain on the old version + // so the next invocation still sees this release as available and retries + // the helper instead of returning "already up to date". + if err := installBinary(newBinaryPath, executablePath); err != nil { + return nil, err + } return warnings, nil } @@ -218,23 +243,79 @@ func verifyArchiveChecksum(checksumPath string, expectedArchiveName string) erro return nil } -// installBinary stages sourcePath next to targetPath (same directory, so the -// final rename is atomic/same-filesystem) and then swaps it into place. +// installBinary stages sourcePath next to targetPath (same filesystem, so the +// final rename is atomic) and then swaps it into place. func installBinary(sourcePath string, targetPath string) error { - stagedPath := targetPath + ".new" - if err := copyFile(sourcePath, stagedPath); err != nil { + staged, err := stageBinary(sourcePath, targetPath) + if err != nil { return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err) } - defer func() { - _ = os.Remove(stagedPath) - }() - if err := replaceBinary(targetPath, stagedPath); err != nil { + // Registered before the promotion attempt and reached on every failure path, + // including a mid-write copy error: each attempt now stages under a fresh + // random name, so a leaked partial file is never reused by the next attempt + // and would otherwise accumulate release-sized garbage in the install + // directory. Unverifiable leftovers from a hard crash are preserved rather + // than removed based only on their public filename shape. + defer staged.discard() + if err := staged.promote(targetPath); err != nil { return fmt.Errorf("install %s: %w", filepath.Base(targetPath), err) } return nil } -func copyFile(sourcePath string, destPath string) (retErr error) { +// stagedBinary is a freshly created staging object holding the verified release +// bytes, together with the handle it was created through. Promotion is bound to +// that object rather than to the staging PATHNAME: under the threat model of +// #742 a lower-privileged principal that can write in the installation +// directory may replace a sibling entry, so re-resolving the staging path at +// swap time would let it substitute its own file into the executable path after +// the verified bytes were written. Each platform closes that handoff with its +// own primitive — see the promote implementations. +type stagedBinary struct { + file *os.File + path string + // dir is a private staging directory that must be removed with the file + // (POSIX). Empty on Windows, which binds the swap to the handle instead. + dir string + // dirHandle is an open descriptor on dir (POSIX only), bound to that + // directory's inode rather than its current pathname. promote renames the + // staged file through it (see stage_other.go) so a principal who can write + // in the installation directory cannot redirect the final rename by + // renaming dir aside and recreating a look-alike directory at the same + // path between the identity check and the rename — a pathname lookup at + // that point would resolve through the impostor instead. nil on Windows. + dirHandle *os.File + // parentHandle is the descriptor-bound parent of dir (POSIX only). + parentHandle *os.File + // promoted records that path now IS the installed binary, so discard must + // not delete it. + promoted bool +} + +// stageBinary creates the staging object for targetPath and writes sourcePath's +// bytes into it through the handle it was created with, leaving that handle open +// for promote. +// +// It is a package var so a test can force a staging failure on every platform: +// the staging location is created exclusively under a name that cannot be known +// in advance, which is exactly what stops a test (like an attacker) from +// occupying it from outside. +var stageBinary = func(sourcePath string, targetPath string) (*stagedBinary, error) { + staged, err := createStagedBinary(targetPath) + if err != nil { + return nil, err + } + if err := staged.copyFrom(sourcePath); err != nil { + staged.discard() + return nil, err + } + return staged, nil +} + +// copyFrom writes sourcePath into the staged file through the handle +// createStagedBinary opened. It never reopens by pathname, so the bytes cannot +// land anywhere other than the object that was exclusively created. +func (staged *stagedBinary) copyFrom(sourcePath string) error { source, err := os.Open(sourcePath) if err != nil { return err @@ -242,17 +323,28 @@ func copyFile(sourcePath string, destPath string) (retErr error) { defer func() { _ = source.Close() }() - dest, err := os.OpenFile(destPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o755) - if err != nil { + if _, err := io.Copy(staged.file, source); err != nil { return err } - defer func() { - if closeErr := dest.Close(); closeErr != nil && retErr == nil { - retErr = closeErr - } - }() - _, retErr = io.Copy(dest, source) - return retErr + return staged.file.Sync() +} + +// discard closes the handle(s) and removes what it created, unless the object +// was already promoted into the executable path. +func (staged *stagedBinary) discard() { + if staged == nil { + return + } + // Removal is bound to the staged object before the handle is released + // wherever the platform allows it (Windows, via delete-on-close). Once the + // handle is gone the staging pathname can be re-resolved, and under the + // writable-install-directory threat model that entry may by then name a file + // this updater never created. + staged.discardOpenObject() + if staged.file != nil { + _ = staged.file.Close() + } + staged.discardPaths() } func downloadFile(ctx context.Context, url string, destPath string) error { diff --git a/internal/update/apply_test.go b/internal/update/apply_test.go index 80e9d9f01..640f5ffa7 100644 --- a/internal/update/apply_test.go +++ b/internal/update/apply_test.go @@ -2,6 +2,8 @@ package update import ( "context" + "errors" + "fmt" "net/http" "net/http/httptest" "os" @@ -138,9 +140,12 @@ func TestApplyStandaloneUpdateReplacesBinary(t *testing.T) { if entries, err := os.ReadDir(installDir); err == nil { for _, entry := range entries { name := entry.Name() - // On Windows, replaceBinary leaves ".old" behind (the running - // binary is renamed aside, not deleted) for later best-effort cleanup. - if name == binaryName || (optionalName != "" && name == optionalName) || name == binaryName+".old" || (optionalName != "" && name == optionalName+".old") { + // On Windows, replacement leaves a namespaced recovery copy of each + // replaced binary for identity-bound cleanup by a later update. + windowsRecovery := runtime.GOOS == "windows" && strings.HasSuffix(name, ".old") && + (strings.HasPrefix(name, binaryName+".zero-update-") || + optionalName != "" && strings.HasPrefix(name, optionalName+".zero-update-")) + if name == binaryName || (optionalName != "" && name == optionalName) || windowsRecovery { continue } t.Fatalf("unexpected extra file left in install dir: %s", name) @@ -169,11 +174,11 @@ func TestApplyStandaloneUpdateWarnsWhenHelperRefreshFails(t *testing.T) { if err := os.WriteFile(existingHelperPath, []byte("old-helper"), 0o755); err != nil { t.Fatalf("WriteFile helper: %v", err) } - // Force installBinary's staging copy to fail by occupying its staged - // ".new" path with a directory instead of a file. - if err := os.MkdirAll(existingHelperPath+".new", 0o755); err != nil { - t.Fatalf("MkdirAll staged path: %v", err) - } + // Force the helper's staging to fail. It cannot be provoked from outside any + // more: the staging location is created exclusively under a name (POSIX: a + // private directory) that nothing else can predict or occupy, which is the + // point of the fix. So fail it through the seam instead. + stubStageBinaryFailure(t, existingHelperPath, errors.New("staging is unavailable in this test")) archiveName := "zero-v0.2.0-linux-x64.tar.gz" archiveDir := t.TempDir() @@ -241,6 +246,98 @@ func TestApplyStandaloneUpdateWarnsWhenHelperRefreshFails(t *testing.T) { } } +// TestApplyStandaloneUpdateFailsWhenHelperRefreshReportsTampering covers +// jatmn's #751 P2. An optional helper that merely fails to refresh stays a +// warning — the sandbox degrades fine on a stale helper. Possible tampering is +// different in kind: the helper's path may now hold content the updater could +// not verify, and helpers are resolved from the install directory and executed +// by the sandbox runner, so reporting Applied: true and exiting 0 would hand +// the operator a success while a sibling executable is suspect. +func TestApplyStandaloneUpdateFailsWhenHelperRefreshReportsTampering(t *testing.T) { + binaryName := "zero" + optionalName := "zero-seccomp" + switch runtime.GOOS { + case "windows": + binaryName = "zero.exe" + optionalName = "zero-windows-command-runner.exe" + case "darwin": + t.Skip("macOS ships no optional helper binaries to refresh") + } + + installDir := t.TempDir() + executablePath := filepath.Join(installDir, binaryName) + if err := os.WriteFile(executablePath, []byte("old-binary"), 0o755); err != nil { + t.Fatalf("WriteFile executable: %v", err) + } + existingHelperPath := filepath.Join(installDir, optionalName) + if err := os.WriteFile(existingHelperPath, []byte("old-helper"), 0o755); err != nil { + t.Fatalf("WriteFile helper: %v", err) + } + stubStageBinaryFailure(t, existingHelperPath, fmt.Errorf("promote: %w", ErrTargetPossiblyTampered)) + + archiveName := "zero-v0.2.0-linux-x64.tar.gz" + archiveDir := t.TempDir() + archivePath := filepath.Join(archiveDir, archiveName) + writeTestTarGz(t, archivePath, map[string]string{ + "zero": "new-binary", + "zero.exe": "new-binary-exe", + "zero-seccomp": "new-helper", + "zero-windows-command-runner.exe": "new-helper-exe", + }) + checksum, err := release.SHA256File(archivePath) + if err != nil { + t.Fatalf("SHA256File: %v", err) + } + checksumText, err := release.FormatSHA256Checksum(checksum, archiveName) + if err != nil { + t.Fatalf("FormatSHA256Checksum: %v", err) + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/" + archiveName: + http.ServeFile(w, r, archivePath) + case "/" + archiveName + ".sha256": + _, _ = w.Write([]byte(checksumText)) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + result := Result{ + LatestVersion: "0.2.0", + ReleaseAsset: AssetCheck{ + Platform: "linux", + Arch: "x64", + ArchiveName: archiveName, + ArchiveURL: server.URL + "/" + archiveName, + ChecksumName: archiveName + ".sha256", + ChecksumURL: server.URL + "/" + archiveName + ".sha256", + ArchiveFound: true, + ChecksumFound: true, + Verified: true, + }, + } + + warnings, err := applyStandaloneUpdate(context.Background(), result, executablePath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("applyStandaloneUpdate error = %v, want it to wrap ErrTargetPossiblyTampered", err) + } + if !strings.Contains(err.Error(), optionalName) { + t.Fatalf("error = %v, want it to name the helper %q", err, optionalName) + } + if len(warnings) != 0 { + t.Fatalf("tampering must not be reported as a warning: %v", warnings) + } + mainData, readErr := os.ReadFile(executablePath) + if readErr != nil { + t.Fatalf("ReadFile main binary: %v", readErr) + } + if string(mainData) != "old-binary" { + t.Fatalf("main binary = %q, want old-binary so a retry still sees the update", mainData) + } +} + func TestApplyStandaloneUpdateRejectsChecksumMismatch(t *testing.T) { binaryName := "zero" if runtime.GOOS == "windows" { diff --git a/internal/update/apply_windows_test.go b/internal/update/apply_windows_test.go new file mode 100644 index 000000000..9acedefe6 --- /dev/null +++ b/internal/update/apply_windows_test.go @@ -0,0 +1,58 @@ +//go:build windows + +package update + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" +) + +func TestApplyAlreadyCurrentRefusesUnresolvedRecoveryState(t *testing.T) { + for _, setup := range []struct { + name string + run func(string) error + }{ + {"relocated recovery", func(target string) error { + return os.WriteFile(target+".old.deadbeef.recovery", []byte("known-good"), 0o600) + }}, + {"marked recovery", func(target string) error { + if err := os.WriteFile(target+".old", []byte("known-good"), 0o600); err != nil { + return err + } + return os.WriteFile(target+".old.keep", []byte("preserve"), 0o600) + }}, + {"missing target", func(target string) error { + if err := os.Remove(target); err != nil { + return err + } + return os.WriteFile(target+".old", []byte("known-good"), 0o600) + }}, + } { + t.Run(setup.name, func(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("binary"), 0o755); err != nil { + t.Fatal(err) + } + if err := setup.run(targetPath); err != nil { + t.Fatal(err) + } + originalExecutable := currentExecutable + currentExecutable = func() (string, error) { return targetPath, nil } + defer func() { currentExecutable = originalExecutable }() + + _, err := Apply(context.Background(), Options{ + CurrentVersion: "1.0.0", + Fetch: func(context.Context, string) (Release, error) { + return releaseForTarget(t, "v1.0.0", "windows", "amd64"), nil + }, + }) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("Apply error = %v, want ErrTargetPossiblyTampered", err) + } + }) + } +} diff --git a/internal/update/recovery_preflight_other.go b/internal/update/recovery_preflight_other.go new file mode 100644 index 000000000..03866445c --- /dev/null +++ b/internal/update/recovery_preflight_other.go @@ -0,0 +1,5 @@ +//go:build !windows + +package update + +func preflightRecoveryState(string) error { return nil } diff --git a/internal/update/replace_other.go b/internal/update/replace_other.go deleted file mode 100644 index 8cda808fc..000000000 --- a/internal/update/replace_other.go +++ /dev/null @@ -1,19 +0,0 @@ -//go:build !windows - -package update - -import "os" - -// replaceBinary installs newPath over targetPath. On POSIX systems renaming -// over a running executable is safe: the process currently executing it keeps -// its open inode, and the rename is atomic within the same filesystem. -func replaceBinary(targetPath string, newPath string) error { - if err := os.Chmod(newPath, 0o755); err != nil { - return err - } - return os.Rename(newPath, targetPath) -} - -// CleanupStaleBinary is a no-op outside Windows, which is the only platform -// that requires renaming a running binary aside instead of replacing it directly. -func CleanupStaleBinary(targetPath string) {} diff --git a/internal/update/replace_windows.go b/internal/update/replace_windows.go index d15c29f1e..bc405e714 100644 --- a/internal/update/replace_windows.go +++ b/internal/update/replace_windows.go @@ -3,9 +3,18 @@ package update import ( + "crypto/sha256" + "encoding/json" + "errors" "fmt" "os" + "path/filepath" + "strings" "time" + "unsafe" + + "github.com/Gitlawb/zero/internal/config" + "golang.org/x/sys/windows" ) const ( @@ -13,33 +22,16 @@ const ( restoreRenameRetryDelay = 100 * time.Millisecond ) -// replaceBinary installs newPath over targetPath. Windows will not let a -// running executable be overwritten or deleted directly, but NTFS does allow -// renaming it aside — the same trick already used for locked config files in -// internal/cli/mcp_config.go's replaceMCPWritableConfigFile. -func replaceBinary(targetPath string, newPath string) error { - oldPath := targetPath + ".old" - _ = os.Remove(oldPath) // best-effort cleanup of a leftover from a previous upgrade - if err := os.Rename(targetPath, oldPath); err != nil { - return fmt.Errorf("rename running binary aside: %w", err) - } - if err := os.Rename(newPath, targetPath); err != nil { - // Retry the restore: a transient Windows file lock (antivirus/indexer - // scanning the just-renamed file, a lingering handle) can make a rename - // fail momentarily, and here failure means targetPath is left missing - // entirely rather than merely stale — worth a short retry to avoid that. - if restoreErr := renameWithRetry(oldPath, targetPath); restoreErr != nil { - return fmt.Errorf("install new binary: %w; additionally failed to restore the original binary: %v (original preserved at %s)", err, restoreErr, oldPath) - } - return fmt.Errorf("install new binary: %w", err) - } - return nil -} - -func renameWithRetry(oldPath string, newPath string) error { +// renameOpenFileWithRetry renames the object file refers to onto newPath, +// retrying while something transient (an on-access scanner, a stale handle from +// the previous process image) still occupies the destination. The retry is on +// the handle-bound rename rather than a pathname rename: the destination is in a +// directory the threat principal can write, so re-resolving the SOURCE pathname +// between attempts would let a substitute be restored instead. +func renameOpenFileWithRetry(file *os.File, newPath string) error { var lastErr error for attempt := 0; attempt < restoreRenameRetryAttempts; attempt++ { - if err := os.Rename(oldPath, newPath); err == nil { + if err := renameRecoveryFileByHandle(file, newPath); err == nil { return nil } else { lastErr = err @@ -49,9 +41,489 @@ func renameWithRetry(oldPath string, newPath string) error { return lastErr } -// CleanupStaleBinary best-effort removes a ".old" file left behind by a -// previous replaceBinary call once the old process holding it has exited. -// Callers should invoke this once at startup for the current executable. -func CleanupStaleBinary(targetPath string) { - _ = os.Remove(targetPath + ".old") +// This file is where ErrTargetPossiblyTampered (declared in apply.go, so +// cross-platform callers can test for it) is produced. It is wrapped into the +// error promote returns when a promotion attempt fails AND restoring the +// original binary to targetPath also fails. That combination means a principal +// who can write in the installation directory occupied targetPath in the gap +// the updater opened by renaming the running binary aside, and Windows would +// not let anything — including the restore — replace it: +// MOVEFILE_REPLACE_EXISTING cannot force past another handle's share-mode lock. +// This is not an ordinary failed update (the previous version simply staying in +// place); the executable path may now hold attacker-controlled bytes, so a +// caller must surface it as a security-relevant condition, not the same "try +// again later" failure as a stalled download. +// +// It is also returned by promote when a PREVIOUS run left that state behind and +// nobody has resolved it yet — see the refusal there. + +// restoreOriginalBinary moves the object file refers to — the binary promote +// moved aside from targetPath, held open since before that rename — back onto +// targetPath after a failed promotion. Everything here is bound to that handle: +// oldPath is only ever used to NAME the object for the operator and to site the +// recovery marker, never re-resolved to decide what gets restored. A failed +// immediate restore is surfaced; the aside copy must not be queued as a reboot +// source because its pathname can be replaced before reboot under the +// writable-directory threat model. +func restoreOriginalBinary(file *os.File, oldPath string, targetPath string) error { + err := renameOpenFileWithRetry(file, targetPath) + if err == nil { + err = verifyPromotedTarget(file, targetPath) + } + if err == nil { + return nil + } + // The error this produces tells the operator their original is preserved at + // oldPath. Record that so a later promotion refuses to treat the recovery + // copy as the ordinary destination for another aside rename. + if markErr := markOldBinaryPreserved(oldPath); markErr != nil { + // The marker could not be established, so nothing on disk identifies + // oldPath as the recovery copy. Move it to a distinct name and report that + // authoritative location to the operator. + if kept, keepErr := keepUnmarkedRecoveryCopy(file, oldPath); keepErr == nil { + return fmt.Errorf( + "%w: %v (the recovery marker could not be written: %v; the last binary this updater verified was moved to the distinct recovery path %s)", + ErrTargetPossiblyTampered, err, markErr, kept, + ) + } else if kept != "" { + // The move succeeded but the post-move verification did not, so + // oldPath is already vacated — point at kept, the path the + // bytes actually landed at, not the path that no longer holds them. + return fmt.Errorf( + "%w: %v (the recovery marker could not be written: %v; the last binary this updater verified was moved to %s but could not be verified there: %v)", + ErrTargetPossiblyTampered, err, markErr, kept, keepErr, + ) + } else { + return fmt.Errorf( + "%w: %v (the recovery marker could not be written: %v — manually copy %s somewhere safe now)", + ErrTargetPossiblyTampered, err, markErr, oldPath, + ) + } + } + return fmt.Errorf("%w: %v", ErrTargetPossiblyTampered, err) +} + +// oldBinaryPreservedSuffix names the marker written beside a ".old" that +// a failed restore left as the last known-good binary. +const oldBinaryPreservedSuffix = ".keep" + +// markOldBinaryPreserved records that oldPath must survive routine cleanup. +// +// The marker is created with the same exclusive, no-follow semantics as a +// staging file, and never truncates. Its pathname is predictable, so under the +// writable-install-directory threat model a lower-privileged writer can +// pre-create it as a hard link or reparse point; opening that with O_TRUNC +// would let the elevated updater write through it into a file of the attacker's +// choosing. CREATE_NEW plus FILE_FLAG_OPEN_REPARSE_POINT plus the same +// fresh-regular-file check refuses that object instead of writing to it. +// +// An existing marker is success, not a rewrite: its contents carry no state +// beyond "this .old is the last verified binary", so there is nothing to update +// and nothing worth opening a pre-existing object for. That also means an entry +// planted at the name by someone else reads as marked, which is the fail-safe +// direction: it makes the next run PRESERVE the recovery copy. +// +// A var so a test can force the failure branch. Every way to make the real +// CreateFile fail here (an unwritable directory, an over-length name) either +// takes the recovery copy down with it or is too platform-fragile to assert on. +var markOldBinaryPreserved = func(oldPath string) error { + markerPath := oldPath + oldBinaryPreservedSuffix + if _, err := os.Lstat(markerPath); err == nil { + return nil + } + pathPtr, err := windows.UTF16PtrFromString(markerPath) + if err != nil { + return err + } + handle, err := windows.CreateFile( + pathPtr, + windows.GENERIC_WRITE|windows.DELETE, + 0, + nil, + windows.CREATE_NEW, + windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + if errors.Is(err, windows.ERROR_FILE_EXISTS) || errors.Is(err, windows.ERROR_ALREADY_EXISTS) { + // Something already occupies the marker name. Whatever it is, this + // function's only job is to make the next run preserve oldPath, and a + // present name does that — without this process writing through an + // object it did not create. + return nil + } + return fmt.Errorf("create recovery marker %s: %w", markerPath, err) + } + marker := os.NewFile(uintptr(handle), markerPath) + if err := verifyFreshRegularFile(handle, markerPath); err != nil { + _ = deleteFileByHandle(marker) + _ = marker.Close() + return err + } + writeErr := writeRecoveryMarker(marker) + // A partial marker must not be left beside a recovery copy that the caller + // then relocates. Delete the object this process created through its own + // handle — never by pathname, which the threat principal can point at + // something else once the handle is released. + var deleteErr error + if writeErr != nil { + deleteErr = deleteFileByHandle(marker) + } + closeErr := marker.Close() + if writeErr == nil { + // A close failure still leaves the fully written marker in place, and + // its presence is the entire state this function records. + _ = closeErr + return nil + } + // Below, the write failed. Each remaining branch decides between reporting + // that failure and conservatively claiming success — and only a confirmed + // absence of the marker earns the report, because claiming success keeps + // oldPath as the authoritative recovery location, which is the safe answer. + if deleteErr != nil { + // The partial marker could not be removed, so it is still there. + return nil + } + if _, statErr := os.Lstat(markerPath); statErr == nil || !errors.Is(statErr, os.ErrNotExist) { + // Something occupies the marker name, or its state cannot be established. + return nil + } + return fmt.Errorf("write recovery marker %s: %w", markerPath, writeErr) +} + +var writeRecoveryMarker = func(marker *os.File) error { + _, err := marker.WriteString("The update at this path failed and could not restore the original binary.\n" + + "The file without this marker's .keep suffix is the last binary this updater verified.\n") + return err +} + +// keepUnmarkedRecoveryCopy moves oldPath out from under routine cleanup when its +// marker could not be established, returning the path it now lives at. +// +// The name is unpredictable for the same reason the staging name is: this runs +// in a directory a lower-privileged principal may be able to write, and a fixed +// recovery name could be pre-created there to make this rename fail or land +// somewhere chosen by someone else. Being unpredictable also means being opaque, +// which is why the caller's error names the path. +func keepUnmarkedRecoveryCopy(file *os.File, oldPath string) (string, error) { + suffix, err := randomStagingSuffix() + if err != nil { + return "", fmt.Errorf("%w: choose recovery path: %v", ErrTargetPossiblyTampered, err) + } + kept := filepath.Join(filepath.Dir(oldPath), filepath.Base(oldPath)+"."+suffix+".recovery") + if _, statErr := os.Lstat(kept); statErr == nil { + return "", fmt.Errorf("%w: recovery path %s already exists", ErrTargetPossiblyTampered, kept) + } else if !errors.Is(statErr, os.ErrNotExist) { + return "", fmt.Errorf("%w: inspect recovery path %s: %v", ErrTargetPossiblyTampered, kept, statErr) + } + if err := renameRecoveryFileByHandle(file, kept); err != nil { + return "", fmt.Errorf("%w: move verified recovery copy to %s: %v", ErrTargetPossiblyTampered, kept, err) + } + if err := verifyPromotedTarget(file, kept); err != nil { + // The rename already reported success, so oldPath is no longer a + // reliable location for the caller to fall back to — surface kept + // anyway so the operator is pointed at the actual (if unverified) + // destination instead of a path the move already vacated. + return kept, fmt.Errorf("%w: verify recovery copy at %s: %v", ErrTargetPossiblyTampered, kept, err) + } + return kept, nil +} + +func openRecoveryCopy(path string) (*os.File, error) { + pathPtr, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, err + } + handle, err := windows.CreateFile( + pathPtr, + windows.DELETE, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, + nil, + windows.OPEN_EXISTING, + windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return nil, err + } + if err := verifyFreshRegularFile(handle, path); err != nil { + _ = windows.CloseHandle(handle) + return nil, err + } + return os.NewFile(uintptr(handle), path), nil +} + +var renameRecoveryFileByHandle = renameOpenFile + +// clearOldBinaryPreserved models the operator accepting the installed binary +// by deleting the recovery marker. The recovery copy itself remains preserved. +func clearOldBinaryPreserved(oldPath string) { + _ = os.Remove(oldPath + oldBinaryPreservedSuffix) +} + +// oldBinaryPreserved reports whether a failed restore marked oldPath as the last +// known-good binary. +// +// Only a definite "the marker is not there" answers false. An Lstat that fails +// for any other reason (permissions, a transient sharing error) leaves the +// question open, and the conservative answer to an open question here is to keep +// the copy: deleting it is irreversible, while keeping it costs one stale file. +func oldBinaryPreserved(oldPath string) bool { + _, err := os.Lstat(oldPath + oldBinaryPreservedSuffix) + return err == nil || !errors.Is(err, os.ErrNotExist) +} + +type recoveryCleanupRecord struct { + Path string `json:"path"` + VolumeSerial uint32 `json:"volumeSerial"` + FileIndexHigh uint32 `json:"fileIndexHigh"` + FileIndexLow uint32 `json:"fileIndexLow"` +} + +type recoveryCleanupQueue struct { + Records []recoveryCleanupRecord `json:"records"` +} + +var recoveryCleanupStateDir = func() (string, error) { + root, err := config.UserConfigDir() + if err != nil { + return "", err + } + return filepath.Join(root, "zero", "update-recovery"), nil +} + +func recoveryCleanupRecordPath(targetPath string) (string, error) { + dir, err := recoveryCleanupStateDir() + if err != nil { + return "", err + } + absolute, err := filepath.Abs(targetPath) + if err != nil { + return "", err + } + digest := sha256.Sum256([]byte(strings.ToLower(filepath.Clean(absolute)))) + return filepath.Join(dir, fmt.Sprintf("%x.json", digest)), nil +} + +func validUpdaterRecoveryPath(targetPath string, recoveryPath string) bool { + if !strings.EqualFold(filepath.Clean(filepath.Dir(targetPath)), filepath.Clean(filepath.Dir(recoveryPath))) { + return false + } + name := strings.ToLower(filepath.Base(recoveryPath)) + prefix := strings.ToLower(filepath.Base(targetPath)) + ".zero-update-" + if !strings.HasPrefix(name, prefix) || !strings.HasSuffix(name, ".old") { + return false + } + suffix := strings.TrimSuffix(strings.TrimPrefix(name, prefix), ".old") + if len(suffix) != 32 { + return false + } + for _, character := range suffix { + if !strings.ContainsRune("0123456789abcdef", character) { + return false + } + } + return true +} + +// prepareRecoveryCleanup opens only the exact object vouched for by trusted +// per-user state from the previous successful promotion. Recovery discovery is +// intentionally broad and filename-based so suspicious state fails closed, but +// destructive cleanup never treats an install-directory name as provenance. +type recoveryCleanupCandidate struct { + file *os.File + record recoveryCleanupRecord +} + +func loadRecoveryCleanupQueue(targetPath string) recoveryCleanupQueue { + recordPath, err := recoveryCleanupRecordPath(targetPath) + if err != nil { + return recoveryCleanupQueue{} + } + data, err := os.ReadFile(recordPath) + if err != nil { + return recoveryCleanupQueue{} + } + 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} + } + } + return queue +} + +func prepareRecoveryCleanup(targetPath string) []recoveryCleanupCandidate { + queue := loadRecoveryCleanupQueue(targetPath) + 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 + // to be retired here or the backlog grows without bound. + retained := queue.Records[:0] + for _, record := range queue.Records { + // Not a name this updater could have produced: unusable as provenance + // no matter what appears at it later. + if !validUpdaterRecoveryPath(targetPath, record.Path) { + continue + } + // Definitively gone — an operator removed it, or a delete this updater + // requested completed once the last handle closed. + if _, err := os.Lstat(record.Path); errors.Is(err, os.ErrNotExist) { + continue + } + retained = append(retained, record) + if oldBinaryPreserved(record.Path) { + continue + } + file, err := openRecoveryCopy(record.Path) + if err != nil { + continue + } + identity, err := recoveryFileIdentity(file) + if err != nil || identity.VolumeSerial != record.VolumeSerial || + identity.FileIndexHigh != record.FileIndexHigh || identity.FileIndexLow != record.FileIndexLow { + _ = file.Close() + continue + } + candidates = append(candidates, recoveryCleanupCandidate{file: file, record: record}) + } + if len(retained) != len(queue.Records) { + queue.Records = retained + _ = writeRecoveryCleanupQueue(targetPath, queue) + } + return candidates +} + +type recoveryIdentity struct { + VolumeSerial uint32 + FileIndexHigh uint32 + FileIndexLow uint32 +} + +func recoveryFileIdentity(file *os.File) (recoveryIdentity, error) { + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(windows.Handle(file.Fd()), &info); err != nil { + return recoveryIdentity{}, err + } + return recoveryIdentity{ + VolumeSerial: info.VolumeSerialNumber, + FileIndexHigh: info.FileIndexHigh, + FileIndexLow: info.FileIndexLow, + }, nil +} + +// appendRecoveryCleanupRecord adds the copy this promotion moved aside to the +// per-user cleanup backlog. The identity comes from the handle the caller held +// across the aside rename, so a pathname substituted afterwards can never be +// 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 { + if !validUpdaterRecoveryPath(targetPath, recoveryPath) { + return fmt.Errorf("invalid updater recovery path %s", recoveryPath) + } + record := recoveryCleanupRecord{ + Path: recoveryPath, + VolumeSerial: identity.VolumeSerial, + FileIndexHigh: identity.FileIndexHigh, + FileIndexLow: identity.FileIndexLow, + } + queue := loadRecoveryCleanupQueue(targetPath) + queue.Records = append(queue.Records, record) + return writeRecoveryCleanupQueue(targetPath, queue) +} + +func writeRecoveryCleanupQueue(targetPath string, queue recoveryCleanupQueue) error { + data, err := json.Marshal(queue) + if err != nil { + return err + } + recordPath, err := recoveryCleanupRecordPath(targetPath) + if err != nil { + return err + } + dir := filepath.Dir(recordPath) + if err := os.MkdirAll(dir, 0o700); err != nil { + return err + } + temporary, err := os.CreateTemp(dir, filepath.Base(recordPath)+".*.tmp") + if err != nil { + return err + } + temporaryPath := temporary.Name() + defer os.Remove(temporaryPath) + if err := temporary.Chmod(0o600); err != nil { + _ = temporary.Close() + return err + } + if _, err := temporary.Write(data); err != nil { + _ = temporary.Close() + return err + } + if err := temporary.Close(); err != nil { + return err + } + return os.Rename(temporaryPath, recordPath) +} + +type fileDispositionInfo struct { + DeleteFile byte +} + +// deleteFileByHandle marks the object file refers to for deletion, which takes +// effect when its last handle closes. It never resolves a pathname, so it can +// only ever delete the object this process already holds open. +func deleteFileByHandle(file *os.File) error { + info := fileDispositionInfo{DeleteFile: 1} + return windows.SetFileInformationByHandle( + windows.Handle(file.Fd()), + windows.FileDispositionInfo, + (*byte)(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ) +} + +func closeRecoveryCleanupCandidates(candidates []recoveryCleanupCandidate) { + for index := range candidates { + if candidates[index].file == nil { + continue + } + _ = candidates[index].file.Close() + candidates[index].file = nil + } +} + +// cleanupSupersededRecoveryCopies marks the exact pre-promotion objects for +// deletion only after the replacement has been verified. The handles deny +// delete sharing, so their entries cannot be substituted in the meantime. +// +// A record is only retired once its object is actually gone. A copy another +// process holds open (an on-access scanner, an operator's editor) stays in the +// backlog and is retried by a later promotion, so transient locks cannot leak +// full binaries. +func cleanupSupersededRecoveryCopies(targetPath string, candidates []recoveryCleanupCandidate) { + defer closeRecoveryCleanupCandidates(candidates) + queue := loadRecoveryCleanupQueue(targetPath) + deleted := make(map[recoveryCleanupRecord]bool) + for index := range candidates { + candidate := &candidates[index] + if err := deleteFileByHandle(candidate.file); err != nil { + continue + } + // FileDispositionInfo removes the entry when the last handle closes, so + // releasing ours is what makes the deletion observable. + _ = candidate.file.Close() + candidate.file = nil + if _, err := os.Lstat(candidate.record.Path); errors.Is(err, os.ErrNotExist) { + deleted[candidate.record] = true + } + } + remaining := queue.Records[:0] + for _, record := range queue.Records { + if !deleted[record] { + remaining = append(remaining, record) + } + } + queue.Records = remaining + _ = writeRecoveryCleanupQueue(targetPath, queue) } diff --git a/internal/update/replace_windows_test.go b/internal/update/replace_windows_test.go index 5649e3dce..e9bd402b5 100644 --- a/internal/update/replace_windows_test.go +++ b/internal/update/replace_windows_test.go @@ -3,63 +3,468 @@ package update import ( + "errors" + "fmt" "os" "path/filepath" + "strings" "testing" + + "golang.org/x/sys/windows" ) -func TestReplaceBinaryReplacesRunningBinary(t *testing.T) { +// 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") - newPath := filepath.Join(dir, "zero.exe.new") + 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() }() - if err := os.WriteFile(targetPath, []byte("old-binary"), 0o755); err != nil { + 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(newPath, []byte("new-binary"), 0o755); err != nil { - t.Fatalf("WriteFile new: %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") - if err := replaceBinary(targetPath, newPath); err != nil { - t.Fatalf("replaceBinary: %v", err) + 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) + } +} - data, err := os.ReadFile(targetPath) +// 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 ".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.Fatalf("ReadFile target: %v", err) + 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) } - if string(data) != "new-binary" { - t.Fatalf("target content = %q, want %q", data, "new-binary") + got, readErr := os.ReadFile(kept) + if readErr != nil { + t.Fatalf("recovery copy was not kept: %v", readErr) } - if _, err := os.Stat(targetPath + ".old"); err != nil { - t.Fatalf("expected the original binary to be preserved at %s.old: %v", targetPath, err) + 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) } } -func TestRenameWithRetrySucceedsImmediately(t *testing.T) { +// 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() - 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) + 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") + } +} - if err := renameWithRetry(src, dst); err != nil { - t.Fatalf("renameWithRetry: %v", err) +// TestRestoreOriginalBinarySurfacesMarkerWriteFailure covers the #751 P3: the +// error promises the original is preserved at .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) } - if _, err := os.Stat(dst); err != nil { - t.Fatalf("expected dst to exist after rename: %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) } } -// A permanently-failing rename (source never appears) must exhaust its -// retries and surface the underlying error, rather than retrying forever. -func TestRenameWithRetryFailsAfterExhaustingAttempts(t *testing.T) { +func TestKeepUnmarkedRecoveryCopyMovesTheOpenedObject(t *testing.T) { dir := t.TempDir() - missing := filepath.Join(dir, "does-not-exist") - dst := filepath.Join(dir, "dst") + 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) + } +} - if err := renameWithRetry(missing, dst); err == nil { - t.Fatal("expected renameWithRetry to fail for a source that never appears") +// 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 } diff --git a/internal/update/stage_other.go b/internal/update/stage_other.go new file mode 100644 index 000000000..ec3e68d2d --- /dev/null +++ b/internal/update/stage_other.go @@ -0,0 +1,225 @@ +//go:build !windows + +package update + +import ( + "crypto/rand" + "encoding/hex" + "errors" + "fmt" + "os" + "path/filepath" + + "golang.org/x/sys/unix" +) + +const stagingDirPrefix = ".zero-stage-" + +const stagingDirectoryCreateAttempts = 100 + +// openStagingDirectory is a test seam for the creation-to-open race. +var openStagingDirectory = func(parentFD int, name string) (int, error) { + return unix.Openat(parentFD, name, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) +} + +// createStagedBinary creates a private directory next to targetPath, binds that +// directory and its parent to descriptors, and creates the staged file through +// the directory descriptor. No later staging operation re-walks the directory +// pathname. +func createStagedBinary(targetPath string) (*stagedBinary, error) { + parentPath := filepath.Dir(targetPath) + parentHandle, err := os.Open(parentPath) + if err != nil { + return nil, fmt.Errorf("open staging parent: %w", err) + } + dirName, createdStat, err := createStagingDirectory(parentHandle) + if err != nil { + _ = parentHandle.Close() + return nil, fmt.Errorf("create staging directory: %w", err) + } + dir := filepath.Join(parentPath, dirName) + dirHandle, err := openAndVerifyStagingDirectory(parentHandle, dirName, createdStat) + if err != nil { + removeStagingDirectoryIfSame(parentHandle, dirName, createdStat) + _ = parentHandle.Close() + return nil, fmt.Errorf("open staging directory: %w", err) + } + path := filepath.Join(dir, filepath.Base(targetPath)) + file, err := createStagingFileAt(dirHandle, filepath.Base(path), path) + if err != nil { + (&stagedBinary{ + path: path, + dir: dir, + dirHandle: dirHandle, + parentHandle: parentHandle, + }).discardPaths() + return nil, err + } + return &stagedBinary{ + file: file, + path: path, + dir: dir, + dirHandle: dirHandle, + parentHandle: parentHandle, + }, nil +} + +// createStagingDirectory creates and stats the directory relative to the +// already-open installation directory. POSIX has no mkdir-and-open primitive, +// so openAndVerifyStagingDirectory additionally verifies that the entry still +// names this object and is a private directory owned by this effective user. +func createStagingDirectory(parent *os.File) (string, unix.Stat_t, error) { + for attempt := 0; attempt < stagingDirectoryCreateAttempts; attempt++ { + var random [16]byte + if _, err := rand.Read(random[:]); err != nil { + return "", unix.Stat_t{}, err + } + name := stagingDirPrefix + hex.EncodeToString(random[:]) + if err := unix.Mkdirat(int(parent.Fd()), name, 0o700); err != nil { + if errors.Is(err, unix.EEXIST) { + continue + } + return "", unix.Stat_t{}, err + } + var created unix.Stat_t + if err := unix.Fstatat(int(parent.Fd()), name, &created, unix.AT_SYMLINK_NOFOLLOW); err != nil { + // Nothing else knows this name yet, so a failure here would otherwise + // leave an empty .zero-stage-* directory behind for good. Removing it + // with rmdir semantics is safe without a prior identity check: the name + // was minted from fresh randomness moments ago, and rmdir refuses + // anything that is not an empty directory, so the worst an entry + // substituted in that window can do is make this call fail. + _ = unix.Unlinkat(int(parent.Fd()), name, unix.AT_REMOVEDIR) + return "", unix.Stat_t{}, err + } + return name, created, nil + } + return "", unix.Stat_t{}, fmt.Errorf("could not allocate a unique staging directory") +} + +func openAndVerifyStagingDirectory(parent *os.File, name string, createdStat unix.Stat_t) (*os.File, error) { + fd, err := openStagingDirectory(int(parent.Fd()), name) + if err != nil { + return nil, err + } + handle := os.NewFile(uintptr(fd), name) + var handleStat unix.Stat_t + if err := unix.Fstat(fd, &handleStat); err != nil { + _ = handle.Close() + return nil, err + } + if handleStat.Dev != createdStat.Dev || + handleStat.Ino != createdStat.Ino || + handleStat.Mode&unix.S_IFMT != unix.S_IFDIR || + handleStat.Mode&0o777 != 0o700 || + int(handleStat.Uid) != os.Geteuid() { + _ = handle.Close() + return nil, fmt.Errorf("staging directory entry was replaced before it could be bound") + } + return handle, nil +} + +// removeStagingDirectoryIfSame removes only the empty directory entry observed +// after creation. It never follows the entry or recursively removes contents; +// a substituted or non-empty directory is preserved. +func removeStagingDirectoryIfSame(parent *os.File, name string, createdStat unix.Stat_t) { + var current unix.Stat_t + if err := unix.Fstatat(int(parent.Fd()), name, ¤t, unix.AT_SYMLINK_NOFOLLOW); err != nil { + return + } + if current.Dev != createdStat.Dev || current.Ino != createdStat.Ino { + return + } + _ = unix.Unlinkat(int(parent.Fd()), name, unix.AT_REMOVEDIR) +} + +// createStagingFileAt creates name inside the directory dir is bound to. +// O_EXCL refuses any pre-existing entry — including a dangling symlink, which +// POSIX guarantees is not resolved — and O_NOFOLLOW refuses a symlink at the +// final component, so neither can redirect the verified bytes elsewhere. +func createStagingFileAt(dir *os.File, name string, displayPath string) (*os.File, error) { + fd, err := unix.Openat( + int(dir.Fd()), + name, + unix.O_CREAT|unix.O_EXCL|unix.O_WRONLY|unix.O_CLOEXEC|unix.O_NOFOLLOW, + 0o755, + ) + if err != nil { + return nil, fmt.Errorf("create %s: %w", displayPath, err) + } + return os.NewFile(uintptr(fd), displayPath), nil +} + +func (staged *stagedBinary) promote(targetPath string) error { + if err := staged.file.Chmod(0o755); err != nil { + return err + } + if err := staged.verifyStagedIdentity(); err != nil { + return err + } + if err := unix.Renameat( + int(staged.dirHandle.Fd()), + filepath.Base(staged.path), + int(staged.parentHandle.Fd()), + filepath.Base(targetPath), + ); err != nil { + return fmt.Errorf("rename staged binary onto %s: %w", targetPath, err) + } + staged.path = targetPath + staged.promoted = true + return nil +} + +func (staged *stagedBinary) verifyStagedIdentity() error { + var handleStat unix.Stat_t + if err := unix.Fstat(int(staged.file.Fd()), &handleStat); err != nil { + return fmt.Errorf("stat staged binary: %w", err) + } + var childStat unix.Stat_t + if err := unix.Fstatat( + int(staged.dirHandle.Fd()), + filepath.Base(staged.path), + &childStat, + unix.AT_SYMLINK_NOFOLLOW, + ); err != nil { + return fmt.Errorf("stat staged binary path %s: %w", staged.path, err) + } + if childStat.Ino != handleStat.Ino || childStat.Dev != handleStat.Dev { + return fmt.Errorf("staged binary %s was replaced after it was written", staged.path) + } + return nil +} + +// discardOpenObject has nothing to do here: discardPaths already removes the +// staged child through the descriptor bound to the private staging directory, +// which no other principal can write, so there is no pathname handoff to close. +func (staged *stagedBinary) discardOpenObject() {} + +// discardPaths removes the child through the bound directory descriptor and +// removes the directory only while its original parent entry still names it. +func (staged *stagedBinary) discardPaths() { + if staged.dirHandle != nil && !staged.promoted { + _ = unix.Unlinkat(int(staged.dirHandle.Fd()), filepath.Base(staged.path), 0) + } + if staged.dirHandle != nil { + var bound unix.Stat_t + var current unix.Stat_t + boundErr := unix.Fstat(int(staged.dirHandle.Fd()), &bound) + currentErr := unix.Fstatat( + int(staged.parentHandle.Fd()), + filepath.Base(staged.dir), + ¤t, + unix.AT_SYMLINK_NOFOLLOW, + ) + _ = staged.dirHandle.Close() + if boundErr == nil && + currentErr == nil && + bound.Dev == current.Dev && + bound.Ino == current.Ino { + _ = unix.Unlinkat(int(staged.parentHandle.Fd()), filepath.Base(staged.dir), unix.AT_REMOVEDIR) + } + } + if staged.parentHandle != nil { + _ = staged.parentHandle.Close() + } +} diff --git a/internal/update/stage_other_test.go b/internal/update/stage_other_test.go new file mode 100644 index 000000000..2393d5eab --- /dev/null +++ b/internal/update/stage_other_test.go @@ -0,0 +1,142 @@ +//go:build !windows + +package update + +import ( + "os" + "path/filepath" + "sync" + "testing" +) + +// stageAtPath drives the production staging primitive — createStagingFileAt, +// the one createStagedBinary calls — against path by binding a descriptor to +// its parent first. The tests below go through it rather than a path-taking +// helper of their own so that weakening the real open flags fails them. +func stageAtPath(path string) (*os.File, error) { + parent, err := os.Open(filepath.Dir(path)) + if err != nil { + return nil, err + } + defer func() { _ = parent.Close() }() + return createStagingFileAt(parent, filepath.Base(path), path) +} + +// TestCreateStagingFileRefusesPrecreatedHardLink is the regression test for +// #742: a lower-privileged attacker who can write in the installation +// directory pre-creates the staging path as a hard link to another file the +// (possibly elevated) updater can write. Staging must fail instead of opening +// and truncating through that link. +func TestCreateStagingFileRefusesPrecreatedHardLink(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "victim") + if err := os.WriteFile(victim, []byte("do not touch"), 0o644); err != nil { + t.Fatalf("WriteFile victim: %v", err) + } + staged := filepath.Join(dir, "staged") + if err := os.Link(victim, staged); err != nil { + t.Fatalf("Link: %v", err) + } + + if _, err := stageAtPath(staged); err == nil { + t.Fatal("staging succeeded through a pre-existing hard link, want error") + } + + data, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("ReadFile victim: %v", err) + } + if string(data) != "do not touch" { + t.Fatalf("victim content = %q, want unchanged", data) + } +} + +// TestCreateStagingFileRefusesPrecreatedSymlink covers the reparse-point +// variant of the same #742 primitive: the staging path pre-created as a +// symlink to another writable file. +func TestCreateStagingFileRefusesPrecreatedSymlink(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "victim") + if err := os.WriteFile(victim, []byte("do not touch"), 0o644); err != nil { + t.Fatalf("WriteFile victim: %v", err) + } + staged := filepath.Join(dir, "staged") + if err := os.Symlink(victim, staged); err != nil { + t.Fatalf("Symlink: %v", err) + } + + if _, err := stageAtPath(staged); err == nil { + t.Fatal("staging succeeded through a pre-existing symlink, want error") + } + + data, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("ReadFile victim: %v", err) + } + if string(data) != "do not touch" { + t.Fatalf("victim content = %q, want unchanged", data) + } +} + +// TestCreateStagingFileSucceedsForFreshPath is the control: a path with +// nothing pre-existing must still work normally. +func TestCreateStagingFileSucceedsForFreshPath(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "staged") + + file, err := stageAtPath(path) + if err != nil { + t.Fatalf("stageAtPath: %v", err) + } + if _, err := file.WriteString("payload"); err != nil { + t.Fatalf("WriteString: %v", err) + } + if err := file.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(data) != "payload" { + t.Fatalf("content = %q, want %q", data, "payload") + } +} + +// TestCreateStagingFileConcurrentRaceOnlyOneWinner exercises a race on a +// single fixed path (installBinary normally avoids this by randomizing the +// name, but the exclusive-creation guarantee must hold regardless): exactly +// one concurrent caller may create the file, and the rest must fail cleanly +// rather than silently truncate the winner's content. +func TestCreateStagingFileConcurrentRaceOnlyOneWinner(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "staged") + + const attempts = 16 + var wg sync.WaitGroup + successes := make([]bool, attempts) + for i := range attempts { + wg.Add(1) + go func(i int) { + defer wg.Done() + file, err := stageAtPath(path) + if err != nil { + return + } + defer func() { _ = file.Close() }() + successes[i] = true + }(i) + } + wg.Wait() + + winners := 0 + for _, ok := range successes { + if ok { + winners++ + } + } + if winners != 1 { + t.Fatalf("concurrent staging winners = %d, want exactly 1", winners) + } +} diff --git a/internal/update/stage_promote_other_test.go b/internal/update/stage_promote_other_test.go new file mode 100644 index 000000000..1429d78bb --- /dev/null +++ b/internal/update/stage_promote_other_test.go @@ -0,0 +1,235 @@ +//go:build !windows + +package update + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +// TestPromoteRefusesASubstitutedStagingEntry is the regression test for the live +// handoff half of #742: randomizing the staging name and creating it exclusively +// stops PRE-creation, but not substitution after the verified bytes are written. +// POSIX cannot rename by descriptor, so staging happens inside a private 0700 +// directory the attacker cannot write to, and promote additionally verifies the +// entry still names the object it wrote before renaming it into place. +func TestPromoteRefusesASubstitutedStagingEntry(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero") + 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) + } + + staged, err := stageBinary(sourcePath, targetPath) + if err != nil { + t.Fatalf("stageBinary: %v", err) + } + defer staged.discard() + + // First line of defence: the staging directory is private, so a principal who + // can write in the installation directory cannot reach the entry at all. + info, err := os.Stat(staged.dir) + if err != nil { + t.Fatalf("Stat staging directory: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o700 { + t.Fatalf("staging directory mode = %#o, want 0700", perm) + } + + // Second line of defence: rehearse the substitution anyway (the test runs as + // the directory's owner, so it can do what an attacker cannot) and require + // promote to refuse instead of installing the substitute. + substitute := filepath.Join(staged.dir, "substitute") + if err := os.WriteFile(substitute, []byte("attacker-binary"), 0o755); err != nil { + t.Fatalf("WriteFile substitute: %v", err) + } + if err := os.Rename(substitute, staged.path); err != nil { + t.Fatalf("Rename substitute over the staging entry: %v", err) + } + + if err := staged.promote(targetPath); err == nil { + t.Fatal("promote installed a substituted staging entry, want a refusal") + } + installed, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("ReadFile target: %v", err) + } + if string(installed) != "old-binary" { + t.Fatalf("target = %q, want the original binary left in place", installed) + } +} + +// TestPromoteSurvivesAncestorDirectoryReplacement is the regression test for a +// review finding on PR #751: the private staging directory's 0700 mode +// protects its CONTENTS from a principal who can write in the installation +// directory, but not its own directory ENTRY — that principal can still +// rename the staging directory itself out of the way and recreate a +// look-alike at the same path, with an attacker file at the same basename, +// in the gap between verifyStagedIdentity returning and the final rename +// running. A plain pathname rename would re-resolve through the impostor at +// that point; promote must instead stay bound to the exact directory whose +// identity was already checked, via the directory descriptor opened when the +// staging directory was created. +func TestPromoteSurvivesAncestorDirectoryReplacement(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero") + 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) + } + + staged, err := stageBinary(sourcePath, targetPath) + if err != nil { + t.Fatalf("stageBinary: %v", err) + } + discarded := false + defer func() { + if !discarded { + staged.discard() + } + }() + + // Rehearse the ancestor swap: move the real staging directory aside (the + // test runs as its owner, so it can do what a merely writable-parent + // attacker — who only needs rename rights on dir, not on the staging + // directory's own contents — can also do), then recreate a look-alike at + // the original path with an attacker file at the same basename. + base := filepath.Base(staged.path) + movedDir := staged.dir + "-moved" + if err := os.Rename(staged.dir, movedDir); err != nil { + t.Fatalf("Rename staging directory aside: %v", err) + } + if err := os.Mkdir(staged.dir, 0o700); err != nil { + t.Fatalf("Mkdir impostor staging directory: %v", err) + } + impostorFile := filepath.Join(staged.dir, base) + if err := os.WriteFile(impostorFile, []byte("attacker-binary"), 0o755); err != nil { + t.Fatalf("WriteFile impostor file: %v", err) + } + + if err := staged.promote(targetPath); err != nil { + t.Fatalf("promote: %v", err) + } + installed, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("ReadFile target: %v", err) + } + if string(installed) != "verified-binary" { + t.Fatalf("target = %q, want the verified bytes from the original staging directory", installed) + } + staged.discard() + discarded = true + // The impostor must be left untouched: promote should never have looked at + // it, let alone consumed or removed it during deferred cleanup. + if impostor, err := os.ReadFile(impostorFile); err != nil { + t.Fatalf("ReadFile impostor file: %v", err) + } else if string(impostor) != "attacker-binary" { + t.Fatalf("impostor file = %q, want it left untouched", impostor) + } +} + +func TestCreateStagedBinaryRejectsDirectoryReplacementBeforeOpen(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero") + var impostorMarker string + original := openStagingDirectory + openStagingDirectory = func(parentFD int, name string) (int, error) { + path := filepath.Join(dir, name) + if err := os.Rename(path, path+"-original"); err != nil { + t.Fatalf("Rename original staging directory: %v", err) + } + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatalf("Mkdir impostor: %v", err) + } + impostorMarker = filepath.Join(path, "keep") + if err := os.WriteFile(impostorMarker, []byte("attacker"), 0o600); err != nil { + t.Fatalf("WriteFile impostor marker: %v", err) + } + return original(parentFD, name) + } + defer func() { openStagingDirectory = original }() + + if staged, err := createStagedBinary(targetPath); err == nil { + staged.discard() + t.Fatal("createStagedBinary accepted a directory replaced before open") + } + if marker, err := os.ReadFile(impostorMarker); err != nil { + t.Fatalf("substituted directory was removed during failed creation: %v", err) + } else if string(marker) != "attacker" { + t.Fatalf("substituted directory marker = %q, want it preserved", marker) + } +} + +func TestCreateStagedBinaryCleansUpAfterDirectoryOpenFailure(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero") + original := openStagingDirectory + openStagingDirectory = func(parentFD int, name string) (int, error) { + return -1, errors.New("forced open failure") + } + defer func() { openStagingDirectory = original }() + + if staged, err := createStagedBinary(targetPath); err == nil { + staged.discard() + t.Fatal("createStagedBinary succeeded despite a forced directory-open failure") + } + assertNoStagingLeftovers(t, dir) +} + +// TestInstallBinaryInstallsVerifiedBytes is the success control: the ordinary +// path must still install the staged bytes, executable, with nothing left over. +func TestInstallBinaryInstallsVerifiedBytes(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero") + 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) + } + installed, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("ReadFile installed: %v", err) + } + if string(installed) != "verified-binary" { + t.Fatalf("installed binary = %q, want the verified bytes", installed) + } + info, err := os.Stat(targetPath) + if err != nil { + t.Fatalf("Stat installed: %v", err) + } + if info.Mode().Perm()&0o100 == 0 { + t.Fatalf("installed binary mode = %#o, want the executable bit set", info.Mode().Perm()) + } + assertNoStagingLeftovers(t, dir) +} + +// TestInstallBinaryCleansUpWhenStagingFails covers the cleanup ordering: a +// failure after the staging object exists must not leave it behind, because each +// attempt now uses a fresh random name that the next attempt never reuses. +func TestInstallBinaryCleansUpWhenStagingFails(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero") + if err := os.WriteFile(targetPath, []byte("old-binary"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + + if err := installBinary(filepath.Join(t.TempDir(), "missing-source"), targetPath); err == nil { + t.Fatal("installBinary with an unreadable source succeeded, want error") + } + assertNoStagingLeftovers(t, dir) +} diff --git a/internal/update/stage_promote_windows_test.go b/internal/update/stage_promote_windows_test.go new file mode 100644 index 000000000..a462041ac --- /dev/null +++ b/internal/update/stage_promote_windows_test.go @@ -0,0 +1,910 @@ +//go:build windows + +package update + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "golang.org/x/sys/windows" +) + +func TestMain(m *testing.M) { + stateDir, err := os.MkdirTemp("", "zero-update-recovery-test-") + if err != nil { + fmt.Fprintf(os.Stderr, "create updater test state directory: %v\n", err) + os.Exit(1) + } + recoveryCleanupStateDir = func() (string, error) { return stateDir, nil } + code := m.Run() + _ = os.RemoveAll(stateDir) + os.Exit(code) +} + +// TestPromoteInstallsTheStagedObjectNotTheStagedPath is the regression test for +// the live handoff half of #742: randomizing the staging name and creating it +// exclusively stops PRE-creation, but not substitution after the verified bytes +// are written. Windows renames through the staging HANDLE, so a substituted entry +// at the staging pathname is simply not what gets promoted. +func TestPromoteInstallsTheStagedObjectNotTheStagedPath(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) + } + + staged, err := stageBinary(sourcePath, targetPath) + if err != nil { + t.Fatalf("stageBinary: %v", err) + } + discarded := false + defer func() { + if !discarded { + staged.discard() + } + }() + + // Rehearse the strongest form of the substitution: the staging entry is + // replaced wholesale between the write and the swap. (A real attacker also has + // to get past the exclusive share mode this handle holds; the test does not, + // which only makes the check stricter.) + substituted := false + if err := os.Remove(staged.path); err == nil { + if err := os.WriteFile(staged.path, []byte("attacker-binary"), 0o755); err != nil { + t.Fatalf("WriteFile substituted staging entry: %v", err) + } + substituted = true + } + + promoteErr := staged.promote(targetPath) + // The staging handle keeps the promoted (or, on failure, discarded) file open + // with an exclusive share mode, so release it before reading installed bytes + // (installBinary's deferred discard does the same). + staged.discard() + discarded = true + + if promoteErr != nil { + // Fully unlinking the staging file (its directory entry removed, then a + // new file recreated at that name) is not something every Windows build + // honors a same-handle rename back into: verifyPromotedTarget's post- + // rename identity check can find nothing at targetPath and fail the + // promotion, which restoreOriginalBinary then recovers from by moving + // the pre-update binary back. That is this test's real security + // property holding — the attacker's substituted bytes are never + // installed — just via the fail-closed path instead of the handle- + // rename defeating the substitution outright. Only accept the failure + // when it actually happened via a real substitution; any other promote + // error is a genuine regression. + if !substituted { + t.Fatalf("promote failed without substitution: %v", promoteErr) + } + if installed, readErr := os.ReadFile(targetPath); readErr == nil && string(installed) == "attacker-binary" { + t.Fatalf("attacker-controlled bytes were installed: %q", installed) + } + t.Logf("promote refused after full-unlink substitution (%v); attacker bytes were not installed", promoteErr) + return + } + + installed, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("ReadFile installed: %v", err) + } + if string(installed) != "verified-binary" { + t.Fatalf("installed binary = %q, want the verified bytes", installed) + } + if !substituted { + t.Log("the staging entry could not be replaced (exclusive share mode); the handle-bound rename was still exercised") + } +} + +// TestPromoteRejectsALyingRenameByHandle is the regression test for a review +// finding on PR #751: SetFileInformationByHandle reporting success is not, on +// its own, proof the object actually ended up at targetPath. Some Windows +// versions have been observed accepting the rename call against a handle +// whose directory entry was substituted out from under it without the object +// actually moving — this simulates that by stubbing the rename to lie, since +// the real trigger condition is Windows-version-specific and not reliably +// reproducible on demand. Without verifyPromotedTarget, promote would return +// nil while targetPath silently ends up missing, reporting a successful +// update that actually stranded the user without an executable at all. +func TestPromoteRejectsALyingRenameByHandle(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) + } + + staged, err := stageBinary(sourcePath, targetPath) + if err != nil { + t.Fatalf("stageBinary: %v", err) + } + defer staged.discard() + + original := renameFileByHandle + renameFileByHandle = func(file *os.File, target string) error { + return nil // lie: report success without touching anything + } + defer func() { renameFileByHandle = original }() + + promoteErr := staged.promote(targetPath) + if promoteErr == nil { + t.Fatal("promote reported success for a rename that never actually happened, want an error") + } + if !strings.Contains(promoteErr.Error(), "unreachable") { + t.Fatalf("error = %q, want it to explain the target is unreachable", promoteErr.Error()) + } + installed, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("ReadFile target: %v", err) + } + if string(installed) != "old-binary" { + t.Fatalf("target = %q, want the original binary restored", installed) + } + if _, err := os.Stat(targetPath + ".old"); !os.IsNotExist(err) { + t.Fatalf(".old leftover survived a successful restore: %v", err) + } +} + +// A promotion failure followed by a blocked restore is security-relevant all +// the way through installBinary; its contextual wrappers must not erase the +// sentinel that callers of Apply use to distinguish possible path tampering. +func TestInstallBinaryPreservesPossibleTamperingError(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) + } + + 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) + } + }) + originalMark := markOldBinaryPreserved + markOldBinaryPreserved = func(string) error { return errors.New("injected marker failure") } + t.Cleanup(func() { markOldBinaryPreserved = originalMark }) + const suffix = "deadbeefdeadbeefdeadbeefdeadbeef" + stubRandomStagingSuffix(t, suffix) + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want it to wrap ErrTargetPossiblyTampered", err) + } + if strings.Contains(err.Error(), "original preserved at "+targetPath+".old") { + t.Fatalf("installBinary error falsely claims the relocated copy remains at .old: %v", err) + } + expectedRecovery := targetPath + ".zero-update-" + suffix + ".old." + suffix + ".recovery" + if !strings.Contains(err.Error(), expectedRecovery) { + t.Fatalf("installBinary error = %v, want the authoritative relocated recovery path", err) + } +} + +// TestPromoteRefusesWhileRecoveryCopyIsMarked covers jatmn's #751 P1: skipping +// the pre-rename cleanup was not enough to protect a marked recovery copy. +// 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 if this promotion then failed, +// restoreOriginalBinary could only move those unverified bytes back. +func TestPromoteRefusesWhileRecoveryCopyIsMarked(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) + } + if err := markOldBinaryPreserved(oldPath); err != nil { + t.Fatalf("markOldBinaryPreserved: %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) + } + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want a refusal wrapping ErrTargetPossiblyTampered", err) + } + // The whole point: the known-good copy is still there afterwards. + got, readErr := os.ReadFile(oldPath) + if readErr != nil { + t.Fatalf("recovery copy was destroyed by the retry: %v", readErr) + } + if string(got) != "known-good" { + t.Fatalf("recovery copy = %q, want the last verified binary", got) + } + // And the error names both moves that end the state. + for _, want := range []string{oldPath, targetPath, oldPath + oldBinaryPreservedSuffix} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want it to name %q", err, want) + } + } + + // Clearing the marker is the operator accepting the installed binary; the + // next promotion proceeds normally without destroying the recovery copy. + clearOldBinaryPreserved(oldPath) + stubRandomStagingSuffix(t, "deadbeef") + if err := installBinary(sourcePath, targetPath); err != nil { + t.Fatalf("installBinary after the operator cleared the marker: %v", err) + } + if data, err := os.ReadFile(targetPath); err != nil || string(data) != "verified-binary" { + t.Fatalf("target = %q err=%v, want the verified bytes installed", data, err) + } + if data, err := os.ReadFile(oldPath); err != nil || string(data) != "known-good" { + t.Fatalf("recovery copy = %q err=%v, want known-good after retry", data, err) + } +} + +func TestPromoteRefusesRetryAfterInterruptedAside(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 recovery copy: %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) + } + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want ErrTargetPossiblyTampered", err) + } + if _, err := os.Lstat(targetPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing target was unexpectedly created: %v", err) + } + if data, err := os.ReadFile(oldPath); err != nil || string(data) != "known-good" { + t.Fatalf("recovery copy = %q err=%v, want known-good", data, err) + } +} + +func TestPromoteRefusesMarkedRandomAsideRecovery(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + canonicalOld := targetPath + ".old" + randomOld := targetPath + ".deadbeef.old" + if err := os.WriteFile(targetPath, []byte("unverified"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + if err := os.WriteFile(canonicalOld, []byte("stale-older-binary"), 0o755); err != nil { + t.Fatalf("WriteFile canonical recovery: %v", err) + } + if err := os.WriteFile(randomOld, []byte("last-known-good"), 0o755); err != nil { + t.Fatalf("WriteFile random recovery: %v", err) + } + if err := markOldBinaryPreserved(randomOld); err != nil { + t.Fatalf("mark random recovery: %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) + } + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want ErrTargetPossiblyTampered", err) + } + for _, want := range []string{randomOld, randomOld + oldBinaryPreservedSuffix} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want marked random recovery path %s", err, want) + } + } + if got, err := os.ReadFile(randomOld); err != nil || string(got) != "last-known-good" { + t.Fatalf("random recovery = %q err=%v, want last-known-good", got, err) + } + if got, err := os.ReadFile(targetPath); err != nil || string(got) != "unverified" { + t.Fatalf("target = %q err=%v, want refusal before promotion", got, err) + } +} + +func TestPromoteRefusesAmbiguousRecoveryWhenTargetIsMissing(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + canonicalOld := targetPath + ".old" + randomOld := targetPath + ".deadbeef.old" + if err := os.WriteFile(canonicalOld, []byte("older-binary"), 0o755); err != nil { + t.Fatalf("WriteFile canonical recovery: %v", err) + } + if err := os.WriteFile(randomOld, []byte("last-running-binary"), 0o755); err != nil { + t.Fatalf("WriteFile random recovery: %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) + } + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want ErrTargetPossiblyTampered", err) + } + for _, want := range []string{"ambiguous", canonicalOld, randomOld} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want %q", err, want) + } + } + if _, err := os.Lstat(targetPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing target was unexpectedly created: %v", err) + } +} + +func TestVerifyPromotedTargetRejectsDifferentRegularFile(t *testing.T) { + dir := t.TempDir() + stagedPath := filepath.Join(dir, "staged.exe") + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(stagedPath, []byte("verified"), 0o755); err != nil { + t.Fatalf("WriteFile staged: %v", err) + } + if err := os.WriteFile(targetPath, []byte("attacker"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + staged, err := os.Open(stagedPath) + if err != nil { + t.Fatalf("Open staged: %v", err) + } + defer func() { _ = staged.Close() }() + + if err := verifyPromotedTarget(staged, targetPath); err == nil { + t.Fatal("verifyPromotedTarget accepted a different regular file at targetPath") + } +} + +func TestVerifyPromotedTargetRejectsAHardLinkedName(t *testing.T) { + dir := t.TempDir() + stagedPath := filepath.Join(dir, "staged.exe") + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(stagedPath, []byte("verified"), 0o755); err != nil { + t.Fatalf("WriteFile staged: %v", err) + } + if err := os.Link(stagedPath, targetPath); err != nil { + t.Fatalf("Link target: %v", err) + } + staged, err := os.Open(stagedPath) + if err != nil { + t.Fatalf("Open staged: %v", err) + } + defer func() { _ = staged.Close() }() + + if err := verifyPromotedTarget(staged, targetPath); err == nil { + t.Fatal("verifyPromotedTarget accepted a second hard-linked name without a completed rename") + } +} + +func TestInstallBinaryThroughReparsePointAncestor(t *testing.T) { + root := t.TempDir() + realDir := filepath.Join(root, "real") + if err := os.Mkdir(realDir, 0o755); err != nil { + t.Fatalf("Mkdir real install directory: %v", err) + } + linkedDir := filepath.Join(root, "linked") + if err := os.Symlink(realDir, linkedDir); err != nil { + t.Skipf("directory symlink unavailable: %v", err) + } + targetPath := filepath.Join(linkedDir, "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 through reparse-point ancestor: %v", err) + } + installed, err := os.ReadFile(filepath.Join(realDir, "zero.exe")) + if err != nil { + t.Fatalf("ReadFile installed: %v", err) + } + if string(installed) != "verified-binary" { + t.Fatalf("installed binary = %q, want the verified bytes", installed) + } +} + +// TestInstallBinaryInstallsVerifiedBytes is the success control for the ordinary +// path: the staged bytes land at the target, the running binary is preserved at +// an updater-owned recovery path, and no staging artifact survives. +func TestInstallBinaryInstallsVerifiedBytes(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) + } + installed, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("ReadFile installed: %v", err) + } + if string(installed) != "verified-binary" { + t.Fatalf("installed binary = %q, want the verified bytes", installed) + } + recoveries, err := existingRecoveryPaths(targetPath) + if err != nil { + t.Fatalf("existingRecoveryPaths: %v", err) + } + if len(recoveries) != 1 || !strings.Contains(filepath.Base(recoveries[0]), ".zero-update-") { + t.Fatalf("recovery paths = %v, want one namespaced updater recovery", recoveries) + } + if old, err := os.ReadFile(recoveries[0]); err != nil { + t.Fatalf("the replaced binary must be preserved as the recovery copy: %v", err) + } else if string(old) != "old-binary" { + t.Fatalf("preserved binary = %q, want the previous one", old) + } + assertNoStagingLeftovers(t, dir) +} + +func TestInstallBinaryPreservesArbitraryOldFilesDuringCleanup(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("version-0"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + manualBackup := targetPath + ".before-manual-patch.old" + if err := os.WriteFile(manualBackup, []byte("manual-backup"), 0o755); err != nil { + t.Fatalf("WriteFile manual backup: %v", err) + } + // Also cover a name that resembles the updater namespace but does not carry + // the exact 128-bit hexadecimal suffix generated by randomStagingSuffix. + lookalike := targetPath + ".zero-update-not-owned.old" + if err := os.WriteFile(lookalike, []byte("lookalike-backup"), 0o755); err != nil { + t.Fatalf("WriteFile lookalike backup: %v", err) + } + plantedNamespaced := targetPath + ".zero-update-00000000000000000000000000000000.old" + if err := os.WriteFile(plantedNamespaced, []byte("planted-namespaced-backup"), 0o755); err != nil { + t.Fatalf("WriteFile planted namespaced backup: %v", err) + } + + for version := 1; version <= 3; version++ { + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte(fmt.Sprintf("version-%d", version)), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + if err := installBinary(sourcePath, targetPath); err != nil { + t.Fatalf("installBinary version %d: %v", version, err) + } + } + + for path, want := range map[string]string{ + manualBackup: "manual-backup", + lookalike: "lookalike-backup", + plantedNamespaced: "planted-namespaced-backup", + } { + if got, err := os.ReadFile(path); err != nil || string(got) != want { + t.Fatalf("backup %s = %q err=%v, want %q", path, got, err, want) + } + } +} + +// TestRecoveryCleanupRefusesSubstitutedAside pins the provenance rule that +// makes handle-bound cleanup safe: a recorded recovery path whose object has +// been swapped out underneath the record is never opened as a cleanup +// candidate, so the substitute is not deleted on the next promotion. +func TestRecoveryCleanupRefusesSubstitutedAside(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + recoveryPath := targetPath + ".zero-update-0123456789abcdef0123456789abcdef.old" + if err := os.WriteFile(recoveryPath, []byte("moved-aside-binary"), 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 := appendRecoveryCleanupRecord(targetPath, recoveryPath, identity); err != nil { + t.Fatalf("appendRecoveryCleanupRecord: %v", err) + } + if err := os.Rename(recoveryPath, recoveryPath+".displaced"); err != nil { + t.Fatalf("Rename recovery: %v", err) + } + if err := os.WriteFile(recoveryPath, []byte("substituted-file"), 0o755); err != nil { + t.Fatalf("WriteFile substitute: %v", err) + } + + candidates := prepareRecoveryCleanup(targetPath) + if len(candidates) != 0 { + t.Fatalf("cleanup candidates = %d, want the substituted aside to be refused", len(candidates)) + } + cleanupSupersededRecoveryCopies(targetPath, candidates) + if got, err := os.ReadFile(recoveryPath); err != nil || string(got) != "substituted-file" { + t.Fatalf("substituted file = %q err=%v, want it left untouched", got, err) + } +} + +// TestAppendRecoveryCleanupRecordRejectsForeignPath keeps the trusted record +// from ever vouching for a name this updater could not have created, which is +// what stops cleanup from deleting an operator's own backup. +func TestAppendRecoveryCleanupRecordRejectsForeignPath(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + foreign := targetPath + ".before-manual-patch.old" + if err := os.WriteFile(foreign, []byte("operator-backup"), 0o755); err != nil { + t.Fatalf("WriteFile backup: %v", err) + } + if err := appendRecoveryCleanupRecord(targetPath, foreign, recoveryIdentity{}); err == nil { + t.Fatal("appendRecoveryCleanupRecord accepted a path this updater never created") + } + recordPath, err := recoveryCleanupRecordPath(targetPath) + if err != nil { + t.Fatalf("recoveryCleanupRecordPath: %v", err) + } + if _, err := os.Lstat(recordPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("trusted cleanup record was written for a foreign backup: %v", err) + } +} + +// TestRecoveryCleanupRetiresRecordsForVanishedCopies keeps the backlog that +// makes transient-lock retries possible from becoming its own unbounded growth: +// a record whose object an operator already removed can never become +// actionable, so it must not be carried forever. +func TestRecoveryCleanupRetiresRecordsForVanishedCopies(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("version-0"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + source := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(source, []byte("version-1"), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + if err := installBinary(source, targetPath); err != nil { + t.Fatalf("installBinary: %v", err) + } + recoveries, err := existingRecoveryPaths(targetPath) + if err != nil || len(recoveries) != 1 { + t.Fatalf("recovery paths = %v err=%v, want exactly one", recoveries, err) + } + if got := recordedRecoveryCleanupCount(t, targetPath); got != 1 { + t.Fatalf("recorded cleanup entries = %d, want 1", got) + } + // The operator removes the recovery copy themselves. + if err := os.Remove(recoveries[0]); err != nil { + t.Fatalf("Remove recovery copy: %v", err) + } + + closeRecoveryCleanupCandidates(prepareRecoveryCleanup(targetPath)) + + if got := recordedRecoveryCleanupCount(t, targetPath); got != 0 { + t.Fatalf("recorded cleanup entries = %d after the copy vanished, want the record retired", got) + } +} + +func recordedRecoveryCleanupCount(t *testing.T, targetPath string) int { + t.Helper() + return len(loadRecoveryCleanupQueue(targetPath).Records) +} + +func TestInstallBinaryBoundsRecoveryCopiesAcrossRepeatedUpgrades(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("version-0"), 0o755); err != nil { + t.Fatalf("WriteFile target: %v", err) + } + + for version := 1; version <= 4; version++ { + contents := fmt.Sprintf("version-%d", version) + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte(contents), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + if err := installBinary(sourcePath, targetPath); err != nil { + t.Fatalf("installBinary version %d: %v", version, err) + } + recoveries, err := existingRecoveryPaths(targetPath) + if err != nil { + t.Fatalf("existingRecoveryPaths: %v", err) + } + if len(recoveries) != 1 { + t.Fatalf("recovery count after version %d = %d (%v), want 1", version, len(recoveries), recoveries) + } + previous, err := os.ReadFile(recoveries[0]) + if err != nil { + t.Fatalf("ReadFile recovery after version %d: %v", version, err) + } + wantPrevious := fmt.Sprintf("version-%d", version-1) + if string(previous) != wantPrevious { + t.Fatalf("recovery after version %d = %q, want %q", version, previous, wantPrevious) + } + } +} + +func TestInstallBinaryRetainsLockedCleanupRecordUntilLaterRetry(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("version-0"), 0o755); err != nil { + t.Fatal(err) + } + install := func(version string) { + source := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(source, []byte(version), 0o755); err != nil { + t.Fatal(err) + } + if err := installBinary(source, targetPath); err != nil { + t.Fatalf("install %s: %v", version, err) + } + } + install("version-1") + recoveries, _ := existingRecoveryPaths(targetPath) + lockedPath, err := windows.UTF16PtrFromString(recoveries[0]) + if err != nil { + t.Fatal(err) + } + locked, err := windows.CreateFile(lockedPath, windows.GENERIC_READ, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + t.Fatal(err) + } + install("version-2") + _ = windows.CloseHandle(locked) + install("version-3") + recoveries, _ = existingRecoveryPaths(targetPath) + if len(recoveries) != 1 { + t.Fatalf("recovery backlog after lock clears = %v, want only newest recovery", recoveries) + } +} + +func TestPromoteRestoresOriginalObjectWhenAsidePathIsSubstituted(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + if err := os.WriteFile(targetPath, []byte("known-good"), 0o755); err != nil { + t.Fatal(err) + } + staged, err := createStagedBinary(targetPath) + if err != nil { + t.Fatal(err) + } + defer staged.discard() + if _, err := staged.file.WriteString("new"); err != nil { + t.Fatal(err) + } + originalRename := renameFileByHandle + originalRecoveryRename := renameRecoveryFileByHandle + var substitutionErr error + call := 0 + renameRecoveryFileByHandle = func(file *os.File, path string) error { + call++ + if err := originalRecoveryRename(file, path); err != nil { + return err + } + if call == 1 { + substitutionErr = os.Rename(path, path+".stolen") + } + return nil + } + renameFileByHandle = func(_ *os.File, _ string) error { + return errors.New("injected promotion failure") + } + defer func() { + renameFileByHandle = originalRename + renameRecoveryFileByHandle = originalRecoveryRename + }() + if err := staged.promote(targetPath); err == nil { + t.Fatal("promote succeeded despite injected failure") + } + contents, err := os.ReadFile(targetPath) + if err != nil || string(contents) != "known-good" { + t.Fatalf("restored target = %q, %v; want original object", contents, err) + } + if substitutionErr == nil { + t.Fatal("aside pathname substitution succeeded while recovery handle was retained") + } +} + +func TestInstallBinaryRefusesRelocatedRecoveryCopy(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) + } + recoveryPath := targetPath + ".old.deadbeef.recovery" + if err := os.WriteFile(recoveryPath, []byte("last-verified"), 0o755); err != nil { + t.Fatalf("WriteFile recovery: %v", err) + } + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte("verified-release"), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want ErrTargetPossiblyTampered", err) + } + if !strings.Contains(err.Error(), recoveryPath) { + t.Fatalf("installBinary error = %v, want recovery path %s", err, recoveryPath) + } + if got, readErr := os.ReadFile(targetPath); readErr != nil || string(got) != "unverified" { + t.Fatalf("target = %q err=%v, want unchanged unverified bytes", got, readErr) + } + if got, readErr := os.ReadFile(recoveryPath); readErr != nil || string(got) != "last-verified" { + t.Fatalf("recovery = %q err=%v, want last-verified", got, readErr) + } +} + +func TestInstallBinaryRefusesRecoveryRelocatedFromRandomizedAside(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) + } + recoveryPath := targetPath + ".aside.old.relocation.recovery" + if err := os.WriteFile(recoveryPath, []byte("last-verified"), 0o755); err != nil { + t.Fatalf("WriteFile recovery: %v", err) + } + sourcePath := filepath.Join(t.TempDir(), "new-binary") + if err := os.WriteFile(sourcePath, []byte("verified-release"), 0o755); err != nil { + t.Fatalf("WriteFile source: %v", err) + } + + err := installBinary(sourcePath, targetPath) + if !errors.Is(err, ErrTargetPossiblyTampered) { + t.Fatalf("installBinary error = %v, want ErrTargetPossiblyTampered", err) + } + if got, readErr := os.ReadFile(recoveryPath); readErr != nil || string(got) != "last-verified" { + t.Fatalf("recovery = %q err=%v, want last-verified", got, readErr) + } +} + +func TestPromotionLockSerializesSameTarget(t *testing.T) { + targetPath := filepath.Join(t.TempDir(), "zero.exe") + releaseFirst, err := acquirePromotionLock(targetPath) + if err != nil { + t.Fatalf("acquire first promotion lock: %v", err) + } + + started := make(chan struct{}) + acquired := make(chan struct{}) + releaseSecond := make(chan struct{}) + secondResult := make(chan error, 1) + go func() { + close(started) + release, err := acquirePromotionLock(strings.ToUpper(targetPath)) + if err != nil { + secondResult <- err + return + } + close(acquired) + <-releaseSecond + release() + secondResult <- nil + }() + <-started + select { + case <-acquired: + releaseFirst() + close(releaseSecond) + <-secondResult + t.Fatal("second promotion acquired the same target lock before release") + case err := <-secondResult: + releaseFirst() + t.Fatalf("acquire second promotion lock: %v", err) + case <-time.After(100 * time.Millisecond): + } + + releaseFirst() + select { + case <-acquired: + close(releaseSecond) + if err := <-secondResult; err != nil { + t.Fatalf("release second promotion lock: %v", err) + } + case err := <-secondResult: + t.Fatalf("acquire second promotion lock: %v", err) + case <-time.After(5 * time.Second): + t.Fatal("second promotion did not acquire the target lock after release") + } +} + +// TestInstallBinaryCleansUpWhenStagingFails covers the cleanup ordering: a +// failure after the staging file exists must not leave it behind, because each +// attempt now uses a fresh random name that the next attempt never reuses. +func TestInstallBinaryCleansUpWhenStagingFails(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) + } + + if err := installBinary(filepath.Join(t.TempDir(), "missing-source"), targetPath); err == nil { + t.Fatal("installBinary with an unreadable source succeeded, want error") + } + assertNoStagingLeftovers(t, dir) +} + +// TestDiscardDeletesTheStagedObjectThroughItsHandle pins that failure-path +// cleanup removes the object this updater staged rather than re-resolving the +// staging name after the handle is released. +func TestDiscardDeletesTheStagedObjectThroughItsHandle(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + staged, err := createStagedBinary(targetPath) + if err != nil { + t.Fatalf("createStagedBinary: %v", err) + } + stagingPath := staged.path + + staged.discard() + + if _, err := os.Lstat(stagingPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("staged object survived discard: %v", err) + } + assertNoStagingLeftovers(t, dir) +} + +// TestDiscardLeavesASubstitutedStagingEntryAlone is the impostor-survival +// counterpart of the POSIX promote tests: cleanup is bound to the staged +// object, so an entry a principal who can write the installation directory +// plants at the staging name once the handle is gone is never deleted. The +// substitution is staged by hand here because the exclusive, no-share handle +// makes it impossible while the updater still holds the object. +func TestDiscardLeavesASubstitutedStagingEntryAlone(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "zero.exe") + staged, err := createStagedBinary(targetPath) + if err != nil { + t.Fatalf("createStagedBinary: %v", err) + } + stagingPath := staged.path + // Run the handle-bound half of discard, then let the substitution win the + // race that a pathname-based removal would lose. + staged.discardOpenObject() + if err := staged.file.Close(); err != nil { + t.Fatalf("Close staged handle: %v", err) + } + const impostorContent = "attacker-owned file that cleanup must not delete" + if err := os.WriteFile(stagingPath, []byte(impostorContent), 0o600); err != nil { + t.Fatalf("WriteFile impostor: %v", err) + } + + staged.discardPaths() + + got, err := os.ReadFile(stagingPath) + if err != nil { + t.Fatalf("cleanup removed the substituted staging entry: %v", err) + } + if string(got) != impostorContent { + t.Fatalf("substituted entry = %q, want it left untouched", got) + } +} diff --git a/internal/update/stage_test_helpers_test.go b/internal/update/stage_test_helpers_test.go new file mode 100644 index 000000000..4ca5e870e --- /dev/null +++ b/internal/update/stage_test_helpers_test.go @@ -0,0 +1,17 @@ +//go:build windows + +package update + +import "testing" + +// stubRandomStagingSuffix overrides randomStagingSuffix to always return a +// fixed value for the duration of t, restoring the original on cleanup. +// stagingFilePath's random suffix is unpredictable by design in production, so +// the Windows staging tests that need the exact path pin it here instead. POSIX +// stages inside a private directory and has no such name to pin. +func stubRandomStagingSuffix(t *testing.T, suffix string) { + t.Helper() + original := randomStagingSuffix + randomStagingSuffix = func() (string, error) { return suffix, nil } + t.Cleanup(func() { randomStagingSuffix = original }) +} diff --git a/internal/update/stage_test_seam_test.go b/internal/update/stage_test_seam_test.go new file mode 100644 index 000000000..2d579f246 --- /dev/null +++ b/internal/update/stage_test_seam_test.go @@ -0,0 +1,44 @@ +package update + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// stubStageBinaryFailure makes stageBinary fail for targetPath (matched by base +// name, since installBinary is called with the real install path) and behave +// normally for everything else, restoring the original on cleanup. +// +// The staging location is created exclusively under an unpredictable name — a +// private directory on POSIX — so a test can no more occupy it than an attacker +// can. This seam is how a staging failure is exercised instead. +func stubStageBinaryFailure(t *testing.T, targetPath string, failure error) { + t.Helper() + original := stageBinary + want := filepath.Base(targetPath) + stageBinary = func(sourcePath string, target string) (*stagedBinary, error) { + if filepath.Base(target) == want { + return nil, failure + } + return original(sourcePath, target) + } + t.Cleanup(func() { stageBinary = original }) +} + +// assertNoStagingLeftovers fails when dir still holds a platform staging +// artifact. Checking both patterns is harmless and keeps this test helper +// consistent across POSIX and Windows. +func assertNoStagingLeftovers(t *testing.T, dir string) { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir %s: %v", dir, err) + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), ".zero-stage-") || strings.HasSuffix(entry.Name(), ".new") { + t.Fatalf("staging leftover survived in the install directory: %s", entry.Name()) + } + } +} diff --git a/internal/update/stage_windows.go b/internal/update/stage_windows.go new file mode 100644 index 000000000..738eb9c84 --- /dev/null +++ b/internal/update/stage_windows.go @@ -0,0 +1,498 @@ +//go:build windows + +package update + +import ( + "crypto/sha256" + "encoding/binary" + "errors" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + "unsafe" + + "golang.org/x/sys/windows" +) + +// createStagedBinary stages beside targetPath under an unpredictable name. Unlike +// POSIX this needs no private directory: Windows can rename an object through the +// very handle it was created with (see promote), so the staging pathname never has +// to be re-resolved and cannot be substituted. +func createStagedBinary(targetPath string) (*stagedBinary, error) { + path, err := stagingFilePath(targetPath) + if err != nil { + return nil, err + } + file, err := createStagingFile(path) + if err != nil { + return nil, err + } + return &stagedBinary{file: file, path: path}, nil +} + +// createStagingFile creates path exclusively and without following any +// reparse point that may already occupy it. CREATE_NEW alone can still +// resolve through an existing reparse point (symlink/junction) when deciding +// whether the target exists — if that reparse point's target is a real file +// writable by this (possibly elevated) process, CreateFile would open and +// truncate it instead. FILE_FLAG_OPEN_REPARSE_POINT makes CreateFile operate +// on the reparse point itself, so CREATE_NEW fails on it exactly like it +// would fail on a pre-existing regular file or hard link. +// +// DELETE access is requested alongside GENERIC_WRITE because promote renames +// this object through the handle, which requires it. +func createStagingFile(path string) (*os.File, error) { + pathPtr, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, err + } + handle, err := windows.CreateFile( + pathPtr, + windows.GENERIC_WRITE|windows.DELETE, + 0, + nil, + windows.CREATE_NEW, + windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return nil, fmt.Errorf("create %s: %w", path, err) + } + file := os.NewFile(uintptr(handle), path) + if err := verifyFreshRegularFile(handle, path); err != nil { + // Delete through the handle rather than by pathname: the object this + // process just created is the only thing it may remove, and its name is + // writable by the threat principal the moment the handle is released. + _ = deleteFileByHandle(file) + _ = file.Close() + return nil, err + } + return file, nil +} + +// verifyFreshRegularFile defends in depth against the handle unexpectedly +// referring to a reparse point, directory, or an object with other hard +// links: CREATE_NEW + FILE_FLAG_OPEN_REPARSE_POINT should already guarantee +// a brand-new regular file, but this catches any surprise before any of the +// verified release bytes are written through the handle. +func verifyFreshRegularFile(handle windows.Handle, path string) error { + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(handle, &info); err != nil { + return fmt.Errorf("stat new staging file %s: %w", path, err) + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return fmt.Errorf("staging file %s is unexpectedly a reparse point", path) + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 { + return fmt.Errorf("staging file %s is unexpectedly a directory", path) + } + if info.NumberOfLinks > 1 { + return fmt.Errorf("staging file %s unexpectedly has %d hard links", path, info.NumberOfLinks) + } + return nil +} + +// promote makes the staged object the installed binary. Windows will not let a +// running executable be overwritten or deleted directly, but NTFS does allow +// renaming it aside — the same trick already used for locked config files in +// internal/cli/mcp_config.go's replaceMCPWritableConfigFile. +// +// The second rename goes through the staging HANDLE +// (SetFileInformationByHandle/FileRenameInfo) instead of the staging pathname. +// A pathname rename would re-resolve the staging entry, so a lower-privileged +// principal that can write in the installation directory could replace that entry +// after the verified bytes were written and have the updater install its file +// instead. Renaming the object the handle already refers to removes that handoff: +// there is no second lookup to win. +func (staged *stagedBinary) promote(targetPath string) error { + releasePromotionLock, err := acquirePromotionLock(targetPath) + if err != nil { + return fmt.Errorf("lock binary promotion: %w", err) + } + defer releasePromotionLock() + + if err := preflightRecoveryStateLocked(targetPath); err != nil { + return err + } + // Always use a namespaced unpredictable aside path. Besides avoiding any + // existing recovery copy, this gives trusted cleanup state a narrow path + // format to validate instead of accepting arbitrary *.old files. + suffix, suffixErr := randomStagingSuffix() + if suffixErr != nil { + return fmt.Errorf("choose recovery path: %w", suffixErr) + } + asidePath := targetPath + ".zero-update-" + suffix + ".old" + cleanupCandidates := prepareRecoveryCleanup(targetPath) + cleanedCandidates := false + defer func() { + if !cleanedCandidates { + closeRecoveryCleanupCandidates(cleanupCandidates) + } + }() + original, openErr := openRecoveryCopy(targetPath) + if openErr != nil { + return fmt.Errorf("open running binary for recovery: %w", openErr) + } + defer func() { _ = original.Close() }() + originalIdentity, identityErr := recoveryFileIdentity(original) + if identityErr != nil { + return fmt.Errorf("capture running binary identity: %w", identityErr) + } + if err := renameRecoveryFileByHandle(original, asidePath); err != nil { + return fmt.Errorf("rename running binary aside: %w", err) + } + if err := verifyPromotedTarget(original, asidePath); err != nil { + return fmt.Errorf("verify running binary aside: %w", err) + } + renameErr := renameFileByHandle(staged.file, targetPath) + if renameErr == nil { + if verifyErr := verifyPromotedTarget(staged.file, targetPath); verifyErr != nil { + renameErr = fmt.Errorf("promoted object unreachable at %s: %w", targetPath, verifyErr) + } + } + if renameErr != nil { + if restoreErr := restoreOriginalBinary(original, asidePath, targetPath); restoreErr != nil { + 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) + } + if err := appendRecoveryCleanupRecord(targetPath, asidePath, originalIdentity); err == nil { + cleanupSupersededRecoveryCopies(targetPath, cleanupCandidates) + cleanedCandidates = true + } + staged.path = targetPath + staged.promoted = true + return nil +} + +func preflightRecoveryState(targetPath string) error { + release, err := acquirePromotionLock(targetPath) + if err != nil { + return fmt.Errorf("lock binary recovery preflight: %w", err) + } + defer release() + return preflightRecoveryStateLocked(targetPath) +} + +func preflightRecoveryStateLocked(targetPath string) error { + relocatedRecoveries, recoveryErr := relocatedRecoveryPaths(targetPath) + if recoveryErr != nil { + return fmt.Errorf("%w: inspect relocated recovery state for %s: %v", ErrTargetPossiblyTampered, targetPath, recoveryErr) + } + if len(relocatedRecoveries) != 0 { + return fmt.Errorf( + "%w: a previous update moved the last binary this updater verified to %s after recovery-marker creation failed; restore the correct recovery binary or remove it after verifying %s before updating again", + ErrTargetPossiblyTampered, strings.Join(relocatedRecoveries, ", "), targetPath, + ) + } + // Refuse to promote while a previous failure left its recovery copy in place. + // + // Skipping the cleanup below is not enough to protect it: os.Rename uses + // MOVEFILE_REPLACE_EXISTING on Windows, so renaming the running binary aside + // would overwrite the last verified copy with whatever now occupies + // targetPath — the very bytes the earlier failure could not verify. If this + // promotion then failed, restoreOriginalBinary could only move those + // unverified bytes back, and the known-good binary would be gone. + // + // Automation cannot resolve this safely on the operator's behalf: only they + // can say whether the file at targetPath is theirs. So this fails closed and + // says exactly which two moves end the state. + markedRecoveries, recoveryErr := markedRecoveryPaths(targetPath) + if recoveryErr != nil { + return fmt.Errorf("%w: inspect previous recovery state for %s: %v", ErrTargetPossiblyTampered, targetPath, recoveryErr) + } + if len(markedRecoveries) != 0 { + recoveryPaths := make([]string, 0, len(markedRecoveries)) + markerPaths := make([]string, 0, len(markedRecoveries)) + for _, recoveryPath := range markedRecoveries { + recoveryPaths = append(recoveryPaths, recoveryPath) + markerPaths = append(markerPaths, recoveryPath+oldBinaryPreservedSuffix) + } + 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", + ErrTargetPossiblyTampered, strings.Join(recoveryPaths, ", "), targetPath, + targetPath, strings.Join(markerPaths, ", "), + ) + } + if _, targetErr := os.Lstat(targetPath); errors.Is(targetErr, os.ErrNotExist) { + recoveryPaths, err := existingRecoveryPaths(targetPath) + if err != nil { + return fmt.Errorf("%w: inspect recovery copies for missing %s: %v", ErrTargetPossiblyTampered, targetPath, err) + } + switch len(recoveryPaths) { + case 1: + return fmt.Errorf( + "%w: %s is missing and %s may be the only recoverable binary; move it back before updating again", + ErrTargetPossiblyTampered, targetPath, recoveryPaths[0], + ) + case 0: + // Let the ordinary aside rename below report the missing target. + default: + return fmt.Errorf( + "%w: %s is missing and multiple recovery binaries exist (%s); resolve the ambiguous recovery state before updating again", + ErrTargetPossiblyTampered, targetPath, strings.Join(recoveryPaths, ", "), + ) + } + } + return nil +} + +// acquirePromotionLock serializes the full target-specific recovery transaction +// across updater processes. Without it, one updater could capture another's +// unmarked aside while that process is still trying to restore or mark it. +// Windows mutex ownership is thread-affine, so the caller remains pinned until +// the returned release function runs. +func acquirePromotionLock(targetPath string) (func(), error) { + absolutePath, err := filepath.Abs(targetPath) + if err != nil { + return nil, err + } + digest := sha256.Sum256([]byte(strings.ToLower(filepath.Clean(absolutePath)))) + name, err := windows.UTF16PtrFromString(fmt.Sprintf("Local\\zero-update-%x", digest)) + if err != nil { + return nil, err + } + + runtime.LockOSThread() + handle, createErr := windows.CreateMutex(nil, false, name) + if createErr != nil && !errors.Is(createErr, windows.ERROR_ALREADY_EXISTS) { + runtime.UnlockOSThread() + return nil, createErr + } + if handle == 0 { + runtime.UnlockOSThread() + return nil, fmt.Errorf("create target mutex returned an invalid handle") + } + event, waitErr := windows.WaitForSingleObject(handle, windows.INFINITE) + if waitErr != nil || event != windows.WAIT_OBJECT_0 && event != windows.WAIT_ABANDONED { + _ = windows.CloseHandle(handle) + runtime.UnlockOSThread() + if waitErr != nil { + return nil, waitErr + } + return nil, fmt.Errorf("wait for target mutex returned %#x", event) + } + return func() { + _ = windows.ReleaseMutex(handle) + _ = windows.CloseHandle(handle) + runtime.UnlockOSThread() + }, nil +} + +// existingRecoveryPaths returns every canonical or randomized aside path that +// could hold a binary moved away from targetPath by a previous promotion. The +// directory is attacker-writable under the threat model, so these names are +// only reasons to fail closed; their presence is never proof of file contents. +func existingRecoveryPaths(targetPath string) ([]string, error) { + dir := filepath.Dir(targetPath) + base := filepath.Base(targetPath) + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + // NTFS is case-insensitive (case-preserving), so a recovery file can exist + // on disk as e.g. "zero.exe.OLD" — fold both sides before matching, or it + // silently drops out of every caller's fail-closed check below. + lowerBase := strings.ToLower(base) + var paths []string + for _, entry := range entries { + name := entry.Name() + lowerName := strings.ToLower(name) + if lowerName == lowerBase+".old" || + (strings.HasPrefix(lowerName, lowerBase+".") && + strings.HasSuffix(lowerName, ".old") && + len(lowerName) > len(lowerBase)+len("..old")) { + paths = append(paths, filepath.Join(dir, name)) + } + } + return paths, nil +} + +// relocatedRecoveryPaths returns copies moved to the distinct recovery name +// after a failed restore could not establish a .keep marker. Unlike ordinary +// unmarked .old files, these are authoritative unresolved recovery state and +// must block every later promotion until the operator resolves them. +func relocatedRecoveryPaths(targetPath string) ([]string, error) { + dir := filepath.Dir(targetPath) + base := strings.ToLower(filepath.Base(targetPath)) + entries, err := os.ReadDir(dir) + if err != nil { + return nil, err + } + prefix := base + "." + var paths []string + for _, entry := range entries { + name := entry.Name() + lowerName := strings.ToLower(name) + if !strings.HasPrefix(lowerName, prefix) || !strings.HasSuffix(lowerName, ".recovery") { + continue + } + // The canonical relocation is .old..recovery. + // A failed promotion that already used a randomized aside can also + // produce ..old..recovery. + middle := strings.TrimSuffix(strings.TrimPrefix(lowerName, prefix), ".recovery") + if strings.HasPrefix(middle, "old.") && len(middle) > len("old.") || + strings.Contains(middle, ".old.") { + paths = append(paths, filepath.Join(dir, name)) + } + } + 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. +func markedRecoveryPaths(targetPath string) ([]string, error) { + recoveryPaths, err := existingRecoveryPaths(targetPath) + if err != nil { + return nil, err + } + var marked []string + for _, recoveryPath := range recoveryPaths { + if oldBinaryPreserved(recoveryPath) { + marked = append(marked, recoveryPath) + } + } + return marked, nil +} + +// verifyPromotedTarget reports whether targetPath names the same object as the +// staged handle after a reported-successful rename. SetFileInformationByHandle +// returning success is not, on its own, proof the object ended up there: a +// handle whose directory entry was removed and replaced out from under it +// (the substitution createStagingFile's exclusive, no-share open defends +// against — see its doc comment) can leave some Windows versions accepting +// the rename call without it taking effect, which would otherwise let +// promote report success while targetPath is left missing entirely. +// +// A metadata-only open is not blocked by staged.file's exclusive data/delete +// sharing. Comparing the volume and file index is independent of path spelling, +// including 8.3 names, case, UNC prefixes, and reparse points in ancestors. +func verifyPromotedTarget(file *os.File, targetPath string) error { + targetPathPtr, err := windows.UTF16PtrFromString(targetPath) + if err != nil { + return err + } + targetHandle, err := windows.CreateFile( + targetPathPtr, + 0, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return fmt.Errorf("open promoted target metadata: %w", err) + } + defer func() { _ = windows.CloseHandle(targetHandle) }() + + var stagedInfo windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(windows.Handle(file.Fd()), &stagedInfo); err != nil { + return fmt.Errorf("query staged object identity: %w", err) + } + var targetInfo windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(targetHandle, &targetInfo); err != nil { + return fmt.Errorf("query promoted target identity: %w", err) + } + if targetInfo.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return fmt.Errorf("promoted target is unexpectedly a reparse point") + } + if targetInfo.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 { + return fmt.Errorf("promoted target is unexpectedly a directory") + } + if stagedInfo.NumberOfLinks != 1 || targetInfo.NumberOfLinks != 1 { + return fmt.Errorf("promoted object unexpectedly has %d hard links", targetInfo.NumberOfLinks) + } + if stagedInfo.VolumeSerialNumber != targetInfo.VolumeSerialNumber || + stagedInfo.FileIndexHigh != targetInfo.FileIndexHigh || + stagedInfo.FileIndexLow != targetInfo.FileIndexLow { + return fmt.Errorf("target path does not name the staged object") + } + return nil +} + +// discardOpenObject schedules the staged object for deletion through the handle +// it was created with, so failure-path cleanup removes exactly the object this +// updater staged. The pathname alternative (os.Remove after the handle is +// closed) re-resolves the staging entry, and a principal who can write in the +// installation directory can substitute that entry in the gap — the same +// handoff createStagingFile and promote are built to avoid. Deletion takes +// effect when the last handle closes, which discard does immediately after. +// +// If the request fails, the staged file is left behind rather than removed by +// name: a leaked file costs disk, deleting an attacker-chosen entry costs the +// operator a file they own. +func (staged *stagedBinary) discardOpenObject() { + if staged.promoted || staged.file == nil { + return + } + _ = deleteFileByHandle(staged.file) +} + +func (staged *stagedBinary) discardPaths() { + // POSIX-only state is present in the shared struct and always nil here. + _ = staged.dir + _ = staged.dirHandle + _ = staged.parentHandle + // Nothing is removed by pathname here; see discardOpenObject. +} + +// fileRenameInfo mirrors FILE_RENAME_INFO. FileName is a variable-length WCHAR +// array that follows the header, so the buffer is sized by hand and the name is +// appended after fileRenameInfoHeaderSize bytes. +type fileRenameInfo struct { + ReplaceIfExists bool + RootDirectory windows.Handle + FileNameLength uint32 +} + +// fileRenameInfoHeaderSize is the offset of FILE_RENAME_INFO's FileName member. +// It is derived rather than hardcoded because the RootDirectory pointer's +// alignment (and therefore the offset) differs between 32- and 64-bit Windows. +var fileRenameInfoHeaderSize = func() uintptr { + var info fileRenameInfo + return unsafe.Offsetof(info.FileNameLength) + unsafe.Sizeof(info.FileNameLength) +}() + +// renameFileByHandle renames the object file refers to, not the object its +// current pathname resolves to. targetPath must be fully qualified. +// +// It is a package var, like stageBinary, so a test can simulate +// SetFileInformationByHandle reporting success without the rename actually +// taking effect — the exact failure mode verifyPromotedTarget defends +// against — without needing to reproduce whatever Windows-version-specific +// condition triggers it for real. +func renameOpenFile(file *os.File, targetPath string) error { + name, err := windows.UTF16FromString(targetPath) + if err != nil { + return err + } + // FILE_RENAME_INFO declares FileName as NUL-terminated even though + // FileNameLength excludes the terminator. Keep the terminator in the buffer: + // omitting it makes the ordinary rename fail with ERROR_PATH_NOT_FOUND on the + // supported Windows runner. + buffer := make([]byte, int(fileRenameInfoHeaderSize)+len(name)*2) + info := (*fileRenameInfo)(unsafe.Pointer(&buffer[0])) + // ReplaceIfExists stays false: promote already renamed the running binary + // aside, so a target that exists again means something raced the update, and + // failing is better than clobbering whatever appeared there. + info.FileNameLength = uint32((len(name) - 1) * 2) + for index, unit := range name { + binary.LittleEndian.PutUint16(buffer[int(fileRenameInfoHeaderSize)+index*2:], unit) + } + if err := windows.SetFileInformationByHandle( + windows.Handle(file.Fd()), + windows.FileRenameInfo, + &buffer[0], + uint32(len(buffer)), + ); err != nil { + return fmt.Errorf("rename staged binary onto %s: %w", targetPath, err) + } + return nil +} + +var renameFileByHandle = renameOpenFile diff --git a/internal/update/stage_windows_test.go b/internal/update/stage_windows_test.go new file mode 100644 index 000000000..c4931ad8b --- /dev/null +++ b/internal/update/stage_windows_test.go @@ -0,0 +1,130 @@ +//go:build windows + +package update + +import ( + "os" + "path/filepath" + "sync" + "testing" +) + +// TestCreateStagingFileRefusesPrecreatedHardLink is the regression test for +// #742: a lower-privileged attacker who can write in the installation +// directory pre-creates the staging path as a hard link to another file the +// (possibly elevated) updater can write. createStagingFile must fail instead +// of opening and truncating through that link. +func TestCreateStagingFileRefusesPrecreatedHardLink(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "victim") + if err := os.WriteFile(victim, []byte("do not touch"), 0o644); err != nil { + t.Fatalf("WriteFile victim: %v", err) + } + staged := filepath.Join(dir, "staged") + if err := os.Link(victim, staged); err != nil { + t.Fatalf("Link: %v", err) + } + + if _, err := createStagingFile(staged); err == nil { + t.Fatal("createStagingFile succeeded through a pre-existing hard link, want error") + } + + data, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("ReadFile victim: %v", err) + } + if string(data) != "do not touch" { + t.Fatalf("victim content = %q, want unchanged", data) + } +} + +// TestCreateStagingFileRefusesPrecreatedSymlink covers the reparse-point +// variant of the same #742 primitive. Creating a file symlink on Windows +// needs SeCreateSymbolicLinkPrivilege (admin) or Developer Mode; skip rather +// than fail where that isn't available. +func TestCreateStagingFileRefusesPrecreatedSymlink(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "victim") + if err := os.WriteFile(victim, []byte("do not touch"), 0o644); err != nil { + t.Fatalf("WriteFile victim: %v", err) + } + staged := filepath.Join(dir, "staged") + if err := os.Symlink(victim, staged); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + if _, err := createStagingFile(staged); err == nil { + t.Fatal("createStagingFile succeeded through a pre-existing symlink, want error") + } + + data, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("ReadFile victim: %v", err) + } + if string(data) != "do not touch" { + t.Fatalf("victim content = %q, want unchanged", data) + } +} + +// TestCreateStagingFileSucceedsForFreshPath is the control: a path with +// nothing pre-existing must still work normally. +func TestCreateStagingFileSucceedsForFreshPath(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "staged") + + file, err := createStagingFile(path) + if err != nil { + t.Fatalf("createStagingFile: %v", err) + } + if _, err := file.WriteString("payload"); err != nil { + t.Fatalf("WriteString: %v", err) + } + if err := file.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(data) != "payload" { + t.Fatalf("content = %q, want %q", data, "payload") + } +} + +// TestCreateStagingFileConcurrentRaceOnlyOneWinner exercises a race on a +// single fixed path (installBinary normally avoids this by randomizing the +// name, but the exclusive-creation guarantee must hold regardless): exactly +// one concurrent caller may create the file, and the rest must fail cleanly +// rather than silently truncate the winner's content. +func TestCreateStagingFileConcurrentRaceOnlyOneWinner(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "staged") + + const attempts = 16 + var wg sync.WaitGroup + successes := make([]bool, attempts) + for i := range attempts { + wg.Add(1) + go func(i int) { + defer wg.Done() + file, err := createStagingFile(path) + if err != nil { + return + } + defer func() { _ = file.Close() }() + successes[i] = true + }(i) + } + wg.Wait() + + winners := 0 + for _, ok := range successes { + if ok { + winners++ + } + } + if winners != 1 { + t.Fatalf("concurrent createStagingFile winners = %d, want exactly 1", winners) + } +} diff --git a/internal/update/staging_name_windows.go b/internal/update/staging_name_windows.go new file mode 100644 index 000000000..d78be6701 --- /dev/null +++ b/internal/update/staging_name_windows.go @@ -0,0 +1,38 @@ +//go:build windows + +package update + +import ( + "crypto/rand" + "encoding/hex" + "fmt" + "path/filepath" +) + +// randomStagingSuffix returns hex-encoded random bytes for stagingFilePath. +// Overridden in tests for a deterministic path; production always takes this +// default, cryptographically random one. +var randomStagingSuffix = func() (string, error) { + buf := make([]byte, 16) + if _, err := rand.Read(buf); err != nil { + return "", err + } + return hex.EncodeToString(buf), nil +} + +// stagingFilePath returns an unpredictable path in targetPath's directory +// (same filesystem, so the later rename into place is atomic). A fixed +// ".new" name is guessable in advance, and a lower-privileged +// process that can write in the installation directory could pre-create it +// as a hard link or reparse point to another file the elevated updater can +// write, turning the staging copy into an arbitrary-file-overwrite primitive. +// createStagingFile's exclusive, no-follow creation is the other half of +// closing that: even a correctly-guessed name can't be opened through. +func stagingFilePath(targetPath string) (string, error) { + suffix, err := randomStagingSuffix() + if err != nil { + return "", fmt.Errorf("generate staging file name: %w", err) + } + name := filepath.Base(targetPath) + "." + suffix + ".new" + return filepath.Join(filepath.Dir(targetPath), name), nil +}