Repository navigation
fix: replace exec() with execFile() in status command to prevent command injection - #762
thebenignhacker wants to merge 3 commits into
Conversation
…dboxStatus The sandboxStatus function passed the sandbox name through `run()` which uses `bash -c` for command execution. While shellQuote() and validateName() provide defense-in-depth, the proper fix eliminates shell interpretation entirely by using spawnSync with an argument array. Detection: npx hackmyagent secure . References: - PSIRT disclosure: tickets 6009892-6010011 - CWE-78: Improper Neutralization of Special Elements used in an OS Command
📝 WalkthroughWalkthroughReplaced a shell-invoking execution in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 `@test/runner.test.js`:
- Around line 204-213: The test currently passes without exercising
sandboxStatus(); update the test to guarantee the successful sandbox lookup and
invocation of sandboxStatus by seeding a registry/fixture or stubbing the lookup
to return a valid sandbox before calling the CLI so the code path reaches
sandboxStatus(), then tighten the static assertions to catch indirect usages
(e.g., detect template-literal interpolation even when assigned to a variable
like const cmd or passed via run(cmd)) by asserting that sandboxStatus's body
uses spawnSync("openshell", ...) and does not call run(...) with a template
interpolation anywhere in the function; do the same for the related test at the
other location to ensure the e2e case is backed by a registry fixture or a
narrower seam that provably hits the status action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f510470-cd02-4387-88e1-6845fb3916ef
📒 Files selected for processing (2)
bin/nemoclaw.jstest/runner.test.js
Broaden the static analysis assertions to detect bash -c, shell: true, and any run/runCapture/runInteractive variant — not just template-literal interpolation. Addresses CodeRabbit review feedback.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/runner.test.js (1)
217-235:⚠️ Potential issue | 🟠 MajorThis mostly re-tests
validateName(), notsandboxStatus().Lines 123-129 already prove names containing
;are rejected, so the payload on Line 223 can fail before thestatuspath executes. That leaves Lines 230-231 able to pass without touching the newspawnSync("openshell", ...)path. Seed a known sandbox/registry fixture or stub the lookup so this case definitely enterssandboxStatus()first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.test.js` around lines 217 - 235, The test "sandboxStatus rejects shell metacharacters in sandbox name (e2e)" is re-testing validateName() instead of exercising sandboxStatus() because the malformed name is rejected earlier; update the test so the payload reaches sandboxStatus() by seeding or stubbing the lookup that resolves sandbox names (e.g., stub the registry lookup or provide a known sandbox fixture) and ensure the spawnSync call that uses openshell/`sandboxStatus()` path is invoked (refer to the test name, spawnSync invocation, and the sandboxStatus/validateName functions) so the assertion verifies sandboxStatus() rejects shell metacharacters rather than failing at validateName().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/runner.test.js`:
- Around line 217-235: The test "sandboxStatus rejects shell metacharacters in
sandbox name (e2e)" is re-testing validateName() instead of exercising
sandboxStatus() because the malformed name is rejected earlier; update the test
so the payload reaches sandboxStatus() by seeding or stubbing the lookup that
resolves sandbox names (e.g., stub the registry lookup or provide a known
sandbox fixture) and ensure the spawnSync call that uses
openshell/`sandboxStatus()` path is invoked (refer to the test name, spawnSync
invocation, and the sandboxStatus/validateName functions) so the assertion
verifies sandboxStatus() rejects shell metacharacters rather than failing at
validateName().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4aab967-119d-438b-b64e-d71059c38764
📒 Files selected for processing (1)
test/runner.test.js
|
Thanks for submitting this PR, it improves the security of the status command by replacing exec() with execFile(), which could prevent command injection attacks and improve the overall security of the NemoClaw project. |
- Register a sandbox in the registry before the e2e test so the CLI reaches sandboxStatus() instead of exiting at the lookup - Assert spawnSync args include sandboxName as a discrete array element - Clean up registry entry in finally block Addresses CodeRabbit review feedback.
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 `@test/runner.test.js`:
- Around line 218-246: The test uses a fixed fakeName and a loose status check
which can cause collisions and false positives; update the test to generate a
unique sandbox name (e.g., append a timestamp/random suffix to the fakeName used
with registerSandbox and removeSandbox) so it cannot collide with existing
registry state, and strengthen the spawnSync assertion by first asserting the
command did not time out or error (e.g., ensure result.status is not null and/or
result.error is undefined) before asserting result.status !== 0 to avoid passing
on a timeout; keep the canary creation/removal and removeSandbox cleanup as-is
but reference registerSandbox, removeSandbox, fakeName and the spawnSync result
when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98c6d783-5695-4a5c-ad8d-90aab42ebe68
📒 Files selected for processing (1)
test/runner.test.js
| it("sandboxStatus rejects shell metacharacters in sandbox name (e2e)", () => { | ||
| // Seed a registry entry so the CLI reaches sandboxStatus() | ||
| // (without a matching sandbox, the CLI exits at the lookup before our code path) | ||
| const registryPath = path.join(import.meta.dirname, "..", "bin", "lib", "registry"); | ||
| const { registerSandbox, removeSandbox } = require(registryPath); | ||
| const fakeName = "canary-test-sb"; | ||
| registerSandbox(fakeName, { model: "test", provider: "test" }); | ||
|
|
||
| const canaryDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-status-canary-")); | ||
| const canary = path.join(canaryDir, "executed"); | ||
| try { | ||
| // Even with a valid sandbox, metacharacters in argv must not execute | ||
| const result = spawnSync("node", [ | ||
| path.join(import.meta.dirname, "..", "bin", "nemoclaw.js"), | ||
| `${fakeName}; touch ${canary}`, | ||
| "status", | ||
| ], { | ||
| encoding: "utf-8", | ||
| timeout: 10000, | ||
| cwd: path.join(import.meta.dirname, ".."), | ||
| }); | ||
| expect(result.status).not.toBe(0); | ||
| expect(fs.existsSync(canary)).toBe(false); | ||
| } finally { | ||
| fs.rmSync(canaryDir, { recursive: true, force: true }); | ||
| removeSandbox(fakeName); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Harden this e2e to avoid false positives and fixture collisions.
Two reliability gaps: (1) fixed fakeName can collide with existing registry state, and (2) expect(result.status).not.toBe(0) can still pass on timeout (status === null). Use a unique sandbox name and assert no timeout/error occurred.
🔧 Suggested patch
- const fakeName = "canary-test-sb";
- registerSandbox(fakeName, { model: "test", provider: "test" });
-
- const canaryDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-status-canary-"));
- const canary = path.join(canaryDir, "executed");
- try {
+ const fakeName = `canary-test-sb-${process.pid}-${Date.now()}`;
+ let canaryDir;
+ try {
+ registerSandbox(fakeName, { model: "test", provider: "test" });
+ canaryDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-status-canary-"));
+ const canary = path.join(canaryDir, "executed");
// Even with a valid sandbox, metacharacters in argv must not execute
const result = spawnSync("node", [
path.join(import.meta.dirname, "..", "bin", "nemoclaw.js"),
`${fakeName}; touch ${canary}`,
"status",
], {
encoding: "utf-8",
timeout: 10000,
cwd: path.join(import.meta.dirname, ".."),
});
+ expect(result.error).toBeUndefined();
+ expect(result.signal).toBeNull();
expect(result.status).not.toBe(0);
expect(fs.existsSync(canary)).toBe(false);
} finally {
- fs.rmSync(canaryDir, { recursive: true, force: true });
+ if (canaryDir) fs.rmSync(canaryDir, { recursive: true, force: true });
removeSandbox(fakeName);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runner.test.js` around lines 218 - 246, The test uses a fixed fakeName
and a loose status check which can cause collisions and false positives; update
the test to generate a unique sandbox name (e.g., append a timestamp/random
suffix to the fakeName used with registerSandbox and removeSandbox) so it cannot
collide with existing registry state, and strengthen the spawnSync assertion by
first asserting the command did not time out or error (e.g., ensure
result.status is not null and/or result.error is undefined) before asserting
result.status !== 0 to avoid passing on a timeout; keep the canary
creation/removal and removeSandbox cleanup as-is but reference registerSandbox,
removeSandbox, fakeName and the spawnSync result when making these changes.
|
Hi — these PRs are not automated. They are companion fixes to a coordinated disclosure filed with NVIDIA PSIRT (tickets 6009892–6010011, 10 findings total). The other 4 PRs (#763, #764, #765, #766) were closed as "automated" or "duplicate," but they address distinct vulnerabilities in different files:
If it's easier to review, I'm happy to consolidate all 5 fixes into this single PR. Let me know how you'd prefer to proceed. |
|
Thanks for identifying this and for the thorough writeup — CWE-78 with PSIRT references is exactly the kind of security contribution we want to see. This was addressed comprehensively in #584 ( Closing as superseded, but your work here helped validate that the fix covered the right surface. |
Summary
sandboxStatus()passes the sandbox name throughrun()which usesspawnSync("bash", ["-c", cmd])for command execution. WhileshellQuote()and
validateName()provide defense-in-depth, the proper fix eliminatesshell interpretation entirely by using
spawnSyncwith an argument array.Change
Replace
run()bash -c call with directspawnSync("openshell", [...args])in
sandboxStatus. This removes shell interpretation from the code paththat handles sandbox names.
Detection
This vulnerability class is detectable via HackMyAgent:
References
Summary by CodeRabbit
Bug Fixes
Tests