🛡️ Sentinel: [CRITICAL] Fix local privilege escalation vulnerability in apt.sh#81
🛡️ Sentinel: [CRITICAL] Fix local privilege escalation vulnerability in apt.sh#81
Conversation
…in apt.sh Replaced unsafe downloads to predictable /tmp paths and current directory with secure mktemp -d subshells. 🚨 Severity: CRITICAL 💡 Vulnerability: Installation scripts (apt.sh) downloaded executable artifacts to predictable locations (e.g., /tmp/yq) and current working directory. 🎯 Impact: Predictable /tmp paths introduce symlink attack vulnerabilities and local privilege escalation when run via sudo. Downloads to CWD risk overwriting local files or executing unverified binaries. 🔧 Fix: Updated the go, yq, lsd, and composer downloads to use a dynamically generated, isolated directory created with `mktemp -d`, wrapped securely within a subshell with a cleanup trap (`trap 'rm -rf "$TMP_DIR"' EXIT`). ✅ Verification: `./build.sh` executed successfully ensuring no regressions. Reading `apt.sh` confirms the subshells correctly isolate the new download logic. 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. |
📝 WalkthroughWalkthroughSecurity vulnerabilities in installation scripts are addressed through safer temporary directory handling. A new documentation entry records the symlink attack risks associated with predictable artifact paths, and the apt.sh installer is updated to use mktemp-generated random directories with trap-based cleanup. 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: 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: Fix the markdownlint errors in .jules/sentinel.md by
adjusting the heading and spacing: change the existing heading "2024-04-06 -
Unsafe File Downloads in Installation Scripts" to an appropriate top-level or
consistent heading level (e.g., prefix with a single '#'), ensure there is a
blank line before and after every heading to satisfy MD022/MD041, and wrap or
hard-break long lines (the vulnerability/learning/prevention paragraphs) to a
reasonable width (MD013, ~80 chars) so no lines exceed the limit; keep the
existing content and detail block structure but reflow text and add the required
blank lines around headings and lists.
In `@tools/os_installers/apt.sh`:
- Around line 276-280: The else branch that handles the Composer checksum
mismatch currently only writes an error but continues; update the block that
compares EXPECTED_CHECKSUM and ACTUAL_CHECKSUM so that on mismatch it prints the
error and then exits with a non-zero status (e.g., exit 1) to fail fast and
prevent reporting a successful install; modify the conditional around the
composer install (composer-setup.php invocation) to ensure only the
successful-check path executes and the mismatch path terminates immediately.
- Around line 211-214: Add SHA256 checksum verification before any privileged
install for Go, yq, and lsd: introduce variables like GO_SHA256 and LSD_SHA256
(and a yq checksum variable) and download corresponding .sha256 or checksum
values immediately after wget for the artifacts referenced by GO_VERSION, yq
download block, and lsd .deb download; then run sha256sum -c (or equivalent) to
verify the downloaded files before running sudo tar -xzf /usr/local installation
or sudo dpkg -i, and fail the script if verification fails—follow the existing
Composer pattern that uses sha256sum -c to validate downloads.
🪄 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: 6c674da0-b34e-45cd-8a3d-9cc2fbcd5adf
📒 Files selected for processing (2)
.jules/sentinel.mdtools/os_installers/apt.sh
| ## 2024-04-06 - Unsafe File Downloads in Installation Scripts | ||
| **Vulnerability:** Installation scripts (`apt.sh`) were downloading executable artifacts directly to predictable locations (`/tmp/yq`) and the current working directory, risking symlink attacks, local privilege escalation, and overwriting existing files. | ||
| **Learning:** Shell scripts running with elevated privileges must never use hardcoded or predictable paths for temporary files, and downloading files to the current working directory is dangerous when executing as root or in unknown environments. | ||
| **Prevention:** Always use secure, randomly generated temporary directories via `mktemp -d` for downloading and processing installation artifacts. Wrap the logic in a subshell `(...)` and pair it with a local trap (e.g., `trap 'rm -rf "$TMP_DIR"' EXIT`) to ensure automatic cleanup upon exit. |
There was a problem hiding this comment.
Fix markdownlint blockers (MD041, MD022, MD013).
The heading level/spacing and long lines currently fail documentation lint checks.
📝 Proposed fix
-## 2024-04-06 - Unsafe File Downloads in Installation Scripts
-**Vulnerability:** Installation scripts (`apt.sh`) were downloading executable artifacts directly to predictable locations (`/tmp/yq`) and the current working directory, risking symlink attacks, local privilege escalation, and overwriting existing files.
-**Learning:** Shell scripts running with elevated privileges must never use hardcoded or predictable paths for temporary files, and downloading files to the current working directory is dangerous when executing as root or in unknown environments.
-**Prevention:** Always use secure, randomly generated temporary directories via `mktemp -d` for downloading and processing installation artifacts. Wrap the logic in a subshell `(...)` and pair it with a local trap (e.g., `trap 'rm -rf "$TMP_DIR"' EXIT`) to ensure automatic cleanup upon exit.
+# 2024-04-06 - Unsafe File Downloads in Installation Scripts
+
+**Vulnerability:** Installation scripts (`apt.sh`) were downloading executable
+artifacts directly to predictable locations (`/tmp/yq`) and the current working
+directory, risking symlink attacks, local privilege escalation, and overwriting
+existing files.
+
+**Learning:** Shell scripts running with elevated privileges must never use
+hardcoded or predictable paths for temporary files, and downloading files to
+the current working directory is dangerous when executing as root or in unknown
+environments.
+
+**Prevention:** Always use secure, randomly generated temporary directories via
+`mktemp -d` for downloading and processing installation artifacts. Wrap logic
+in a subshell `(...)` and pair it with a local trap (e.g.,
+`trap 'rm -rf "$TMP_DIR"' EXIT`) to ensure automatic cleanup upon exit.📝 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-04-06 - Unsafe File Downloads in Installation Scripts | |
| **Vulnerability:** Installation scripts (`apt.sh`) were downloading executable artifacts directly to predictable locations (`/tmp/yq`) and the current working directory, risking symlink attacks, local privilege escalation, and overwriting existing files. | |
| **Learning:** Shell scripts running with elevated privileges must never use hardcoded or predictable paths for temporary files, and downloading files to the current working directory is dangerous when executing as root or in unknown environments. | |
| **Prevention:** Always use secure, randomly generated temporary directories via `mktemp -d` for downloading and processing installation artifacts. Wrap the logic in a subshell `(...)` and pair it with a local trap (e.g., `trap 'rm -rf "$TMP_DIR"' EXIT`) to ensure automatic cleanup upon exit. | |
| # 2024-04-06 - Unsafe File Downloads in Installation Scripts | |
| **Vulnerability:** Installation scripts (`apt.sh`) were downloading executable | |
| artifacts directly to predictable locations (`/tmp/yq`) and the current working | |
| directory, risking symlink attacks, local privilege escalation, and overwriting | |
| existing files. | |
| **Learning:** Shell scripts running with elevated privileges must never use | |
| hardcoded or predictable paths for temporary files, and downloading files to | |
| the current working directory is dangerous when executing as root or in unknown | |
| environments. | |
| **Prevention:** Always use secure, randomly generated temporary directories via | |
| `mktemp -d` for downloading and processing installation artifacts. Wrap logic | |
| in a subshell `(...)` and pair it with a local trap (e.g., | |
| `trap 'rm -rf "$TMP_DIR"' EXIT`) to ensure automatic cleanup upon exit. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 292] 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: 246] 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: 254] 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-06 - Unsafe File Do..."] 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-04-06 - Unsafe File Downloads in Installation Scripts"] 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, Fix the markdownlint errors in
.jules/sentinel.md by adjusting the heading and spacing: change the existing
heading "2024-04-06 - Unsafe File Downloads in Installation Scripts" to an
appropriate top-level or consistent heading level (e.g., prefix with a single
'#'), ensure there is a blank line before and after every heading to satisfy
MD022/MD041, and wrap or hard-break long lines (the
vulnerability/learning/prevention paragraphs) to a reasonable width (MD013, ~80
chars) so no lines exceed the limit; keep the existing content and detail block
structure but reflow text and add the required blank lines around headings and
lists.
| GO_VERSION="1.23.4" | ||
| wget "https://go.dev/dl/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" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current external-download blocks and nearby integrity checks.
rg -n -C2 'go\.dev/dl/go|mikefarah/yq/releases/download|lsd-rs/lsd/releases/download|sha256sum|checksum|gpg|cosign' tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 3117
Add SHA256 checksum verification for Go, yq, and lsd downloads before privileged installation.
These packages download external artifacts and immediately extract or install them as root without verifying checksums. Other tools in this same file (GitHub CLI, Docker, Yarn, Terraform, Composer) implement integrity verification; these three should follow the same pattern.
Apply checksum verification immediately after each download:
- Go (line 212): Download
.tar.gz, verify SHA256 before extraction - yq (line 242): Download checksum file, verify downloaded binary against it
- lsd (line 256): Download
.deb, verify SHA256 before installation
Store SHA256 values as script variables (e.g., GO_SHA256, LSD_SHA256) and use sha256sum -c to verify, matching the pattern already used for Composer on line 279.
🤖 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 211 - 214, Add SHA256 checksum
verification before any privileged install for Go, yq, and lsd: introduce
variables like GO_SHA256 and LSD_SHA256 (and a yq checksum variable) and
download corresponding .sha256 or checksum values immediately after wget for the
artifacts referenced by GO_VERSION, yq download block, and lsd .deb download;
then run sha256sum -c (or equivalent) to verify the downloaded files before
running sudo tar -xzf /usr/local installation or sudo dpkg -i, and fail the
script if verification fails—follow the existing Composer pattern that uses
sha256sum -c to validate downloads.
| if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then | ||
| sudo php composer-setup.php --quiet --install-dir=/usr/local/bin --filename=composer | ||
| else | ||
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | ||
| fi |
There was a problem hiding this comment.
Fail fast on Composer checksum mismatch.
On checksum mismatch, the script only logs and continues. This should exit non-zero so the install doesn’t report success with missing/compromised Composer.
🛠️ Proposed fix
if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then
sudo php composer-setup.php --quiet --install-dir=/usr/local/bin --filename=composer
else
>&2 echo 'ERROR: Invalid installer checksum for Composer'
+ 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.
| if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then | |
| sudo php composer-setup.php --quiet --install-dir=/usr/local/bin --filename=composer | |
| else | |
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | |
| fi | |
| if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then | |
| sudo php composer-setup.php --quiet --install-dir=/usr/local/bin --filename=composer | |
| else | |
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | |
| 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 276 - 280, The else branch that
handles the Composer checksum mismatch currently only writes an error but
continues; update the block that compares EXPECTED_CHECKSUM and ACTUAL_CHECKSUM
so that on mismatch it prints the error and then exits with a non-zero status
(e.g., exit 1) to fail fast and prevent reporting a successful install; modify
the conditional around the composer install (composer-setup.php invocation) to
ensure only the successful-check path executes and the mismatch path terminates
immediately.
1 participant
🛡️ Sentinel: [CRITICAL] Fix local privilege escalation vulnerability in apt.sh
🚨 Severity: CRITICAL
💡 Vulnerability: Installation scripts (apt.sh) downloaded executable artifacts to predictable locations (e.g., /tmp/yq) and current working directory.
🎯 Impact: Predictable /tmp paths introduce symlink attack vulnerabilities and local privilege escalation when run via sudo. Downloads to CWD risk overwriting local files or executing unverified binaries.
🔧 Fix: Updated the go, yq, lsd, and composer downloads to use a dynamically generated, isolated directory created with
mktemp -d, wrapped securely within a subshell with a cleanup trap (trap 'rm -rf "$TMP_DIR"' EXIT).✅ Verification:
./build.shexecuted successfully ensuring no regressions. Readingapt.shconfirms the subshells correctly isolate the new download logic.PR created automatically by Jules for task 5767885975628387240 started by @kidchenko
Summary by CodeRabbit
Bug Fixes
Documentation