Skip to content

fix: use execFile with arg arrays to prevent command injection#48

Closed
harshahemanth wants to merge 1 commit intoNVIDIA:mainfrom
harshahemanth:fix/execfile-command-injection
Closed

fix: use execFile with arg arrays to prevent command injection#48
harshahemanth wants to merge 1 commit intoNVIDIA:mainfrom
harshahemanth:fix/execfile-command-injection

Conversation

@harshahemanth
Copy link

@harshahemanth harshahemanth commented Mar 16, 2026

Summary

  • Replace exec() with execFile() in status.ts to eliminate shell command injection risk
  • Pass arguments as an array instead of interpolating into a command string, so shell metacharacters in sandboxName are treated as literals
  • execFile() does not spawn a shell, so args like ; rm -rf / in a tampered sandbox name have no effect

Details

getSandboxStatus() interpolates sandboxName into a shell command string passed to exec(). If sandboxName is ever sourced from a tampered config or user input, this is a command injection vector. execFile() with an args array bypasses the shell entirely.

Also updated getInferenceStatus() for consistency, even though it has no user-controlled input.

Summary by CodeRabbit

  • Refactor
    • Improved internal command execution for OpenShell status and inference queries. The status check and inference retrieval functionality remain unchanged with the same error handling and timeout behavior.

@wscurran wscurran added NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). security Something isn't secure labels Mar 20, 2026
@wscurran
Copy link
Contributor

Thanks for putting on your security hat and tackling the command injection vulnerability in status.ts by switching to execFile() and properly handling arguments as an array, which should prevent issues like shell metacharacters in sandboxName being treated as commands.

@wscurran wscurran added the priority: high Important issue that should be resolved in the next release label Mar 20, 2026
exec() spawns a shell, so interpolated args like sandboxName are
vulnerable to shell metacharacter injection. execFile() bypasses the
shell entirely by passing args as an array directly to the process.
@harshahemanth harshahemanth force-pushed the fix/execfile-command-injection branch from 96c2723 to b0c1d00 Compare March 20, 2026 23:08
@coderabbitai
Copy link

coderabbitai bot commented Mar 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ea93e17d-28af-49a6-8403-70fc6743a34f

📥 Commits

Reviewing files that changed from the base of the PR and between 867f27d and b0c1d00.

📒 Files selected for processing (1)
  • nemoclaw/src/commands/status.ts

📝 Walkthrough

Walkthrough

Refactored command execution in the status module from child_process.exec with shell-interpreted strings to child_process.execFile with argument arrays for OpenShell sandbox and inference queries. Functional behavior, error handling, and timeouts remain identical.

Changes

Cohort / File(s) Summary
Command Execution Refactoring
nemoclaw/src/commands/status.ts
Replaced exec with execFile for OpenShell command invocations, converting from shell string format ("openshell sandbox status ... --json") to argument array format (["sandbox","status",...,"--json"]). Error handling, timeouts, and JSON parsing unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A rabbit once struggled with shell escapes long,
Till arguments in arrays proved safer and strong!
No more string interpretation's risky dance—
execFile runs cleaner, a security stance! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main security fix: switching from exec() to execFile() with argument arrays to prevent command injection vulnerabilities.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can get early access to new features in CodeRabbit.

Enable the early_access setting to enable early access features such as new models, tools, and more.

ericksoa added a commit that referenced this pull request Mar 21, 2026
Comprehensive fix for shell injection vulnerabilities where user input
(instance names, sandbox names, model names, API keys) was interpolated
unsanitized into shell commands via run()/runInteractive()/execSync().

Changes:
- Add shellQuote() and validateName() to runner.js as shared utilities
- Replace all execSync() with execFileSync() in deploy (no shell)
- Apply shellQuote() to every user-controlled variable in shell commands
  across nemoclaw.js, onboard.js, nim.js, policies.js
- Add RFC 1123 name validation at CLI dispatch for sandbox/instance names
- Fix path traversal in policies.js loadPreset()
- Replace predictable temp file with mkdtempSync()
- Remove duplicate shellQuote() definitions (now single source in runner.js)
- 9 new test cases for shellQuote, validateName, and path traversal

Supersedes #55, #110, #475, #540, #48, #171.
@ericksoa
Copy link
Contributor

Closing — the vulnerable status.ts file was removed in #492, and #584 hardens all remaining CLI entry points against command injection. Thanks for flagging this.

@ericksoa ericksoa closed this Mar 21, 2026
kjw3 pushed a commit that referenced this pull request Mar 21, 2026
* security: fix command injection across all CLI entry points

Comprehensive fix for shell injection vulnerabilities where user input
(instance names, sandbox names, model names, API keys) was interpolated
unsanitized into shell commands via run()/runInteractive()/execSync().

Changes:
- Add shellQuote() and validateName() to runner.js as shared utilities
- Replace all execSync() with execFileSync() in deploy (no shell)
- Apply shellQuote() to every user-controlled variable in shell commands
  across nemoclaw.js, onboard.js, nim.js, policies.js
- Add RFC 1123 name validation at CLI dispatch for sandbox/instance names
- Fix path traversal in policies.js loadPreset()
- Replace predictable temp file with mkdtempSync()
- Remove duplicate shellQuote() definitions (now single source in runner.js)
- 9 new test cases for shellQuote, validateName, and path traversal

Supersedes #55, #110, #475, #540, #48, #171.

* fix: deduplicate shellQuote in local-inference.js

Import shellQuote from runner.js instead of defining a local copy.
Single source of truth for shell quoting across the codebase.

* security: fix telegram bridge injection + add regression guards

Telegram bridge:
- Replace execSync with execFileSync for ssh-config retrieval
- shellQuote message, API key, and session ID in remote command
- Validate SANDBOX_NAME at startup
- Use mkdtempSync for temp SSH config (not predictable path)

Regression tests:
- nemoclaw.js must not use execSync
- Single shellQuote definition in bin/
- CLI rejects malicious sandbox names (e2e, no mocking)
- telegram-bridge.js validates SANDBOX_NAME and avoids execSync

* security: address CodeRabbit review findings on shell injection PR

- Shell-quote secret values written to .env before remote source
- Wrap scp upload in try/finally to guarantee temp secret cleanup
- Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard
- Replace predictable Date.now() temp path with mkdtempSync in policies
- Strengthen e2e test with canary file to prove payload never executes
- Fix merge-introduced test expectations for shellQuote single-quote format
jnun pushed a commit to jnun/EasyClaw that referenced this pull request Mar 21, 2026
)

* security: fix command injection across all CLI entry points

Comprehensive fix for shell injection vulnerabilities where user input
(instance names, sandbox names, model names, API keys) was interpolated
unsanitized into shell commands via run()/runInteractive()/execSync().

Changes:
- Add shellQuote() and validateName() to runner.js as shared utilities
- Replace all execSync() with execFileSync() in deploy (no shell)
- Apply shellQuote() to every user-controlled variable in shell commands
  across nemoclaw.js, onboard.js, nim.js, policies.js
- Add RFC 1123 name validation at CLI dispatch for sandbox/instance names
- Fix path traversal in policies.js loadPreset()
- Replace predictable temp file with mkdtempSync()
- Remove duplicate shellQuote() definitions (now single source in runner.js)
- 9 new test cases for shellQuote, validateName, and path traversal

Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171.

* fix: deduplicate shellQuote in local-inference.js

Import shellQuote from runner.js instead of defining a local copy.
Single source of truth for shell quoting across the codebase.

* security: fix telegram bridge injection + add regression guards

Telegram bridge:
- Replace execSync with execFileSync for ssh-config retrieval
- shellQuote message, API key, and session ID in remote command
- Validate SANDBOX_NAME at startup
- Use mkdtempSync for temp SSH config (not predictable path)

Regression tests:
- nemoclaw.js must not use execSync
- Single shellQuote definition in bin/
- CLI rejects malicious sandbox names (e2e, no mocking)
- telegram-bridge.js validates SANDBOX_NAME and avoids execSync

* security: address CodeRabbit review findings on shell injection PR

- Shell-quote secret values written to .env before remote source
- Wrap scp upload in try/finally to guarantee temp secret cleanup
- Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard
- Replace predictable Date.now() temp path with mkdtempSync in policies
- Strengthen e2e test with canary file to prove payload never executes
- Fix merge-introduced test expectations for shellQuote single-quote format
Ryuketsukami pushed a commit to Ryuketsukami/NemoClaw that referenced this pull request Mar 24, 2026
)

* security: fix command injection across all CLI entry points

Comprehensive fix for shell injection vulnerabilities where user input
(instance names, sandbox names, model names, API keys) was interpolated
unsanitized into shell commands via run()/runInteractive()/execSync().

Changes:
- Add shellQuote() and validateName() to runner.js as shared utilities
- Replace all execSync() with execFileSync() in deploy (no shell)
- Apply shellQuote() to every user-controlled variable in shell commands
  across nemoclaw.js, onboard.js, nim.js, policies.js
- Add RFC 1123 name validation at CLI dispatch for sandbox/instance names
- Fix path traversal in policies.js loadPreset()
- Replace predictable temp file with mkdtempSync()
- Remove duplicate shellQuote() definitions (now single source in runner.js)
- 9 new test cases for shellQuote, validateName, and path traversal

Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171.

* fix: deduplicate shellQuote in local-inference.js

Import shellQuote from runner.js instead of defining a local copy.
Single source of truth for shell quoting across the codebase.

* security: fix telegram bridge injection + add regression guards

Telegram bridge:
- Replace execSync with execFileSync for ssh-config retrieval
- shellQuote message, API key, and session ID in remote command
- Validate SANDBOX_NAME at startup
- Use mkdtempSync for temp SSH config (not predictable path)

Regression tests:
- nemoclaw.js must not use execSync
- Single shellQuote definition in bin/
- CLI rejects malicious sandbox names (e2e, no mocking)
- telegram-bridge.js validates SANDBOX_NAME and avoids execSync

* security: address CodeRabbit review findings on shell injection PR

- Shell-quote secret values written to .env before remote source
- Wrap scp upload in try/finally to guarantee temp secret cleanup
- Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard
- Replace predictable Date.now() temp path with mkdtempSync in policies
- Strengthen e2e test with canary file to prove payload never executes
- Fix merge-introduced test expectations for shellQuote single-quote format
jessesanford pushed a commit to jessesanford/NemoClaw that referenced this pull request Mar 24, 2026
)

* security: fix command injection across all CLI entry points

Comprehensive fix for shell injection vulnerabilities where user input
(instance names, sandbox names, model names, API keys) was interpolated
unsanitized into shell commands via run()/runInteractive()/execSync().

Changes:
- Add shellQuote() and validateName() to runner.js as shared utilities
- Replace all execSync() with execFileSync() in deploy (no shell)
- Apply shellQuote() to every user-controlled variable in shell commands
  across nemoclaw.js, onboard.js, nim.js, policies.js
- Add RFC 1123 name validation at CLI dispatch for sandbox/instance names
- Fix path traversal in policies.js loadPreset()
- Replace predictable temp file with mkdtempSync()
- Remove duplicate shellQuote() definitions (now single source in runner.js)
- 9 new test cases for shellQuote, validateName, and path traversal

Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171.

* fix: deduplicate shellQuote in local-inference.js

Import shellQuote from runner.js instead of defining a local copy.
Single source of truth for shell quoting across the codebase.

* security: fix telegram bridge injection + add regression guards

Telegram bridge:
- Replace execSync with execFileSync for ssh-config retrieval
- shellQuote message, API key, and session ID in remote command
- Validate SANDBOX_NAME at startup
- Use mkdtempSync for temp SSH config (not predictable path)

Regression tests:
- nemoclaw.js must not use execSync
- Single shellQuote definition in bin/
- CLI rejects malicious sandbox names (e2e, no mocking)
- telegram-bridge.js validates SANDBOX_NAME and avoids execSync

* security: address CodeRabbit review findings on shell injection PR

- Shell-quote secret values written to .env before remote source
- Wrap scp upload in try/finally to guarantee temp secret cleanup
- Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard
- Replace predictable Date.now() temp path with mkdtempSync in policies
- Strengthen e2e test with canary file to prove payload never executes
- Fix merge-introduced test expectations for shellQuote single-quote format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). priority: high Important issue that should be resolved in the next release security Something isn't secure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants