🛡️ Sentinel: [CRITICAL] Fix symlink vulnerability in atomic writes#76
🛡️ Sentinel: [CRITICAL] Fix symlink vulnerability in atomic writes#76
Conversation
Fixed a critical security vulnerability where predictable temporary file names (.tmp) were used for atomic file operations. By switching to os.CreateTemp with a random suffix, this prevents a malicious actor from orchestrating a symlink attack (CWE-377) during writes. Furthermore, we explicitly close the file before os.Rename to ensure cross-platform compatibility (Windows). 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 predictable temporary file symlink vulnerability by replacing string-concatenated temporary paths with os.CreateTemp in the burnrate and sparkline packages. The review feedback identifies an ignored error from os.Rename in sparkline.go and recommends using f.Chmod on the open file descriptor instead of the file path to prevent potential TOCTOU race conditions.
| return | ||
| } | ||
|
|
||
| os.Rename(tmp, path) |
There was a problem hiding this comment.
The error returned by os.Rename is currently ignored. If the rename operation fails (e.g., due to a file lock or permission issue), the temporary file will remain on disk and the data will not be persisted. The error should be handled, and the temporary file should be removed on failure to ensure a clean state.
if err := os.Rename(tmp, path); err != nil {
os.Remove(tmp)
}| if err := tmpFile.Close(); err != nil { | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
| if err := os.Chmod(tmpPath, 0644); err != nil { | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } |
There was a problem hiding this comment.
To avoid a potential TOCTOU (Time-of-check to time-of-use) race condition, it is safer to use tmpFile.Chmod(0644) on the open file descriptor before closing it, rather than calling os.Chmod(tmpPath, 0644) on the file path after it has been closed. This ensures the permissions are applied to the exact file that was created, which is consistent with the security goals of this PR.
| if err := tmpFile.Close(); err != nil { | |
| os.Remove(tmpPath) | |
| return nil, false, err | |
| } | |
| if err := os.Chmod(tmpPath, 0644); err != nil { | |
| 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 := tmpFile.Close(); err != nil { | ||
| os.Remove(tmp) | ||
| return | ||
| } | ||
|
|
||
| if err := os.Chmod(tmp, 0644); err != nil { | ||
| os.Remove(tmp) | ||
| return | ||
| } |
There was a problem hiding this comment.
Similar to the improvement in burnrate.go, using tmpFile.Chmod(0644) before closing the file handle is more secure than calling os.Chmod on the path after closing, as it prevents potential manipulation of the file path between the two operations.
| if err := tmpFile.Close(); err != nil { | |
| os.Remove(tmp) | |
| return | |
| } | |
| if err := os.Chmod(tmp, 0644); err != nil { | |
| os.Remove(tmp) | |
| return | |
| } | |
| if err := tmpFile.Chmod(0644); err != nil { | |
| tmpFile.Close() | |
| os.Remove(tmp) | |
| return | |
| } | |
| if err := tmpFile.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 filenames (
path + ".tmp") were used alongsideos.WriteFileandos.Renamefor atomic writes in shared directories (likeos.TempDir()).🎯 Impact: This created a race condition vulnerability to symlink attacks (CWE-377). An attacker could pre-create a symlink with the predictable name, causing the application to follow it and overwrite or corrupt an arbitrary file with the application's permissions.
🔧 Fix: Replaced the predictable string concatenation with
os.CreateTemp(filepath.Dir(path), "prefix-*.tmp"). This relies onO_EXCLat the OS level to ensure the file is newly created. The code also now explicitly appliesChmodto match the expected0644permissions (sinceCreateTempuses0600), and importantly,Close()s the file descriptor before callingos.Rename()to prevent "file in use" errors on Windows. Finally,filepath.Dir(path)is used to guarantee the temporary file exists on the same filesystem mount to preventEXDEVcross-device rename errors.✅ Verification: Verified by checking that
os.CreateTempis correctly implemented ininternal/burnrate/burnrate.goandinternal/sparkline/sparkline.go, tests passed successfully (go test ./...), and.jules/sentinel.mdhas been updated with the learning.PR created automatically by Jules for task 8992919498063563158 started by @himattm