security(update): stage binary replacement at an unpredictable, exclusive path

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
+// "<target>.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
 	}

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
-	// "<helper>.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)
 	}
 

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)
+}

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)
+	}
+}

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 })
+}

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
+}