fix(sandbox): reject a symlinked corporate CA at managed startup - #8668
Conversation
The managed startup entrypoints tested the baked corporate CA path with `[ -s ]` and then read it with `cat`. Both follow symlinks, so replacing /usr/local/share/nemoclaw/corporate-ca.pem with a symlink made startup merge the symlink target into the runtime trust bundle and report success. Untrusted trust-anchor content reached the bundle that curl, python, git, and node then verify against. The image bakes that path as a root-owned 0444 regular file, so a symlink there is never a legitimate state. Reject it before any read. This fails closed instead of warning, unlike the recoverable merge failures below it, because continuing would consume attacker-chosen bytes rather than fall back to OpenShell-only trust. The documented contract already required this: "Every imported source must be a regular, readable, non-symlink PEM file that is not group-writable or world-writable." The runtime did not enforce it. Both the OpenClaw and Hermes entrypoints carried the defect; the merged-bundle output path already had the equivalent guard. Closes #8650 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
📝 WalkthroughWalkthroughBoth startup scripts now reject symlinked or non-regular corporate CA paths. Descriptor-based reads prevent symlink replacement during merging. Parameterized tests cover OpenClaw and Hermes, including diagnostics and merged-bundle absence. ChangesCorporate CA merge protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 3196-3199: Replace the separate symlink check and later read in
scripts/nemoclaw-start.sh:3196-3199 and agents/hermes/start.sh:1672-1675 with a
no-follow file-descriptor read or equivalent atomic validation/read operation,
preserving rejection of symlinks and preventing replacement races. In
test/corporate-ca-runtime-merge.test.ts:58-81, add a deterministic
replacement-after-validation test that verifies the merged CA bundle is not
created.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2258fe0f-d8d8-489a-bc34-eb0c56f224d2
📒 Files selected for processing (3)
agents/hermes/start.shscripts/nemoclaw-start.shtest/corporate-ca-runtime-merge.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
The path check added for #8650 rejects a symlink planted before startup, but it resolves the path a second time when `cat` reads the file. A process that replaces the pathname between the two operations redirects the read through a new symlink, so attacker-selected bytes still reach the trust bundle. Append through a descriptor opened with O_NOFOLLOW and verified as a regular file. The type check and the read now share one descriptor, so no second path resolution exists to race. This matches how the TypeScript managed-startup reader already opens managed files. An ordinary read failure keeps the existing warn-and-continue behavior; only a rejected trust anchor fails closed, so a permission or I/O error still falls back to OpenShell-only trust rather than stopping the sandbox. Refs #8650 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
The The corporate CA is now appended through a descriptor opened with Failure handling stays split so this does not widen what stops a sandbox:
Test coverage is the extracted-entrypoint test the finding asked for. It swaps the CA file for a symlink immediately after the path check, so only the nofollow read can still reject it, and asserts a non-zero exit, that the diagnostic never echoes the link target's contents, and that no merged bundle is produced. That test is red on the previous commit and green on this one, which isolates the new defense: 894f586 already contained the With the fix restored, the file is 14 passed, and the 7 suites that source these entrypoints are 65 passed / 3 skipped. I also took the Still unverified by me: I reproduced this only at the shell-block level through the existing harness that slices the real Signed-off-by: Dongni Yang dongniy@nvidia.com |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/corporate-ca-runtime-merge.test.ts (1)
106-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the swap injection fail loudly if the anchor line changes.
.replace()returns the input unchanged when the literal[ -s "$_NEMOCLAW_CORPORATE_CA_FILE" ] || return 0is not found. Any reformatting of that line in either script turns the race setup into a no-op. The test then exercises the ordinary success path instead of the swap.Assert that the replacement applied.
♻️ Proposed guard
+ const anchor = `[ -s "$_NEMOCLAW_CORPORATE_CA_FILE" ] || return 0`; + const block = mergeBlock(script, end, corp, merged); + expect(block).toContain(anchor); - const raced = mergeBlock(script, end, corp, merged).replace( - `[ -s "$_NEMOCLAW_CORPORATE_CA_FILE" ] || return 0`, - `[ -s "$_NEMOCLAW_CORPORATE_CA_FILE" ] || return 0\n${swap}`, - ); + const raced = block.replace(anchor, `${anchor}\n${swap}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/corporate-ca-runtime-merge.test.ts` around lines 106 - 109, Update the swap injection in the test around mergeBlock so it verifies that the anchor replacement actually occurred. Assert that the result differs from the original merged script, causing the test to fail loudly when the corporate CA anchor line changes or is absent.Source: Path instructions
scripts/nemoclaw-start.sh (1)
3241-3270: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThe corporate CA merge logic is now duplicated verbatim across both entrypoints.
merge_corporate_proxy_cais byte-identical in both scripts, and this PR adds about 30 more duplicated lines, including a security-critical embedded Python program. Any future correction to the trust-anchor check must be applied twice, and a partial fix would leave one entrypoint weaker than the other. TheELOOP-only errno mapping is one concrete example that would need editing in both copies.
scripts/nemoclaw-start.sh#L3241-L3270: extract the embedded Python appender into a shared file, for examplescripts/lib/append-corporate-ca.py, and invoke it withpython3 -I <path> "$_NEMOCLAW_CORPORATE_CA_FILE" "$_tmp".agents/hermes/start.sh#L1724-L1746: invoke the same shared file instead of carrying a second copy of the program.If the two entrypoints cannot share a file because they are baked into separate images, add a check that fails CI when the two
merge_corporate_proxy_cabodies diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/nemoclaw-start.sh` around lines 3241 - 3270, The corporate CA appender is duplicated across both entrypoints. Extract the embedded Python program from scripts/nemoclaw-start.sh lines 3241-3270 into a shared scripts/lib/append-corporate-ca.py and invoke it with python3 -I and the source/target arguments; update agents/hermes/start.sh lines 1724-1746 to use the same file instead of embedding its own copy. If sharing is impossible, add CI validation that both merge_corporate_proxy_ca bodies remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 3241-3270: The corporate CA appender is duplicated across both
entrypoints. Extract the embedded Python program from scripts/nemoclaw-start.sh
lines 3241-3270 into a shared scripts/lib/append-corporate-ca.py and invoke it
with python3 -I and the source/target arguments; update agents/hermes/start.sh
lines 1724-1746 to use the same file instead of embedding its own copy. If
sharing is impossible, add CI validation that both merge_corporate_proxy_ca
bodies remain identical.
In `@test/corporate-ca-runtime-merge.test.ts`:
- Around line 106-109: Update the swap injection in the test around mergeBlock
so it verifies that the anchor replacement actually occurred. Assert that the
result differs from the original merged script, causing the test to fail loudly
when the corporate CA anchor line changes or is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3dd28e8-0682-436c-95c7-7551071c1752
📒 Files selected for processing (3)
agents/hermes/start.shscripts/nemoclaw-start.shtest/corporate-ca-runtime-merge.test.ts
wscurran
added
bug-fix
3 participants
Summary
The managed startup entrypoints tested the baked corporate CA path with
[ -s ]and then read it withcat. Both follow symlinks, so replacing/usr/local/share/nemoclaw/corporate-ca.pemwith a symlink made startup merge the symlink target into the runtime trust bundle and report success. After this change, startup rejects a symlink at that path before any read and exits non-zero with a diagnostic that names the path only.Related Issue
Closes #8650
Changes
scripts/nemoclaw-start.shandagents/hermes/start.sh: reject a symlinked corporate CA inmerge_corporate_proxy_cabefore the first read.test/corporate-ca-runtime-merge.test.ts: one case per entrypoint that plants a symlink to a non-certificate file, asserts the non-zero exit and the diagnostic, and asserts no merged bundle is produced.This adds no abstraction, configuration, fallback, or compatibility layer. It adds one guard on one path.
Why this one fails closed
Every other failure in
merge_corporate_proxy_cawarns and returns, leaving OpenShell-only trust intact. That is correct for those cases: a failed temp-file creation or a failedchmodmeans no corporate anchor is added, which is safe. A symlink at the baked path is different in kind. The image writes that path as a root-owned0444regular file (Dockerfile:673-674), so a symlink there is never a legitimate state, and continuing would merge attacker-chosen bytes into the bundle that curl, python, git, and node verify against. Failing closed cannot break a healthy sandbox, because a healthy sandbox never has a symlink there.The merged-bundle output path already carried the equivalent guard, so this restores symmetry between the two ends of the same function.
Already the documented contract
docs/security/configure-corporate-ca-trust.mdxstates: "Every imported source must be a regular, readable, non-symlink PEM file that is not group-writable or world-writable." The runtime did not enforce the non-symlink half. The TypeScript managed-startup reader already enforces it by opening withO_NOFOLLOW(src/lib/onboard/managed-startup/image-runtime.ts); only the two shell entrypoints were out of compliance.Type of Change
Quality Gates
docs/security/configure-corporate-ca-trust.mdxalready documents the non-symlink requirement this change enforces. No page documented the previous permissive behavior.Documentation Writer Review
no-docs-neededdocs/security/configure-corporate-ca-trust.mdxanddocs/reference/troubleshooting.mdx. The corporate CA page already requires a regular, non-symlink PEM source, so this change makes the runtime match the published contract rather than altering it.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project integration test/corporate-ca-runtime-merge.test.ts— 12 passed. Both new cases fail on the unfixed scripts (expected [Function] to throw an error) and pass after. The seven suites that source these entrypoints pass together: 63 passed, 3 skipped.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Notes for reviewers
Shell gates:
shfmt -i 2 -ci -bn -dclean on both scripts.shellcheckoutput is byte-identical before and after this change (7 findings both ways, all pre-existing and outside the changed region) — verified by re-running against the stashed tree.npm run checks:repositorypasses, including the source-shape test budget and the test file size budget.I verified the defect by execution rather than by reading: the two new cases are red on unmodified
mainbecause the merge currently succeeds on a symlinked source, and green after. No sandbox build or GPU is needed, becausetest/corporate-ca-runtime-merge.test.tsslices the real merge block out of both entrypoints and runs it underbash.The reproduction in #8650 used
--agent openclaw, which routes toscripts/nemoclaw-start.sh. While confirming that, I foundagents/hermes/start.shcarried the identical[ -s ]+catpattern, so both are fixed and both are covered.agents/langchain-deepagents-code/start.shhas no corporate CA merge and needed no change.Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit