🛡️ Sentinel: [CRITICAL] Fix symlink vulnerability in atomic file writes#82
🛡️ Sentinel: [CRITICAL] Fix symlink vulnerability in atomic file writes#82
Conversation
Replaced predictable temporary file creation with os.CreateTemp in internal/sparkline/sparkline.go, internal/burnrate/burnrate.go, and internal/update/update.go. This prevents potential symlink attacks where an attacker could pre-create a predictable temp file as a symlink to overwrite an arbitrary target. 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 mitigates symlink vulnerabilities during atomic file writes by transitioning from predictable temporary filenames to os.CreateTemp in the burnrate, sparkline, and update packages. A new sentinel document is also included to document this security practice. The review feedback identifies several instances where errors from Close() and Chmod() are ignored, which could result in corrupted files being finalized. It is recommended to handle these errors and use Sync() to ensure data durability before renaming.
| _, err = io.Copy(out, resp.Body) | ||
| out.Close() |
There was a problem hiding this comment.
The error from out.Close() is ignored, which can mask failures during the final flush of the downloaded binary to disk. If io.Copy succeeds but Close() fails (e.g., due to insufficient disk space), the current logic will proceed to install a potentially truncated or corrupted binary. You should capture and check the error from Close().
_, err = io.Copy(out, resp.Body)
if cerr := out.Close(); err == nil {
err = cerr
}| if _, err := tmpFile.Write(data); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
| tmpFile.Chmod(0644) | ||
| tmpFile.Close() |
There was a problem hiding this comment.
The errors from tmpFile.Chmod(0644) and tmpFile.Close() are currently ignored. For reliable atomic writes, it is important to check the error returned by Close() because this is when the operating system flushes remaining data to disk and reports any write failures (e.g., disk full). Additionally, for true durability, consider calling tmpFile.Sync() before closing to ensure the data is persisted to the physical storage before the rename operation occurs.
if _, err := tmpFile.Write(data); err != nil {
tmpFile.Close()
os.Remove(tmpPath)
return nil, false, err
}
tmpFile.Chmod(0644)
if err := tmpFile.Close(); err != nil {
os.Remove(tmpPath)
return nil, false, err
}| if _, err := tmpFile.Write(data); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmp) | ||
| return | ||
| } | ||
| os.Rename(tmp, path) | ||
| tmpFile.Chmod(0644) | ||
| tmpFile.Close() |
There was a problem hiding this comment.
The errors from tmpFile.Chmod(0644) and tmpFile.Close() are ignored. Checking the Close() error is essential to ensure that all data has been successfully flushed to disk. Without this check, the subsequent os.Rename might succeed even if the file content is incomplete or corrupted due to a flush failure.
| if _, err := tmpFile.Write(data); err != nil { | |
| tmpFile.Close() | |
| os.Remove(tmp) | |
| return | |
| } | |
| os.Rename(tmp, path) | |
| tmpFile.Chmod(0644) | |
| tmpFile.Close() | |
| if _, err := tmpFile.Write(data); err != nil { | |
| tmpFile.Close() | |
| os.Remove(tmp) | |
| return | |
| } | |
| tmpFile.Chmod(0644) | |
| if err := tmpFile.Close(); err != nil { | |
| os.Remove(tmp) | |
| return | |
| } |
himattm
deleted the
sentinel/fix-symlink-vuln-atomic-writes-9415786252766909468
branch
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Insecure Temporary File Creation (CWE-377). Predictable temporary filenames (e.g.,
.tmpor.new) were used prior toos.Renamefor atomic file writes.🎯 Impact: An attacker with local access could pre-create these predictable paths as symlinks to arbitrary files, causing the application to overwrite them when running with elevated privileges.
🔧 Fix: Replaced predictable file creation logic with
os.CreateTempto safely generate unpredictable temporary filenames inside the correct target directory.✅ Verification: Code compiles, and
go test ./...passes successfully. Changes were reviewed and approved.PR created automatically by Jules for task 9415786252766909468 started by @himattm