mirror of
https://github.com/AstrBotDevs/AstrBot.git
synced 2026-10-16 15:06:04 +08:00
fix: block shared-path attacks on local runtime file exchange
Sandboxed processes can write to shared temp roots, so a named session log there could be replaced with a symlink to a host file that host-side reads and appends would follow. Keep managed shell output on an anonymous temporary file handle instead of a named path. Also stop _resolve_path_from_sandbox from consulting a booter under the local runtime: workspace and local-file checks already enforce caller permissions, and the fallback branch probed the host shell without them. Reject such paths with FileNotFoundError instead.
This commit is contained in:
@@ -1,19 +1,21 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import hashlib
|
||||
import locale
|
||||
import os
|
||||
import shutil
|
||||
import signal
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
import threading
|
||||
import time
|
||||
import uuid
|
||||
from _thread import LockType
|
||||
from collections.abc import Callable
|
||||
from dataclasses import dataclass, field
|
||||
from pathlib import Path
|
||||
from typing import Any, cast
|
||||
from typing import Any, BinaryIO, cast
|
||||
|
||||
if sys.version_info < (3, 14):
|
||||
import python_ripgrep
|
||||
@@ -140,7 +142,8 @@ class _LocalShellSession:
|
||||
creator_is_admin: bool
|
||||
sandboxed: bool
|
||||
process: SandboxProcess | asyncio.subprocess.Process
|
||||
output_path: Path
|
||||
output_file: BinaryIO
|
||||
output_lock: LockType
|
||||
started_at: float
|
||||
output_event: asyncio.Event
|
||||
reader_task: asyncio.Task[None]
|
||||
@@ -314,19 +317,18 @@ class LocalShellComponent(ShellComponent):
|
||||
|
||||
working_dir = Path(cwd).resolve() if cwd else Path(get_astrbot_root()).resolve()
|
||||
session_id = f"sh_{uuid.uuid4().hex[:16]}"
|
||||
owner_digest = hashlib.sha256(owner_id.encode("utf-8")).hexdigest()[:16]
|
||||
output_dir = Path(get_astrbot_system_tmp_path()) / "shell" / owner_digest
|
||||
output_dir = Path(get_astrbot_system_tmp_path())
|
||||
output_dir.mkdir(parents=True, exist_ok=True)
|
||||
output_path = output_dir / f"{session_id}.log"
|
||||
output_path.touch()
|
||||
|
||||
# Configuration invalidation must also see processes still being spawned.
|
||||
async with self._sessions_lock:
|
||||
if not permission_check():
|
||||
output_path.unlink(missing_ok=True)
|
||||
raise PermissionError(
|
||||
"Local shell permissions changed; retry the command."
|
||||
)
|
||||
# Shared temporary roots are writable by sandboxed processes. Keep
|
||||
# output on an anonymous handle to prevent redirecting host I/O.
|
||||
output_file = tempfile.TemporaryFile(mode="w+b", dir=output_dir)
|
||||
output_lock = threading.Lock()
|
||||
try:
|
||||
if sandboxed:
|
||||
process = await create_process_sandbox().spawn_shell(
|
||||
@@ -376,8 +378,8 @@ class LocalShellComponent(ShellComponent):
|
||||
stderr=asyncio.subprocess.STDOUT,
|
||||
**process_kwargs,
|
||||
)
|
||||
except Exception:
|
||||
output_path.unlink(missing_ok=True)
|
||||
except BaseException:
|
||||
output_file.close()
|
||||
raise
|
||||
|
||||
output_event = asyncio.Event()
|
||||
@@ -386,24 +388,25 @@ class LocalShellComponent(ShellComponent):
|
||||
if process.stdout is None:
|
||||
return
|
||||
output_size = 0
|
||||
with output_path.open("ab") as output_file:
|
||||
while chunk := await process.stdout.read(8192):
|
||||
if sandboxed:
|
||||
remaining = _LOCAL_SANDBOX_MAX_OUTPUT_BYTES - output_size
|
||||
if remaining <= 0:
|
||||
session.output_limited = True
|
||||
process.terminate()
|
||||
return
|
||||
if len(chunk) > remaining:
|
||||
chunk = chunk[:remaining]
|
||||
session.output_limited = True
|
||||
output_file.write(chunk)
|
||||
output_file.flush()
|
||||
output_size += len(chunk)
|
||||
output_event.set()
|
||||
if session.output_limited:
|
||||
while chunk := await process.stdout.read(8192):
|
||||
if sandboxed:
|
||||
remaining = _LOCAL_SANDBOX_MAX_OUTPUT_BYTES - output_size
|
||||
if remaining <= 0:
|
||||
session.output_limited = True
|
||||
process.terminate()
|
||||
return
|
||||
if len(chunk) > remaining:
|
||||
chunk = chunk[:remaining]
|
||||
session.output_limited = True
|
||||
with output_lock:
|
||||
output_file.seek(0, os.SEEK_END)
|
||||
output_file.write(chunk)
|
||||
output_file.flush()
|
||||
output_size += len(chunk)
|
||||
output_event.set()
|
||||
if session.output_limited:
|
||||
process.terminate()
|
||||
return
|
||||
|
||||
reader_task = asyncio.create_task(
|
||||
_capture_output(),
|
||||
@@ -421,7 +424,8 @@ class LocalShellComponent(ShellComponent):
|
||||
creator_is_admin=creator_is_admin,
|
||||
sandboxed=sandboxed,
|
||||
process=process,
|
||||
output_path=output_path,
|
||||
output_file=output_file,
|
||||
output_lock=output_lock,
|
||||
started_at=time.time(),
|
||||
output_event=output_event,
|
||||
reader_task=reader_task,
|
||||
@@ -528,8 +532,8 @@ class LocalShellComponent(ShellComponent):
|
||||
)
|
||||
)
|
||||
try:
|
||||
output_size = session.output_path.stat().st_size
|
||||
except OSError:
|
||||
output_size = os.fstat(session.output_file.fileno()).st_size
|
||||
except (OSError, ValueError):
|
||||
output_size = session.cursor
|
||||
items.append(
|
||||
{
|
||||
@@ -588,14 +592,13 @@ class LocalShellComponent(ShellComponent):
|
||||
raise ValueError("`cursor` must be greater than or equal to 0.")
|
||||
|
||||
def _read_output() -> tuple[bytes, int, int]:
|
||||
try:
|
||||
output_size = session.output_path.stat().st_size
|
||||
except FileNotFoundError:
|
||||
return b"", read_cursor, read_cursor
|
||||
normalized_cursor = min(read_cursor, output_size)
|
||||
with session.output_path.open("rb") as output_file:
|
||||
output_file.seek(normalized_cursor)
|
||||
raw_output = output_file.read(max_output_chars)
|
||||
with session.output_lock:
|
||||
if session.output_file.closed:
|
||||
return b"", read_cursor, read_cursor
|
||||
output_size = os.fstat(session.output_file.fileno()).st_size
|
||||
normalized_cursor = min(read_cursor, output_size)
|
||||
session.output_file.seek(normalized_cursor)
|
||||
raw_output = session.output_file.read(max_output_chars)
|
||||
return (
|
||||
raw_output,
|
||||
normalized_cursor + len(raw_output),
|
||||
@@ -956,11 +959,8 @@ class LocalShellComponent(ShellComponent):
|
||||
await timeout_task
|
||||
except asyncio.CancelledError:
|
||||
pass
|
||||
session.output_path.unlink(missing_ok=True)
|
||||
try:
|
||||
session.output_path.parent.rmdir()
|
||||
except OSError:
|
||||
pass
|
||||
with session.output_lock:
|
||||
session.output_file.close()
|
||||
|
||||
|
||||
@dataclass
|
||||
|
||||
@@ -179,6 +179,12 @@ class SendMessageToUserTool(FunctionTool[AstrAgentContext]):
|
||||
f"Blocked path: {local_candidate}."
|
||||
)
|
||||
|
||||
# Local runtime has no separate sandbox: the workspace and local-file
|
||||
# branches above already enforced the caller's permissions, so probing
|
||||
# the host shell here would bypass them and expose host paths.
|
||||
if is_local_runtime(context):
|
||||
raise FileNotFoundError(f"{component_type} path does not exist: {path}")
|
||||
|
||||
try:
|
||||
sb = await get_booter(
|
||||
context.context.context,
|
||||
|
||||
@@ -25,7 +25,6 @@ from astrbot.core.computer.process_sandbox import (
|
||||
from astrbot.core.tools.computer_tools import fs, python, shell
|
||||
from astrbot.dashboard.services import stat_service
|
||||
|
||||
|
||||
requires_local_sandbox = pytest.mark.skipif(
|
||||
not (
|
||||
(sys.platform.startswith("linux") and shutil.which("bwrap"))
|
||||
@@ -35,6 +34,50 @@ requires_local_sandbox = pytest.mark.skipif(
|
||||
)
|
||||
|
||||
|
||||
@requires_local_sandbox
|
||||
@pytest.mark.asyncio
|
||||
async def test_managed_shell_output_cannot_be_redirected_outside_sandbox(
|
||||
monkeypatch, tmp_path
|
||||
):
|
||||
"""Keep host output reads on the original handle after shared-path attacks."""
|
||||
workspace = tmp_path / "workspace"
|
||||
shared_temp = tmp_path / "shared-temp"
|
||||
workspace.mkdir()
|
||||
shared_temp.mkdir()
|
||||
secret = tmp_path / "host-only.txt"
|
||||
secret.write_text("host-only marker", encoding="utf-8")
|
||||
monkeypatch.setattr(local, "get_astrbot_system_tmp_path", lambda: str(shared_temp))
|
||||
code = f"""
|
||||
from pathlib import Path
|
||||
|
||||
secret = Path({str(secret)!r})
|
||||
assert not secret.exists(), "The host file must be outside the sandbox."
|
||||
for log in Path({str(shared_temp)!r}).rglob("sh_*.log"):
|
||||
log.unlink()
|
||||
log.symlink_to(secret)
|
||||
print("original process output")
|
||||
"""
|
||||
component = local.LocalShellComponent()
|
||||
try:
|
||||
result = await component.exec_managed(
|
||||
shlex.join(["python", "-c", code]),
|
||||
owner_id="test:GroupMessage:output",
|
||||
creator_id="member",
|
||||
creator_is_admin=False,
|
||||
sandboxed=True,
|
||||
permission_check=lambda: True,
|
||||
cwd=str(workspace),
|
||||
writable_roots=(shared_temp,),
|
||||
timeout=10,
|
||||
)
|
||||
assert result["exit_code"] == 0, result
|
||||
assert result["stdout"] == "original process output\n"
|
||||
assert result["session_closed"] is True
|
||||
assert secret.read_text(encoding="utf-8") == "host-only marker"
|
||||
finally:
|
||||
await component.shutdown_sessions()
|
||||
|
||||
|
||||
@requires_local_sandbox
|
||||
def test_runtime_probe_launches_native_sandbox(monkeypatch, tmp_path):
|
||||
"""Verify the dashboard startup check against each platform's real backend."""
|
||||
@@ -96,7 +139,9 @@ def test_local_sandbox_protects_venv_inside_writable_roots(
|
||||
attachments = tmp_path / "attachments"
|
||||
workspace.mkdir()
|
||||
attachments.mkdir()
|
||||
parent = {"workspace": workspace, "writable_root": attachments}.get(location, tmp_path)
|
||||
parent = {"workspace": workspace, "writable_root": attachments}.get(
|
||||
location, tmp_path
|
||||
)
|
||||
prefix = parent / ".venv"
|
||||
base_python = Path(sys._base_executable)
|
||||
subprocess.run(
|
||||
@@ -166,7 +211,9 @@ print("virtualenv is read-only")
|
||||
[str(executable), "-c", code],
|
||||
SandboxSpec(
|
||||
workspace=workspace,
|
||||
writable_roots=(package.parent if location == "writable_packages" else attachments,),
|
||||
writable_roots=(
|
||||
package.parent if location == "writable_packages" else attachments,
|
||||
),
|
||||
),
|
||||
timeout=15,
|
||||
)
|
||||
|
||||
@@ -1268,6 +1268,7 @@ async def test_managed_shell_cleans_children_after_leader_exits():
|
||||
session.permission_check = None
|
||||
await asyncio.wait_for(shell.shutdown_sessions(invalid_only=True), 8)
|
||||
assert session.reader_task.done()
|
||||
assert session.output_file.closed
|
||||
assert not shell._sessions
|
||||
finally:
|
||||
await shell.shutdown_sessions()
|
||||
@@ -1510,6 +1511,7 @@ async def test_managed_shell_hard_timeout_terminates_session():
|
||||
timeout=1,
|
||||
yield_time_ms=0,
|
||||
)
|
||||
session = shell._sessions[result["session_id"]]
|
||||
|
||||
try:
|
||||
timed_out = await shell.poll_session(
|
||||
@@ -1523,6 +1525,7 @@ async def test_managed_shell_hard_timeout_terminates_session():
|
||||
assert timed_out["status"] == "timed_out"
|
||||
assert timed_out["exit_code"] is not None
|
||||
assert timed_out["session_closed"] is True
|
||||
assert session.output_file.closed
|
||||
finally:
|
||||
await shell.shutdown_sessions()
|
||||
|
||||
@@ -1540,10 +1543,12 @@ async def test_managed_shell_keeps_completed_session_until_output_is_drained():
|
||||
yield_time_ms=5_000,
|
||||
max_output_chars=10_000,
|
||||
)
|
||||
session = shell._sessions[result["session_id"]]
|
||||
|
||||
try:
|
||||
assert result["status"] == "completed"
|
||||
assert result["has_more"] is True
|
||||
assert not session.output_file.closed
|
||||
output = result["stdout"]
|
||||
while result["has_more"]:
|
||||
result = await shell.poll_session(
|
||||
@@ -1557,6 +1562,7 @@ async def test_managed_shell_keeps_completed_session_until_output_is_drained():
|
||||
|
||||
assert output.splitlines() == ["x" * 25000]
|
||||
assert result["session_closed"] is True
|
||||
assert session.output_file.closed
|
||||
assert await shell.list_sessions(
|
||||
owner_id="owner-a",
|
||||
requester_id="user-a",
|
||||
|
||||
@@ -291,7 +291,9 @@ async def test_send_message_empty_messages_returns_error():
|
||||
async def test_send_message_missing_image_path_stops_before_send(tmp_path, monkeypatch):
|
||||
"""Missing image paths fail before sending any message components."""
|
||||
tool = SendMessageToUserTool()
|
||||
ctx = _make_context()
|
||||
# Sandbox runtime so the booter is still consulted for missing paths;
|
||||
# local runtime now rejects them before any booter call.
|
||||
ctx = _make_context(runtime="sandbox")
|
||||
missing_image_path = tmp_path / "missing.png"
|
||||
|
||||
async def mock_get_booter(*args, **kwargs):
|
||||
@@ -525,3 +527,55 @@ async def test_send_message_downloads_trailing_slash_sandbox_file_with_basename(
|
||||
sent_chain = ctx.context.context.send_message.await_args.args[1]
|
||||
sent_file = sent_chain.chain[0]
|
||||
assert sent_file.name == "export"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_message_local_runtime_skips_sandbox_file_probe(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
"""Local runtime must resolve send-file paths only via permission-checked branches.
|
||||
|
||||
Falling through to the booter branch would probe the host shell without
|
||||
the caller's filesystem permissions.
|
||||
"""
|
||||
workspace = tmp_path / "workspace"
|
||||
workspace.mkdir()
|
||||
secret = tmp_path / "host-only.txt"
|
||||
secret.write_text("host-only marker", encoding="utf-8")
|
||||
allowed = workspace / "allowed.txt"
|
||||
allowed.write_text("workspace file", encoding="utf-8")
|
||||
|
||||
async def mock_workspace_root_for_context(_context):
|
||||
return workspace
|
||||
|
||||
async def mock_get_booter(*args, **kwargs):
|
||||
del args, kwargs
|
||||
raise AssertionError("local runtime must not query a sandbox booter")
|
||||
|
||||
monkeypatch.setattr(
|
||||
"astrbot.core.tools.message_tools.workspace_root_for_context",
|
||||
mock_workspace_root_for_context,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"astrbot.core.tools.message_tools.get_booter",
|
||||
mock_get_booter,
|
||||
)
|
||||
|
||||
tool = SendMessageToUserTool()
|
||||
ctx = _make_context(
|
||||
role="member",
|
||||
local_permissions={
|
||||
"member": {
|
||||
"allow_execution": False,
|
||||
"allow_network": False,
|
||||
"filesystem_scope": "workspace",
|
||||
}
|
||||
},
|
||||
)
|
||||
|
||||
resolved, downloaded = await tool._resolve_path_from_sandbox(ctx, "allowed.txt")
|
||||
assert resolved == str(allowed.resolve())
|
||||
assert downloaded is False
|
||||
|
||||
with pytest.raises(FileNotFoundError):
|
||||
await tool._resolve_path_from_sandbox(ctx, "host-only.txt")
|
||||
|
||||
Reference in New Issue
Block a user