mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(sandbox): reject shell-variable path prefixes in the host-bash guard (#6930)
* fix(sandbox): reject shell-variable path prefixes in the host-bash guard
The local path guard scans commands for absolute paths with a regex whose
lookbehind skips a `/` that follows a word character. `~/`, `${HOME}/` and
`"$HOME"/` were rejected, but the bare `$HOME/...` form (quoted or not)
was never matched, so `cat $HOME/.ssh/id_rsa` ran on the host when
`sandbox.allow_host_bash` was enabled.
Scan `$NAME/...` separately and reject it, since the variable's value is
unknown at validation time. Variables inside URLs and non-ASCII prose keep
the existing exemptions. `$PWD/...` stays allowed because it names the same
file as the equivalent relative path, which the guard already accepts,
unless the command rebinds `PWD` itself.
* docs(changelog): reference #6930 for the host-bash variable path fix
* fix(sandbox): cover positional parameters and CDPATH in the host-bash guard
Review follow-up. `cat $1/etc/passwd` slipped both scans: the new variable
scan only matched names, and the absolute-path regex skips a `/` after a
digit. `bash -c` gets no positionals, so `$1` expands to nothing and the
shell opens `/etc/passwd`. Match `$0`-`$9` as well, but keep awk-style
arithmetic such as `$1/1024` allowed, since it was accepted before and
standard host roots do not start with a digit.
`CDPATH=$HOME cd docs` moved a relative `cd` outside the workspace, taking
both `$PWD/x` and a plain relative `x` with it. Reject any command that
mentions CDPATH in the working-directory check, so the `$PWD` exemption's
equivalence with relative paths holds.
* fix(sandbox): cover named homes and non-ASCII variable paths in the host-bash guard
- Reject `~name/...`, which expands to that user's home and slips the
absolute-path scan the same way `$HOME/...` did.
- Stop applying the non-ASCII prose exemption to the variable scan:
`$HOME/文档/...` is a host path.
- Allow awk field division `$1/$2`, which main accepts, and capture the
remainder in a lookahead so skipping `$1/` does not hide a later
`$2/etc/passwd`.
This commit is contained in:
Hyeonsang Cho
GitHub
parent
34ccd79351
commit
6a6aeb4bf2
15
CHANGELOG.md
15
CHANGELOG.md
@ -5968,6 +5968,20 @@ This release closes that milestone with **621 merged pull requests**.
|
||||
whose head stamp was lost logged 29 (SQLite) or 36 (Postgres), burying the
|
||||
real type mismatch the warning exists to catch. Both sides are now compared
|
||||
as the connected database spells them. ([#6865])
|
||||
- **sandbox:** With host bash enabled, the local path guard now rejects a
|
||||
shell variable used as a path prefix, such as `cat $HOME/.ssh/id_rsa`.
|
||||
`~/`, `${HOME}/` and `"$HOME"/` were already rejected, but the
|
||||
absolute-path scan skips a `/` that follows a word character, so the
|
||||
bare `$HOME/...` form (quoted or not) reached the host. Positional
|
||||
parameters are covered too: `bash -c` gets none, so `$1/etc/passwd`
|
||||
opens `/etc/passwd`; awk-style `$1/1024` and `$1/$2` stay allowed. A
|
||||
named home such as `~root/.ssh/id_rsa` is rejected the same way, and a
|
||||
non-ASCII segment after a variable (`$HOME/文档`) is treated as a path,
|
||||
not prose. Commands that mention `CDPATH` are rejected, because it moves
|
||||
a relative `cd` outside the workspace. `$PWD/...` stays allowed because it
|
||||
names the same file as the equivalent relative path, unless the command
|
||||
rebinds `PWD` itself. Variables inside URLs are unaffected. The guard
|
||||
remains best-effort and is not an isolation boundary. ([#6930])
|
||||
|
||||
### Security
|
||||
|
||||
@ -11763,3 +11777,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag.
|
||||
[#6876]: https://github.com/bytedance/deer-flow/pull/6876
|
||||
[#6897]: https://github.com/bytedance/deer-flow/pull/6897
|
||||
[#6909]: https://github.com/bytedance/deer-flow/pull/6909
|
||||
[#6930]: https://github.com/bytedance/deer-flow/pull/6930
|
||||
|
||||
@ -4528,6 +4528,16 @@
|
||||
数据库升级时都会记录一条，丢失 head stamp 的 schema 会记录 29 条（SQLite）或
|
||||
36 条（Postgres），淹没了这个警告本应发现的真实类型不一致。现在两侧都按所连接
|
||||
数据库的写法进行比较。([#6865])
|
||||
- **沙箱：** 启用 host bash 时，本地路径守卫现在会拒绝把 shell 变量用作路径前缀的
|
||||
命令，例如 `cat $HOME/.ssh/id_rsa`。`~/`、`${HOME}/` 和 `"$HOME"/` 原本就会被拒绝，
|
||||
但绝对路径扫描会跳过紧跟在单词字符之后的 `/`，因此不带花括号的 `$HOME/...`
|
||||
（无论是否加引号）会直达主机。位置参数同样会被检查：`bash -c` 不带位置参数，
|
||||
因此 `$1/etc/passwd` 会打开 `/etc/passwd`；awk 风格的 `$1/1024` 和 `$1/$2` 仍然
|
||||
允许。`~root/.ssh/id_rsa` 这类指定用户的主目录同样会被拒绝，变量之后的非 ASCII
|
||||
路径段（`$HOME/文档`）按路径处理，不再视为普通文本。提及 `CDPATH` 的命令会被拒绝，
|
||||
因为它会把相对路径的 `cd` 移出工作区。`$PWD/...` 仍然允许，因为它与对应的相对路径
|
||||
指向同一文件，除非命令自己重新绑定了 `PWD`。URL 中的变量不受影响。该守卫仍是尽力
|
||||
而为的检查，并非隔离边界。([#6930])
|
||||
|
||||
### 安全
|
||||
|
||||
@ -9380,3 +9390,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子
|
||||
[#6876]: https://github.com/bytedance/deer-flow/pull/6876
|
||||
[#6897]: https://github.com/bytedance/deer-flow/pull/6897
|
||||
[#6909]: https://github.com/bytedance/deer-flow/pull/6909
|
||||
[#6930]: https://github.com/bytedance/deer-flow/pull/6930
|
||||
|
||||
@ -69,6 +69,25 @@ _SANDBOX_AUTHORIZATION_CHECKED: ContextVar[bool] = ContextVar(
|
||||
)
|
||||
|
||||
_ABSOLUTE_PATH_PATTERN = re.compile(r"(?<![:\w])(?<!:/)/(?:[^\s\"'`;&|<>()]+)")
|
||||
# ``$NAME/...`` expands to a host path at runtime (``$HOME/.ssh/id_rsa``), but
|
||||
# the absolute-path regex skips a ``/`` that follows a word character, so the
|
||||
# bare-name form needs its own scan. Positional ``$1/etc/passwd`` slips the
|
||||
# same way, and ``bash -c`` gets no positionals, so it expands to ``/etc/passwd``.
|
||||
# ``${NAME}/``, ``~/`` and special parameters (``$@/``, ``$?/``) already surface
|
||||
# through the absolute-path regex because ``}``, ``~``, ``@`` and ``?`` are not
|
||||
# word chars. The remainder is captured in a lookahead so a skipped prefix does
|
||||
# not hide a later one (``$1/$2/etc/passwd``).
|
||||
_SHELL_VARIABLE_PATH_PATTERN = re.compile(r"\$([A-Za-z_][A-Za-z0-9_]*|[0-9])/(?=([^\s\"'`;&|<>()]*))")
|
||||
# ``~name/`` expands to that user's home (``~root/.ssh/id_rsa``) and slips the
|
||||
# absolute-path regex for the same reason. Tilde expansion only applies at the
|
||||
# start of a word or after ``=``/``:``, so ``HEAD~1/`` stays out of scope.
|
||||
_NAMED_TILDE_PATH_PATTERN = re.compile(r"(?<![^\s=:;&|<>()\"'`])~[A-Za-z0-9._-]+/[^\s\"'`;&|<>()]*")
|
||||
# ``PWD`` written anywhere except as a ``$PWD`` expansion (``PWD=/``,
|
||||
# ``read PWD``, ``for PWD in``) may rebind it away from the workspace.
|
||||
_PWD_REBIND_PATTERN = re.compile(r"(?<!\$)\bPWD\b")
|
||||
# ``CDPATH`` makes a relative ``cd docs`` land outside the workspace without any
|
||||
# absolute path in the command.
|
||||
_CDPATH_PATTERN = re.compile(r"\bCDPATH\b")
|
||||
# A ``{...}`` block holding a single identifier-like placeholder (e.g. ``{id}``
|
||||
# in a REST template or ``{port}`` in an f-string). Bash brace expansion such as
|
||||
# ``{passwd,shadow}`` or ``{,.bak}`` does NOT match (commas/dots/empty inner).
|
||||
@ -1294,6 +1313,8 @@ def _validate_local_bash_shell_tokens(command: str, allowed_paths: list[str], to
|
||||
"""Conservatively reject relative path escapes missed by absolute-path scanning."""
|
||||
if re.search(r"\$\([^)]*\b(?:cd|pushd)\b", command):
|
||||
raise PermissionError(f"Unsafe working directory change in command substitution. Use paths under {VIRTUAL_PATH_PREFIX}")
|
||||
if _CDPATH_PATTERN.search(command):
|
||||
raise PermissionError(f"Unsafe working directory change via CDPATH. Use paths under {VIRTUAL_PATH_PREFIX}")
|
||||
|
||||
for token in tokens:
|
||||
if _is_shell_command_separator(token) or _is_shell_redirection_operator(token):
|
||||
@ -1416,6 +1437,10 @@ def validate_local_bash_command_paths(command: str, thread_data: ThreadDataState
|
||||
A small allowlist of common system path prefixes is kept for executable
|
||||
and device references (e.g. /bin/sh, /dev/null). Quoted shell words are
|
||||
validated whole, so a space inside quotes is not treated as a new segment.
|
||||
A shell variable used as a path prefix (``$HOME/...``, ``$1/...``) or a named
|
||||
home (``~root/...``) is rejected like ``${HOME}/...`` and ``~/...``, since its
|
||||
runtime value is unknown here;
|
||||
``$PWD/...`` is checked like the relative path it is equivalent to.
|
||||
A windows-incompatible segment still rejects a path that is not already
|
||||
stored on the host; an existing host file may be read or renamed.
|
||||
"""
|
||||
@ -1448,6 +1473,31 @@ def validate_local_bash_command_paths(command: str, thread_data: ThreadDataState
|
||||
|
||||
unsafe_paths.append(candidate)
|
||||
|
||||
# A variable's value is unknown here, so its path is never allowed. The one
|
||||
# exception is ``$PWD``: ``$PWD/.venv/bin`` names the same file as the
|
||||
# relative ``.venv/bin``, which this guard already accepts, unless the
|
||||
# command rebinds ``PWD`` itself.
|
||||
# The prose exemption used above does not apply: ``$HOME/文档`` is a real
|
||||
# host directory.
|
||||
pwd_is_workspace = not _PWD_REBIND_PATTERN.search(command)
|
||||
for match in _SHELL_VARIABLE_PATH_PATTERN.finditer(command):
|
||||
if _is_in_spans(match.start(), url_spans):
|
||||
continue
|
||||
name, remainder = match.group(1), match.group(2)
|
||||
if name == "PWD" and pwd_is_workspace:
|
||||
continue
|
||||
# ``$1/1024`` and ``$1/$2`` are awk-style field arithmetic
|
||||
# (``awk '{print $1/$2}'``), not paths: standard top-level host
|
||||
# directories do not start with a digit.
|
||||
if name.isdigit() and remainder.removeprefix("$")[:1].isdigit():
|
||||
continue
|
||||
unsafe_paths.append(match.group() + remainder)
|
||||
|
||||
for match in _NAMED_TILDE_PATH_PATTERN.finditer(command):
|
||||
if _is_in_spans(match.start(), url_spans):
|
||||
continue
|
||||
unsafe_paths.append(match.group())
|
||||
|
||||
if unsafe_paths:
|
||||
unsafe = ", ".join(sorted(dict.fromkeys(unsafe_paths)))
|
||||
raise PermissionError(f"Unsafe absolute paths in command: {unsafe}. Use paths under {VIRTUAL_PATH_PREFIX}. {_LOCAL_BASH_PATH_RECOVERY_GUIDANCE}")
|
||||
|
||||
@ -1013,6 +1013,151 @@ def test_validate_local_bash_command_paths_blocks_brace_expansion_host_paths(com
|
||||
validate_local_bash_command_paths(command, _THREAD_DATA)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("command", "rejected"),
|
||||
[
|
||||
# The absolute-path regex skips a ``/`` right after a word character,
|
||||
# so ``$HOME/...`` slipped through while ``~/``, ``${HOME}/`` and
|
||||
# ``"$HOME"/`` were rejected. All four expand to the same host path.
|
||||
("cat $HOME/.ssh/id_rsa", "$HOME/.ssh/id_rsa"),
|
||||
('cat "$HOME/.ssh/id_rsa"', "$HOME/.ssh/id_rsa"),
|
||||
("ls $HOME/", "$HOME/"),
|
||||
("cp /mnt/user-data/workspace/rc $HOME/.bashrc", "$HOME/.bashrc"),
|
||||
("ls $DEER_FLOW_HOME/users", "$DEER_FLOW_HOME/users"),
|
||||
("echo ok && cat $_cfg/x", "$_cfg/x"),
|
||||
# ``cd`` leaves OLDPWD at the Gateway's own working directory.
|
||||
("cat $OLDPWD/config.yaml", "$OLDPWD/config.yaml"),
|
||||
# ``$PWD`` is the workspace only while the command does not rebind it.
|
||||
("PWD=/; cat $PWD/etc/passwd", "$PWD/etc/passwd"),
|
||||
("read PWD; cat $PWD/x", "$PWD/x"),
|
||||
("for PWD in ~; do cat $PWD/.ssh/id_rsa; done", "$PWD/.ssh/id_rsa"),
|
||||
("export PWD=/etc && cat $PWD/passwd", "$PWD/passwd"),
|
||||
# ``bash -c`` gets no positionals, so ``$1`` expands to nothing.
|
||||
("cat $1/etc/passwd", "$1/etc/passwd"),
|
||||
('cat "$2/etc/passwd"', "$2/etc/passwd"),
|
||||
("ls $0/", "$0/"),
|
||||
# Skipping ``$1/`` as arithmetic must not hide the ``$2/`` after it.
|
||||
("cat $1/$2/etc/passwd", "$2/etc/passwd"),
|
||||
# Non-ASCII after a variable prefix is still a host path.
|
||||
("cat $HOME/文档/marker", "$HOME/文档/marker"),
|
||||
# ``~name`` expands to that user's home.
|
||||
("cat ~root/.ssh/id_rsa", "~root/.ssh/id_rsa"),
|
||||
("ls ~deploy/", "~deploy/"),
|
||||
("make --directory=~deploy/app", "~deploy/app"),
|
||||
],
|
||||
)
|
||||
def test_validate_local_bash_command_paths_blocks_variable_prefixed_host_paths(command: str, rejected: str) -> None:
|
||||
with pytest.raises(PermissionError, match="Unsafe absolute paths") as exc_info:
|
||||
validate_local_bash_command_paths(command, _THREAD_DATA)
|
||||
|
||||
assert rejected in str(exc_info.value)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"command",
|
||||
[
|
||||
# Variables that are not used as a path prefix stay allowed.
|
||||
"echo $HOME",
|
||||
"echo $PATH",
|
||||
"echo $((total/2))",
|
||||
# A variable inside a URL is a URL segment, not a host path.
|
||||
"curl https://api.github.com/repos/$OWNER/$REPO/issues",
|
||||
"curl https://example.com/~alice/index.html",
|
||||
# ``~`` mid-word is not tilde expansion.
|
||||
"cat drafts~v2/notes.md",
|
||||
# ``$PWD/x`` names the same file as the relative ``x``, which the guard
|
||||
# already accepts (``$PWD/../x`` is rejected like ``../x``).
|
||||
"cat $PWD/notes.txt",
|
||||
'export PATH="$PWD/.venv/bin:$PATH"',
|
||||
"cd /mnt/user-data/uploads && cat $PWD/report.pdf",
|
||||
# awk field arithmetic is not a path.
|
||||
"du -k /mnt/user-data/workspace | awk '{print $1/1024}'",
|
||||
"awk -F, '{print $1/$2}' /mnt/user-data/uploads/data.csv",
|
||||
],
|
||||
)
|
||||
def test_validate_local_bash_command_paths_allows_non_path_variable_uses(command: str) -> None:
|
||||
validate_local_bash_command_paths(command, _THREAD_DATA)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"command",
|
||||
[
|
||||
# CDPATH relocates a relative ``cd`` outside the workspace, taking both
|
||||
# ``$PWD`` and plain relative paths with it.
|
||||
"CDPATH=$HOME cd docs && cat $PWD/x",
|
||||
"CDPATH=/ cd etc && cat passwd",
|
||||
"export CDPATH=/; cd etc; cat passwd",
|
||||
],
|
||||
)
|
||||
def test_validate_local_bash_command_paths_rejects_cdpath_relocation(command: str) -> None:
|
||||
with pytest.raises(PermissionError, match="Unsafe working directory change via CDPATH"):
|
||||
validate_local_bash_command_paths(command, _THREAD_DATA)
|
||||
|
||||
|
||||
def test_validate_local_bash_command_paths_rejects_pwd_traversal_like_relative_traversal() -> None:
|
||||
for command in ("cat ../../etc/passwd", "cat $PWD/../../etc/passwd"):
|
||||
with pytest.raises(PermissionError, match="path traversal"):
|
||||
validate_local_bash_command_paths(command, _THREAD_DATA)
|
||||
|
||||
|
||||
def test_bash_tool_rejects_variable_prefixed_host_path_before_execution(monkeypatch) -> None:
|
||||
runtime = SimpleNamespace(
|
||||
state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()},
|
||||
context={"thread_id": "thread-1"},
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"deerflow.sandbox.tools.ensure_sandbox_initialized",
|
||||
lambda runtime: SimpleNamespace(execute_command=lambda command: pytest.fail("unsafe command should not execute")),
|
||||
)
|
||||
monkeypatch.setattr("deerflow.sandbox.tools.ensure_thread_directories_exist", lambda runtime: None)
|
||||
monkeypatch.setattr("deerflow.sandbox.tools.is_host_bash_allowed", lambda: True)
|
||||
|
||||
result = bash_tool.func(runtime=runtime, description="read key", command="cat $HOME/.ssh/id_rsa")
|
||||
|
||||
assert "Unsafe absolute paths in command: $HOME/.ssh/id_rsa" in result
|
||||
|
||||
|
||||
@pytest.mark.skipif(os.name == "nt", reason="POSIX shell expands $HOME")
|
||||
@pytest.mark.parametrize(
|
||||
("command", "rejected"),
|
||||
[
|
||||
('cat "$HOME/.ssh/id_rsa"', "Unsafe absolute paths in command: $HOME/.ssh/id_rsa"),
|
||||
('CDPATH="$HOME" cd .ssh && cat $PWD/id_rsa', "Unsafe working directory change via CDPATH"),
|
||||
('cat "$HOME/文档/id_rsa"', "Unsafe absolute paths in command: $HOME/文档/id_rsa"),
|
||||
],
|
||||
)
|
||||
def test_local_bash_tool_does_not_read_variable_prefixed_host_file(tmp_path: Path, monkeypatch, command: str, rejected: str) -> None:
|
||||
"""End to end through a real ``LocalSandbox``: before the fix the shell
|
||||
expanded ``$HOME`` and printed the host file."""
|
||||
home = tmp_path / "home"
|
||||
(home / ".ssh").mkdir(parents=True)
|
||||
(home / ".ssh" / "id_rsa").write_text("host-private-key")
|
||||
(home / "文档").mkdir()
|
||||
(home / "文档" / "id_rsa").write_text("host-private-key")
|
||||
monkeypatch.setenv("HOME", str(home))
|
||||
thread_root = tmp_path / "threads" / "t1" / "user-data"
|
||||
thread_data = {f"{name}_path": str(thread_root / name) for name in ("workspace", "uploads", "outputs")}
|
||||
for path in thread_data.values():
|
||||
Path(path).mkdir(parents=True)
|
||||
workspace = str(Path(thread_data["workspace_path"]).resolve())
|
||||
sandbox = LocalSandbox(
|
||||
id="local",
|
||||
path_mappings=[PathMapping(container_path=f"{VIRTUAL_PATH_PREFIX}/workspace", local_path=workspace)],
|
||||
)
|
||||
runtime = SimpleNamespace(
|
||||
state={"sandbox": {"sandbox_id": "local"}, "thread_data": thread_data},
|
||||
context={"thread_id": "t1"},
|
||||
config={},
|
||||
)
|
||||
monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox)
|
||||
monkeypatch.setattr("deerflow.sandbox.tools.is_host_bash_allowed", lambda: True)
|
||||
|
||||
result = bash_tool.func(runtime=runtime, description="read key", command=command)
|
||||
|
||||
assert "host-private-key" not in result
|
||||
assert rejected in result
|
||||
|
||||
|
||||
def test_bash_tool_rejects_host_bash_when_local_sandbox_default(monkeypatch) -> None:
|
||||
runtime = SimpleNamespace(
|
||||
state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()},
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user