🛡️ Sentinel: [HIGH] Fix insecure temporary file creation#49
🛡️ Sentinel: [HIGH] Fix insecure temporary file creation#49
Conversation
Vulnerability: Predictable temporary file names (`path + ".tmp"`) were being used with `os.WriteFile` in `os.TempDir()` (a shared directory) to perform atomic file writes. Impact: A local attacker could launch a symlink attack by pre-creating the predictable `.tmp` file as a symlink pointing to an arbitrary file. When Prism runs and `os.WriteFile` executes, it follows the symlink, potentially overwriting the arbitrary file with Prism's cache data using the application user's privileges. Fix: Replaced `os.WriteFile` with `os.CreateTemp`, which correctly passes the `O_EXCL` flag when opening the file. This ensures the temporary file does not already exist, mitigating the symlink attack. We also create the file in the same directory as the target path to ensure `os.Rename` won't fail due to moving across filesystems. The temporary file is properly closed before calling `os.Rename` to avoid open-file locking errors. 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 involving insecure temporary file creation by replacing predictable file names with os.CreateTemp in internal/burnrate/burnrate.go and internal/sparkline/sparkline.go. It also adds a sentinel document to track and prevent this vulnerability in the future. Feedback was provided to improve the robustness of the file-writing logic by using tmpFile.Sync() for data durability and defer os.Remove() to simplify cleanup and error handling.
| tmp := tmpFile.Name() | ||
|
|
||
| if _, err := tmpFile.Write(data); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmp) | ||
| return | ||
| } | ||
| 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) | ||
|
|
||
| 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, consider using tmpFile.Sync() for durability and defer os.Remove(tmp) to simplify the cleanup logic. This makes the atomic write process more robust.
tmp := tmpFile.Name()
defer os.Remove(tmp)
if _, err := tmpFile.Write(data); err != nil {
tmpFile.Close()
return
}
if err := tmpFile.Sync(); err != nil {
tmpFile.Close()
return
}
if err := tmpFile.Close(); err != nil {
return
}
if err := os.Chmod(tmp, 0644); err != nil {
return
}
if err := os.Rename(tmp, path); err != nil {
return
}
Understood. Acknowledging that this work is now superseded by the direct commit on main and stopping work on this task. |
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Predictable temporary file names were used with
os.WriteFilein shared directories like/tmp.🎯 Impact: This allowed symlink attacks where a malicious user could pre-create a predictable symlink.
os.WriteFilewould follow the symlink and overwrite arbitrary files with the privileges of the user running Prism.🔧 Fix: Replaced
os.WriteFilewithos.CreateTemp, which usesO_EXCLflag to ensure the temporary file does not exist, effectively mitigating symlink attacks. The temporary files are generated in the same target directory and closed beforeos.Rename. Also documented in.jules/sentinel.md.✅ Verification: Ran
go test ./...andgo build ./...which both completed successfully. Review approved the logic as "rock-solid and production-ready."PR created automatically by Jules for task 1521368908011730218 started by @himattm