diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..afee6c8 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,7 @@ +## 2025-02-18 - Fix Predictable Temporary File Symlink Vulnerability + +**Vulnerability:** The code previously used string concatenation (e.g., `path + ".tmp"`) with `os.WriteFile` and `os.Rename` for atomic file writes in shared directories like `os.TempDir()`. This pattern is vulnerable to symlink attacks, where an attacker could pre-create a symlink with the predictable name pointing to an arbitrary file they want to overwrite. + +**Learning:** Predictable temporary file names in shared directories (like `/tmp`) are inherently insecure for atomic writes. Even if the content being written isn't malicious, the *location* being written to can be controlled via symlinks, leading to arbitrary file overwrite or corruption issues. Furthermore, creating a file with `os.CreateTemp` ensures the OS opens it with `O_EXCL`, preventing any prior symlink from being followed. Finally, on Windows, file handles must be explicitly closed before the `os.Rename` step, or it will fail. + +**Prevention:** For atomic writes in shared directories, *always* use `os.CreateTemp` with an unpredictable pattern (like `prism-*.tmp`). Ensure the temporary file is created in the same directory as the target file (using `filepath.Dir()`) to avoid cross-device rename errors. Explicitly `Chmod` to match required permissions, handle the error gracefully by closing and removing the temporary file, and explicitly close the file descriptor before calling `os.Rename`. diff --git a/internal/burnrate/burnrate.go b/internal/burnrate/burnrate.go index 8cb680c..c4455d1 100644 --- a/internal/burnrate/burnrate.go +++ b/internal/burnrate/burnrate.go @@ -64,10 +64,28 @@ func LoadOrCreateSnapshotAt(sessionID string, currentCost float64, now time.Time return nil, false, err } - tmpPath := path + ".tmp" - if err := os.WriteFile(tmpPath, data, 0644); err != nil { + // Use os.CreateTemp to avoid symlink vulnerability with predictable names + tmpFile, err := os.CreateTemp(filepath.Dir(path), "prism-burn-*.tmp") + if err != nil { + return nil, false, err + } + tmpPath := tmpFile.Name() + + if _, err := tmpFile.Write(data); err != nil { + tmpFile.Close() + os.Remove(tmpPath) + return nil, false, err + } + // explicit close is important especially on Windows before rename + if err := tmpFile.Close(); err != nil { + os.Remove(tmpPath) + return nil, false, err + } + if err := os.Chmod(tmpPath, 0644); err != nil { + os.Remove(tmpPath) return nil, false, err } + if err := os.Rename(tmpPath, path); err != nil { os.Remove(tmpPath) return nil, false, err diff --git a/internal/sparkline/sparkline.go b/internal/sparkline/sparkline.go index f4c755c..d66b692 100644 --- a/internal/sparkline/sparkline.go +++ b/internal/sparkline/sparkline.go @@ -138,10 +138,31 @@ func Save(sessionID, metric string, b *Buffer) { if err != nil { return } - tmp := path + ".tmp" - if err := os.WriteFile(tmp, data, 0644); err != nil { + + // Use os.CreateTemp to avoid symlink vulnerability with predictable names + tmpFile, err := os.CreateTemp(filepath.Dir(path), "prism-spark-*.tmp") + if err != nil { + return + } + tmp := tmpFile.Name() + + if _, err := tmpFile.Write(data); err != nil { + tmpFile.Close() + os.Remove(tmp) + return + } + + // explicit close is important especially on Windows before rename + if err := tmpFile.Close(); err != nil { + os.Remove(tmp) + return + } + + if err := os.Chmod(tmp, 0644); err != nil { + os.Remove(tmp) return } + os.Rename(tmp, path) }