🛡️ Sentinel: [CRITICAL] Fix predictable temp file symlink attack in atomic file writes#69
🛡️ Sentinel: [CRITICAL] Fix predictable temp file symlink attack in atomic file writes#69
Conversation
…ties Replaced uses of `os.WriteFile` with a predictable temporary filename `path + ".tmp"` in `os.TempDir()` with `os.CreateTemp` to generate unpredictable temporary files for atomic writes. This mitigates a classic symlink attack where an attacker could overwrite arbitrary files with the privileges of the user running the program. 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 file names by implementing os.CreateTemp in internal/burnrate/burnrate.go and internal/sparkline/sparkline.go. The changes also include documentation of the vulnerability in a new sentinel file. Feedback was provided to improve the reliability of these atomic writes by suggesting the use of f.Sync() and proper error handling for f.Close() to ensure data is fully flushed to disk before the final rename operation.
| if _, err := f.Write(data); err != nil { | ||
| f.Close() | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
| f.Chmod(0644) | ||
| f.Close() |
There was a problem hiding this comment.
To ensure the atomic write is durable and complete, it is recommended to call f.Sync() before closing the file, and to check the error returned by f.Close(). If f.Close() fails, it may indicate that the data was not successfully flushed to disk, and the subsequent os.Rename could result in a corrupted or incomplete file.
if _, err := f.Write(data); err != nil {
f.Close()
os.Remove(tmpPath)
return nil, false, err
}
if err := f.Sync(); err != nil {
f.Close()
os.Remove(tmpPath)
return nil, false, err
}
if err := f.Chmod(0644); err != nil {
f.Close()
os.Remove(tmpPath)
return nil, false, err
}
if err := f.Close(); err != nil {
os.Remove(tmpPath)
return nil, false, err
}| if _, err := f.Write(data); err != nil { | ||
| f.Close() | ||
| os.Remove(tmp) | ||
| return | ||
| } | ||
| os.Rename(tmp, path) | ||
| f.Chmod(0644) | ||
| f.Close() |
There was a problem hiding this comment.
Consider adding f.Sync() and checking the error from f.Close() to ensure data durability and integrity before renaming the temporary file. This ensures that the file is fully written to disk before it replaces the target file.
if _, err := f.Write(data); err != nil {
f.Close()
os.Remove(tmp)
return
}
if err := f.Sync(); err != nil {
f.Close()
os.Remove(tmp)
return
}
if err := f.Chmod(0644); err != nil {
f.Close()
os.Remove(tmp)
return
}
if err := f.Close(); err != nil {
os.Remove(tmp)
return
}
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Predictable temporary files were created in a shared directory (
os.TempDir()), allowing for a local symlink attack to overwrite arbitrary files.🎯 Impact: An attacker could exploit this vulnerability to escalate privileges or corrupt sensitive system files running with the privileges of the executing user.
🔧 Fix: Replaced uses of
os.WriteFileto intermediate, predictably named files ininternal/burnrate/burnrate.goandinternal/sparkline/sparkline.gowithos.CreateTempwhich safely generates unique, unpredictable temporary files. Addressed file closure, renaming logic, and permission inheritance correctly. Added.jules/sentinel.mdjournal entry with findings.✅ Verification: Ran
go test ./...successfully and verified correct functionality without symlink issues. Allos.WriteFilecalls used for this purpose were removed from the program.PR created automatically by Jules for task 13382186907770207822 started by @himattm