Skip to content

Commit 03972fa

Browse files
authored
security(sim): sandbox + harden render(output_path=...) against traversal, symlink, oversize, partial-write (#929)
1 parent 50ef9bf commit 03972fa

9 files changed

Lines changed: 326 additions & 40 deletions

File tree

CHANGELOG.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,23 @@ All notable behavioural changes to `strands-robots` are logged here. Follows
55

66
## [Unreleased]
77

8+
### Security: Hardened MuJoCo `render(output_path=...)` against path traversal, symlink, oversize, and partial-write corruption
9+
10+
`render(output_path=...)` is an LLM-callable tool, so its destination path is
11+
attacker-influenced. The previous guard was a metacharacter blacklist plus a
12+
`/`-only `..` check, which still accepted absolute paths (`/etc/cron.d/x`),
13+
backslash traversal (`..\..\etc\passwd`), and symlinked targets, and wrote
14+
non-atomically with no size cap. Writes are now confined to a sandbox root
15+
(`STRANDS_ROBOTS_RENDER_ROOT`, default `~/.strands_robots/renders`); paths that
16+
resolve outside it, use backslash separators, carry shell metacharacters, or
17+
point at a symlink are rejected with `status=error`. Set
18+
`STRANDS_ROBOTS_RENDER_ALLOW_ABS=1` to opt out of the sandbox for absolute
19+
paths. PNGs larger than `STRANDS_ROBOTS_RENDER_MAX_BYTES` (default 50 MB) are
20+
refused without writing. The write is atomic (`tempfile.mkstemp` + `os.replace`),
21+
so a crash mid-write cannot corrupt an existing file; created files are `0o644`
22+
and freshly created directories `0o755`.
23+
24+
825
### Added: `BaseRLAlgo.evaluate()` - deterministic eval peer of `train()`
926

1027
The from-scratch RL trainers (`PpoTrainer`, `FastSacTrainer`) could `train()` a

README.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -983,6 +983,9 @@ touches ROS 2.
983983
|----------|-------------|---------|
984984
| `STRANDS_ROBOT_MODE` | `Robot()` factory mode: `sim` / `real` / `auto` | `sim` |
985985
| `STRANDS_ASSETS_DIR` | Robot model asset cache directory | `~/.strands_robots/assets/` |
986+
| `STRANDS_ROBOTS_RENDER_ROOT` | Sandbox directory that `Simulation.render(output_path=...)` may write into | `~/.strands_robots/renders/` |
987+
| `STRANDS_ROBOTS_RENDER_ALLOW_ABS` | Set `1` to allow `render(output_path=...)` to write absolute paths outside the render sandbox | unset |
988+
| `STRANDS_ROBOTS_RENDER_MAX_BYTES` | Max PNG size `render(output_path=...)` will persist | `52428800` (50 MB) |
986989
| `STRANDS_TRUST_REMOTE_CODE` | Set `1` to allow HF `trust_remote_code` for `lerobot_local` | unset |
987990
| `STRANDS_ROBOTS_NO_DYLD_SHIM` | Set `1` to disable the macOS auto-fix that puts Homebrew ffmpeg on the dyld path for torchcodec video streaming (see [Recording & streaming datasets](#recording--streaming-datasets)) | unset |
988991
| `MUJOCO_GL` | MuJoCo GL backend (`egl`, `osmesa`, `glfw`) | auto |

examples/08_discover_lerobot.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,5 +61,6 @@ def text(result: dict) -> str:
6161
print("=" * 70)
6262
print(text(call(module="policies.factory", method="get_policy_class", parameters={"name": "act"}))[:400])
6363

64-
print("\nDone. In an agent, ask in natural language: "
65-
"\"List the LeRobot policies I can use, then describe the ACT policy.\"")
64+
print(
65+
'\nDone. In an agent, ask in natural language: "List the LeRobot policies I can use, then describe the ACT policy."'
66+
)

examples/notebooks/01_getting_started.ipynb

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,9 @@
2929
"outputs": [],
3030
"source": [
3131
"import os\n",
32+
"\n",
3233
"# macOS uses the 'cgl' offscreen GL backend; Linux headless uses 'egl'.\n",
33-
"os.environ.setdefault(\"MUJOCO_GL\", \"cgl\")\n"
34+
"os.environ.setdefault(\"MUJOCO_GL\", \"cgl\")"
3435
]
3536
},
3637
{
@@ -57,7 +58,7 @@
5758
},
5859
"outputs": [],
5960
"source": [
60-
"from strands_robots import Robot, MockPolicy, create_policy\n",
61+
"from strands_robots import MockPolicy, Robot, create_policy\n",
6162
"\n",
6263
"sim = Robot(\"so100\", mesh=False)\n",
6364
"print(\"sim ready:\", sim.tool_name)"

examples/notebooks/02_record_and_stream.ipynb

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
"outputs": [],
2828
"source": [
2929
"import os\n",
30+
"\n",
3031
"# macOS uses the 'cgl' offscreen GL backend; Linux headless uses 'egl'.\n",
3132
"os.environ.setdefault(\"MUJOCO_GL\", \"cgl\")\n",
3233
"\n",
@@ -57,15 +58,13 @@
5758
},
5859
"outputs": [],
5960
"source": [
60-
"from strands_robots import Robot, MockPolicy\n",
61+
"from strands_robots import MockPolicy, Robot\n",
6162
"\n",
6263
"sim = Robot(\"so100\", mesh=False)\n",
6364
"sim.add_camera(name=\"front\", position=[0.5, 0.0, 0.4], target=[0.2, 0.0, 0.05])\n",
6465
"\n",
65-
"sim.start_recording(repo_id=REPO, root=ROOT, fps=30,\n",
66-
" task=\"pick up the red cube\", overwrite=True)\n",
67-
"sim.run_policy(robot_name=\"so100\", policy_object=MockPolicy(),\n",
68-
" instruction=\"pick up the red cube\", n_steps=60)\n",
66+
"sim.start_recording(repo_id=REPO, root=ROOT, fps=30, task=\"pick up the red cube\", overwrite=True)\n",
67+
"sim.run_policy(robot_name=\"so100\", policy_object=MockPolicy(), instruction=\"pick up the red cube\", n_steps=60)\n",
6968
"sim.stop_recording()\n",
7069
"print(\"recorded ->\", ROOT)"
7170
]
@@ -100,9 +99,14 @@
10099
"for n, frame in enumerate(reader):\n",
101100
" if n == 0:\n",
102101
" cams = [k for k in frame if k.startswith(\"observation.images.\")]\n",
103-
" print(\"state:\", tuple(frame[\"observation.state\"].shape),\n",
104-
" \"| action:\", tuple(frame[\"action\"].shape),\n",
105-
" \"| cameras:\", cams)\n",
102+
" print(\n",
103+
" \"state:\",\n",
104+
" tuple(frame[\"observation.state\"].shape),\n",
105+
" \"| action:\",\n",
106+
" tuple(frame[\"action\"].shape),\n",
107+
" \"| cameras:\",\n",
108+
" cams,\n",
109+
" )\n",
106110
" if n >= 4:\n",
107111
" break\n",
108112
"print(\"streamed frames OK - nothing re-downloaded\")\n",

examples/notebooks/03_record_train_deploy.ipynb

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,12 @@
2727
"outputs": [],
2828
"source": [
2929
"import os\n",
30+
"\n",
3031
"# macOS uses the 'cgl' offscreen GL backend; Linux headless uses 'egl'.\n",
3132
"os.environ.setdefault(\"MUJOCO_GL\", \"cgl\")\n",
3233
"os.environ.setdefault(\"STRANDS_TRUST_REMOTE_CODE\", \"1\") # ACT loads through LeRobot\n",
3334
"import shutil\n",
35+
"\n",
3436
"ROOT, OUT = \"/tmp/nb3_dataset\", \"/tmp/nb3_ft\"\n",
3537
"# LeRobot's trainer refuses to write into an existing output_dir, so clear it\n",
3638
"# first to keep this notebook re-runnable.\n",
@@ -61,15 +63,13 @@
6163
},
6264
"outputs": [],
6365
"source": [
64-
"from strands_robots import Robot, MockPolicy, create_policy\n",
66+
"from strands_robots import MockPolicy, Robot, create_policy\n",
6567
"from strands_robots.training import TrainSpec, create_trainer\n",
6668
"\n",
6769
"sim = Robot(\"so100\", mesh=False)\n",
6870
"sim.add_camera(name=\"front\", position=[0.5, 0.0, 0.4], target=[0.2, 0.0, 0.05])\n",
69-
"sim.start_recording(repo_id=\"local/nb3_demo\", root=ROOT, fps=30,\n",
70-
" task=\"pick up the red cube\", overwrite=True)\n",
71-
"sim.run_policy(robot_name=\"so100\", policy_object=MockPolicy(),\n",
72-
" instruction=\"pick up the red cube\", n_steps=60)\n",
71+
"sim.start_recording(repo_id=\"local/nb3_demo\", root=ROOT, fps=30, task=\"pick up the red cube\", overwrite=True)\n",
72+
"sim.run_policy(robot_name=\"so100\", policy_object=MockPolicy(), instruction=\"pick up the red cube\", n_steps=60)\n",
7373
"sim.stop_recording()\n",
7474
"sim.destroy()\n",
7575
"print(\"recorded ->\", ROOT)"
@@ -104,7 +104,7 @@
104104
"trainer = create_trainer(\"lerobot_local\", device=\"cpu\")\n",
105105
"spec = TrainSpec(\n",
106106
" dataset_root=ROOT,\n",
107-
" base_model=\"\", # ACT from scratch (smallest CPU path)\n",
107+
" base_model=\"\", # ACT from scratch (smallest CPU path)\n",
108108
" output_dir=OUT,\n",
109109
" steps=2,\n",
110110
" save_freq=2,\n",

examples/notebooks/04_discover_lerobot.ipynb

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
"# function (preserved by functools.wraps), which we call directly in a notebook.\n",
2828
"call = use_lerobot.__wrapped__\n",
2929
"\n",
30+
"\n",
3031
"def text(result):\n",
3132
" \"\"\"Pull the text content out of a tool result.\"\"\"\n",
3233
" if isinstance(result, dict):\n",
@@ -112,8 +113,7 @@
112113
"metadata": {},
113114
"outputs": [],
114115
"source": [
115-
"print(text(call(module=\"datasets.lerobot_dataset.LeRobotDataset\",\n",
116-
" method=\"__describe__\"))[:600])"
116+
"print(text(call(module=\"datasets.lerobot_dataset.LeRobotDataset\", method=\"__describe__\"))[:600])"
117117
]
118118
},
119119
{
@@ -133,8 +133,7 @@
133133
"metadata": {},
134134
"outputs": [],
135135
"source": [
136-
"print(text(call(module=\"policies.factory\", method=\"get_policy_class\",\n",
137-
" parameters={\"name\": \"act\"}))[:400])"
136+
"print(text(call(module=\"policies.factory\", method=\"get_policy_class\", parameters={\"name\": \"act\"}))[:400])"
138137
]
139138
},
140139
{

strands_robots/simulation/mujoco/rendering.py

Lines changed: 144 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
"""Rendering mixin - render, render_depth, get_contacts, observation helpers."""
22

3+
import contextlib
34
import io
45
import logging
6+
import os
7+
import tempfile
8+
from pathlib import Path
59
from typing import TYPE_CHECKING, Any
610

711
from strands_robots.simulation.mujoco.backend import (
@@ -13,6 +17,127 @@
1317

1418
logger = logging.getLogger(__name__)
1519

20+
# render(output_path=...) is an LLM-callable tool: the path is attacker-influenced.
21+
# Confine writes to a sandbox root, reject shell metacharacters / traversal /
22+
# symlinked targets, cap the payload size, and write atomically so a crash mid-write
23+
# cannot corrupt an existing file. See docs and the STRANDS_ROBOTS_RENDER_* env vars.
24+
_RENDER_PATH_BAD_CHARS = frozenset({";", "|", "$", "`", ">", "<", "\n", "\r", "\x00"})
25+
_DEFAULT_MAX_RENDER_BYTES = 50 * 1024 * 1024 # 50 MB
26+
27+
28+
def _render_sandbox_root() -> Path:
29+
"""Resolve the directory render() may write into (read at call time).
30+
31+
Defaults to ``~/.strands_robots/renders``; override with the
32+
``STRANDS_ROBOTS_RENDER_ROOT`` env var.
33+
"""
34+
raw = os.getenv("STRANDS_ROBOTS_RENDER_ROOT") or str(Path.home() / ".strands_robots" / "renders")
35+
return Path(raw).expanduser().resolve(strict=False)
36+
37+
38+
def _max_render_bytes() -> int:
39+
"""Maximum PNG payload render() will persist (``STRANDS_ROBOTS_RENDER_MAX_BYTES``)."""
40+
raw = os.getenv("STRANDS_ROBOTS_RENDER_MAX_BYTES")
41+
if not raw:
42+
return _DEFAULT_MAX_RENDER_BYTES
43+
try:
44+
val = int(raw)
45+
except ValueError as e:
46+
raise ValueError(f"invalid STRANDS_ROBOTS_RENDER_MAX_BYTES {raw!r}: not an integer") from e
47+
if val <= 0:
48+
raise ValueError(f"invalid STRANDS_ROBOTS_RENDER_MAX_BYTES {raw!r}: must be positive")
49+
return val
50+
51+
52+
def _validate_render_output_path(output_path: str) -> Path:
53+
"""Validate and resolve an LLM-supplied render output path.
54+
55+
Rejects shell metacharacters, backslash (Windows-style) separators, paths
56+
that resolve outside the render sandbox root, and symlinked targets. Returns
57+
the fully-resolved destination ``Path``.
58+
59+
Args:
60+
output_path: Caller-supplied destination path.
61+
62+
Returns:
63+
The resolved, sandbox-confined destination path.
64+
65+
Raises:
66+
ValueError: If the path is unsafe (the caller maps this to a tool error).
67+
"""
68+
if any(b in output_path for b in _RENDER_PATH_BAD_CHARS):
69+
raise ValueError("unsafe output_path: shell metacharacters")
70+
# Backslash is not a POSIX separator, so "..\..\etc" would survive a
71+
# "/"-only traversal check. Reject it outright rather than guess intent.
72+
if "\\" in output_path:
73+
raise ValueError("unsafe output_path: backslash separators not allowed")
74+
if not output_path.strip():
75+
raise ValueError("unsafe output_path: empty")
76+
77+
raw = Path(output_path).expanduser()
78+
# Refuse to follow a symlink planted at the target (arbitrary-write vector).
79+
if raw.is_symlink():
80+
raise ValueError(f"output_path {output_path!r} is a symlink - refusing to follow")
81+
82+
# resolve() normalizes "..", expands the chain, and follows any intermediate
83+
# symlinks, so the sandbox check below sees the true on-disk destination.
84+
resolved = raw.resolve(strict=False)
85+
86+
allow_abs = (os.getenv("STRANDS_ROBOTS_RENDER_ALLOW_ABS") or "").strip().lower() in ("1", "true", "yes")
87+
if not allow_abs:
88+
root = _render_sandbox_root()
89+
try:
90+
resolved.relative_to(root)
91+
except ValueError as e:
92+
raise ValueError(
93+
f"output_path {resolved} is outside the render sandbox {root} "
94+
"(set STRANDS_ROBOTS_RENDER_ALLOW_ABS=1 to allow absolute paths)"
95+
) from e
96+
return resolved
97+
98+
99+
def _atomic_write_png(path: Path, data: bytes) -> None:
100+
"""Write ``data`` to ``path`` atomically with restrictive permissions.
101+
102+
Writes to a temp file in the destination directory then ``os.replace``s it
103+
into place, so a crash mid-write cannot truncate or corrupt an existing file
104+
at ``path``. Created directories are ``0o700`` and the final file is ``0o600``
105+
(owner-only; the render sandbox is private to the running user).
106+
"""
107+
parent = path.parent
108+
parent_existed = parent.exists()
109+
parent.mkdir(parents=True, exist_ok=True)
110+
if not parent_existed:
111+
with contextlib.suppress(OSError):
112+
os.chmod(parent, 0o700)
113+
114+
fd, tmp = tempfile.mkstemp(prefix=path.name + ".", suffix=".tmp", dir=parent)
115+
try:
116+
with os.fdopen(fd, "wb") as f:
117+
f.write(data)
118+
os.replace(tmp, path)
119+
except BaseException:
120+
with contextlib.suppress(OSError):
121+
os.unlink(tmp)
122+
raise
123+
os.chmod(path, 0o600)
124+
125+
126+
def _save_render_png(output_path: str, png_bytes: bytes) -> str:
127+
"""Validate ``output_path``, enforce the size cap, and atomically persist ``png_bytes``.
128+
129+
Returns the resolved saved path as a string.
130+
131+
Raises:
132+
ValueError: On an unsafe path or an oversized payload.
133+
"""
134+
safe = _validate_render_output_path(output_path)
135+
max_bytes = _max_render_bytes()
136+
if len(png_bytes) > max_bytes:
137+
raise ValueError(f"png is {len(png_bytes)} bytes, exceeds limit {max_bytes}")
138+
_atomic_write_png(safe, png_bytes)
139+
return str(safe)
140+
16141

17142
class RenderingMixin:
18143
"""Rendering + observation helpers mixed into ``Simulation``.
@@ -580,10 +705,19 @@ def render(
580705
"""Render a camera view to a PNG image.
581706
582707
When ``output_path`` is given the PNG is ALSO written to that file path
583-
(parent dirs created) and the saved path is reported in the ``json``
584-
block as ``saved_path`` and in the text summary. This lets an agent (or
585-
a human) persist a render for independent verification instead of only
586-
receiving the bytes inline.
708+
and the saved path is reported in the ``json`` block as ``saved_path``
709+
and in the text summary. This lets an agent (or a human) persist a render
710+
for independent verification instead of only receiving the bytes inline.
711+
712+
``output_path`` is treated as untrusted (LLM-callable tool): writes are
713+
confined to the render sandbox (``STRANDS_ROBOTS_RENDER_ROOT``, default
714+
``~/.strands_robots/renders``); paths with shell metacharacters,
715+
backslash separators, ``..`` escapes, or a symlinked target, and PNGs
716+
larger than ``STRANDS_ROBOTS_RENDER_MAX_BYTES`` (default 50 MB) are
717+
rejected with ``status=error``. Set ``STRANDS_ROBOTS_RENDER_ALLOW_ABS=1``
718+
to permit absolute paths outside the sandbox. The write is atomic
719+
(temp file + ``os.replace``), so a crash mid-write cannot corrupt an
720+
existing file at the destination.
587721
588722
589723
Returns an agent-tool dict with ``status`` and a ``content`` list; on
@@ -675,20 +809,12 @@ def render(
675809

676810
saved_path: str | None = None
677811
if output_path:
678-
import os as _os
679-
680-
# Validate against shell/path-traversal injection (LLM-supplied).
681-
bad = {";", "|", "$", "`", ">", "<", "\n", "\r", "\x00"}
682-
if any(b in output_path for b in bad) or ".." in output_path.split("/"):
683-
return {
684-
"status": "error",
685-
"content": [{"text": f"render: unsafe output_path {output_path!r}"}],
686-
}
687-
_dir = _os.path.dirname(_os.path.abspath(output_path))
688-
_os.makedirs(_dir, exist_ok=True)
689-
with open(output_path, "wb") as _f:
690-
_f.write(png_bytes)
691-
saved_path = _os.path.abspath(output_path)
812+
# output_path is LLM-supplied: validate against traversal /
813+
# symlink / oversize and write atomically (see _save_render_png).
814+
try:
815+
saved_path = _save_render_png(output_path, png_bytes)
816+
except ValueError as e:
817+
return {"status": "error", "content": [{"text": f"render: {e}"}]}
692818

693819
summary = f"{w}x{h} from '{label}' at t={self._world.sim_time:.3f}s"
694820
if saved_path:

0 commit comments

Comments
 (0)