fix(security): add binaries restrictions to all messaging and preset policies#645
fix(security): add binaries restrictions to all messaging and preset policies#645
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded 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)
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
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
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
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.
wscurran
added
security
…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
cv
force-pushed
the
fix/messaging-binaries-restriction
branch
from
198e3d3 to
77c03c3
Compare
3 participants
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