🛡️ Sentinel: [CRITICAL] Fix predictable /tmp path vulnerability in apt.sh#63
🛡️ Sentinel: [CRITICAL] Fix predictable /tmp path vulnerability in apt.sh#63
Conversation
…t.sh 🚨 Severity: CRITICAL 💡 Vulnerability: The script downloaded `yq` to a predictable `/tmp/yq` path before executing `sudo mv`. In world-writable directories like `/tmp`, local attackers can create symlinks or files beforehand, leading to local privilege escalation or arbitrary file overwrites when `sudo` interacts with the path. 🎯 Impact: Local privilege escalation, arbitrary file overwrite via symlink attacks. 🔧 Fix: Used `mktemp -d` to create an isolated, secure temporary directory, downloaded `yq` into it, safely performed the `sudo mv`, and securely deleted the directory using `rm -rf`. Added entry to `.jules/sentinel.md` to record the learning. ✅ Verification: Ran `bash -n tools/os_installers/apt.sh` and `./build.sh` to ensure no syntax errors or regressions were introduced. 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. |
📝 WalkthroughWalkthroughDocumentation added for a temporary directory security vulnerability in the apt.sh installer script. The vulnerability fix updates the yq binary download process to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
📝 Coding Plan
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: 3
🤖 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-4: Make the first line a top-level heading and fix heading
spacing and line lengths: change the opening line to a top-level H1 (e.g., "#
2024-05-24 - Avoid predictable /tmp files in installation scripts"), ensure
there is a blank line after that heading (MD022), then reflow the subsequent
lines so no line exceeds the repo's max (wrap the Vulnerability, Learning, and
Prevention sections to fit MD013) and keep each section as separate subheadings
or bold labels to preserve structure; edit the content in .jules/sentinel.md
accordingly and keep the same wording but split long sentences across lines to
satisfy the linters (addresses MD041, MD022, MD013).
In `@tools/os_installers/apt.sh`:
- Around line 235-236: Add integrity verification for the yq binary before
moving it to /usr/local/bin: after downloading "${YQ_VERSION}/yq_linux_amd64" to
"$tmp_dir/yq", also download the corresponding "checksums" file for that release
into "$tmp_dir/checksums", compute and compare the sha256 of "$tmp_dir/yq"
against the entry for "yq_linux_amd64" in "$tmp_dir/checksums" (fail the script
with an error message and non‑zero exit if the checksum does not match), and
only then run the privileged install step (sudo mv "$tmp_dir/yq"
/usr/local/bin/yq); reference the YQ_VERSION and tmp_dir variables and the
"$tmp_dir/yq" artifact when implementing the verification and failure path.
- Around line 234-238: After creating tmp_dir with tmp_dir=$(mktemp -d), add a
trap on EXIT to always remove it (e.g., trap 'rm -rf "$tmp_dir"' EXIT) so
failures between the wget/move/chmod steps won't leave the temp dir behind;
place the trap immediately after the mktemp line and ensure the trap references
the tmp_dir variable (and is robust if tmp_dir is empty or unset).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7569d5c4-970f-4a6d-ad91-e020367987dd
📒 Files selected for processing (2)
.jules/sentinel.mdtools/os_installers/apt.sh
| ## 2024-05-24 - [Avoid predictable /tmp files in installation scripts] | ||
| **Vulnerability:** In `tools/os_installers/apt.sh`, the `yq` binary was downloaded directly to a predictable path `/tmp/yq` before being moved with `sudo` to a global path. This can expose the system to symlink attacks or local privilege escalation since `/tmp` is a world-writable directory. | ||
| **Learning:** Hardcoding paths in `/tmp` for temporary files, especially those executed or moved with `sudo` privileges, introduces a vulnerability where malicious users could preemptively create a symlink or a file at that location. | ||
| **Prevention:** Always use `mktemp -d` to create securely generated, isolated temporary directories (e.g. `tmp_dir=$(mktemp -d)`) and use these directories to store downloaded artifacts, ensuring they are deleted via `rm -rf` afterwards. |
There was a problem hiding this comment.
Fix markdownlint blockers (MD041, MD022, MD013) in this new file.
CI currently fails because Line 1 is not a top-level heading, heading spacing is missing, and Lines 2–4 exceed max line length.
📝 Suggested lint-compliant rewrite
-## 2024-05-24 - [Avoid predictable /tmp files in installation scripts]
-**Vulnerability:** In `tools/os_installers/apt.sh`, the `yq` binary was downloaded directly to a predictable path `/tmp/yq` before being moved with `sudo` to a global path. This can expose the system to symlink attacks or local privilege escalation since `/tmp` is a world-writable directory.
-**Learning:** Hardcoding paths in `/tmp` for temporary files, especially those executed or moved with `sudo` privileges, introduces a vulnerability where malicious users could preemptively create a symlink or a file at that location.
-**Prevention:** Always use `mktemp -d` to create securely generated, isolated temporary directories (e.g. `tmp_dir=$(mktemp -d)`) and use these directories to store downloaded artifacts, ensuring they are deleted via `rm -rf` afterwards.
+# Sentinel Security Learnings
+
+## 2024-05-24 - Avoid predictable /tmp files in installation scripts
+
+**Vulnerability:** In `tools/os_installers/apt.sh`, the `yq` binary was
+downloaded to predictable `/tmp/yq` before `sudo` move to a global path.
+This enables symlink attacks and local privilege escalation in `/tmp`.
+
+**Learning:** Hardcoding `/tmp` paths for temporary files—especially files
+executed or moved with `sudo`—allows pre-creation of malicious files or
+symlinks by local users.
+
+**Prevention:** Use `mktemp -d` to create isolated temp directories
+(`tmp_dir=$(mktemp -d)`), store artifacts there, and delete them with
+`rm -rf` afterward.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2024-05-24 - [Avoid predictable /tmp files in installation scripts] | |
| **Vulnerability:** In `tools/os_installers/apt.sh`, the `yq` binary was downloaded directly to a predictable path `/tmp/yq` before being moved with `sudo` to a global path. This can expose the system to symlink attacks or local privilege escalation since `/tmp` is a world-writable directory. | |
| **Learning:** Hardcoding paths in `/tmp` for temporary files, especially those executed or moved with `sudo` privileges, introduces a vulnerability where malicious users could preemptively create a symlink or a file at that location. | |
| **Prevention:** Always use `mktemp -d` to create securely generated, isolated temporary directories (e.g. `tmp_dir=$(mktemp -d)`) and use these directories to store downloaded artifacts, ensuring they are deleted via `rm -rf` afterwards. | |
| # Sentinel Security Learnings | |
| ## 2024-05-24 - Avoid predictable /tmp files in installation scripts | |
| **Vulnerability:** In `tools/os_installers/apt.sh`, the `yq` binary was | |
| downloaded to predictable `/tmp/yq` before `sudo` move to a global path. | |
| This enables symlink attacks and local privilege escalation in `/tmp`. | |
| **Learning:** Hardcoding `/tmp` paths for temporary files—especially files | |
| executed or moved with `sudo`—allows pre-creation of malicious files or | |
| symlinks by local users. | |
| **Prevention:** Use `mktemp -d` to create isolated temp directories | |
| (`tmp_dir=$(mktemp -d)`), store artifacts there, and delete them with | |
| `rm -rf` afterward. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 237] 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: 233] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
[failure] 2-2: Line length
.jules/sentinel.md:2:81 MD013/line-length Line length [Expected: 80; Actual: 292] 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-05-24 - [Avoid predict..."] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md041.md
[failure] 1-1: Headings should be surrounded by blank lines
.jules/sentinel.md:1 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## 2024-05-24 - [Avoid predictable /tmp files in installation scripts]"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
🪛 LanguageTool
[locale-violation] ~4-~4: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... ensuring they are deleted via rm -rf afterwards.
(AFTERWARDS_US)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/sentinel.md around lines 1 - 4, Make the first line a top-level
heading and fix heading spacing and line lengths: change the opening line to a
top-level H1 (e.g., "# 2024-05-24 - Avoid predictable /tmp files in installation
scripts"), ensure there is a blank line after that heading (MD022), then reflow
the subsequent lines so no line exceeds the repo's max (wrap the Vulnerability,
Learning, and Prevention sections to fit MD013) and keep each section as
separate subheadings or bold labels to preserve structure; edit the content in
.jules/sentinel.md accordingly and keep the same wording but split long
sentences across lines to satisfy the linters (addresses MD041, MD022, MD013).
| tmp_dir=$(mktemp -d) | ||
| wget "https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_linux_amd64" -O "$tmp_dir/yq" | ||
| sudo mv "$tmp_dir/yq" /usr/local/bin/yq | ||
| sudo chmod +x /usr/local/bin/yq | ||
| rm -rf "$tmp_dir" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether yq installation block has trap-based cleanup around mktemp usage.
rg -n -C3 'Installing yq|mktemp -d|trap .*EXIT|rm -rf "\$tmp_dir"|wget .*yq_linux_amd64|sudo (mv|install) .*yq' tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 484
🏁 Script executed:
# Check if set -e is enabled at line 7
head -10 tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 283
🏁 Script executed:
# Get broader context around the yq installation block to check for error handling
sed -n '230,240p' tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 399
Ensure temporary directory cleanup runs on failure paths.
Because set -e is enabled (Line 7), any failure on Line 235–Line 237 exits before Line 238, leaving the temp directory behind. Add an EXIT trap right after creating tmp_dir.
💡 Suggested fix
- tmp_dir=$(mktemp -d)
+ tmp_dir="$(mktemp -d)"
+ cleanup_yq_tmp() { rm -rf "$tmp_dir"; }
+ trap cleanup_yq_tmp EXIT
wget "https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_linux_amd64" -O "$tmp_dir/yq"
sudo mv "$tmp_dir/yq" /usr/local/bin/yq
sudo chmod +x /usr/local/bin/yq
- rm -rf "$tmp_dir"
+ cleanup_yq_tmp
+ trap - EXIT🤖 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 234 - 238, After creating tmp_dir
with tmp_dir=$(mktemp -d), add a trap on EXIT to always remove it (e.g., trap
'rm -rf "$tmp_dir"' EXIT) so failures between the wget/move/chmod steps won't
leave the temp dir behind; place the trap immediately after the mktemp line and
ensure the trap references the tmp_dir variable (and is robust if tmp_dir is
empty or unset).
| wget "https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_linux_amd64" -O "$tmp_dir/yq" | ||
| sudo mv "$tmp_dir/yq" /usr/local/bin/yq |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub release yq v4.44.6, what is the official checksum (or signature) verification workflow for the yq_linux_amd64 binary, including the exact checksum file URL and verification command?
💡 Result:
The official checksum verification workflow for the yq_linux_amd64 binary in GitHub release v4.44.6 of mikefarah/yq is as follows: 1. Download the binary from: https://github.com/mikefarah/yq/releases/download/v4.44.6/yq_linux_amd64 2. Download the checksum files: - checksums: https://github.com/mikefarah/yq/releases/download/v4.44.6/checksums - checksums_hashes_order: https://github.com/mikefarah/yq/releases/download/v4.44.6/checksums_hashes_order - (optionally) extract-checksum.sh: https://github.com/mikefarah/yq/releases/download/v4.44.6/extract-checksum.sh 3. Extract the SHA-256 checksum using the provided script (recommended): ./extract-checksum.sh SHA-256 yq_linux_amd64 This outputs the expected SHA-256 hash for the binary (e.g., "acebc9d07aa2d0e482969b2c080ee306e8f58efbd6f2d857eefbce6469da1473" from example usage). 4. Verify the checksum: sha256sum yq_linux_amd64 Compare the output to the extracted hash. Alternatively, pipe the extracted line to a verifier like sha256sum --check or rhash -c -. This workflow uses multiple hash algorithms in a single 'checksums' file, with 'checksums_hashes_order' defining the column order. The extract script simplifies retrieval of the specific SHA-256 value without depending on tool versions. No GPG signatures are provided; verification relies on these checksums. v4.44.6 exists as confirmed by release references and actions (Dec 2024), following the standard pattern seen in later releases like v4.52.4.
Citations:
- 1: https://github.com/mikefarah/yq/releases
- 2: https://github.com/mikefarah/yq/actions/runs/12210940780
- 3: https://github.com/mikefarah/yq/blob/master/scripts/extract-checksum.sh
- 4: Please publish file hashes with releases mikefarah/yq#801
- 5:
rhashis failing verification on the new release mikefarah/yq#1011
🏁 Script executed:
# Check the file size and examine the relevant lines and context
wc -l tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 92
🏁 Script executed:
# Read the context around lines 235-236 to see YQ_VERSION and any verification
sed -n '220,250p' tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 948
🏁 Script executed:
# Search for any checksum or signature verification in the entire file
grep -n "checksum\|signature\|sha256\|md5\|gpg\|verify" tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 1484
Add checksum verification for yq binary before installation.
Line 235 downloads yq binary from GitHub and line 236 installs it to /usr/local/bin with sudo without integrity verification. While this script implements verification for other packages (Composer, Docker, Yarn, etc.), yq installation skips this critical step.
The yq project provides official checksums at: https://github.com/mikefarah/yq/releases/download/v4.44.6/checksums
Verify the downloaded binary before privileged installation:
wget "https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/checksums" -O "$tmp_dir/checksums"
sha256sum "$tmp_dir/yq" | grep -f <(grep yq_linux_amd64 "$tmp_dir/checksums") || { echo "Checksum mismatch"; exit 1; }
🤖 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 235 - 236, Add integrity
verification for the yq binary before moving it to /usr/local/bin: after
downloading "${YQ_VERSION}/yq_linux_amd64" to "$tmp_dir/yq", also download the
corresponding "checksums" file for that release into "$tmp_dir/checksums",
compute and compare the sha256 of "$tmp_dir/yq" against the entry for
"yq_linux_amd64" in "$tmp_dir/checksums" (fail the script with an error message
and non‑zero exit if the checksum does not match), and only then run the
privileged install step (sudo mv "$tmp_dir/yq" /usr/local/bin/yq); reference the
YQ_VERSION and tmp_dir variables and the "$tmp_dir/yq" artifact when
implementing the verification and failure path.
1 participant
🛡️ Sentinel: [CRITICAL] Fix predictable
/tmppath vulnerability inapt.sh🚨 Severity: CRITICAL
💡 Vulnerability: The
tools/os_installers/apt.shscript previously downloaded theyqbinary directly to/tmp/yqbefore executing asudo mv /tmp/yq /usr/local/bin/yq. Because/tmpis a world-writable directory, a local attacker could pre-create a file or symlink at/tmp/yq, which might allow them to overwrite sensitive system files or execute arbitrary code when thesudocommand interacts with it.🎯 Impact: Local Privilege Escalation (LPE), Symlink Attacks, Arbitrary file overwrites.
🔧 Fix:
/tmp/yqpath with a securely generated directory usingmktemp -d.sudo mvfrom the secure directory to/usr/local/bin/yq.rm -rfto ensure the temporary directory is deleted after use..jules/sentinel.mdas per the Sentinel guidelines.✅ Verification:
bash -n tools/os_installers/apt.sh../build.shscript to verify there are no test regressions.PR created automatically by Jules for task 9215587834297415694 started by @kidchenko
Summary by CodeRabbit
Bug Fixes
Documentation