From f62c4fc56e14ff9cda86e1b6656cc88fab5dc125 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 19 Jul 2026 14:02:52 +0200 Subject: [PATCH] security(update): stage binary replacement at an unpredictable, exclusive path Fixes #742. installBinary staged downloaded release bytes at the fixed .new path and opened it with O_CREATE|O_WRONLY|O_TRUNC. In an installation directory writable by a lower-privileged process, that process could pre-create .new as a hard link or reparse point to another file the (possibly elevated) updater can write; the truncating open then overwrote that unintended file with verified Zero executable bytes. stagingFilePath now generates a cryptographically random suffix so the path can't be targeted in advance, and platform-specific createStagingFile opens it exclusively without following a pre-existing link: O_CREATE|O_EXCL on POSIX (which POSIX guarantees fails on a pre-existing path, symlink or not, without resolving it), and CreateFile with CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT plus a post-open GetFileInformationByHandle check on Windows (not a reparse point, not a directory, single hard link). replace_windows.go's rename-swap sequence is unchanged: the dangerous operation was always the truncating open of the staged path, not the .old rename/removal that follows. Co-Authored-By: Claude Sonnet 5 --- internal/update/apply.go | 37 +++++- internal/update/apply_test.go | 12 +- internal/update/stage_other.go | 14 +++ internal/update/stage_other_test.go | 129 ++++++++++++++++++++ internal/update/stage_test_helpers_test.go | 15 +++ internal/update/stage_windows.go | 65 +++++++++++ internal/update/stage_windows_test.go | 130 +++++++++++++++++++++ 7 files changed, 397 insertions(+), 5 deletions(-) create mode 100644 internal/update/stage_other.go create mode 100644 internal/update/stage_other_test.go create mode 100644 internal/update/stage_test_helpers_test.go create mode 100644 internal/update/stage_windows.go create mode 100644 internal/update/stage_windows_test.go diff --git a/internal/update/apply.go b/internal/update/apply.go index 29736de1a..901c0afa8 100644 --- a/internal/update/apply.go +++ b/internal/update/apply.go @@ -2,6 +2,8 @@ package update import ( "context" + "crypto/rand" + "encoding/hex" "fmt" "io" "net/http" @@ -220,7 +222,10 @@ func verifyArchiveChecksum(checksumPath string, expectedArchiveName string) erro // installBinary stages sourcePath next to targetPath (same directory, so the // final rename is atomic/same-filesystem) and then swaps it into place. func installBinary(sourcePath string, targetPath string) error { - stagedPath := targetPath + ".new" + stagedPath, err := stagingFilePath(targetPath) + if err != nil { + return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err) + } if err := copyFile(sourcePath, stagedPath); err != nil { return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err) } @@ -233,6 +238,34 @@ func installBinary(sourcePath string, targetPath string) error { return nil } +// 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 +} + func copyFile(sourcePath string, destPath string) (retErr error) { source, err := os.Open(sourcePath) if err != nil { @@ -241,7 +274,7 @@ 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) + dest, err := createStagingFile(destPath) if err != nil { return err } diff --git a/internal/update/apply_test.go b/internal/update/apply_test.go index 379273498..6e1a89b23 100644 --- a/internal/update/apply_test.go +++ b/internal/update/apply_test.go @@ -168,9 +168,15 @@ 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 { + // Force installBinary's staging copy to fail by pinning the random + // staging suffix and occupying the resulting path with a directory + // instead of a file. + stubRandomStagingSuffix(t, "test-fixed-suffix") + stagedHelperPath, err := stagingFilePath(existingHelperPath) + if err != nil { + t.Fatalf("stagingFilePath: %v", err) + } + if err := os.MkdirAll(stagedHelperPath, 0o755); err != nil { t.Fatalf("MkdirAll staged path: %v", err) } diff --git a/internal/update/stage_other.go b/internal/update/stage_other.go new file mode 100644 index 000000000..2ebc087d7 --- /dev/null +++ b/internal/update/stage_other.go @@ -0,0 +1,14 @@ +//go:build !windows + +package update + +import "os" + +// createStagingFile creates path exclusively so a pre-existing hard link or +// symlink at that path (which a lower-privileged attacker may have staged in +// a writable installation directory) can never be opened through: per POSIX, +// O_CREAT|O_EXCL fails with EEXIST if path already exists — including a +// dangling symlink — without following it. +func createStagingFile(path string) (*os.File, error) { + return os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o755) +} diff --git a/internal/update/stage_other_test.go b/internal/update/stage_other_test.go new file mode 100644 index 000000000..27138ec3c --- /dev/null +++ b/internal/update/stage_other_test.go @@ -0,0 +1,129 @@ +//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: 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 := 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/stage_test_helpers_test.go b/internal/update/stage_test_helpers_test.go new file mode 100644 index 000000000..2dd495fd4 --- /dev/null +++ b/internal/update/stage_test_helpers_test.go @@ -0,0 +1,15 @@ +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 tests that need to know (or pre-occupy) the exact staging path pin it +// here instead. +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_windows.go b/internal/update/stage_windows.go new file mode 100644 index 000000000..cf9b47acc --- /dev/null +++ b/internal/update/stage_windows.go @@ -0,0 +1,65 @@ +//go:build windows + +package update + +import ( + "fmt" + "os" + + "golang.org/x/sys/windows" +) + +// 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. +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, + 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) + } + if err := verifyFreshRegularFile(handle, path); err != nil { + _ = windows.CloseHandle(handle) + _ = os.Remove(path) + return nil, err + } + return os.NewFile(uintptr(handle), path), 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 +} 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) + } +}