🛡️ Sentinel: [HIGH] Fix predictable /tmp path vulnerability#80
🛡️ Sentinel: [HIGH] Fix predictable /tmp path vulnerability#80
Conversation
This change resolves a vulnerability in `tools/os_installers/apt.sh` where a hardcoded, predictable temporary file path (`/tmp/yq`) was used for downloading and installing the `yq` binary. This pattern is vulnerable to local symlink attacks, allowing an attacker to overwrite arbitrary files when the script executes `sudo mv`. The fix uses `mktemp -d` inside a subshell to securely generate a randomized temporary directory and ensures cleanup via an `EXIT` trap. This prevents predictable path exploitation while handling the download and move operations securely. Co-authored-by: kidchenko <5432753+kidchenko@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. |
📝 WalkthroughWalkthroughThis PR adds security documentation for a temporary file vulnerability and implements a fix in the apt installer script by replacing a hardcoded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/os_installers/apt.sh (2)
245-250:⚠️ Potential issue | 🟠 MajorSame predictable path vulnerability exists here.
The lsd installation also downloads to the current directory before running
sudo dpkg -i. Apply the same secure pattern.🛡️ Proposed fix using secure temp directory
# Install lsd (LSDeluxe) echo "Installing lsd..." if ! command -v lsd &> /dev/null; then + ( + TMP_DIR=$(mktemp -d) + trap 'rm -rf "$TMP_DIR"' EXIT LSD_VERSION="1.1.5" - wget "https://github.com/lsd-rs/lsd/releases/download/v${LSD_VERSION}/lsd_${LSD_VERSION}_amd64.deb" - sudo dpkg -i "lsd_${LSD_VERSION}_amd64.deb" - rm "lsd_${LSD_VERSION}_amd64.deb" + wget "https://github.com/lsd-rs/lsd/releases/download/v${LSD_VERSION}/lsd_${LSD_VERSION}_amd64.deb" -O "$TMP_DIR/lsd.deb" + sudo dpkg -i "$TMP_DIR/lsd.deb" + ) fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/os_installers/apt.sh` around lines 245 - 250, The current lsd install block writes the .deb into the current directory (LSD_VERSION, wget and sudo dpkg -i "lsd_${LSD_VERSION}_amd64.deb"), creating a predictable path vulnerability; change it to create a secure temporary directory (mktemp -d), download the .deb into that temp dir using the full temp path, install it by passing the full temp file path to sudo dpkg -i, and ensure you remove the temp file/dir and use a trap for cleanup on exit or error so no artifacts remain in the working directory.
206-213:⚠️ Potential issue | 🟠 MajorSame predictable path vulnerability exists here.
The Go installation downloads to the current working directory (predictable path), then uses
sudo tar. This is the same symlink attack vector that was fixed foryq. An attacker could pre-create a symlink at the expected tarball path.Apply the same
mktemp -dpattern:🛡️ Proposed fix using secure temp directory
# Install Go echo "Installing Go..." if ! command -v go &> /dev/null; then + ( + TMP_DIR=$(mktemp -d) + trap 'rm -rf "$TMP_DIR"' EXIT GO_VERSION="1.23.4" - wget "https://go.dev/dl/go${GO_VERSION}.linux-amd64.tar.gz" + wget "https://go.dev/dl/go${GO_VERSION}.linux-amd64.tar.gz" -O "$TMP_DIR/go.tar.gz" sudo rm -rf /usr/local/go - sudo tar -C /usr/local -xzf "go${GO_VERSION}.linux-amd64.tar.gz" - rm "go${GO_VERSION}.linux-amd64.tar.gz" + sudo tar -C /usr/local -xzf "$TMP_DIR/go.tar.gz" echo "NOTE: Add 'export PATH=\$PATH:/usr/local/go/bin' to your shell profile" + ) fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/os_installers/apt.sh` around lines 206 - 213, The Go installer is vulnerable to a predictable-path symlink attack because it downloads "go${GO_VERSION}.linux-amd64.tar.gz" into the current directory and then runs sudo tar against that filename; change the flow in tools/os_installers/apt.sh to create a secure temp directory with mktemp -d, download the tarball into that temp dir (use a full temp path), run sudo tar -C /usr/local -xzf against the temp-file path, then securely remove the temp file/dir; update the block that sets GO_VERSION and calls wget/tar/rm to use the temp dir pattern and ensure all references to the tarball use the temp path rather than a predictable filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.jules/sentinel.md:
- Around line 1-5: The markdown fails lint rules MD041 and MD013: change the
existing second-level heading "## 2024-04-05 - Predictable Temporary File Path
in Installation Scripts" to a top-level heading (prefix with a single "#") to
satisfy MD041, and reflow / wrap the long lines in the description and bullet
list (the lines under "Vulnerability", "Learning", "Prevention" and the numbered
lint failures) to 80 characters or less to satisfy MD013; preserve the same
content and markup (headings, bold labels, list structure) but break long
sentences into shorter lines or additional short paragraphs so the file passes
the linter.
---
Outside diff comments:
In `@tools/os_installers/apt.sh`:
- Around line 245-250: The current lsd install block writes the .deb into the
current directory (LSD_VERSION, wget and sudo dpkg -i
"lsd_${LSD_VERSION}_amd64.deb"), creating a predictable path vulnerability;
change it to create a secure temporary directory (mktemp -d), download the .deb
into that temp dir using the full temp path, install it by passing the full temp
file path to sudo dpkg -i, and ensure you remove the temp file/dir and use a
trap for cleanup on exit or error so no artifacts remain in the working
directory.
- Around line 206-213: The Go installer is vulnerable to a predictable-path
symlink attack because it downloads "go${GO_VERSION}.linux-amd64.tar.gz" into
the current directory and then runs sudo tar against that filename; change the
flow in tools/os_installers/apt.sh to create a secure temp directory with mktemp
-d, download the tarball into that temp dir (use a full temp path), run sudo tar
-C /usr/local -xzf against the temp-file path, then securely remove the temp
file/dir; update the block that sets GO_VERSION and calls wget/tar/rm to use the
temp dir pattern and ensure all references to the tarball use the temp path
rather than a predictable filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 209eca80-f0fb-4b25-865b-3398a4e0fc13
📒 Files selected for processing (2)
.jules/sentinel.mdtools/os_installers/apt.sh
| ## 2024-04-05 - Predictable Temporary File Path in Installation Scripts | ||
|
|
||
| **Vulnerability:** Predictable temporary file paths (`/tmp/yq`) are used for downloading files before moving them to privileged directories (`/usr/local/bin`) in `tools/os_installers/apt.sh`. | ||
| **Learning:** Hardcoding a predictable file path in `/tmp` makes the script vulnerable to symlink attacks. An attacker could pre-create `/tmp/yq` as a symlink pointing to a sensitive file, causing the subsequent `sudo mv` command to overwrite the target file or potentially leading to arbitrary code execution if the downloaded binary is a payload that gets moved to a privileged path. | ||
| **Prevention:** Always use secure, randomized temporary directories using `mktemp -d` when downloading or processing temporary files. Avoid hardcoding predictable file paths, especially when mixing unprivileged commands (`wget`) with privileged commands (`sudo`). Use a local cleanup trap inside a subshell for temporary directory removal. |
There was a problem hiding this comment.
Fix markdown lint failures.
Static analysis flagged several issues:
- Line 1: Should start with a top-level heading (
#) per MD041 - Lines 3-5: Exceed 80 character line length per MD013
📝 Proposed fix for lint compliance
+# Sentinel Security Documentation
+
-## 2024-04-05 - Predictable Temporary File Path in Installation Scripts
+## 2024-04-05 - Predictable Temporary File Path in Installation Scripts
-**Vulnerability:** Predictable temporary file paths (`/tmp/yq`) are used for downloading files before moving them to privileged directories (`/usr/local/bin`) in `tools/os_installers/apt.sh`.
-**Learning:** Hardcoding a predictable file path in `/tmp` makes the script vulnerable to symlink attacks. An attacker could pre-create `/tmp/yq` as a symlink pointing to a sensitive file, causing the subsequent `sudo mv` command to overwrite the target file or potentially leading to arbitrary code execution if the downloaded binary is a payload that gets moved to a privileged path.
-**Prevention:** Always use secure, randomized temporary directories using `mktemp -d` when downloading or processing temporary files. Avoid hardcoding predictable file paths, especially when mixing unprivileged commands (`wget`) with privileged commands (`sudo`). Use a local cleanup trap inside a subshell for temporary directory removal.
+**Vulnerability:** Predictable temporary file paths (`/tmp/yq`) are used for
+downloading files before moving them to privileged directories
+(`/usr/local/bin`) in `tools/os_installers/apt.sh`.
+
+**Learning:** Hardcoding a predictable file path in `/tmp` makes the script
+vulnerable to symlink attacks. An attacker could pre-create `/tmp/yq` as a
+symlink pointing to a sensitive file, causing the subsequent `sudo mv` command
+to overwrite the target file or potentially leading to arbitrary code execution
+if the downloaded binary is a payload that gets moved to a privileged path.
+
+**Prevention:** Always use secure, randomized temporary directories using
+`mktemp -d` when downloading or processing temporary files. Avoid hardcoding
+predictable file paths, especially when mixing unprivileged commands (`wget`)
+with privileged commands (`sudo`). Use a local cleanup trap inside a subshell
+for temporary directory removal.🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 5-5: Line length
.jules/sentinel.md:5:81 MD013/line-length Line length [Expected: 80; Actual: 339] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 385] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 3-3: Line length
.jules/sentinel.md:3:81 MD013/line-length Line length [Expected: 80; Actual: 191] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 1-1: First line in a file should be a top-level heading
.jules/sentinel.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "## 2024-04-05 - Predictable Te..."] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md041.md
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/sentinel.md around lines 1 - 5, The markdown fails lint rules MD041
and MD013: change the existing second-level heading "## 2024-04-05 - Predictable
Temporary File Path in Installation Scripts" to a top-level heading (prefix with
a single "#") to satisfy MD041, and reflow / wrap the long lines in the
description and bullet list (the lines under "Vulnerability", "Learning",
"Prevention" and the numbered lint failures) to 80 characters or less to satisfy
MD013; preserve the same content and markup (headings, bold labels, list
structure) but break long sentences into shorter lines or additional short
paragraphs so the file passes the linter.
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Predictable temporary file path (
/tmp/yq) intools/os_installers/apt.shwas used with a mixture of unprivileged (wget) and privileged (sudo mv) commands, creating a local privilege escalation/symlink attack vector.🎯 Impact: A local attacker could pre-create a symlink at
/tmp/yqpointing to an arbitrary sensitive file, causing it to be overwritten or compromised when the script runs.🔧 Fix: Replaced the hardcoded path with a securely generated, randomized temporary directory using
mktemp -d. The process is wrapped in a subshell with a localizedEXITtrap to ensure reliable cleanup.✅ Verification: Validated the updated script syntax using
./build.sh lint. The updated installation process isolates the download and execution context cleanly.PR created automatically by Jules for task 12059556955293892260 started by @kidchenko
Summary by CodeRabbit
Bug Fixes
Documentation