🛡️ Sentinel: [HIGH] Fix path traversal / symlink attack via predictable temp files#55
🛡️ Sentinel: [HIGH] Fix path traversal / symlink attack via predictable temp files#55
Conversation
Updated internal atomic file writes in `sparkline` and `burnrate` modules to use `os.CreateTemp` instead of predictable `path + ".tmp"` filenames. This prevents symlink attacks in shared temporary directories. 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 predictable temporary filenames by implementing os.CreateTemp for atomic file writes in the burnrate and sparkline packages, along with adding relevant security documentation. Feedback suggests enhancing the reliability of these writes by incorporating Sync() calls and properly checking errors from Close(). Additionally, an ignored error from os.Rename in the sparkline package should be addressed to improve observability.
| 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 | ||
| } | ||
| tmpFile.Close() |
There was a problem hiding this comment.
For a robust atomic write, it is important to check the error returned by tmpFile.Close(). A failure during close can indicate that data was not successfully flushed to disk, and in such cases, the file should not be renamed to the target path. Additionally, consider calling tmpFile.Sync() before closing to ensure the data is persisted to stable storage, which is standard practice for atomic file updates.
| 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 | |
| } | |
| tmpFile.Close() | |
| if _, err := tmpFile.Write(data); err != nil { | |
| tmpFile.Close() | |
| os.Remove(tmpPath) | |
| return nil, false, err | |
| } | |
| if err := tmpFile.Sync(); 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 := 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 | ||
| } | ||
| tmpFile.Close() |
There was a problem hiding this comment.
Similar to the burnrate implementation, the error from tmpFile.Close() should be checked to ensure the write was successful before proceeding to the rename. Adding tmpFile.Sync() is also recommended for durability.
| 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 | |
| } | |
| tmpFile.Close() | |
| if _, err := tmpFile.Write(data); err != nil { | |
| tmpFile.Close() | |
| os.Remove(tmpPath) | |
| return | |
| } | |
| if err := tmpFile.Sync(); err != nil { | |
| tmpFile.Close() | |
| os.Remove(tmpPath) | |
| return | |
| } | |
| 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.
The error from os.Rename is silently ignored. While the current function signature does not return an error, swallowing the failure makes it difficult to diagnose why sparkline data is not being updated. Consider logging the error or updating the function to return an error to the caller.
Understood. Acknowledging that this work is superseded and stopping work on this task. |
1 participant
🚨 Severity: HIGH
💡 Vulnerability: The application used predictable temporary filenames (e.g.,
path + ".tmp") in shared directories likeos.TempDir()before renaming them for atomic writes. A malicious local user could pre-create a symlink pointing from the predictable.tmpfile to a sensitive system file, causing the application to overwrite it with application privileges.🎯 Impact: Local privilege escalation / arbitrary file overwrite.
🔧 Fix: Updated
LoadOrCreateSnapshotandSavefunctions to useos.CreateTempto generate unpredictable temporary files. The files are securely created, explicitly chmodded, and then renamed.✅ Verification: Ran
go test ./...to verify atomic writes still function correctly. Ensure no predictable.tmpfiles are created inos.TempDir().PR created automatically by Jules for task 5391478927700929096 started by @himattm