Conversation
…wd downloads This patch resolves critical vulnerabilities found in `tools/os_installers/apt.sh` where several installations (Go, yq, lsd, and Composer) were downloading package binaries into predictable tmp paths (`/tmp/yq`) or downloading executable artifacts directly to the current working directory. This creates symlink attack risks (for predictable paths) and risks overwriting existing files or executing attacker-controlled binaries. To mitigate this risk, `mktemp -d` is now used to create securely generated, isolated temporary directories for these downloads. 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 documentation entry was added and installer scripts were updated to use securely generated temporary directories via 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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![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: Change the markdown to satisfy markdownlint: make the first
line an H1 (use a single leading "#") and ensure there is a blank line before
and after that H1, then wrap or reflow the long sentences on lines 2–4 to
shorter lines (under the repo's line-length limit) and/or split them into
multiple paragraphs or bullet points; keep the same content but reformat the
heading and line breaks so the file (.jules/sentinel.md) passes the docs lint
job.
In `@tools/os_installers/apt.sh`:
- Around line 264-270: The script currently only logs an error on Composer
checksum mismatch (comparison between EXPECTED_CHECKSUM and ACTUAL_CHECKSUM) and
continues; update the else branch so after printing the error it exits with a
non-zero status to fail provisioning (e.g., call exit 1), ensuring temporary
files (TMP_DIR) still get cleaned up before exit; refer to the checksum check
and the composer installer invocation (composer-setup.php) to locate the block
to modify.
- Line 209: The script downloads third-party artifacts (e.g., the Go archive
fetched with wget to "$TMP_DIR/go.tar.gz" using GO_VERSION, plus yq and lsd
downloads) without verifying integrity; update the wget steps to also fetch the
official checksum or signature (and, where provided, the vendor's GPG public
key) for each artifact (Go, yq, lsd), verify the downloaded file before
extraction/installation using sha256sum (or gpg --verify for signed releases),
and fail the script if verification fails; specifically, add logic surrounding
the GO download (variables GO_VERSION and TMP_DIR), the yq download step, and
the lsd download step to download the corresponding .sha256/.sha256sum or .asc
file, perform the appropriate verification command, and abort on mismatch so
only verified artifacts are installed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fcbb969-cdf8-41a1-ba66-fc44c01a413c
📒 Files selected for processing (2)
.jules/sentinel.mdtools/os_installers/apt.sh
| ## 2024-05-18 - [Predictable /tmp paths & insecure working directory usage] | ||
| **Vulnerability:** The apt.sh script downloaded packages into a predictable location (/tmp/yq) or the current working directory. This creates symlink attack risks (for predictable paths) and risks overwriting existing files or executing attacker-controlled binaries. | ||
| **Learning:** Hardcoded, predictable /tmp paths can be exploited by an unprivileged user by symlinking the path to a sensitive file. Downloading to the current working directory risks writing files where they shouldn't be. | ||
| **Prevention:** Always use securely generated random directories like `mktemp -d` to handle downloads instead of relying on the current working directory or predictable temporary file paths. |
There was a problem hiding this comment.
Fix markdownlint blockers (heading level/spacing/line length).
Line 1 should be an H1 with surrounding blank lines, and Line 2-4 exceed line-length limits from the failing docs lint job.
Suggested fix
-## 2024-05-18 - [Predictable /tmp paths & insecure working directory usage]
-**Vulnerability:** The apt.sh script downloaded packages into a predictable location (/tmp/yq) or the current working directory. This creates symlink attack risks (for predictable paths) and risks overwriting existing files or executing attacker-controlled binaries.
-**Learning:** Hardcoded, predictable /tmp paths can be exploited by an unprivileged user by symlinking the path to a sensitive file. Downloading to the current working directory risks writing files where they shouldn't be.
-**Prevention:** Always use securely generated random directories like `mktemp -d` to handle downloads instead of relying on the current working directory or predictable temporary file paths.
+# 2024-05-18 - Predictable /tmp paths & insecure working directory usage
+
+**Vulnerability:** The `apt.sh` script downloaded packages into a predictable
+location (`/tmp/yq`) or the current working directory. This creates symlink
+attack risks and can overwrite files or execute attacker-controlled binaries.
+
+**Learning:** Hardcoded, predictable `/tmp` paths can be exploited by an
+unprivileged user by symlinking that path to a sensitive file. Downloading to
+the current working directory also risks writing files where they should not be.
+
+**Prevention:** Always use securely generated random directories like
+`mktemp -d` for downloads instead of predictable temporary paths or the current
+working directory.📝 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 /tmp paths & insecure working directory usage] | |
| **Vulnerability:** The apt.sh script downloaded packages into a predictable location (/tmp/yq) or the current working directory. This creates symlink attack risks (for predictable paths) and risks overwriting existing files or executing attacker-controlled binaries. | |
| **Learning:** Hardcoded, predictable /tmp paths can be exploited by an unprivileged user by symlinking the path to a sensitive file. Downloading to the current working directory risks writing files where they shouldn't be. | |
| **Prevention:** Always use securely generated random directories like `mktemp -d` to handle downloads instead of relying on the current working directory or predictable temporary file paths. | |
| # 2024-05-18 - Predictable /tmp paths & insecure working directory usage | |
| **Vulnerability:** The `apt.sh` script downloaded packages into a predictable | |
| location (`/tmp/yq`) or the current working directory. This creates symlink | |
| attack risks and can overwrite files or execute attacker-controlled binaries. | |
| **Learning:** Hardcoded, predictable `/tmp` paths can be exploited by an | |
| unprivileged user by symlinking that path to a sensitive file. Downloading to | |
| the current working directory also risks writing files where they should not be. | |
| **Prevention:** Always use securely generated random directories like | |
| `mktemp -d` for downloads instead of predictable temporary paths or the current | |
| working directory. |
🧰 Tools
🪛 GitHub Check: Lint Documentation
[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 190] 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: 222] 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: 266] 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 /..."] 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 /tmp paths & insecure working directory usage]"] 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, Change the markdown to satisfy
markdownlint: make the first line an H1 (use a single leading "#") and ensure
there is a blank line before and after that H1, then wrap or reflow the long
sentences on lines 2–4 to shorter lines (under the repo's line-length limit)
and/or split them into multiple paragraphs or bullet points; keep the same
content but reformat the heading and line breaks so the file
(.jules/sentinel.md) passes the docs lint job.
| GO_VERSION="1.23.4" | ||
| wget "https://go.dev/dl/go${GO_VERSION}.linux-amd64.tar.gz" | ||
| TMP_DIR=$(mktemp -d) | ||
| wget -q "https://go.dev/dl/go${GO_VERSION}.linux-amd64.tar.gz" -O "$TMP_DIR/go.tar.gz" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether downloaded third-party artifacts in apt installer are integrity-checked.
rg -nC3 'go\.dev/dl|mikefarah/yq|lsd-rs/lsd|checksum|sha256|sha512|gpg' tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 3546
🏁 Script executed:
# Check the full context around Go, yq, and lsd downloads to ensure no verification is missed
sed -n '206,215p' tools/os_installers/apt.sh
echo "---"
sed -n '233,242p' tools/os_installers/apt.sh
echo "---"
sed -n '244,252p' tools/os_installers/apt.shRepository: kidchenko/dotfiles
Length of output: 1070
Implement integrity verification for third-party artifacts.
Lines 209 (Go), 236 (yq), and 247 (lsd) download executables/packages over HTTPS without checksum or signature verification before installation. HTTPS protects transport but not against artifact tampering at the source, CDN, or downstream. The same script verifies checksums for Composer (line 268) and uses GPG keys for GitHub CLI, Docker, Yarn, and Terraform—apply the same rigor to Go, yq, and lsd.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/os_installers/apt.sh` at line 209, The script downloads third-party
artifacts (e.g., the Go archive fetched with wget to "$TMP_DIR/go.tar.gz" using
GO_VERSION, plus yq and lsd downloads) without verifying integrity; update the
wget steps to also fetch the official checksum or signature (and, where
provided, the vendor's GPG public key) for each artifact (Go, yq, lsd), verify
the downloaded file before extraction/installation using sha256sum (or gpg
--verify for signed releases), and fail the script if verification fails;
specifically, add logic surrounding the GO download (variables GO_VERSION and
TMP_DIR), the yq download step, and the lsd download step to download the
corresponding .sha256/.sha256sum or .asc file, perform the appropriate
verification command, and abort on mismatch so only verified artifacts are
installed.
| 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 | ||
| rm -rf "$TMP_DIR" | ||
| else | ||
| >&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 closed on Composer checksum mismatch.
On checksum mismatch (Line 267), the script only logs and continues. This should terminate with non-zero status to avoid silent partial provisioning.
Suggested fix
if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then
sudo php "$TMP_DIR/composer-setup.php" --quiet --install-dir=/usr/local/bin --filename=composer
rm -rf "$TMP_DIR"
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.
| 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 | |
| rm -rf "$TMP_DIR" | |
| else | |
| >&2 echo 'ERROR: Invalid installer checksum for Composer' | |
| rm composer-setup.php | |
| rm -rf "$TMP_DIR" | |
| fi | |
| if [ "$EXPECTED_CHECKSUM" = "$ACTUAL_CHECKSUM" ]; then | |
| sudo php "$TMP_DIR/composer-setup.php" --quiet --install-dir=/usr/local/bin --filename=composer | |
| rm -rf "$TMP_DIR" | |
| else | |
| >&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 264 - 270, The script currently only
logs an error on Composer checksum mismatch (comparison between
EXPECTED_CHECKSUM and ACTUAL_CHECKSUM) and continues; update the else branch so
after printing the error it exits with a non-zero status to fail provisioning
(e.g., call exit 1), ensuring temporary files (TMP_DIR) still get cleaned up
before exit; refer to the checksum check and the composer installer invocation
(composer-setup.php) to locate the block to modify.
1 participant
🚨 Severity: HIGH
💡 Vulnerability:
tools/os_installers/apt.shdownloaded packages to predictable locations (like/tmp/yq) or directly into the current working directory.🎯 Impact: This creates a vector for local privilege escalation via symlink attacks, and downloading to the CWD risks writing files where they shouldn't be or overwriting critical files.
🔧 Fix: Refactored the script to use securely generated, isolated temporary directories via
mktemp -dto house all downloaded artifacts. The temporary directories are then safely removed.✅ Verification: Ran
./build.shwhich executedshellcheck(syntax checks) for all project scripts, passing without error forapt.sh. Additionally added asentinel.mdjournal entry detailing this finding and preventative measures.PR created automatically by Jules for task 15768480330397275714 started by @kidchenko
Summary by CodeRabbit