Skip to content

Commit ec8b347

Browse files
congwang-mkcongwang
authored andcommitted
fix(sandbox): address review feedback on execute_file and ABI message
Code-review fixes from PR #1367: * execute_file no longer allowlists the script's entire parent directory for Landlock read. Only the script file itself is granted read access, so sibling files on the host are never exposed (gemini flagged this as high severity). Verified end-to-end against real sandlock: a script that tries to read a sibling file is denied. * _build_sandbox_kwargs filters extra_readable with os.path.exists instead of os.path.isdir, so individual files (not just directories) can be allowlisted — required for the file-only execute_file path. * Corrected the Landlock kernel version in the fail-loud error message. The wrapper now requires ABI >= min_landlock_abi() (currently v6), which shipped in Linux 6.12 — not 6.7. The message is phrased around the ABI version so it stays correct as the SDK's minimum changes. * test_sandlock_execution_success now uses bytes for stdout/stderr (to exercise the _decode() byte path, matching real sandlock) and sets result.success = True explicitly. Note: the suggestion to also add temp_dir/working_dir to fs_readable was not applied — sandlock's fs_writable already grants read access ("fs_readable ... in addition to writable paths"), confirmed by test.
1 parent c0c4eee commit ec8b347

2 files changed

Lines changed: 21 additions & 13 deletions

File tree

src/praisonai/praisonai/sandbox/sandlock.py

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,9 @@ def __init__(
7676
)
7777

7878
# Fail loud if Landlock isn't supported on this kernel. sandlock
79-
# requires a minimum Landlock ABI (currently v6, Linux 6.7+); query
80-
# it rather than hard-coding so we track the SDK's own requirement.
79+
# requires a minimum Landlock ABI (currently v6, which shipped in
80+
# Linux 6.12); query it rather than hard-coding so we track the SDK's
81+
# own requirement.
8182
try:
8283
abi = self._sandlock.landlock_abi_version()
8384
min_abi = self._sandlock.min_landlock_abi()
@@ -87,10 +88,11 @@ def __init__(
8788
) from e
8889
if abi < min_abi:
8990
raise RuntimeError(
90-
"SandlockSandbox requires Landlock support (Linux kernel "
91-
f">= 6.7 with CONFIG_SECURITY_LANDLOCK=y and ABI >= {min_abi}). "
92-
f"This kernel reports Landlock ABI version {abi}. Use "
93-
"SubprocessSandbox explicitly if weaker isolation is acceptable."
91+
"SandlockSandbox requires Landlock ABI >= "
92+
f"{min_abi} (Linux kernel >= 6.12 with "
93+
"CONFIG_SECURITY_LANDLOCK=y). This kernel reports Landlock "
94+
f"ABI version {abi}. Use SubprocessSandbox explicitly if "
95+
"weaker isolation is acceptable."
9496
)
9597

9698
@property
@@ -159,8 +161,12 @@ def _build_sandbox_kwargs(
159161
]
160162
allowed_read_paths = [p for p in _candidate_read_paths if os.path.isdir(p)]
161163
if extra_readable:
164+
# extra_readable may name individual files (e.g. the single script
165+
# passed to execute_file), so accept any path that exists — not
166+
# just directories. Landlock grants read on a named file without
167+
# exposing its siblings.
162168
allowed_read_paths.extend(
163-
p for p in extra_readable if os.path.isdir(p)
169+
p for p in extra_readable if os.path.exists(p)
164170
)
165171

166172
allowed_write_paths = []
@@ -358,9 +364,9 @@ async def execute_file(
358364
"""Execute a file in the sandbox.
359365
360366
The script is passed to the interpreter by path rather than slurped
361-
through ``-c``, so large scripts don't hit ARG_MAX. The file's
362-
parent directory is added to the Landlock read allowlist for this
363-
run so the sandboxed process can actually open it.
367+
through ``-c``, so large scripts don't hit ARG_MAX. Only the script
368+
file itself — not its parent directory is added to the Landlock
369+
read allowlist, so sibling files on the host are never exposed.
364370
"""
365371
if not self._is_running:
366372
await self.start()
@@ -386,7 +392,7 @@ async def execute_file(
386392
limits=limits,
387393
env=env,
388394
working_dir=self._temp_dir,
389-
extra_readable=[os.path.dirname(abs_path)],
395+
extra_readable=[abs_path],
390396
)
391397

392398
async def run_command(

src/praisonai/tests/unit/sandbox/test_sandlock_sandbox.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,11 @@ async def test_sandlock_execution_success(self):
117117
"""Test successful code execution with sandlock."""
118118
mock_sandlock = Mock()
119119
mock_result = Mock()
120+
mock_result.success = True
120121
mock_result.exit_code = 0
121-
mock_result.stdout = "Hello, World!"
122-
mock_result.stderr = ""
122+
# Real sandlock returns bytes; exercise the _decode() byte path.
123+
mock_result.stdout = b"Hello, World!"
124+
mock_result.stderr = b""
123125

124126
mock_sandlock.Sandbox.return_value = Mock(run=Mock(return_value=mock_result))
125127
mock_sandlock.landlock_abi_version.return_value = 6 # >= 6, so available

0 commit comments

Comments
 (0)