fix(security): add binaries restrictions to all messaging and preset policies#645
Conversation
📝 WalkthroughWalkthroughAdded explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nemoclaw-blueprint/policies/presets/npm.yaml (1)
12-28:⚠️ Potential issue | 🟠 MajorSwitch npm/yarn endpoints to
access: fullinstead of TLS termination.
binarieshardening is good, but this preset should not rely onprotocol: rest+tls: terminatefor npm/yarn registry traffic. That mode can break CONNECT-based flows.🔧 Proposed fix
network_policies: npm_yarn: name: npm_yarn endpoints: - host: registry.npmjs.org port: 443 - protocol: rest - enforcement: enforce - tls: terminate - rules: - - allow: { method: GET, path: "/**" } + access: full - host: registry.yarnpkg.com port: 443 - protocol: rest - enforcement: enforce - tls: terminate - rules: - - allow: { method: GET, path: "/**" } + access: full binaries: - { path: /usr/local/bin/npm } - { path: /usr/local/bin/yarn }Based on learnings: In NemoClaw package-manager presets (including npm), use
access: fullinstead ofprotocol: rest+tls: terminatebecause TLS termination can break CONNECT-based proxying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/npm.yaml` around lines 12 - 28, The npm/yarn registry entries currently use "protocol: rest" with "tls: terminate", which breaks CONNECT-based flows; update the entries for host: registry.npmjs.org and host: registry.yarnpkg.com to remove or stop relying on "protocol: rest" + "tls: terminate" and instead set the policy to use "access: full" (preserving enforcement: enforce and the existing rules/binaries). Ensure you replace the tls/protocol fields with access: full for both registry.npmjs.org and registry.yarnpkg.com entries so CONNECT traffic is allowed.nemoclaw-blueprint/policies/presets/pypi.yaml (1)
14-25:⚠️ Potential issue | 🟠 MajorUse CONNECT-compatible access mode for the PyPI preset.
This preset still uses
protocol: rest+tls: terminate, which is fragile for package-manager traffic and can break installs/auth flows. Switch these endpoints toaccess: fulland rely on the newbinariesallowlist for process-level restriction.Suggested patch
- host: pypi.org port: 443 - protocol: rest + access: full enforcement: enforce - tls: terminate - rules: - - allow: { method: GET, path: "/**" } - host: files.pythonhosted.org port: 443 - protocol: rest + access: full enforcement: enforce - tls: terminate - rules: - - allow: { method: GET, path: "/**" }Based on learnings: In
nemoclaw-blueprint/policies/presets/, package-manager presets such as PyPI that use CONNECT tunneling should useaccess: fullinstead oftls: terminate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/pypi.yaml` around lines 14 - 25, Update the PyPI preset entries that currently specify protocol: rest and tls: terminate to use CONNECT-compatible access by replacing those keys with access: full for the hosts (notably the blocks containing host: files.pythonhosted.org and the other PyPI host block), and remove the protocol/tls lines; keep the existing enforcement and rules and rely on the binaries allowlist for process-level restriction instead of terminating TLS.
🤖 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/security-binaries-restriction.test.js`:
- Around line 60-67: The substring check using includes("binaries:") is too
weak; instead parse each preset with a real YAML parser (e.g., js-yaml's load)
and assert the parsed object contains a "binaries" key (or use a strict
line-regex like /^\s*binaries:\s*/m if you cannot add a dependency). Update the
loop that reads presets (variables: presets, PRESETS_DIR, missing, currently
using includes("binaries:")) to try yaml.load(content) and push the file to
missing when the resulting object is falsy or does not have
Object.prototype.hasOwnProperty.call(parsed, "binaries"); fall back to the regex
check only if yaml parsing fails.
---
Outside diff comments:
In `@nemoclaw-blueprint/policies/presets/npm.yaml`:
- Around line 12-28: The npm/yarn registry entries currently use "protocol:
rest" with "tls: terminate", which breaks CONNECT-based flows; update the
entries for host: registry.npmjs.org and host: registry.yarnpkg.com to remove or
stop relying on "protocol: rest" + "tls: terminate" and instead set the policy
to use "access: full" (preserving enforcement: enforce and the existing
rules/binaries). Ensure you replace the tls/protocol fields with access: full
for both registry.npmjs.org and registry.yarnpkg.com entries so CONNECT traffic
is allowed.
In `@nemoclaw-blueprint/policies/presets/pypi.yaml`:
- Around line 14-25: Update the PyPI preset entries that currently specify
protocol: rest and tls: terminate to use CONNECT-compatible access by replacing
those keys with access: full for the hosts (notably the blocks containing host:
files.pythonhosted.org and the other PyPI host block), and remove the
protocol/tls lines; keep the existing enforcement and rules and rely on the
binaries allowlist for process-level restriction instead of terminating TLS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98245e1e-c12f-43de-90dd-12e522f9eb2b
📒 Files selected for processing (11)
nemoclaw-blueprint/policies/openclaw-sandbox.yamlnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/docker.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/jira.yamlnemoclaw-blueprint/policies/presets/npm.yamlnemoclaw-blueprint/policies/presets/outlook.yamlnemoclaw-blueprint/policies/presets/pypi.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/policies/presets/telegram.yamltest/security-binaries-restriction.test.js
cv
left a comment
There was a problem hiding this comment.
Thanks for closing the exfiltration vector — binary allowlisting is the right approach here. A few suggestions inline.
| - allow: { method: GET, path: "/**" } | ||
| binaries: | ||
| - { path: /usr/local/bin/npm } | ||
| - { path: /usr/local/bin/yarn } |
There was a problem hiding this comment.
npm, yarn, and npx are shell scripts / symlinks that spawn node under the hood. If the sandbox checks the actual process making the syscall, these entries won't match — node is the real caller. Same concern applies to pip/pip3 in the pypi preset (they're Python wrappers).
Can you verify how the sandbox engine resolves the binary? If it checks the top-level process, these are fine. If it checks the process making the network call, we need node and python3 here instead (or in addition).
Also, npx uses the same registries and should be included:
| - { path: /usr/local/bin/yarn } | |
| binaries: | |
| - { path: /usr/local/bin/npm } | |
| - { path: /usr/local/bin/npx } | |
| - { path: /usr/local/bin/yarn } |
| - allow: { method: GET, path: "/**" } | ||
| binaries: | ||
| - { path: /usr/local/bin/pip } | ||
| - { path: /usr/local/bin/pip3 } |
There was a problem hiding this comment.
Same wrapper-script concern as npm — pip and pip3 are Python scripts that delegate to python3. If the sandbox checks the process making the actual network call, these won't match.
| - { path: /usr/local/bin/pip3 } | |
| binaries: | |
| - { path: /usr/local/bin/pip } | |
| - { path: /usr/local/bin/pip3 } | |
| - { path: /usr/local/bin/python3 } |
|
|
||
| const missing = []; | ||
| for (const file of presets) { | ||
| const content = fs.readFileSync(path.join(PRESETS_DIR, file), "utf-8"); |
There was a problem hiding this comment.
This substring check would pass if binaries: appeared in a YAML comment. The baseline test already does proper structure-aware parsing — this should be at least a regex match for a real YAML key:
| const content = fs.readFileSync(path.join(PRESETS_DIR, file), "utf-8"); | |
| if (!/^\s+binaries:\s*$/m.test(content)) { |
cv
left a comment
There was a problem hiding this comment.
LGTM — good security hardening. Restricting network policies to specific binaries closes the exfiltration vector while preserving the out-of-box experience. Regression tests ensure future presets can't skip this.
…policies Restrict messaging endpoints (Telegram, Discord) in the baseline sandbox policy and all presets to specific binaries, preventing arbitrary processes (curl, wget, python) from reaching messaging APIs for data exfiltration. Closes #272
198e3d3 to
77c03c3
Compare
cv
left a comment
There was a problem hiding this comment.
Binary path restrictions are consistent with existing baseline entries (openclaw_docs, npm_registry). The test enforces that all future network_policies entries and presets include a binaries: section.
Two notes for awareness (not blocking):
- The restriction is path-based. An attacker with write access to /usr/local/bin could replace or symlink an allowed binary. This is defense-in-depth, not a hard boundary — consistent with how the existing policies use this field.
- node -e allows arbitrary HTTP requests that satisfy the /usr/local/bin/node path check. Inherent to interpreter-based restrictions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/security-binaries-restriction.test.js (1)
54-60: Minor inconsistency with baseline test regex.The baseline test (line 43) uses
/^\s+binaries:/which accepts inline values (e.g.,binaries: [/usr/bin/node]), while this preset test uses/^\s+binaries:\s*$/mwhich requiresbinaries:to be on its own line. This is fine if all presets use the multi-line list format, but could cause false negatives if someone adds a preset with inline syntax.For consistency, consider aligning with the baseline approach:
Optional: align regex with baseline test
for (const file of presets) { const content = fs.readFileSync(path.join(PRESETS_DIR, file), "utf-8"); - if (!/^\s+binaries:\s*$/m.test(content)) { + if (!/^\s+binaries:/m.test(content)) { missing.push(file); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/security-binaries-restriction.test.js` around lines 54 - 60, The preset test currently uses a stricter regex (/^\s+binaries:\s*$/m) that requires "binaries:" to be on its own line; update the check in the loop that reads PRESETS_DIR (where presets, missing, and content are used) to use the same relaxed pattern as the baseline (e.g., match /^\s+binaries:/ with the m flag) so inline values like "binaries: [/usr/bin/node]" are accepted; locate the for loop over presets and replace the regex used in the if condition accordingly to align behavior with the baseline test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/security-binaries-restriction.test.js`:
- Around line 54-60: The preset test currently uses a stricter regex
(/^\s+binaries:\s*$/m) that requires "binaries:" to be on its own line; update
the check in the loop that reads PRESETS_DIR (where presets, missing, and
content are used) to use the same relaxed pattern as the baseline (e.g., match
/^\s+binaries:/ with the m flag) so inline values like "binaries:
[/usr/bin/node]" are accepted; locate the for loop over presets and replace the
regex used in the if condition accordingly to align behavior with the baseline
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 993cdcd6-d3ab-48be-a3ad-5037cccdfc9a
📒 Files selected for processing (1)
test/security-binaries-restriction.test.js
…policies (NVIDIA#645) * fix(security): add binaries restrictions to all messaging and preset policies Restrict messaging endpoints (Telegram, Discord) in the baseline sandbox policy and all presets to specific binaries, preventing arbitrary processes (curl, wget, python) from reaching messaging APIs for data exfiltration. Closes NVIDIA#272 * fix: add interpreter binaries to npm/pypi presets, strengthen test regex * fix: convert node:test to vitest in security-binaries-restriction.test.js --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
…policies (NVIDIA#645) * fix(security): add binaries restrictions to all messaging and preset policies Restrict messaging endpoints (Telegram, Discord) in the baseline sandbox policy and all presets to specific binaries, preventing arbitrary processes (curl, wget, python) from reaching messaging APIs for data exfiltration. Closes NVIDIA#272 * fix: add interpreter binaries to npm/pypi presets, strengthen test regex * fix: convert node:test to vitest in security-binaries-restriction.test.js --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
…policies (NVIDIA#645) * fix(security): add binaries restrictions to all messaging and preset policies Restrict messaging endpoints (Telegram, Discord) in the baseline sandbox policy and all presets to specific binaries, preventing arbitrary processes (curl, wget, python) from reaching messaging APIs for data exfiltration. Closes NVIDIA#272 * fix: add interpreter binaries to npm/pypi presets, strengthen test regex * fix: convert node:test to vitest in security-binaries-restriction.test.js --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Summary
Add
binaries:restrictions to all messaging entries in the baseline sandbox policy and all policy presets, preventing arbitrary processes (curl, wget, python) from reaching external APIs for data exfiltration.Credit to @gn00295120 whose work on #611 identified the exfiltration risk from unrestricted messaging endpoints in the baseline policy. This PR takes an alternative approach — adding binary restrictions rather than removing messaging from the baseline — to preserve the out-of-box experience for hobbyist users while closing the exfiltration vector.
Changes
Baseline policy (
openclaw-sandbox.yaml):/usr/local/bin/nodeAll presets (
presets/*.yaml):/usr/local/bin/node/usr/bin/docker/usr/local/bin/python3,/usr/local/bin/node/usr/local/bin/node/usr/local/bin/npm,/usr/local/bin/yarn/usr/local/bin/pip,/usr/local/bin/pip3Regression test (
test/security-binaries-restriction.test.js):network_policiesentry has abinaries:sectionbinaries:sectionCloses #272
Test plan
node --test test/security-binaries-restriction.test.js— 2/2 passnpm test— 189/189 passSummary by CodeRabbit
Chores
Tests