🛡️ Sentinel: [CRITICAL] Fix predictable temporary filename vulnerability#73
🛡️ Sentinel: [CRITICAL] Fix predictable temporary filename vulnerability#73
Conversation
…omic writes Replaced predictable temp file creation (`path + ".tmp"`) with `os.CreateTemp` for secure atomic file writes in `internal/burnrate/burnrate.go` and `internal/sparkline/sparkline.go` to prevent symlink attacks. 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 implementing atomic file writes using os.CreateTemp in internal/burnrate/burnrate.go and internal/sparkline/sparkline.go. Feedback was provided to replace the manual cleanup logic on error paths with the idiomatic Go defer pattern for closing and removing temporary files, which improves maintainability and reduces the risk of resource leaks.
| dir := filepath.Dir(path) | ||
| tmpFile, err := os.CreateTemp(dir, "prism-burn-*") | ||
| 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 | ||
| } | ||
| if err := tmpFile.Chmod(0644); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
| if err := tmpFile.Close(); err != nil { | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
|
|
||
| if err := os.Rename(tmpPath, path); err != nil { | ||
| os.Remove(tmpPath) | ||
| return nil, false, err |
There was a problem hiding this comment.
The manual cleanup of the temporary file and its handle on every error path is repetitive and error-prone. Using defer for cleanup is the idiomatic Go approach; it ensures that the temporary file is removed and the file handle is closed regardless of how the function exits (including panics). Since os.Rename moves the file, the deferred os.Remove will simply become a no-op on success.
dir := filepath.Dir(path)
tmpFile, err := os.CreateTemp(dir, "prism-burn-*")
if err != nil {
return nil, false, err
}
tmpPath := tmpFile.Name()
defer os.Remove(tmpPath)
defer tmpFile.Close()
if _, err := tmpFile.Write(data); err != nil {
return nil, false, err
}
if err := tmpFile.Chmod(0644); err != nil {
return nil, false, err
}
if err := tmpFile.Close(); err != nil {
return nil, false, err
}
if err := os.Rename(tmpPath, path); err != nil {
return nil, false, err
}| dir := filepath.Dir(path) | ||
| tmpFile, err := os.CreateTemp(dir, "prism-spark-*") | ||
| if err != nil { | ||
| return | ||
| } | ||
| tmpPath := tmpFile.Name() | ||
|
|
||
| if _, err := tmpFile.Write(data); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return | ||
| } | ||
| os.Rename(tmp, path) | ||
| if err := tmpFile.Chmod(0644); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return | ||
| } | ||
| if err := tmpFile.Close(); err != nil { | ||
| os.Remove(tmpPath) | ||
| return | ||
| } | ||
|
|
||
| if err := os.Rename(tmpPath, path); err != nil { | ||
| os.Remove(tmpPath) | ||
| } |
There was a problem hiding this comment.
Similar to the implementation in burnrate.go, the manual cleanup logic here is redundant. Adopting the defer pattern improves maintainability and ensures resources are properly released even if the function logic grows or encounters unexpected failures.
dir := filepath.Dir(path)
tmpFile, err := os.CreateTemp(dir, "prism-spark-*")
if err != nil {
return
}
tmpPath := tmpFile.Name()
defer os.Remove(tmpPath)
defer tmpFile.Close()
if _, err := tmpFile.Write(data); err != nil {
return
}
if err := tmpFile.Chmod(0644); err != nil {
return
}
if err := tmpFile.Close(); err != nil {
return
}
if err := os.Rename(tmpPath, path); err != nil {
return
}
Understood. Acknowledging that this work is now obsolete as the changes have been superseded by a direct commit on main. Stopping work on this task. |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Predictable temporary filenames during atomic file writes using
os.WriteFile+os.Rename. This predictable path (<filename>.tmp) could allow an attacker to create a symlink with that name before the write occurs, tricking the application into writing data into or overwriting unintended files, potentially leading to a privilege escalation or denial-of-service attack.🎯 Impact: Without a secure randomized temporary filename, file operations (especially in
os.TempDir()or shared spaces) are highly susceptible to malicious symlinks.🔧 Fix: Utilized
os.CreateTempto generate random unpredictable temporary filenames alongside the target file (filepath.Dir(path)). Once writing finishes, explicitly managed0644file permissions (Chmod), safely closed the file handles, and finalized withos.Rename. Also added error cleanup.✅ Verification: Compiled the Go application (
go build ./...) and ran all tests (go test ./...), which completed successfully. The secure implementation also safely manages file closures before renaming, avoiding Windows cross-device link/locking errors.PR created automatically by Jules for task 3689979196399062848 started by @himattm