🛡️ Sentinel: [CRITICAL] Fix predictable temporary filenames (Symlink attack)#87
🛡️ Sentinel: [CRITICAL] Fix predictable temporary filenames (Symlink attack)#87
Conversation
…attack) Fixed predictable temporary filenames being used during atomic file writes (`path + ".tmp"`) in `os.TempDir()` within `internal/burnrate/burnrate.go` and `internal/sparkline/sparkline.go`. Atomic file writes often use a predictable `.tmp` suffix to create a temporary file before renaming. In shared directories like `os.TempDir()`, this predictable naming allows attackers to pre-create the file as a symlink, tricking the application into overwriting unintended files (Symlink attack). The code now correctly utilizes `os.CreateTemp` to securely create an unpredictable temporary file in the target directory before writing to it and renaming it. Co-authored-by: himattm <6266621+himattm@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a security vulnerability related to predictable temporary filenames by replacing manual path concatenation with os.CreateTemp in internal/burnrate/burnrate.go and internal/sparkline/sparkline.go. Documentation regarding this vulnerability was also added to .jules/sentinel.md. The review feedback suggests using defer os.Remove and f.Chmod to simplify the error handling logic and ensure the temporary files are consistently cleaned up.
| tmp := f.Name() | ||
| if _, err := f.Write(data); err != nil { | ||
| f.Close() | ||
| os.Remove(tmp) | ||
| return | ||
| } | ||
| if err := f.Close(); err != nil { | ||
| os.Remove(tmp) | ||
| return | ||
| } | ||
| if err := os.Chmod(tmp, 0644); err != nil { | ||
| os.Remove(tmp) | ||
| return | ||
| } | ||
| os.Rename(tmp, path) | ||
| if err := os.Rename(tmp, path); err != nil { | ||
| os.Remove(tmp) | ||
| } |
There was a problem hiding this comment.
Similar to the changes in burnrate.go, using f.Chmod and defer os.Remove would make the code more idiomatic and robust. This reduces boilerplate and ensures the temporary file is cleaned up on all error paths, including the final rename.
| tmp := f.Name() | |
| if _, err := f.Write(data); err != nil { | |
| f.Close() | |
| os.Remove(tmp) | |
| return | |
| } | |
| if err := f.Close(); err != nil { | |
| os.Remove(tmp) | |
| return | |
| } | |
| if err := os.Chmod(tmp, 0644); err != nil { | |
| os.Remove(tmp) | |
| return | |
| } | |
| os.Rename(tmp, path) | |
| if err := os.Rename(tmp, path); err != nil { | |
| os.Remove(tmp) | |
| } | |
| tmp := f.Name() | |
| defer os.Remove(tmp) | |
| if _, err := f.Write(data); err != nil { | |
| f.Close() | |
| return | |
| } | |
| if err := f.Chmod(0644); err != nil { | |
| f.Close() | |
| return | |
| } | |
| if err := f.Close(); err != nil { | |
| return | |
| } | |
| if err := os.Rename(tmp, path); err != nil { | |
| return | |
| } |
himattm
deleted the
sentinel-fix-predictable-tmp-files-11334240315250071954
branch
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Found predictable temporary filenames being used during atomic file writes (
path + ".tmp") inos.TempDir()withininternal/burnrate/burnrate.goandinternal/sparkline/sparkline.go.🎯 Impact: In shared directories like
os.TempDir(), this predictable naming allows attackers to pre-create the file as a symlink, tricking the application into overwriting unintended files (Symlink attack).🔧 Fix: The code now correctly utilizes
os.CreateTempto securely create an unpredictable temporary file in the target directory before writing to it and renaming it. Also implementedClosechecking before setting modes and renaming.✅ Verification: Run
go test ./...andgo fmt ./.... Reviewgit diffoninternal/burnrate/burnrate.goandinternal/sparkline/sparkline.goto ensureos.CreateTempreplaced string concatenation in predictable temp files.PR created automatically by Jules for task 11334240315250071954 started by @himattm