🛡️ Sentinel: [HIGH] Fix symlink attack vulnerability in atomic file writes#58
🛡️ Sentinel: [HIGH] Fix symlink attack vulnerability in atomic file writes#58
Conversation
…rites This commit fixes a symlink attack vulnerability in `internal/sparkline/sparkline.go` and `internal/burnrate/burnrate.go`. The previous implementation used a predictable temporary file name (`path + ".tmp"`) with `os.WriteFile` before calling `os.Rename`. This could allow an attacker to pre-create a symlink at the predictable path in a shared directory (like `os.TempDir()`), causing the application to overwrite an arbitrary sensitive file when performing the atomic write. The issue has been resolved by replacing `os.WriteFile` with `os.CreateTemp` to generate an unpredictable temporary file name. Additional care was taken to explicitly set permissions using `f.Chmod(0644)` to match the original behavior (since `os.CreateTemp` defaults to `0600`) and to explicitly close the file handle (`f.Close()`) before calling `os.Rename` to prevent lock issues on Windows. A deferred `os.Remove(tmp)` ensures cleanup of the temporary file in case the rename operation fails. 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 symlink attack vulnerabilities in internal/burnrate/burnrate.go and internal/sparkline/sparkline.go by implementing os.CreateTemp for atomic file writes. The review feedback highlights a security concern where file permissions are relaxed before data is written, potentially leading to information disclosure in shared directories, and recommends adding f.Sync() for better data durability.
| // Match original permissions (os.CreateTemp defaults to 0600) | ||
| if err := f.Chmod(0644); err != nil { | ||
| f.Close() | ||
| return nil, false, err | ||
| } | ||
|
|
||
| if _, err := f.Write(data); err != nil { | ||
| f.Close() | ||
| return nil, false, err | ||
| } |
There was a problem hiding this comment.
In shared directories like /tmp, calling f.Chmod(0644) before writing data allows other users to read the file's contents while they are being written. It is more secure to keep the default 0600 permissions during the write and only change them to 0644 after the data is successfully written and synced. Additionally, calling f.Sync() ensures that the data is persisted to disk before the rename, which is a best practice for atomic file writes to prevent data loss or corruption during a system crash.
| // Match original permissions (os.CreateTemp defaults to 0600) | ||
| if err := f.Chmod(0644); err != nil { | ||
| f.Close() | ||
| return | ||
| } | ||
|
|
||
| if _, err := f.Write(data); err != nil { | ||
| f.Close() | ||
| return | ||
| } |
There was a problem hiding this comment.
Similar to the change in burnrate.go, the file permissions should be updated only after the data has been written and synced. This prevents other users in a shared directory from reading the file while it is in an incomplete state. Adding f.Sync() is also recommended to ensure durability before the atomic rename.
himattm
deleted the
sentinel/fix-symlink-attack-atomic-writes-8487149965762085523
branch
Understood. Acknowledging that this work is now obsolete as it has been superseded by a direct commit on main, and stopping work on this task. |
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Symlink attack during atomic file writes in shared directories. The code used predictable temporary file names (e.g.,
path + ".tmp") withos.WriteFilebefore callingos.Rename.🎯 Impact: An attacker could pre-create a symlink at the predictable path pointing to a sensitive file. When the application performs the atomic write, it would follow the symlink and overwrite the sensitive file with the new data.
🔧 Fix: Used
os.CreateTempto generate an unpredictable temporary file name. Replicated the original permissions by explicitly callingf.Chmod(0644). Explicitly closed the file usingf.Close()before renaming to avoid Windows locking issues. Useddefer os.Remove(tmp)for cleanup if the process fails.✅ Verification:
go test ./...andgo fmt ./...completed successfully. Modifications in.jules/sentinel.mdproperly document the vulnerability and its prevention.PR created automatically by Jules for task 8487149965762085523 started by @himattm