From a2fa19bcf9886976021185504ed94342070201a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 2 Jun 2026 17:32:30 +0000 Subject: [PATCH] Use os.CreateTemp for atomic file rewrites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the hand-rolled temp file pattern in File.Write with os.CreateTemp, which opens with O_RDWR|O_CREATE|O_EXCL|0600 and a CSPRNG-quality random suffix. The combination defends against the symlink/race attack documented in issue #3: 1. The previous temp-file name came from RandomString(20) backed by the global math/rand PRNG, which a co-resident attacker could observe or predict. 2. os.WriteFile opens with O_WRONLY|O_CREATE|O_TRUNC (no O_EXCL), so if an attacker pre-created the temp path as a symlink to a path the running user could write, the open followed the symlink and the new content (or the truncate) hit the attacker-chosen target. os.CreateTemp closes both halves of the bug in one call. Temp files now share the `.find-replace.*` prefix (leading dot keeps them hidden from `ls`) which gives issue #21 a stable glob to filter on. Permission preservation is maintained explicitly: os.CreateTemp creates the file 0600, so the temp file is Chmod'd back to the original mode.Perm() before the rename. Setuid/setgid/sticky bits are intentionally NOT preserved here — see issue #18. The defer os.Remove(tempName) cleanup path is preserved; only the temp-file primitive changes. The unused RandomString helper (and its dedicated tests) are removed along with strings.go and strings_test.go; the only remaining caller was BenchmarkNova, which now uses a fixed find/replace pair (more reproducible than two random characters). --- file_handling.go | 41 ++++++++- file_handling_test.go | 208 ++++++++++++++++++++++++++++++++++++++++++ find_replace_test.go | 5 +- strings.go | 18 ---- strings_test.go | 35 ------- 5 files changed, 248 insertions(+), 59 deletions(-) delete mode 100644 strings.go delete mode 100644 strings_test.go diff --git a/file_handling.go b/file_handling.go index 7c0b1a7..a5a15ae 100644 --- a/file_handling.go +++ b/file_handling.go @@ -88,25 +88,56 @@ func (f *File) Read() (string, error) { return builder.String(), nil } +// tempFilePrefix is the prefix used for the atomic-rewrite temp file +// created in the target's directory. The leading dot keeps the file hidden +// from `ls`, and the shared prefix gives a future "skip orphan temp files +// on a subsequent walk" pass (issue #21) a stable glob to filter on. +const tempFilePrefix = ".find-replace." + // Write atomically replaces the file with content, via a temp file + rename. +// +// The temp file is created with os.CreateTemp, which opens with +// O_RDWR|O_CREATE|O_EXCL|0600 and a CSPRNG-quality random suffix. The +// combination defends against the symlink/race attack in issue #3: a +// co-resident attacker cannot predict the name, and even if they could, +// O_EXCL would refuse to clobber an attacker-pre-created path (which +// os.WriteFile + O_CREATE used to silently follow as a symlink). +// // A deferred os.Remove(tempName) ensures the temp file is cleaned up if any -// step after its creation fails (including the rename); on success the remove -// is a no-op because the file has already been renamed away. +// step after its creation fails (including the rename); on success the +// remove is a no-op because the file has already been renamed away. func (f *File) Write(content string) error { mode, err := f.Mode() if err != nil { return err } - tempName := filepath.Join(f.Dir(), RandomString(20)) - if err := os.WriteFile(tempName, []byte(content), mode); err != nil { + tmp, err := os.CreateTemp(f.Dir(), tempFilePrefix+"*") + if err != nil { return fmt.Errorf("create tempfile in %v: %w", f.Dir(), err) } - // Make sure the temp file is removed if the rename below fails. On + tempName := tmp.Name() + // Make sure the temp file is removed if any step below fails. On // success, the rename has already moved the file to f.Path so this is // a no-op (we deliberately ignore the not-exist error). defer os.Remove(tempName) + if _, err := tmp.WriteString(content); err != nil { + tmp.Close() + return fmt.Errorf("write tempfile %v: %w", tempName, err) + } + // Preserve the original file's permission bits. os.CreateTemp creates + // the file 0600; without this Chmod the rewrite would silently downgrade + // permissions. Setuid/setgid/sticky bits are intentionally NOT preserved + // in this PR — see issue #18 for the separate threat model. + if err := tmp.Chmod(mode.Perm()); err != nil { + tmp.Close() + return fmt.Errorf("chmod tempfile %v: %w", tempName, err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close tempfile %v: %w", tempName, err) + } + log.Printf("Rewriting %v", f.Path) if err := os.Rename(tempName, f.Path); err != nil { return fmt.Errorf("atomically move temp file %v to %v: %w", tempName, f.Path, err) diff --git a/file_handling_test.go b/file_handling_test.go index 91ee538..c00f48b 100644 --- a/file_handling_test.go +++ b/file_handling_test.go @@ -1,7 +1,10 @@ package main import ( + "io/fs" + "os" "path/filepath" + "runtime" "testing" ) @@ -77,3 +80,208 @@ func TestNewFile(t *testing.T) { }) } } + +// writeAndPrime is a tiny helper that primes the file's cached Info (Write +// requires it) and runs Write, returning the resulting error. +func writeAndPrime(tb testing.TB, f *File, content string) error { + tb.Helper() + if _, err := f.Info(); err != nil { + tb.Fatalf("Info(%q): %v", f.Path, err) + } + return f.Write(content) +} + +// TestWrite_TempFileUsesORDWREXCL documents the security invariant we get +// from os.CreateTemp: the temp file is opened with O_RDWR|O_CREATE|O_EXCL, +// so a pre-existing file at the same path would cause the open to fail +// rather than clobber. The CSPRNG-quality suffix on the temp name means an +// attacker cannot reasonably pre-create the right name; this test pre- +// creates a distractor under the same prefix to assert it survives the +// rewrite unchanged. +func TestWrite_TempFileUsesORDWREXCL(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.txt") + if err := os.WriteFile(target, []byte("alpha"), 0644); err != nil { + t.Fatalf("WriteFile(%q): %v", target, err) + } + + // Pre-create a sentinel file under the agreed-upon prefix. Whatever the + // random suffix happens to be, it must not collide with this name (the + // CSPRNG suffix guarantees that statistically); even if it did, + // O_EXCL would cause the open to fail, leaving the sentinel untouched. + sentinelName := tempFilePrefix + "collision" + sentinel := filepath.Join(dir, sentinelName) + const sentinelContent = "do-not-touch" + if err := os.WriteFile(sentinel, []byte(sentinelContent), 0644); err != nil { + t.Fatalf("WriteFile(%q): %v", sentinel, err) + } + + f := newFileOrFatal(t, target) + if err := writeAndPrime(t, f, "beta"); err != nil { + t.Fatalf("Write(%q): %v", target, err) + } + + // The target file should be rewritten. + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile(%q): %v", target, err) + } + if string(got) != "beta" { + t.Errorf("target contents = %q; want %q", string(got), "beta") + } + + // The sentinel must be exactly as we left it. + gotSentinel, err := os.ReadFile(sentinel) + if err != nil { + t.Fatalf("ReadFile(%q): %v", sentinel, err) + } + if string(gotSentinel) != sentinelContent { + t.Errorf("sentinel contents = %q; want %q (Write clobbered a same-prefix file)", string(gotSentinel), sentinelContent) + } +} + +// TestWrite_TempFilePrefixVisible enforces the agreed-upon temp-file name +// prefix. On a successful rewrite, the temp file has been renamed over the +// target, so no `.find-replace.*` entry should linger in the target's +// directory. +func TestWrite_TempFilePrefixVisible(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.txt") + if err := os.WriteFile(target, []byte("alpha"), 0644); err != nil { + t.Fatalf("WriteFile(%q): %v", target, err) + } + + f := newFileOrFatal(t, target) + if err := writeAndPrime(t, f, "beta"); err != nil { + t.Fatalf("Write(%q): %v", target, err) + } + + // After a successful Write, no temp file should remain. + matches, err := filepath.Glob(filepath.Join(dir, tempFilePrefix+"*")) + if err != nil { + t.Fatalf("Glob(%q*): %v", tempFilePrefix, err) + } + if len(matches) != 0 { + t.Errorf("leftover temp files matching %q*: %v", tempFilePrefix, matches) + } +} + +// TestWrite_PreservesOriginalPermissionBits verifies that the rewrite +// preserves the original file's permission bits (mode.Perm() — setuid/ +// setgid/sticky preservation is out of scope, tracked separately in +// issue #18). Without an explicit Chmod after CreateTemp, the rewritten +// file would silently become 0600. +func TestWrite_PreservesOriginalPermissionBits(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission-bit semantics differ on Windows") + } + + tests := []struct { + name string + mode os.FileMode + }{ + {name: "0644", mode: 0644}, + {name: "0600", mode: 0600}, + {name: "0755", mode: 0755}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.txt") + if err := os.WriteFile(target, []byte("alpha"), tc.mode); err != nil { + t.Fatalf("WriteFile(%q, _, %v): %v", target, tc.mode, err) + } + // Some umasks would alter the on-disk mode; force it. + if err := os.Chmod(target, tc.mode); err != nil { + t.Fatalf("Chmod(%q, %v): %v", target, tc.mode, err) + } + + f := newFileOrFatal(t, target) + if err := writeAndPrime(t, f, "beta"); err != nil { + t.Fatalf("Write(%q): %v", target, err) + } + + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat(%q) after Write: %v", target, err) + } + if got := info.Mode().Perm(); got != tc.mode { + t.Errorf("mode after Write = %v; want %v", got, tc.mode) + } + }) + } +} + +// TestWrite_DoesNotFollowAttackerPreparedSymlinkAtTempPath simulates an +// attacker who has pre-created a symlink under the agreed temp-file prefix +// pointing at a sensitive target. The rewrite must NOT follow that symlink +// — it gets a fresh random name from os.CreateTemp, and even on a +// (statistically impossible) name collision O_EXCL would fail. The decoy +// must remain a symlink to the same destination, and the destination's +// contents must be unchanged. +func TestWrite_DoesNotFollowAttackerPreparedSymlinkAtTempPath(t *testing.T) { + requireSymlinks(t) + + dir := t.TempDir() + target := filepath.Join(dir, "target.txt") + if err := os.WriteFile(target, []byte("alpha"), 0644); err != nil { + t.Fatalf("WriteFile(%q): %v", target, err) + } + + // "Sensitive" file the attacker hopes to clobber via the symlink. + sensitiveDir := t.TempDir() + sensitive := filepath.Join(sensitiveDir, "victim") + const sensitiveContent = "must-not-change" + if err := os.WriteFile(sensitive, []byte(sensitiveContent), 0644); err != nil { + t.Fatalf("WriteFile(%q): %v", sensitive, err) + } + + // Pre-create a symlink in the rewrite directory under the temp prefix + // pointing at the sensitive file. + decoy := filepath.Join(dir, tempFilePrefix+"attacker") + if err := os.Symlink(sensitive, decoy); err != nil { + t.Fatalf("Symlink(%q, %q): %v", sensitive, decoy, err) + } + + f := newFileOrFatal(t, target) + if err := writeAndPrime(t, f, "beta"); err != nil { + t.Fatalf("Write(%q): %v", target, err) + } + + // (a) Rewrite succeeded. + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile(%q): %v", target, err) + } + if string(got) != "beta" { + t.Errorf("target contents = %q; want %q", string(got), "beta") + } + + // (b) The decoy still exists as a symlink (was not opened-and-written + // through, was not replaced by a regular file). + info, err := os.Lstat(decoy) + if err != nil { + t.Fatalf("Lstat(%q): %v (decoy symlink was removed)", decoy, err) + } + if info.Mode()&fs.ModeSymlink == 0 { + t.Errorf("Lstat(%q).Mode() = %v; want a symlink (was replaced with a regular file)", decoy, info.Mode()) + } + + // (c) The symlink's destination is unchanged. + gotSensitive, err := os.ReadFile(sensitive) + if err != nil { + t.Fatalf("ReadFile(%q): %v", sensitive, err) + } + if string(gotSensitive) != sensitiveContent { + t.Errorf("sensitive contents = %q; want %q (symlink was followed)", string(gotSensitive), sensitiveContent) + } + + // Sanity check: the symlink in the rewrite dir is still pointing at the + // same place. + if dst, err := os.Readlink(decoy); err != nil { + t.Fatalf("Readlink(%q): %v", decoy, err) + } else if dst != sensitive { + t.Errorf("Readlink(%q) = %q; want %q", decoy, dst, sensitive) + } +} diff --git a/find_replace_test.go b/find_replace_test.go index d8b62f3..45c1fe1 100644 --- a/find_replace_test.go +++ b/find_replace_test.go @@ -698,7 +698,10 @@ func BenchmarkNova(b *testing.B) { for n := 0; n < b.N; n++ { b.StopTimer() d := CloneRepoToTestDir(b, "git@github.com:openstack/nova.git") - fr := findReplace{find: RandomString(2), replace: RandomString(2)} + // A fixed find/replace pair keeps the benchmark reproducible; the + // historical use of random two-character strings here did not + // meaningfully exercise different code paths. + fr := findReplace{find: "fo", replace: "ba"} b.StartTimer() fr.WalkDir(d) } diff --git a/strings.go b/strings.go deleted file mode 100644 index b546a31..0000000 --- a/strings.go +++ /dev/null @@ -1,18 +0,0 @@ -package main - -import "math/rand" - -var characters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") - -// randomString generates a random base-62 string of the given length (or returns an -// empty string). -func RandomString(n int) string { - if n <= 0 { - return "" - } - b := make([]rune, n) - for i := range b { - b[i] = characters[rand.Intn(len(characters))] - } - return string(b) -} diff --git a/strings_test.go b/strings_test.go deleted file mode 100644 index 408c03b..0000000 --- a/strings_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package main - -import ( - "testing" -) - -// assertRandomStringLength ensures that the generated string matches the -// desired length. -func assertRandomStringLength(t *testing.T, ask int, want int) { - t.Helper() - got := len(RandomString(ask)) - if got != want { - t.Errorf("len(RandomString(%v)) = %v; want %v", ask, got, want) - } -} - -func TestRandomStringLengthNegativeOne(t *testing.T) { - assertRandomStringLength(t, -1, 0) -} - -func TestRandomStringLengthZero(t *testing.T) { - assertRandomStringLength(t, 0, 0) -} - -func TestRandomStringLengthOne(t *testing.T) { - assertRandomStringLength(t, 1, 1) -} - -func TestRandomStringLengthTen(t *testing.T) { - assertRandomStringLength(t, 10, 10) -} - -func TestRandomStringLengthTwenty(t *testing.T) { - assertRandomStringLength(t, 20, 20) -}