🛡️ Sentinel: [CRITICAL] Fix predictable temporary file vulnerability (symlink attack)#53
🛡️ Sentinel: [CRITICAL] Fix predictable temporary file vulnerability (symlink attack)#53
Conversation
…(symlink attack) The `tools/os_installers/apt.sh` script previously downloaded the `yq` binary to a predictable, hardcoded path `/tmp/yq`. Since `/tmp` is world-writable, this could lead to local privilege escalation via a symlink attack because `yq` was later moved to `/usr/local/bin/yq` utilizing `sudo`. Replaced the hardcoded predictable path by using `mktemp -d` to generate a secure, temporary directory, and recorded this critical security learning in `.jules/sentinel.md`. 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 addresses a security vulnerability by adding documentation of a predictable temporary file risk and updating the apt.sh script to use secure temporary directories via mktemp instead of hardcoded /tmp/yq paths during yq installation. 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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
🧹 Nitpick comments (1)
tools/os_installers/apt.sh (1)
234-238: Ensure the temp directory is cleaned up on failure paths too.With
set -e, any failure after Line 234 exits before Line 238, so the temp directory only gets removed on the success path. A scopedEXITtrap avoids leaking temp dirs and also lets you fold the move/chmod into one step.Suggested hardening
- 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" + ( + TMP_DIR="$(mktemp -d)" + trap 'rm -rf "$TMP_DIR"' EXIT + wget "https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_linux_amd64" -O "$TMP_DIR/yq" + sudo install -m 0755 "$TMP_DIR/yq" /usr/local/bin/yq + )🤖 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, The temp directory created in TMP_DIR via mktemp -d can leak on failures; add a trap on EXIT to remove "$TMP_DIR" (e.g., trap 'rm -rf "$TMP_DIR"' EXIT) immediately after TMP_DIR is created, and replace the separate sudo mv + sudo chmod steps by a single atomic install step (or ensure cleanup still runs if mv fails) so that TMP_DIR is always removed on script exit; reference TMP_DIR, mktemp -d, wget, mv and chmod in your changes.
🤖 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: The markdown fails linting: add a top-level H1 header with a
blank line before and after it, and wrap long lines to ≤80 columns (break long
sentences like the Vulnerability, Learning, and Prevention lines into multiple
shorter lines or use markdown lists) in .jules/sentinel.md; ensure inline
code/paths (e.g., `/tmp/yq`, `tools/os_installers/apt.sh`, `mktemp`) remain as
inline code and that the advisory still contains the same content but with lines
wrapped and proper header spacing.
---
Nitpick comments:
In `@tools/os_installers/apt.sh`:
- Around line 234-238: The temp directory created in TMP_DIR via mktemp -d can
leak on failures; add a trap on EXIT to remove "$TMP_DIR" (e.g., trap 'rm -rf
"$TMP_DIR"' EXIT) immediately after TMP_DIR is created, and replace the separate
sudo mv + sudo chmod steps by a single atomic install step (or ensure cleanup
still runs if mv fails) so that TMP_DIR is always removed on script exit;
reference TMP_DIR, mktemp -d, wget, mv and chmod in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 787cd36f-57d9-458c-8e92-9a5d532a2a46
📒 Files selected for processing (2)
.jules/sentinel.mdtools/os_installers/apt.sh
| ## 2024-05-18 - [Predictable Temporary File Vulnerability] | ||
| **Vulnerability:** Predictable temporary file path `/tmp/yq` used in `tools/os_installers/apt.sh` to download and install `yq` as root. | ||
| **Learning:** Hardcoding a predictable file path in the world-writable directory `/tmp` could allow an attacker to launch a symlink attack or pre-create the file to gain privilege escalation when the script later runs `sudo mv /tmp/yq /usr/local/bin/yq`. This is especially dangerous in setup scripts that may be run by different users or multiple times. | ||
| **Prevention:** Always use `mktemp` (e.g., `mktemp -d`) to create secure, unpredictable temporary directories or files when downloading artifacts or storing intermediate data, especially if they are going to be accessed by `sudo` later. No newline at end of file |
There was a problem hiding this comment.
Fix the markdownlint failures in this advisory.
This file currently fails the docs lint job: Line 1 needs an H1 with surrounding blank lines, and Lines 2-4 exceed the configured 80-column limit.
Suggested markdown cleanup
-## 2024-05-18 - [Predictable Temporary File Vulnerability]
-**Vulnerability:** Predictable temporary file path `/tmp/yq` used in `tools/os_installers/apt.sh` to download and install `yq` as root.
-**Learning:** Hardcoding a predictable file path in the world-writable directory `/tmp` could allow an attacker to launch a symlink attack or pre-create the file to gain privilege escalation when the script later runs `sudo mv /tmp/yq /usr/local/bin/yq`. This is especially dangerous in setup scripts that may be run by different users or multiple times.
-**Prevention:** Always use `mktemp` (e.g., `mktemp -d`) to create secure, unpredictable temporary directories or files when downloading artifacts or storing intermediate data, especially if they are going to be accessed by `sudo` later.
+# 2024-05-18 - Predictable Temporary File Vulnerability
+
+**Vulnerability:** Predictable temporary file path `/tmp/yq` used in
+`tools/os_installers/apt.sh` to download and install `yq` as root.
+
+**Learning:** Hardcoding a predictable file path in the world-writable
+directory `/tmp` could allow an attacker to launch a symlink attack or
+pre-create the file to gain privilege escalation when the script later runs
+`sudo mv /tmp/yq /usr/local/bin/yq`. This is especially dangerous in setup
+scripts that may be run by different users or multiple times.
+
+**Prevention:** Always use `mktemp` (e.g., `mktemp -d`) to create secure,
+unpredictable temporary directories or files when downloading artifacts or
+storing intermediate data, especially if they will be accessed by `sudo`
+later.📝 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-18 - [Predictable Temporary File Vulnerability] | |
| **Vulnerability:** Predictable temporary file path `/tmp/yq` used in `tools/os_installers/apt.sh` to download and install `yq` as root. | |
| **Learning:** Hardcoding a predictable file path in the world-writable directory `/tmp` could allow an attacker to launch a symlink attack or pre-create the file to gain privilege escalation when the script later runs `sudo mv /tmp/yq /usr/local/bin/yq`. This is especially dangerous in setup scripts that may be run by different users or multiple times. | |
| **Prevention:** Always use `mktemp` (e.g., `mktemp -d`) to create secure, unpredictable temporary directories or files when downloading artifacts or storing intermediate data, especially if they are going to be accessed by `sudo` later. | |
| # 2024-05-18 - Predictable Temporary File Vulnerability | |
| **Vulnerability:** Predictable temporary file path `/tmp/yq` used in | |
| `tools/os_installers/apt.sh` to download and install `yq` as root. | |
| **Learning:** Hardcoding a predictable file path in the world-writable | |
| directory `/tmp` could allow an attacker to launch a symlink attack or | |
| pre-create the file to gain privilege escalation when the script later runs | |
| `sudo mv /tmp/yq /usr/local/bin/yq`. This is especially dangerous in setup | |
| scripts that may be run by different users or multiple times. | |
| **Prevention:** Always use `mktemp` (e.g., `mktemp -d`) to create secure, | |
| unpredictable temporary directories or files when downloading artifacts or | |
| storing intermediate data, especially if they will be accessed by `sudo` | |
| later. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 236] 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: 354] 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: 135] 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-18 - [Predictable T..."] 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-18 - [Predictable Temporary File Vulnerability]"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
🪛 LanguageTool
[style] ~4-~4: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...g intermediate data, especially if they are going to be accessed by sudo later.
(GOING_TO_WILL)
🤖 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, The markdown fails linting: add a
top-level H1 header with a blank line before and after it, and wrap long lines
to ≤80 columns (break long sentences like the Vulnerability, Learning, and
Prevention lines into multiple shorter lines or use markdown lists) in
.jules/sentinel.md; ensure inline code/paths (e.g., `/tmp/yq`,
`tools/os_installers/apt.sh`, `mktemp`) remain as inline code and that the
advisory still contains the same content but with lines wrapped and proper
header spacing.
1 participant
tools/os_installers/apt.shscript previously downloaded theyqbinary to a predictable, hardcoded path/tmp/yq. Since/tmpis world-writable, this could lead to local privilege escalation via a symlink attack becauseyqwas later moved to/usr/local/bin/yqutilizingsudo./tmp/yqby usingmktemp -dto securely generate a random, unique temporary directory for the download. Also created.jules/sentinel.mdto document the finding../build.shand./build.sh syntaxto ensure the script parses and has no syntax errors.PR created automatically by Jules for task 11895535113787785508 started by @kidchenko
Summary by CodeRabbit
Security
Documentation