🛡️ Sentinel: [HIGH] Fix insecure downloads in apt.sh#66
🛡️ Sentinel: [HIGH] Fix insecure downloads in apt.sh#66
Conversation
Replaced predictable temporary paths and current working directory downloads with securely generated unique directories using `mktemp -d` to prevent local privilege escalation and unintended overwrites. Recorded the 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. |
📝 WalkthroughWalkthroughA security vulnerability fix is documented and implemented: the 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)
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
🧹 Nitpick comments (1)
tools/os_installers/apt.sh (1)
208-212: Usetrapto guarantee temp-dir cleanup on early failure.If
wget,tar,mv,dpkg, orphpfails,set -eexits beforerm -rf "$TMP_DIR"and leaves temp artifacts behind. Consider a small helper withtrapfor guaranteed cleanup.Also applies to: 235-239, 246-249, 259-270
🤖 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 208 - 212, The temp-dir created with TMP_DIR=$(mktemp -d) can be left behind if commands fail because set -e exits before rm -rf "$TMP_DIR"; add a cleanup helper and a trap so the directory is always removed on EXIT. After creating TMP_DIR in the blocks that download Go (and the similar blocks at lines 235-239, 246-249, 259-270), register a trap like: create a cleanup_temp_dir function that removes "$TMP_DIR" and unset the trap after successful cleanup, then call trap cleanup_temp_dir EXIT immediately after TMP_DIR is set so wget/tar/dpkg/php failures still trigger cleanup; reference TMP_DIR, mktemp -d, and the download/extract sequence to locate where to add the trap.
🤖 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: Update the markdown to satisfy MD041/MD022/MD013 and fix the
typo: replace the current level-2 heading with a top-level H1 using the exact
heading text "2024-05-18 - Prevented Insecure Downloads in apt.sh", ensure there
is a single blank line after the heading, reflow long lines to a sensible wrap
width (e.g., 80 chars) so no line exceeds the lint limit, and correct the typo
"predictible" to "predictable" throughout the document.
In `@tools/os_installers/apt.sh`:
- Around line 209-211: The downloaded Go archive (wget
"...go${GO_VERSION}.linux-amd64.tar.gz" to "$TMP_DIR") must have integrity
verified before running privileged extraction (sudo tar -C /usr/local -xzf ...);
modify the installation flow to fetch the official checksum/signature for the Go
release (and do the same for other artifacts like yq/lsd), verify the archive in
"$TMP_DIR" using shasum/sha256sum or gpg against the vendor-supplied
checksum/signature, and abort with an error if verification fails; only after
successful verification perform sudo rm -rf /usr/local/go and sudo tar -C
/usr/local -xzf "$TMP_DIR/go${GO_VERSION}.linux-amd64.tar.gz".
- Around line 268-270: The Composer checksum failure branch currently echoes an
error and removes TMP_DIR but does not stop execution; update the Composer
checksum mismatch handling (the block that echoes "ERROR: Invalid installer
checksum for Composer" and calls rm -rf "$TMP_DIR") to exit with a non-zero
status after cleanup (e.g., call exit 1) so the script fails fast on checksum
verification errors and does not continue as if successful.
---
Nitpick comments:
In `@tools/os_installers/apt.sh`:
- Around line 208-212: The temp-dir created with TMP_DIR=$(mktemp -d) can be
left behind if commands fail because set -e exits before rm -rf "$TMP_DIR"; add
a cleanup helper and a trap so the directory is always removed on EXIT. After
creating TMP_DIR in the blocks that download Go (and the similar blocks at lines
235-239, 246-249, 259-270), register a trap like: create a cleanup_temp_dir
function that removes "$TMP_DIR" and unset the trap after successful cleanup,
then call trap cleanup_temp_dir EXIT immediately after TMP_DIR is set so
wget/tar/dpkg/php failures still trigger cleanup; reference TMP_DIR, mktemp -d,
and the download/extract sequence to locate where to add the trap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3f404ad-b16d-422b-aa88-ccac3ecb54c0
📒 Files selected for processing (2)
.jules/sentinel.mdtools/os_installers/apt.sh
| ## 2024-05-18 - Prevented Insecure Downloads in apt.sh | ||
| **Vulnerability:** Predictable temporary paths (e.g. `/tmp/yq`) and writing directly to the current working directory. | ||
| **Learning:** Hardcoding predictible paths and downloading directly to the root/current working directory opens the system to path traversal, privilege escalation, and unintended overwrites if the file is replaced with a malicious artifact or overwritten by another concurrent process. | ||
| **Prevention:** Use `mktemp -d` to generate securely isolated, unique directories before writing any fetched artifacts to disk, and use `rm -rf` to clean them up afterward. No newline at end of file |
There was a problem hiding this comment.
Fix markdownlint failures (heading level, blank line, line length) and typo.
Current lines violate MD041/MD022/MD013 and will keep docs lint red. Reflowing and using an H1 resolves all reported issues.
📝 Suggested patch
-## 2024-05-18 - Prevented Insecure Downloads in apt.sh
-**Vulnerability:** Predictable temporary paths (e.g. `/tmp/yq`) and writing directly to the current working directory.
-**Learning:** Hardcoding predictible paths and downloading directly to the root/current working directory opens the system to path traversal, privilege escalation, and unintended overwrites if the file is replaced with a malicious artifact or overwritten by another concurrent process.
-**Prevention:** Use `mktemp -d` to generate securely isolated, unique directories before writing any fetched artifacts to disk, and use `rm -rf` to clean them up afterward.
+# 2024-05-18 - Prevented Insecure Downloads in apt.sh
+
+**Vulnerability:** Predictable temporary paths (e.g. `/tmp/yq`) and writing
+directly to the current working directory.
+
+**Learning:** Hardcoding predictable paths and downloading directly to the
+root/current working directory opens the system to path traversal, privilege
+escalation, and unintended overwrites if the file is replaced with a malicious
+artifact or overwritten by another concurrent process.
+
+**Prevention:** Use `mktemp -d` to generate securely isolated, unique
+directories before writing any fetched artifacts to disk, and use `rm -rf` to
+clean them up 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-18 - Prevented Insecure Downloads in apt.sh | |
| **Vulnerability:** Predictable temporary paths (e.g. `/tmp/yq`) and writing directly to the current working directory. | |
| **Learning:** Hardcoding predictible paths and downloading directly to the root/current working directory opens the system to path traversal, privilege escalation, and unintended overwrites if the file is replaced with a malicious artifact or overwritten by another concurrent process. | |
| **Prevention:** Use `mktemp -d` to generate securely isolated, unique directories before writing any fetched artifacts to disk, and use `rm -rf` to clean them up afterward. | |
| # 2024-05-18 - Prevented Insecure Downloads in apt.sh | |
| **Vulnerability:** Predictable temporary paths (e.g. `/tmp/yq`) and writing | |
| directly to the current working directory. | |
| **Learning:** Hardcoding predictable paths and downloading directly to the | |
| root/current working directory opens the system to path traversal, privilege | |
| escalation, and unintended overwrites if the file is replaced with a malicious | |
| artifact or overwritten by another concurrent process. | |
| **Prevention:** Use `mktemp -d` to generate securely isolated, unique | |
| directories before writing any fetched artifacts to disk, and use `rm -rf` to | |
| clean them up afterward. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 172] 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: 285] 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: 118] 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 - Prevented Inse..."] 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 - Prevented Insecure Downloads in apt.sh"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.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 - 4, Update the markdown to satisfy
MD041/MD022/MD013 and fix the typo: replace the current level-2 heading with a
top-level H1 using the exact heading text "2024-05-18 - Prevented Insecure
Downloads in apt.sh", ensure there is a single blank line after the heading,
reflow long lines to a sensible wrap width (e.g., 80 chars) so no line exceeds
the lint limit, and correct the typo "predictible" to "predictable" throughout
the document.
| wget "https://go.dev/dl/go${GO_VERSION}.linux-amd64.tar.gz" -O "$TMP_DIR/go${GO_VERSION}.linux-amd64.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${GO_VERSION}.linux-amd64.tar.gz" |
There was a problem hiding this comment.
Add integrity verification before privileged install/extract.
These artifacts are downloaded and then installed/extracted with sudo, but there’s no checksum/signature validation for Go/yq/lsd. A compromised upstream/release channel would execute attacker-controlled content as root.
🔐 Suggested pattern (apply to each downloaded artifact)
TMP_DIR=$(mktemp -d)
wget "https://go.dev/dl/go${GO_VERSION}.linux-amd64.tar.gz" -O "$TMP_DIR/go${GO_VERSION}.linux-amd64.tar.gz"
+GO_SHA256="REPLACE_WITH_OFFICIAL_SHA256"
+echo "${GO_SHA256} $TMP_DIR/go${GO_VERSION}.linux-amd64.tar.gz" | sha256sum -c -
sudo rm -rf /usr/local/go
sudo tar -C /usr/local -xzf "$TMP_DIR/go${GO_VERSION}.linux-amd64.tar.gz"
rm -rf "$TMP_DIR"Also applies to: 236-238, 247-248
🤖 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 209 - 211, The downloaded Go archive
(wget "...go${GO_VERSION}.linux-amd64.tar.gz" to "$TMP_DIR") must have integrity
verified before running privileged extraction (sudo tar -C /usr/local -xzf ...);
modify the installation flow to fetch the official checksum/signature for the Go
release (and do the same for other artifacts like yq/lsd), verify the archive in
"$TMP_DIR" using shasum/sha256sum or gpg against the vendor-supplied
checksum/signature, and abort with an error if verification fails; only after
successful verification perform sudo rm -rf /usr/local/go and sudo tar -C
/usr/local -xzf "$TMP_DIR/go${GO_VERSION}.linux-amd64.tar.gz".
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | ||
| rm composer-setup.php | ||
| rm -rf "$TMP_DIR" | ||
| fi |
There was a problem hiding this comment.
Fail fast when Composer checksum verification fails.
On checksum mismatch, the script logs an error and continues. That returns a misleading successful run state and hides a security-critical failure. Exit non-zero after cleanup.
✅ Minimal fix
else
>&2 echo 'ERROR: Invalid installer checksum for Composer'
rm -rf "$TMP_DIR"
+ exit 1
fi📝 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.
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | |
| rm composer-setup.php | |
| rm -rf "$TMP_DIR" | |
| fi | |
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | |
| rm -rf "$TMP_DIR" | |
| exit 1 | |
| 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 268 - 270, The Composer checksum
failure branch currently echoes an error and removes TMP_DIR but does not stop
execution; update the Composer checksum mismatch handling (the block that echoes
"ERROR: Invalid installer checksum for Composer" and calls rm -rf "$TMP_DIR") to
exit with a non-zero status after cleanup (e.g., call exit 1) so the script
fails fast on checksum verification errors and does not continue as if
successful.
1 participant
🚨 Severity: HIGH
💡 Vulnerability: The
apt.shinstaller script downloaded executable binaries directly to predictable temporary paths like/tmp/yqor straight to the current working directory, which opened the system up to path traversal, privilege escalation, and unintended file clobbering by malicious actors or concurrent processes during the OS setup phase.🎯 Impact: If exploited during execution, a local attacker could create malicious symlinks at predictable paths or replace downloaded binaries in the current directory, resulting in arbitrary code execution with potentially elevated privileges since parts of
apt.shusesudo.🔧 Fix: Changed the download and extraction processes for
yq,Go,lsd, andComposerintools/os_installers/apt.shto construct, utilize, and clean up isolated, randomly generated temporary directories usingmktemp -d. Recorded this security context in the.jules/sentinel.mdjournal.✅ Verification: Verified by executing the full validation suite via
./build.shto ensure no syntax issues or build errors were introduced.PR created automatically by Jules for task 17657335695860221003 started by @kidchenko
Summary by CodeRabbit