mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
fix(file_tools): reject '..' traversal in V4A patch headers
V4A patch '*** Update File:', '*** Add File:', '*** Delete File:' headers come from patch CONTENT, not the explicit `path=` argument. That makes them attacker-influenceable through skill content, web extract output, prompt injection, and other surfaces the agent processes. Headers like '*** Update File: ../../../etc/shadow' would resolve relative to the agent's cwd; in deployment configurations where that cwd is deep enough to land outside Hermes' protected paths, the write could land somewhere the agent operator did not intend. Reject any V4A header containing a '..' path component before applying the patch. The explicit `path=` argument on patch_tool is UNCHANGED — the agent legitimately uses '..' there (e.g. `patch path='../other_module/x.py'` from a worktree dir is normal cross-module editing). Regression tests: V4A Update header with traversal rejected, V4A Add header with traversal rejected, patch_v4a never invoked when rejection fires. Salvaged from PR #29395 by @waefrebeorn. The original PR added has_traversal_component as a blanket reject on read_file_tool, write_file_tool, patch_tool's explicit path, and search_tool — that would break legitimate agent operation where '..' is normal. Also dropped the over-eager skills_guard pattern additions (pickle.loads/marshal.loads/ctypes.CDLL/importlib at high/critical severity would false-positive on legit data-science and FFI skills). Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
This commit is contained in:
parent
00bd24e27c
commit
5faea3f618
2 changed files with 56 additions and 1 deletions
|
|
@ -211,6 +211,45 @@ class TestPatchHandler:
|
|||
assert "error" in result
|
||||
assert "Unknown mode" in result["error"]
|
||||
|
||||
@patch("tools.file_tools._get_file_ops")
|
||||
def test_patch_v4a_rejects_traversal_in_update_header(self, mock_get):
|
||||
"""V4A '*** Update File:' headers come from patch content, which can
|
||||
carry prompt-injection-controlled paths (skill content, web extract).
|
||||
``..`` traversal in the header must be rejected before the patch is
|
||||
applied, even though the explicit ``path=`` arg is allowed to use
|
||||
``..`` for legitimate cross-worktree edits."""
|
||||
from tools.file_tools import patch_tool
|
||||
result = json.loads(patch_tool(
|
||||
mode="patch",
|
||||
patch=(
|
||||
"*** Begin Patch\n"
|
||||
"*** Update File: ../../../etc/shadow\n"
|
||||
"@@ -1,3 +1,3 @@\n"
|
||||
"-old\n"
|
||||
"+new\n"
|
||||
"*** End Patch\n"
|
||||
),
|
||||
))
|
||||
assert "error" in result
|
||||
assert "traversal" in result["error"].lower()
|
||||
# patch_v4a must not be invoked when the header is rejected
|
||||
mock_get.return_value.patch_v4a.assert_not_called()
|
||||
|
||||
@patch("tools.file_tools._get_file_ops")
|
||||
def test_patch_v4a_rejects_traversal_in_add_header(self, mock_get):
|
||||
from tools.file_tools import patch_tool
|
||||
result = json.loads(patch_tool(
|
||||
mode="patch",
|
||||
patch=(
|
||||
"*** Begin Patch\n"
|
||||
"*** Add File: ../../../tmp/dropped.py\n"
|
||||
"+print('pwned')\n"
|
||||
"*** End Patch\n"
|
||||
),
|
||||
))
|
||||
assert "error" in result
|
||||
assert "traversal" in result["error"].lower()
|
||||
|
||||
|
||||
class TestSearchHandler:
|
||||
@patch("tools.file_tools._get_file_ops")
|
||||
|
|
|
|||
|
|
@ -910,8 +910,24 @@ def patch_tool(mode: str = "replace", path: str = None, old_string: str = None,
|
|||
_paths_to_check.append(path)
|
||||
if mode == "patch" and patch:
|
||||
import re as _re
|
||||
from tools.path_security import has_traversal_component
|
||||
for _m in _re.finditer(r'^\*\*\*\s+(?:Update|Add|Delete)\s+File:\s*(.+)$', patch, _re.MULTILINE):
|
||||
_paths_to_check.append(_m.group(1).strip())
|
||||
v4a_path = _m.group(1).strip()
|
||||
# V4A path headers come from patch CONTENT, not the explicit
|
||||
# ``path=`` arg — so they're more attacker-influenceable (skill
|
||||
# content, web extract, prompt injection). Reject ``..`` traversal
|
||||
# in V4A headers: a legitimate multi-file patch from a single cwd
|
||||
# can always emit absolute paths or paths relative to the agent's
|
||||
# cwd without ``..``. The explicit ``path=`` arg is unchanged
|
||||
# because the agent uses relative ``..`` paths legitimately
|
||||
# (e.g. ``patch path="../other_module/x.py"`` from a worktree).
|
||||
if has_traversal_component(v4a_path):
|
||||
return tool_error(
|
||||
f"V4A patch header contains '..' traversal: {v4a_path!r}. "
|
||||
"Use the agent's cwd-relative path (no '..') or an absolute "
|
||||
"path in '*** Update File:' / '*** Add File:' / '*** Delete File:' headers."
|
||||
)
|
||||
_paths_to_check.append(v4a_path)
|
||||
for _p in _paths_to_check:
|
||||
sensitive_err = _check_sensitive_path(_p, task_id)
|
||||
if sensitive_err:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue