🛡️ Sentinel: [HIGH] Fix predictable temp file symlink attack#88
🛡️ Sentinel: [HIGH] Fix predictable temp file symlink attack#88
Conversation
Fixed a vulnerability in internal/sparkline/sparkline.go and internal/burnrate/burnrate.go where predictable temporary file names (e.g., path + ".tmp") were used during atomic writes in shared temporary directories. Replaced with os.CreateTemp and handled permissions appropriately 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. |
There was a problem hiding this comment.
Code Review
This pull request mitigates symlink attack vulnerabilities by replacing predictable temporary filenames with os.CreateTemp in internal/burnrate/burnrate.go and internal/sparkline/sparkline.go. A sentinel documentation file was also added to record this security improvement. Feedback was provided regarding internal/sparkline/sparkline.go, where the error from os.Rename is currently ignored, potentially leading to leaked temporary files if the rename operation fails.
| return | ||
| } | ||
|
|
||
| os.Rename(tmp, path) |
There was a problem hiding this comment.
The error from os.Rename is ignored here. Since os.CreateTemp generates a unique filename for every call, failing to rename the file will result in a leaked temporary file in the system's temp directory. Over time, this can lead to disk space exhaustion. You should handle the error and remove the temporary file if the rename fails, consistent with the implementation in internal/burnrate/burnrate.go.
if err := os.Rename(tmp, path); err != nil {
os.Remove(tmp)
}
Understood. Acknowledging that this work is now obsolete and superseded by the direct commit on main, and stopping work on this task. |
🚨 Severity: HIGH
💡 Vulnerability: Predictable temporary file names (e.g.,
path + ".tmp") were used in world-writable directories (os.TempDir()) during atomic file writes ininternal/sparkline/sparkline.goandinternal/burnrate/burnrate.go. This allows malicious actors to create symlinks and overwrite arbitrary files becauseos.WriteFilefollows symlinks.🎯 Impact: Local privilege escalation or data corruption, as an attacker could pre-create a symlink with the predictable
.tmpname pointing to a sensitive file, causing Prism to overwrite it.🔧 Fix: Replaced the predictable
path + ".tmp"logic withos.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*.tmp"). Properly ensured the temporary file is created safely, has its permissions set to0644usingf.Chmod(0644), and is explicitly closed before performingos.Rename.✅ Verification: Verified that tests pass via
go test ./...and the logic securely sets up unpredictable file names usingos.CreateTemp.PR created automatically by Jules for task 17575191897006099380 started by @himattm