security(sim): sandbox + harden render(output_path=...) against traversal, symlink, oversize, partial-write

[cagataycali]

Summary

Simulation.render(output_path=...) (added in #915) is an LLM-callable tool, so its destination path is attacker-influenced. The guard shipped in #915 was a shell-metacharacter blacklist plus a /-only .. check, which leaves several arbitrary-write vectors open. This hardens it.

Vectors the current guard accepts (pre-fix)

/etc/cron.d/x          -> ACCEPTED  (absolute path, no sandbox)
..\..\etc\passwd      -> ACCEPTED  (backslash traversal; split('/') misses it)
./link.png -> symlink  -> ACCEPTED  (abspath does not resolve symlinks)

There is also no payload-size cap and the write is non-atomic (open(path,'wb') truncates the target before the write completes, so a crash mid-write corrupts an existing file in place).

Change

Replaces the inline block in render() with three small, GL-free helpers in strands_robots/simulation/mujoco/rendering.py:

• _validate_render_output_path - rejects shell metacharacters, backslash separators, and symlinked targets; Path.resolve() normalizes .. and follows the chain, then the destination is confined to a sandbox root.
• _max_render_bytes / size cap - PNGs larger than the limit are refused without writing.
• _atomic_write_png - tempfile.mkstemp + os.replace, so a crash mid-write cannot corrupt an existing file. The render sandbox is private to the running user, so created files are 0o600 and freshly created dirs 0o700 (owner-only, no world/group access).

New configuration (documented in README)

Env var	Purpose	Default
STRANDS_ROBOTS_RENDER_ROOT	Sandbox dir render(output_path=...) may write into	~/.strands_robots/renders/
STRANDS_ROBOTS_RENDER_ALLOW_ABS	Set 1 to allow absolute paths outside the sandbox	unset
STRANDS_ROBOTS_RENDER_MAX_BYTES	Max PNG size persisted	52428800 (50 MB)

Tests

tests/simulation/mujoco/test_render_output_path.py (22 cases, GL-free, Linux+macOS). Each vector above is asserted rejected, plus: valid in-sandbox write, absolute opt-in, oversize rejection, malformed size-cap env, owner-only perms (0o600/0o700), and an atomic-write test that monkeypatches os.replace to fail and asserts the existing file content is preserved with no temp-file leak. The traversal/symlink/absolute cases fail against the pre-fix code and pass after.

22 passed   # test_render_output_path.py
123 passed, 12 skipped   # + test_rendering / test_agenttool_contract / test_tool_spec

Lint clean: ruff check, ruff format --check, mypy strands_robots tests tests_integ (Success, 0 issues).

Visual proof (headless MuJoCo, MUJOCO_GL=egl)

SO-101 rendered via the hardened path: a valid in-sandbox path saves (0o600), while an absolute escape and backslash traversal are rejected with status=error.

VALID  -> success | 640x480 -> saved <sandbox>/demo/render_ok.png   (perms 0o600)
ESCAPE -> error   | render: output_path /etc/cron.d/evil is outside the render sandbox ...
BSLASH -> error   | render: unsafe output_path: backslash separators not allowed

Follow-up (out of scope here): start_cameras_recording / export_xml(output_path=...) take LLM-supplied paths with the same metachar-only pattern and should get the same treatment.

Refs #915.

[yinsong1986]

Summary

Hardens Simulation.render(output_path=...) (an LLM-callable, attacker-influenced sink) against the arbitrary-write vectors that the #915 metachar-blacklist guard let through. The inline guard is replaced by three GL-free helpers in rendering.py: _validate_render_output_path (rejects shell metacharacters, backslash separators, symlinked targets, and confines the Path.resolve()-normalized destination to a sandbox root), a size cap via _max_render_bytes, and _atomic_write_png (tempfile.mkstemp + os.replace with temp cleanup on failure). Three new documented env vars (STRANDS_ROBOTS_RENDER_ROOT, STRANDS_ROBOTS_RENDER_ALLOW_ABS, STRANDS_ROBOTS_RENDER_MAX_BYTES) control sandbox root, absolute opt-out, and the cap.

The diff matches its description and is a net security improvement. The validation logic is sound: .. traversal and absolute escapes are caught by resolve() + relative_to(root); intermediate symlinks that escape the sandbox are also caught by the same containment check; oversize payloads are rejected before any write. The error contract is intact — _save_render_png raises ValueError mapped to a tool error dict, and an OSError from _atomic_write_png (e.g. EACCES creating the parent dir) is caught by render()'s outer except Exception and returned as a structured status=error rather than crashing the hot path. New env vars are documented in README per the PR #86 "document every env var" learning, and the validate-before-sink pattern follows the PR #92 LLM-input-safety convention. 22 GL-free regression tests pin each vector. No blocking concerns found.

[cagataycali]

Addressed the two CodeQL "overly permissive file permissions" alerts (593/594): the render sandbox is private to the running user, so created files are now 0o600 and freshly created dirs 0o700 (owner-only) instead of 0o644/0o755. Perms assertions updated to match; scoped mypy clean and the 22-case suite passes.

[yinsong1986]

Summary

Hardens the LLM-callable Simulation.render(output_path=...) sink (added in #915) against the arbitrary-write vectors the old metachar-blacklist + /-only .. guard let through. The inline block is replaced by three GL-free helpers in rendering.py: _validate_render_output_path (rejects shell metacharacters, backslash separators, and symlinked final targets, then Path.resolve()-normalizes ../symlink chains and confines the destination under a sandbox root), a size cap via _max_render_bytes, and _atomic_write_png (tempfile.mkstemp + os.replace with owner-only 0o600/0o700 perms). The guard is fail-closed: absolute paths, sibling-of-sandbox prefixes, and CWD-relative paths that resolve outside the root are all rejected unless the operator opts in with STRANDS_ROBOTS_RENDER_ALLOW_ABS=1. I traced each advertised vector and confirmed the resolve()+relative_to() confinement is path-component aware (not a string-prefix compare) and that intermediate symlinks are resolved before the sandbox check.

What's good

• New env vars (STRANDS_ROBOTS_RENDER_ROOT / _ALLOW_ABS / _MAX_BYTES) are documented in README's Configuration table and CHANGELOG, matching the AGENTS.md rule that every STRANDS_* var is documented in the same PR.
• Error paths return structured {"status": "error", ...} dicts (ValueError caught in render()); no uncaught exceptions on the hot path.
• Malformed STRANDS_ROBOTS_RENDER_MAX_BYTES raises rather than silently defaulting - matches AGENTS.md "warn/raise on unrecognized env values".
• The CodeQL world-readable finding on the prior commit (0o644/0o755) is already addressed in the head commit (0o600/0o700), with matching perms assertions in the tests.
• Comprehensive GL-free regression tests cover traversal, backslash, symlink, absolute opt-in, oversize, malformed-cap, perms, and atomic-write-failure-preserves-existing.

Verification suggestions

Standard CI (hatch run test, ruff, mypy) is sufficient. To spot-check confinement interactively: STRANDS_ROBOTS_RENDER_ROOT=/tmp/sbx python -c "from strands_robots.simulation.mujoco.rendering import _validate_render_output_path as v; v('/etc/cron.d/x')" should raise ValueError (outside sandbox).

[cagataycali]

Closing as already-merged. The full substance of this PR is now present on main: strands_robots/simulation/mujoco/rendering.py carries the sandbox-root confinement, traversal/symlink rejection, oversize cap, and atomic partial-write guard, and tests/simulation/mujoco/test_render_output_path.py (10 pinned regression tests) is in place. git diff main...<this-branch> after merging main in resolves to an empty tree — there is no remaining delta to land. The hardening reached main via the security/harden-device-connect-tools line, so this branch is now a no-op. Closing to keep the PR queue drift-free; no code is lost.