From 76545e95465bbf0164e35f31961022bc452d5f8c Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Sun, 22 Mar 2026 21:15:53 -0700 Subject: [PATCH 1/2] fix(security): download installers to file before execution Replace all curl-pipe-to-shell patterns with download-to-tempfile: - install.sh: Ollama installer (2 locations) - scripts/install.sh: NodeSource setup - scripts/brev-setup.sh: NodeSource setup - bin/nemoclaw.js: remote uninstall fallback Each uses mktemp -d, downloads to file, executes, cleans up. Prevents partial-download execution and allows inspection. SHA-256 pinning isn't practical for rolling-release upstream URLs. 4 regression tests verify no curl-pipe-to-shell in any of these files. Closes #574, #576, #577, #583. --- bin/nemoclaw.js | 25 +++++++++++++++---------- install.sh | 14 ++++++++++++-- scripts/brev-setup.sh | 7 ++++++- scripts/install.sh | 7 ++++++- test/runner.test.js | 37 +++++++++++++++++++++++++++++++++++++ 5 files changed, 76 insertions(+), 14 deletions(-) diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 2010cfeb2..811a8189a 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -238,17 +238,22 @@ function uninstall(args) { exitWithSpawnResult(result); } + // Download to file before execution — prevents partial-download execution. + // Upstream URL is a rolling release so SHA-256 pinning isn't practical. console.log(` Local uninstall script not found; falling back to ${REMOTE_UNINSTALL_URL}`); - const forwardedArgs = args.map(shellQuote).join(" "); - const command = forwardedArgs.length > 0 - ? `curl -fsSL ${shellQuote(REMOTE_UNINSTALL_URL)} | bash -s -- ${forwardedArgs}` - : `curl -fsSL ${shellQuote(REMOTE_UNINSTALL_URL)} | bash`; - const result = spawnSync("bash", ["-c", command], { - stdio: "inherit", - cwd: ROOT, - env: process.env, - }); - exitWithSpawnResult(result); + const uninstallDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-uninstall-")); + const uninstallScript = path.join(uninstallDir, "uninstall.sh"); + try { + execFileSync("curl", ["-fsSL", REMOTE_UNINSTALL_URL, "-o", uninstallScript], { stdio: "inherit" }); + const result = spawnSync("bash", [uninstallScript, ...args], { + stdio: "inherit", + cwd: ROOT, + env: process.env, + }); + exitWithSpawnResult(result); + } finally { + fs.rmSync(uninstallDir, { recursive: true, force: true }); + } } function showStatus() { diff --git a/install.sh b/install.sh index 0452d2827..47d8851a2 100755 --- a/install.sh +++ b/install.sh @@ -367,14 +367,24 @@ install_or_upgrade_ollama() { info "Ollama v${current} meets minimum requirement (>= v${OLLAMA_MIN_VERSION})" else info "Ollama v${current:-unknown} is below v${OLLAMA_MIN_VERSION} — upgrading…" - curl -fsSL https://ollama.com/install.sh | sh + # Upstream URL is a rolling release so SHA-256 pinning isn't practical, + # but download-then-execute allows inspection and prevents partial-download execution. + tmpdir=$(mktemp -d) + curl -fsSL https://ollama.com/install.sh -o "$tmpdir/install_ollama.sh" + sh "$tmpdir/install_ollama.sh" + rm -rf "$tmpdir" info "Ollama upgraded to $(get_ollama_version)" fi else # No ollama — only install if a GPU is present if detect_gpu; then info "GPU detected — installing Ollama…" - curl -fsSL https://ollama.com/install.sh | sh + # Upstream URL is a rolling release so SHA-256 pinning isn't practical, + # but download-then-execute allows inspection and prevents partial-download execution. + tmpdir=$(mktemp -d) + curl -fsSL https://ollama.com/install.sh -o "$tmpdir/install_ollama.sh" + sh "$tmpdir/install_ollama.sh" + rm -rf "$tmpdir" info "Ollama installed: v$(get_ollama_version)" else warn "No GPU detected — skipping Ollama installation." diff --git a/scripts/brev-setup.sh b/scripts/brev-setup.sh index cc8701ba9..466a8c207 100755 --- a/scripts/brev-setup.sh +++ b/scripts/brev-setup.sh @@ -39,7 +39,12 @@ export DEBIAN_FRONTEND=noninteractive # --- 0. Node.js (needed for services) --- if ! command -v node >/dev/null 2>&1; then info "Installing Node.js..." - curl -fsSL https://deb.nodesource.com/setup_22.x | sudo -E bash - >/dev/null 2>&1 + # Upstream URL is a rolling release so SHA-256 pinning isn't practical, + # but download-then-execute allows inspection and prevents partial-download execution. + tmpdir=$(mktemp -d) + curl -fsSL https://deb.nodesource.com/setup_22.x -o "$tmpdir/setup_node.sh" + sudo -E bash "$tmpdir/setup_node.sh" >/dev/null 2>&1 + rm -rf "$tmpdir" sudo apt-get install -y -qq nodejs >/dev/null 2>&1 info "Node.js $(node --version) installed" else diff --git a/scripts/install.sh b/scripts/install.sh index b00de5e86..3b0e6f3dc 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -235,7 +235,12 @@ install_node() { brew link --overwrite node@22 2>/dev/null || true ;; nodesource) - curl -fsSL https://deb.nodesource.com/setup_22.x | sudo -E bash - >/dev/null 2>&1 + # Upstream URL is a rolling release so SHA-256 pinning isn't practical, + # but download-then-execute allows inspection and prevents partial-download execution. + tmpdir=$(mktemp -d) + curl -fsSL https://deb.nodesource.com/setup_22.x -o "$tmpdir/setup_node.sh" + sudo -E bash "$tmpdir/setup_node.sh" >/dev/null 2>&1 + rm -rf "$tmpdir" sudo apt-get install -y -qq nodejs >/dev/null 2>&1 ;; none) diff --git a/test/runner.test.js b/test/runner.test.js index f35c88256..3e2ff909e 100644 --- a/test/runner.test.js +++ b/test/runner.test.js @@ -208,4 +208,41 @@ describe("runner helpers", () => { expect(!src.includes("execSync")).toBeTruthy(); }); }); + + describe("curl-pipe-to-shell guards (#574, #583)", () => { + // Match actual curl-pipe-to-shell executions, not printf/echo documentation + const isViolation = (line) => { + const trimmed = line.trim(); + if (trimmed.startsWith("#") || trimmed.startsWith("printf") || trimmed.startsWith("echo")) return false; + return /curl\s[^|]*\|\s*(sh|bash|sudo)/.test(trimmed); + }; + + it("install.sh does not pipe curl to shell", () => { + const src = fs.readFileSync(path.join(import.meta.dirname, "..", "install.sh"), "utf-8"); + const violations = src.split("\n").filter(isViolation); + expect(violations).toEqual([]); + }); + + it("scripts/install.sh does not pipe curl to shell", () => { + const src = fs.readFileSync(path.join(import.meta.dirname, "..", "scripts", "install.sh"), "utf-8"); + const violations = src.split("\n").filter(isViolation); + expect(violations).toEqual([]); + }); + + it("scripts/brev-setup.sh does not pipe curl to shell", () => { + const src = fs.readFileSync(path.join(import.meta.dirname, "..", "scripts", "brev-setup.sh"), "utf-8"); + const violations = src.split("\n").filter(isViolation); + expect(violations).toEqual([]); + }); + + it("bin/nemoclaw.js does not pipe curl to shell", () => { + const src = fs.readFileSync(path.join(import.meta.dirname, "..", "bin", "nemoclaw.js"), "utf-8"); + const violations = src.split("\n").filter((l) => { + const t = l.trim(); + if (t.startsWith("//") || t.startsWith("*")) return false; + return /curl.*\|\s*(sh|bash|sudo)/.test(t); + }); + expect(violations).toEqual([]); + }); + }); }); From babfb2ed5f3f8c059856aa4352efe59e64fbf48c Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Sun, 22 Mar 2026 21:45:40 -0700 Subject: [PATCH 2/2] fix: address review feedback on tmpdir cleanup and test coverage - Move exitWithSpawnResult outside try/finally in nemoclaw.js so process.exit does not bypass tmpdir cleanup - Wrap download in inner try/catch for user-friendly error on failure - Use subshell + trap EXIT for tmpdir cleanup in all shell scripts so set -euo pipefail exits don't leak temp directories - Normalize test input (join continuations, collapse multiline pipes) to catch line-wrapped curl-pipe-to-shell regressions - Tighten violation regex to match sh/bash/sudo bash but not sudo gpg --- bin/nemoclaw.js | 12 +++++++--- install.sh | 20 +++++++++------- scripts/brev-setup.sh | 10 ++++---- scripts/install.sh | 10 ++++---- test/runner.test.js | 54 +++++++++++++++++++++++++++++-------------- 5 files changed, 70 insertions(+), 36 deletions(-) diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 811a8189a..d31c83dd5 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -243,17 +243,23 @@ function uninstall(args) { console.log(` Local uninstall script not found; falling back to ${REMOTE_UNINSTALL_URL}`); const uninstallDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-uninstall-")); const uninstallScript = path.join(uninstallDir, "uninstall.sh"); + let result; try { - execFileSync("curl", ["-fsSL", REMOTE_UNINSTALL_URL, "-o", uninstallScript], { stdio: "inherit" }); - const result = spawnSync("bash", [uninstallScript, ...args], { + try { + execFileSync("curl", ["-fsSL", REMOTE_UNINSTALL_URL, "-o", uninstallScript], { stdio: "inherit" }); + } catch { + console.error(` Failed to download uninstall script from ${REMOTE_UNINSTALL_URL}`); + process.exit(1); + } + result = spawnSync("bash", [uninstallScript, ...args], { stdio: "inherit", cwd: ROOT, env: process.env, }); - exitWithSpawnResult(result); } finally { fs.rmSync(uninstallDir, { recursive: true, force: true }); } + exitWithSpawnResult(result); } function showStatus() { diff --git a/install.sh b/install.sh index 47d8851a2..ead3e952a 100755 --- a/install.sh +++ b/install.sh @@ -369,10 +369,12 @@ install_or_upgrade_ollama() { info "Ollama v${current:-unknown} is below v${OLLAMA_MIN_VERSION} — upgrading…" # Upstream URL is a rolling release so SHA-256 pinning isn't practical, # but download-then-execute allows inspection and prevents partial-download execution. - tmpdir=$(mktemp -d) - curl -fsSL https://ollama.com/install.sh -o "$tmpdir/install_ollama.sh" - sh "$tmpdir/install_ollama.sh" - rm -rf "$tmpdir" + ( + tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT + curl -fsSL https://ollama.com/install.sh -o "$tmpdir/install_ollama.sh" + sh "$tmpdir/install_ollama.sh" + ) info "Ollama upgraded to $(get_ollama_version)" fi else @@ -381,10 +383,12 @@ install_or_upgrade_ollama() { info "GPU detected — installing Ollama…" # Upstream URL is a rolling release so SHA-256 pinning isn't practical, # but download-then-execute allows inspection and prevents partial-download execution. - tmpdir=$(mktemp -d) - curl -fsSL https://ollama.com/install.sh -o "$tmpdir/install_ollama.sh" - sh "$tmpdir/install_ollama.sh" - rm -rf "$tmpdir" + ( + tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT + curl -fsSL https://ollama.com/install.sh -o "$tmpdir/install_ollama.sh" + sh "$tmpdir/install_ollama.sh" + ) info "Ollama installed: v$(get_ollama_version)" else warn "No GPU detected — skipping Ollama installation." diff --git a/scripts/brev-setup.sh b/scripts/brev-setup.sh index 466a8c207..2f123a613 100755 --- a/scripts/brev-setup.sh +++ b/scripts/brev-setup.sh @@ -41,10 +41,12 @@ if ! command -v node >/dev/null 2>&1; then info "Installing Node.js..." # Upstream URL is a rolling release so SHA-256 pinning isn't practical, # but download-then-execute allows inspection and prevents partial-download execution. - tmpdir=$(mktemp -d) - curl -fsSL https://deb.nodesource.com/setup_22.x -o "$tmpdir/setup_node.sh" - sudo -E bash "$tmpdir/setup_node.sh" >/dev/null 2>&1 - rm -rf "$tmpdir" + ( + tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT + curl -fsSL https://deb.nodesource.com/setup_22.x -o "$tmpdir/setup_node.sh" + sudo -E bash "$tmpdir/setup_node.sh" >/dev/null 2>&1 + ) sudo apt-get install -y -qq nodejs >/dev/null 2>&1 info "Node.js $(node --version) installed" else diff --git a/scripts/install.sh b/scripts/install.sh index 3b0e6f3dc..1b56c6f4e 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -237,10 +237,12 @@ install_node() { nodesource) # Upstream URL is a rolling release so SHA-256 pinning isn't practical, # but download-then-execute allows inspection and prevents partial-download execution. - tmpdir=$(mktemp -d) - curl -fsSL https://deb.nodesource.com/setup_22.x -o "$tmpdir/setup_node.sh" - sudo -E bash "$tmpdir/setup_node.sh" >/dev/null 2>&1 - rm -rf "$tmpdir" + ( + tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT + curl -fsSL https://deb.nodesource.com/setup_22.x -o "$tmpdir/setup_node.sh" + sudo -E bash "$tmpdir/setup_node.sh" >/dev/null 2>&1 + ) sudo apt-get install -y -qq nodejs >/dev/null 2>&1 ;; none) diff --git a/test/runner.test.js b/test/runner.test.js index 3e2ff909e..15fe3b00a 100644 --- a/test/runner.test.js +++ b/test/runner.test.js @@ -210,39 +210,59 @@ describe("runner helpers", () => { }); describe("curl-pipe-to-shell guards (#574, #583)", () => { - // Match actual curl-pipe-to-shell executions, not printf/echo documentation - const isViolation = (line) => { - const trimmed = line.trim(); - if (trimmed.startsWith("#") || trimmed.startsWith("printf") || trimmed.startsWith("echo")) return false; - return /curl\s[^|]*\|\s*(sh|bash|sudo)/.test(trimmed); + // Strip comment lines, then join line continuations so multiline + // curl ... |\n bash patterns are caught by the single-line regex. + const stripComments = (src, commentPrefix) => + src.split("\n").filter((l) => !l.trim().startsWith(commentPrefix)).join("\n"); + + const joinContinuations = (src) => + src.replace(/\\\n\s*/g, " "); + + const collapseMultilinePipes = (src) => + src.replace(/\|\s*\n\s*/g, "| "); + + const normalize = (src, commentPrefix) => + collapseMultilinePipes(joinContinuations(stripComments(src, commentPrefix))); + + const shellViolationRe = /curl\s[^|]*\|\s*(sh|bash|sudo\s+(-\S+\s+)*(sh|bash))\b/; + const jsViolationRe = /curl.*\|\s*(sh|bash|sudo\s+(-\S+\s+)*(sh|bash))\b/; + + const findShellViolations = (src) => { + const normalized = normalize(src, "#"); + return normalized.split("\n").filter((line) => { + const t = line.trim(); + if (t.startsWith("printf") || t.startsWith("echo")) return false; + return shellViolationRe.test(t); + }); + }; + + const findJsViolations = (src) => { + const normalized = normalize(src, "//"); + return normalized.split("\n").filter((line) => { + const t = line.trim(); + if (t.startsWith("*")) return false; + return jsViolationRe.test(t); + }); }; it("install.sh does not pipe curl to shell", () => { const src = fs.readFileSync(path.join(import.meta.dirname, "..", "install.sh"), "utf-8"); - const violations = src.split("\n").filter(isViolation); - expect(violations).toEqual([]); + expect(findShellViolations(src)).toEqual([]); }); it("scripts/install.sh does not pipe curl to shell", () => { const src = fs.readFileSync(path.join(import.meta.dirname, "..", "scripts", "install.sh"), "utf-8"); - const violations = src.split("\n").filter(isViolation); - expect(violations).toEqual([]); + expect(findShellViolations(src)).toEqual([]); }); it("scripts/brev-setup.sh does not pipe curl to shell", () => { const src = fs.readFileSync(path.join(import.meta.dirname, "..", "scripts", "brev-setup.sh"), "utf-8"); - const violations = src.split("\n").filter(isViolation); - expect(violations).toEqual([]); + expect(findShellViolations(src)).toEqual([]); }); it("bin/nemoclaw.js does not pipe curl to shell", () => { const src = fs.readFileSync(path.join(import.meta.dirname, "..", "bin", "nemoclaw.js"), "utf-8"); - const violations = src.split("\n").filter((l) => { - const t = l.trim(); - if (t.startsWith("//") || t.startsWith("*")) return false; - return /curl.*\|\s*(sh|bash|sudo)/.test(t); - }); - expect(violations).toEqual([]); + expect(findJsViolations(src)).toEqual([]); }); }); });