🛡️ Sentinel: [HIGH] Fix insecure temporary file handling in apt installer#71
🛡️ Sentinel: [HIGH] Fix insecure temporary file handling in apt installer#71
Conversation
Updated `tools/os_installers/apt.sh` to download executables and installation artifacts into a secure temporary directory generated by `mktemp -d` rather than the current working directory or predictable paths like `/tmp/`. Included an EXIT trap to ensure the secure cleanup of these files. Also documented this security finding in `.jules/sentinel.md` as required by the Sentinel process. 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 vulnerability report documenting insecure temporary file handling in installer scripts was added to the documentation. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: Add a file-level H1 at the top of .jules/sentinel.md, ensure
there is a blank line above and below the entry heading "2024-05-24 - [Insecure
Temporary File Handling in Installer Scripts]" to satisfy MD022, and reflow/wrap
the paragraph text (the Vulnerability, Learning, and Prevention lines) to a
reasonable line length to fix MD013; keep the existing heading text and content
but split long lines into wrapped paragraphs and ensure one blank line separates
headings from content to resolve MD041/MD022/MD013 together.
In `@tools/os_installers/apt.sh`:
- Around line 257-265: The Composer integrity branch currently only logs an
error and continues; change the else branch that handles EXPECTED_CHECKSUM vs
ACTUAL_CHECKSUM so it fails closed by exiting with a non-zero status (e.g., call
exit 1) after printing the error; update the block around EXPECTED_CHECKSUM,
ACTUAL_CHECKSUM and the sudo php install step (the composer installer
checksum/installation section) to ensure the script stops when the checksum
mismatches rather than proceeding.
- Around line 212-214: The installers in tools/os_installers/apt.sh currently
wget binaries and run privileged install steps (e.g. the Go download using
GO_VERSION and TMP_DIR followed by sudo tar -C /usr/local, plus the yq and lsd
download/install sections) without verifying integrity; update each installer to
fetch the upstream checksum/signature asset for the pinned release, verify the
downloaded artifact (e.g. compute sha256sum and compare against upstream
checksum or verify signature with GPG) before performing any sudo actions, and
abort with a clear error on mismatch; for Go use the go.dev SHA256 API for
GO_VERSION and verify "$TMP_DIR/go${GO_VERSION}.linux-amd64.tar.gz" before sudo
tar, for yq use the release checksums/checksums-bsd asset and verify the binary
before moving to /usr/local/bin, and for lsd fetch the GitHub release
checksum/signature and verify the .deb before running dpkg.
🪄 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: 06f46677-3c0b-4d5f-aec2-4ffb9748c415
📒 Files selected for processing (2)
.jules/sentinel.mdtools/os_installers/apt.sh
| ## 2024-05-24 - [Insecure Temporary File Handling in Installer Scripts] | ||
| **Vulnerability:** Scripts like `tools/os_installers/apt.sh` downloaded executables or installation artifacts directly into the current working directory or predictable temporary locations (e.g., `/tmp/yq`), and then moved or executed them with elevated (`sudo`) privileges. | ||
| **Learning:** This exposes the installation process to local privilege escalation, symlink attacks, and unintentional file overwriting. Downloading to predictable locations allows a local attacker to pre-create files or symlinks at those paths. | ||
| **Prevention:** Always use securely generated random directories (e.g., `mktemp -d`) for downloading and processing temporary files during installation, especially when elevated privileges are involved. Pair `mktemp -d` with a `trap 'rm -rf "$TMP_DIR"' EXIT` to ensure automatic cleanup. No newline at end of file |
There was a problem hiding this comment.
Fix the markdownlint failures in this new note.
This entry currently trips MD041, MD022, and MD013, so the docs lint job stays red. Add a file-level H1, blank lines around the entry heading, and wrap the paragraphs.
💡 Suggested fix
+# Sentinel learnings
+
## 2024-05-24 - [Insecure Temporary File Handling in Installer Scripts]
-**Vulnerability:** Scripts like `tools/os_installers/apt.sh` downloaded executables or installation artifacts directly into the current working directory or predictable temporary locations (e.g., `/tmp/yq`), and then moved or executed them with elevated (`sudo`) privileges.
-**Learning:** This exposes the installation process to local privilege escalation, symlink attacks, and unintentional file overwriting. Downloading to predictable locations allows a local attacker to pre-create files or symlinks at those paths.
-**Prevention:** Always use securely generated random directories (e.g., `mktemp -d`) for downloading and processing temporary files during installation, especially when elevated privileges are involved. Pair `mktemp -d` with a `trap 'rm -rf "$TMP_DIR"' EXIT` to ensure automatic cleanup.
+
+**Vulnerability:** Scripts like `tools/os_installers/apt.sh` downloaded
+executables or installation artifacts directly into the current working
+directory or predictable temporary locations (e.g., `/tmp/yq`), and then moved
+or executed them with elevated (`sudo`) privileges.
+
+**Learning:** This exposes the installation process to local privilege
+escalation, symlink attacks, and unintentional file overwriting. Downloading to
+predictable locations allows a local attacker to pre-create files or symlinks at
+those paths.
+
+**Prevention:** Always use securely generated random directories (e.g.,
+`mktemp -d`) for downloading and processing temporary files during installation,
+especially when elevated privileges are involved. Pair `mktemp -d` with a
+`trap 'rm -rf "$TMP_DIR"' EXIT` to ensure automatic cleanup.📝 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 - [Insecure Temporary File Handling in Installer Scripts] | |
| **Vulnerability:** Scripts like `tools/os_installers/apt.sh` downloaded executables or installation artifacts directly into the current working directory or predictable temporary locations (e.g., `/tmp/yq`), and then moved or executed them with elevated (`sudo`) privileges. | |
| **Learning:** This exposes the installation process to local privilege escalation, symlink attacks, and unintentional file overwriting. Downloading to predictable locations allows a local attacker to pre-create files or symlinks at those paths. | |
| **Prevention:** Always use securely generated random directories (e.g., `mktemp -d`) for downloading and processing temporary files during installation, especially when elevated privileges are involved. Pair `mktemp -d` with a `trap 'rm -rf "$TMP_DIR"' EXIT` to ensure automatic cleanup. | |
| # Sentinel learnings | |
| ## 2024-05-24 - [Insecure Temporary File Handling in Installer Scripts] | |
| **Vulnerability:** Scripts like `tools/os_installers/apt.sh` downloaded | |
| executables or installation artifacts directly into the current working | |
| directory or predictable temporary locations (e.g., `/tmp/yq`), and then moved | |
| or executed them with elevated (`sudo`) privileges. | |
| **Learning:** This exposes the installation process to local privilege | |
| escalation, symlink attacks, and unintentional file overwriting. Downloading to | |
| predictable locations allows a local attacker to pre-create files or symlinks at | |
| those paths. | |
| **Prevention:** Always use securely generated random directories (e.g., | |
| `mktemp -d`) for downloading and processing temporary files during installation, | |
| especially when elevated privileges are involved. Pair `mktemp -d` with a | |
| `trap 'rm -rf "$TMP_DIR"' EXIT` to ensure automatic cleanup. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 287] 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: 244] 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: 274] 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 - [Insecure Temp..."] 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 - [Insecure Temporary File Handling in Installer 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, Add a file-level H1 at the top of
.jules/sentinel.md, ensure there is a blank line above and below the entry
heading "2024-05-24 - [Insecure Temporary File Handling in Installer Scripts]"
to satisfy MD022, and reflow/wrap the paragraph text (the Vulnerability,
Learning, and Prevention lines) to a reasonable line length to fix MD013; keep
the existing heading text and content but split long lines into wrapped
paragraphs and ensure one blank line separates headings from content to resolve
MD041/MD022/MD013 together.
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Go 1.23.4:"
curl -fsSL 'https://go.dev/dl/?mode=json&include=all' \
| jq -r '.[] | select(.version=="go1.23.4") | .files[] | select(.filename=="go1.23.4.linux-amd64.tar.gz") | {filename, sha256}'
echo
echo "yq v4.44.6 assets:"
curl -fsSL 'https://api.github.com/repos/mikefarah/yq/releases/tags/v4.44.6' \
| jq -r '.assets[]?.name'
echo
echo "lsd v1.1.5 assets:"
curl -fsSL 'https://api.github.com/repos/lsd-rs/lsd/releases/tags/v1.1.5' \
| jq -r '.assets[]?.name'Repository: kidchenko/dotfiles
Length of output: 2024
🏁 Script executed:
# Find the file and check its size first
wc -l tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 92
🏁 Script executed:
# Read the specific sections mentioned in the review
echo "=== Lines 200-220 (context for 212-214) ==="
sed -n '200,220p' tools/os_installers/apt.sh
echo
echo "=== Lines 225-255 (context for 237-239 and 246-247) ==="
sed -n '225,255p' tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 1989
Add checksum/signature verification before executing privileged operations.
All three tools download binaries from the network and immediately execute them as root without verification:
- Go 1.23.4: downloads and extracts as root (lines 212-214)
- yq v4.44.6: downloads and installs to
/usr/local/binas root (lines 237-239) - lsd 1.1.5: downloads and installs with
dpkgas root (lines 246-247)
While the temp-dir fix mitigates symlink/path attacks, these installers remain vulnerable to network tampering (MITM). Upstream provides verification data for all pinned versions:
- Go: SHA256 checksums via
go.devAPI - yq:
checksumsandchecksums-bsdfiles in release assets - lsd: standard GitHub release assets
Implement checksum verification (or use signed package sources where available) before each privileged step.
🤖 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 212 - 214, The installers in
tools/os_installers/apt.sh currently wget binaries and run privileged install
steps (e.g. the Go download using GO_VERSION and TMP_DIR followed by sudo tar -C
/usr/local, plus the yq and lsd download/install sections) without verifying
integrity; update each installer to fetch the upstream checksum/signature asset
for the pinned release, verify the downloaded artifact (e.g. compute sha256sum
and compare against upstream checksum or verify signature with GPG) before
performing any sudo actions, and abort with a clear error on mismatch; for Go
use the go.dev SHA256 API for GO_VERSION and verify
"$TMP_DIR/go${GO_VERSION}.linux-amd64.tar.gz" before sudo tar, for yq use the
release checksums/checksums-bsd asset and verify the binary before moving to
/usr/local/bin, and for lsd fetch the GitHub release checksum/signature and
verify the .deb before running dpkg.
| EXPECTED_CHECKSUM="$(php -r 'copy("https://composer.github.io/installer.sig", "php://stdout");')" | ||
| php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" | ||
| ACTUAL_CHECKSUM="$(php -r "echo hash_file('sha384', 'composer-setup.php');")" | ||
| php -r "copy('https://getcomposer.org/installer', '$TMP_DIR/composer-setup.php');" | ||
| ACTUAL_CHECKSUM="$(php -r "echo hash_file('sha384', '$TMP_DIR/composer-setup.php');")" | ||
|
|
||
| if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then | ||
| sudo php composer-setup.php --quiet --install-dir=/usr/local/bin --filename=composer | ||
| rm composer-setup.php | ||
| sudo php "$TMP_DIR/composer-setup.php" --quiet --install-dir=/usr/local/bin --filename=composer | ||
| else | ||
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | ||
| rm composer-setup.php | ||
| fi |
There was a problem hiding this comment.
Make the Composer integrity check fail closed.
If the checksum mismatches, this only logs and continues, so the script can still reach the final success banner with Composer silently skipped. Treat that branch as fatal.
💡 Suggested fix
if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then
sudo php "$TMP_DIR/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.
| EXPECTED_CHECKSUM="$(php -r 'copy("https://composer.github.io/installer.sig", "php://stdout");')" | |
| php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" | |
| ACTUAL_CHECKSUM="$(php -r "echo hash_file('sha384', 'composer-setup.php');")" | |
| php -r "copy('https://getcomposer.org/installer', '$TMP_DIR/composer-setup.php');" | |
| ACTUAL_CHECKSUM="$(php -r "echo hash_file('sha384', '$TMP_DIR/composer-setup.php');")" | |
| if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then | |
| sudo php composer-setup.php --quiet --install-dir=/usr/local/bin --filename=composer | |
| rm composer-setup.php | |
| sudo php "$TMP_DIR/composer-setup.php" --quiet --install-dir=/usr/local/bin --filename=composer | |
| else | |
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | |
| rm composer-setup.php | |
| fi | |
| EXPECTED_CHECKSUM="$(php -r 'copy("https://composer.github.io/installer.sig", "php://stdout");')" | |
| php -r "copy('https://getcomposer.org/installer', '$TMP_DIR/composer-setup.php');" | |
| ACTUAL_CHECKSUM="$(php -r "echo hash_file('sha384', '$TMP_DIR/composer-setup.php');")" | |
| if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then | |
| sudo php "$TMP_DIR/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 257 - 265, The Composer integrity
branch currently only logs an error and continues; change the else branch that
handles EXPECTED_CHECKSUM vs ACTUAL_CHECKSUM so it fails closed by exiting with
a non-zero status (e.g., call exit 1) after printing the error; update the block
around EXPECTED_CHECKSUM, ACTUAL_CHECKSUM and the sudo php install step (the
composer installer checksum/installation section) to ensure the script stops
when the checksum mismatches rather than proceeding.
1 participant
🚨 Severity: HIGH
💡 Vulnerability: The script
tools/os_installers/apt.shdownloaded executables or installation artifacts directly into the current working directory or predictable temporary locations (e.g.,/tmp/yq), and then moved or executed them with elevated (sudo) privileges. This exposed the installation process to local privilege escalation, symlink attacks, and unintentional file overwriting.🎯 Impact: A local attacker could pre-create files or symlinks at predictable paths (like
/tmp/yq), allowing them to overwrite critical system files or execute arbitrary code with elevated privileges during the installation process.🔧 Fix: Refactored the script to create a secure, randomized temporary directory using
mktemp -dand paired it with anEXITtrap for automatic cleanup. Updated the installation commands for Go, yq, lsd, and Composer to download artifacts into this isolated directory.✅ Verification: Run
./build.shto ensure there are no syntax errors. Manually verifytools/os_installers/apt.shto ensure it runs correctly and downloads files to a temporary directory before moving them to their final destination. Review.jules/sentinel.mdto see the documented vulnerability.PR created automatically by Jules for task 28599786978884121 started by @kidchenko
Summary by CodeRabbit
Release Notes
Security
Documentation