fix(security): stop piping curl output directly to sudo bash#869
fix(security): stop piping curl output directly to sudo bash#869fdzdev wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughReplaces an inline remote-script pipeline with a temp-file workflow: download NodeSource 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 unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/brev-setup.sh`:
- Around line 42-45: After creating the temp file with node_setup="$(mktemp)",
add an EXIT trap to always remove that file on any exit (success or failure) so
the temp file isn't leaked; specifically, register a trap like trap 'rm -f
"$node_setup"' EXIT immediately after the mktemp call (and keep the existing rm
-f "$node_setup" for the normal path or remove the explicit rm and rely on the
trap if preferred) to guarantee cleanup even when set -euo pipefail causes early
exits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f09ee8e-c4b1-4253-9856-5e21ceba8d14
📒 Files selected for processing (1)
scripts/brev-setup.sh
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
This is security theater — the threat model for curl | sudo bash is that an attacker controls the HTTP response (MITM, compromised CDN, DNS hijack). Downloading to a temp file and immediately executing it doesn't change that; no human inspects the file in between, so the attack surface is identical.
Real mitigations would be something like:
- Pin a checksum of the expected installer script and verify before executing
- Use the distro package manager directly with a signed apt repo instead of nodesource's bootstrap script
- Verify a GPG signature on the downloaded script
As-is this just adds complexity (mktemp, trap, cleanup) without actually reducing risk.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/brev-setup.sh (1)
47-48: Add checksum tool fallback for minimal-image portability.
shasumis not guaranteed on all minimal images. The codebase already uses this pattern ininstall.sh(checkingsha256sumfirst, then falling back toshasum). Currently, failure to find a tool reports as "checksum mismatch," which is misleading. Implement the same fallback here to detect missing tools explicitly rather than masking it as a verification error.Suggested patch
- echo "$NODESOURCE_SHA256 $node_setup" | shasum -a 256 -c - >/dev/null \ - || fail "NodeSource installer checksum mismatch — expected $NODESOURCE_SHA256" + if command -v sha256sum >/dev/null 2>&1; then + echo "$NODESOURCE_SHA256 $node_setup" | sha256sum -c - >/dev/null \ + || fail "NodeSource installer checksum mismatch — expected $NODESOURCE_SHA256" + elif command -v shasum >/dev/null 2>&1; then + echo "$NODESOURCE_SHA256 $node_setup" | shasum -a 256 -c - >/dev/null \ + || fail "NodeSource installer checksum mismatch — expected $NODESOURCE_SHA256" + else + fail "No SHA-256 verification tool found (need sha256sum or shasum)" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/brev-setup.sh` around lines 47 - 48, The current checksum step pipes to shasum and reports a misleading "checksum mismatch" when shasum is absent; update the checksum logic in the brev-setup.sh block that contains the echo "... | shasum -a 256 -c -" command to first detect and use sha256sum if available, otherwise use shasum if available, and if neither exists call fail with a clear message like "checksum tool not found"; keep the verification behavior the same (fail on mismatch) but ensure the absence of tools is detected explicitly and reported via the existing fail function instead of being masked as a mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/brev-setup.sh`:
- Around line 47-48: The current checksum step pipes to shasum and reports a
misleading "checksum mismatch" when shasum is absent; update the checksum logic
in the brev-setup.sh block that contains the echo "... | shasum -a 256 -c -"
command to first detect and use sha256sum if available, otherwise use shasum if
available, and if neither exists call fail with a clear message like "checksum
tool not found"; keep the verification behavior the same (fail on mismatch) but
ensure the absence of tools is detected explicitly and reported via the existing
fail function instead of being masked as a mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ade4f10e-8a37-4173-872c-4c0aa422a6c8
📒 Files selected for processing (1)
scripts/brev-setup.sh
brev-setup.sh pipes remote script content directly to sudo -E bash which executes arbitrary remote code as root without inspection (CWE-250, NVBUG 6002888). Download to a temp file first, then execute — the script is auditable on disk before root execution. Made-with: Cursor
set -e can exit before rm -f if curl or sudo fails. Add a trap to guarantee cleanup on all exit paths, then clear it after normal cleanup completes. Made-with: Cursor
Address review feedback: download-then-execute without verification has the same attack surface as curl|bash. Pin the SHA-256 hash of setup_22.x and verify before execution, matching the NVM installer pattern already in install.sh. Made-with: Cursor
fdzdev
force-pushed
the
fix/brev-curl-sudo-pipe
branch
from
db57ed5 to
8d86fe6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/brev-setup.sh`:
- Around line 42-49: The install entrypoint still pipes the NodeSource installer
directly into bash; replicate the secure pattern used for
NODESOURCE_URL/NODESOURCE_SHA256 by creating a temp file (mktemp), using curl
-fsSL to download to that temp (store in a variable like node_setup), adding a
trap to rm -f the temp, verify the file with shasum -a 256 -c against
NODESOURCE_SHA256 and on mismatch call fail, then execute with sudo -E bash
"$node_setup" (instead of piping), mirroring the logic from the existing block
that uses NODESOURCE_URL, NODESOURCE_SHA256, node_setup, trap and fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 951135c0-1221-4182-9c90-3c2bb839c5c7
📒 Files selected for processing (1)
scripts/brev-setup.sh
| NODESOURCE_URL="https://deb.nodesource.com/setup_22.x" | ||
| NODESOURCE_SHA256="575583bbac2fccc0b5edd0dbc03e222d9f9dc8d724da996d22754d6411104fd1" | ||
| node_setup="$(mktemp)" | ||
| trap 'rm -f "$node_setup"' EXIT | ||
| curl -fsSL "$NODESOURCE_URL" -o "$node_setup" | ||
| echo "$NODESOURCE_SHA256 $node_setup" | shasum -a 256 -c - >/dev/null \ | ||
| || fail "NodeSource installer checksum mismatch — expected $NODESOURCE_SHA256" | ||
| sudo -E bash "$node_setup" >/dev/null 2>&1 |
There was a problem hiding this comment.
Security fix is incomplete across install entrypoints.
This block is secured, but scripts/install.sh Line 238 still uses curl -fsSL ... | sudo -E bash - without checksum verification. If that path is used, the same class of risk remains. Please apply the same temp-file + pinned SHA-256 verification there as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/brev-setup.sh` around lines 42 - 49, The install entrypoint still
pipes the NodeSource installer directly into bash; replicate the secure pattern
used for NODESOURCE_URL/NODESOURCE_SHA256 by creating a temp file (mktemp),
using curl -fsSL to download to that temp (store in a variable like node_setup),
adding a trap to rm -f the temp, verify the file with shasum -a 256 -c against
NODESOURCE_SHA256 and on mismatch call fail, then execute with sudo -E bash
"$node_setup" (instead of piping), mirroring the logic from the existing block
that uses NODESOURCE_URL, NODESOURCE_SHA256, node_setup, trap and fail.
2 participants
Summary
brev-setup.shpipescurloutput directly tosudo -E bash -, executing arbitrary remote code as root without inspection (CWE-250, NVBUG 6002888)sudo -E bash "$tmpfile"— the script is auditable on disk before root executionsudo apt-getandsudo installcalls are left as-is (they legitimately need root for system package management)Test plan
brev-setup.shon a fresh Brev VM without Node.js — Node.js installs successfullyls /tmp/tmp.*shows no leftover)curlfails (e.g. no network),set -eexits beforesudo bashrunsSummary by CodeRabbit