mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-30 06:41:51 +00:00
fix(update): reject symlink members in update ZIP
_update_via_zip downloads a source ZIP from GitHub and calls zipfile.ZipFile.extractall. The existing zip-slip path guard validates each member's path stays under tmp_dir, but does not check member type — so a ZIP containing a symlink member would still be materialized by extractall, and a symlink target could point outside the extracted tree (or to a sensitive system path). This isn't a high-likelihood threat for hermes-agent's actual GitHub source ZIPs (we don't ship symlinks), but the extractall path runs as the user's account and a compromised mirror could plant arbitrary files via the symlink → target → write chain. Reject any member whose Unix mode bits (upper 16 bits of external_attr) are S_IFLNK before extractall. Hermes source ZIPs contain only regular files and directories; a symlink member is unambiguously suspicious. Regression tests cover: symlink member rejection (raises ValueError, caught by the outer try/except as a clean SystemExit, no extraction), and the happy-path verification that a normal ZIP doesn't trigger the symlink reject message. Salvaged from PR #15881 by @codeblackhole1024. The remaining pieces of that PR were already on main or contradicted explicit design decisions: - config.yaml write-deny: already in agent/file_safety.py's control_file_names denylist (the modern guard); the proposed addition to build_write_denied_paths was the legacy path. - Quick commands danger detection: contradicts the explicit cli.py:8491-8492 comment 'shell=True is intentional: quick_commands are user-defined shell snippets from config.yaml — not agent/LLM controlled.' - Memory plugin shlex.split for dep checks: already on main (hermes_cli/memory_setup.py:133). Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
This commit is contained in:
parent
5f20322d23
commit
bd2756dd22
2 changed files with 129 additions and 1 deletions
|
|
@ -7000,8 +7000,13 @@ def _update_via_zip(args):
|
|||
urlretrieve(zip_url, zip_path)
|
||||
|
||||
print("→ Extracting...")
|
||||
import stat as _stat
|
||||
with zipfile.ZipFile(zip_path, "r") as zf:
|
||||
# Validate paths to prevent zip-slip (path traversal)
|
||||
# Validate paths to prevent zip-slip (path traversal) AND reject
|
||||
# symlink members. A GitHub source ZIP for hermes-agent itself
|
||||
# should never contain symlinks — they'd point outside the
|
||||
# extracted tree and let an attacker who can compromise the
|
||||
# update mirror plant arbitrary files via the update path.
|
||||
tmp_dir_real = os.path.realpath(tmp_dir)
|
||||
for member in zf.infolist():
|
||||
member_path = os.path.realpath(os.path.join(tmp_dir, member.filename))
|
||||
|
|
@ -7012,6 +7017,13 @@ def _update_via_zip(args):
|
|||
raise ValueError(
|
||||
f"Zip-slip detected: {member.filename} escapes extraction directory"
|
||||
)
|
||||
# Unix mode lives in the upper 16 bits of external_attr;
|
||||
# mask to the file-type bits.
|
||||
mode = (member.external_attr >> 16) & 0o170000
|
||||
if _stat.S_ISLNK(mode):
|
||||
raise ValueError(
|
||||
f"ZIP contains unsupported symlink member: {member.filename}"
|
||||
)
|
||||
zf.extractall(tmp_dir)
|
||||
|
||||
# GitHub ZIPs extract to hermes-agent-<branch>/
|
||||
|
|
|
|||
116
tests/hermes_cli/test_update_zip_symlink_reject.py
Normal file
116
tests/hermes_cli/test_update_zip_symlink_reject.py
Normal file
|
|
@ -0,0 +1,116 @@
|
|||
"""Regression: _update_via_zip must reject ZIP members with symlink mode.
|
||||
|
||||
A symlink member in a downloaded update ZIP would let an attacker who can
|
||||
serve / MITM the update mirror plant a symlink that extractall() then
|
||||
follows, writing arbitrary file content outside the staging directory.
|
||||
The Linux mode bits live in the upper 16 bits of ``ZipInfo.external_attr``;
|
||||
we explicitly reject any member whose type bits are S_IFLNK.
|
||||
"""
|
||||
|
||||
import io
|
||||
import os
|
||||
import stat
|
||||
import tempfile
|
||||
import zipfile
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
def _build_zip_with_symlink_member(zip_path: str, link_name: str, target: str) -> None:
|
||||
"""Write a ZIP containing a single member with S_IFLNK mode bits set."""
|
||||
with zipfile.ZipFile(zip_path, "w") as zf:
|
||||
info = zipfile.ZipInfo(link_name)
|
||||
# Upper 16 bits = Unix mode; mark as symlink (0o120000) + 0o777 perms.
|
||||
info.external_attr = (stat.S_IFLNK | 0o777) << 16
|
||||
# The "data" of a symlink ZIP member is the link target string.
|
||||
zf.writestr(info, target)
|
||||
|
||||
|
||||
def _build_normal_zip(zip_path: str) -> None:
|
||||
"""Write a regular ZIP with a normal file member (no symlink)."""
|
||||
with zipfile.ZipFile(zip_path, "w") as zf:
|
||||
zf.writestr("hermes-agent-main/README.md", "ok\n")
|
||||
|
||||
|
||||
def test_update_via_zip_rejects_symlink_member(tmp_path, monkeypatch):
|
||||
"""A symlink member in the update ZIP must raise before extractall."""
|
||||
zip_path = tmp_path / "evil.zip"
|
||||
_build_zip_with_symlink_member(
|
||||
str(zip_path),
|
||||
link_name="hermes-agent-main/evil-link",
|
||||
target="/etc/passwd",
|
||||
)
|
||||
|
||||
from hermes_cli.main import _update_via_zip
|
||||
|
||||
args = type("Args", (), {})()
|
||||
|
||||
# Patch urlretrieve to "download" our pre-built malicious ZIP into the
|
||||
# _update_via_zip tempdir. Capture the tempdir so we can prove no
|
||||
# extraction happened.
|
||||
captured = {}
|
||||
original_mkdtemp = tempfile.mkdtemp
|
||||
|
||||
def capturing_mkdtemp(*args, **kwargs):
|
||||
d = original_mkdtemp(*args, **kwargs)
|
||||
captured["tmp_dir"] = d
|
||||
return d
|
||||
|
||||
def fake_urlretrieve(url, dest):
|
||||
# Copy our malicious zip into the destination dest path.
|
||||
with open(zip_path, "rb") as src, open(dest, "wb") as dst:
|
||||
dst.write(src.read())
|
||||
return dest, None
|
||||
|
||||
with patch("tempfile.mkdtemp", side_effect=capturing_mkdtemp), \
|
||||
patch("urllib.request.urlretrieve", side_effect=fake_urlretrieve):
|
||||
# _update_via_zip catches ValueError, prints the message, and exits 1.
|
||||
# That's the contract: a malicious ZIP must fail the update, not
|
||||
# silently materialize a symlink.
|
||||
with pytest.raises(SystemExit) as exc_info:
|
||||
_update_via_zip(args)
|
||||
assert exc_info.value.code == 1
|
||||
|
||||
# Belt: confirm extractall never produced the link.
|
||||
tmp_dir = captured.get("tmp_dir")
|
||||
if tmp_dir:
|
||||
evil_path = os.path.join(tmp_dir, "hermes-agent-main", "evil-link")
|
||||
assert not os.path.lexists(evil_path), (
|
||||
"symlink member should never be materialized"
|
||||
)
|
||||
|
||||
|
||||
def test_update_via_zip_accepts_normal_member(tmp_path, monkeypatch, capsys):
|
||||
"""A ZIP with only regular file members must extract without raising.
|
||||
|
||||
Sanity check that the symlink reject didn't break the happy path.
|
||||
Wraps just enough of _update_via_zip's I/O to verify extraction
|
||||
proceeds past the symlink check. We let the rest of the function
|
||||
fail naturally (no real git checkout to update); the assertion is
|
||||
just that we got past the ZIP validation.
|
||||
"""
|
||||
zip_path = tmp_path / "normal.zip"
|
||||
_build_normal_zip(str(zip_path))
|
||||
|
||||
from hermes_cli.main import _update_via_zip
|
||||
|
||||
args = type("Args", (), {})()
|
||||
|
||||
def fake_urlretrieve(url, dest):
|
||||
with open(zip_path, "rb") as src, open(dest, "wb") as dst:
|
||||
dst.write(src.read())
|
||||
return dest, None
|
||||
|
||||
with patch("urllib.request.urlretrieve", side_effect=fake_urlretrieve):
|
||||
# The function will fail later (no real install dir to update into),
|
||||
# but it must get past the ZIP validation without raising
|
||||
# ValueError("symlink member").
|
||||
try:
|
||||
_update_via_zip(args)
|
||||
except SystemExit:
|
||||
pass
|
||||
|
||||
captured = capsys.readouterr()
|
||||
assert "symlink member" not in captured.out
|
||||
assert "symlink member" not in captured.err
|
||||
Loading…
Add table
Add a link
Reference in a new issue