mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(tools): keep shell snapshots owner-only
BaseEnvironment writes shell snapshots and cwd metadata through the process umask. With a common 022 umask, snapshot files containing exported environment state landed at mode 0644 even though they can include env-carried credentials from the parent process. Set umask 077 only around Hermes metadata writes: the initial snapshot bootstrap and the post-command snapshot/cwd refresh. User commands still run under the caller's original umask, while Hermes-owned snapshot and cwd files are created owner-only. This intentionally does not copy the source PR's global orphan sweep; deleting all matching /tmp snapshot files could interfere with concurrent Hermes processes. The security-critical local disclosure fix is the file mode clamp. This is salvageable because the source report still identifies a concrete credential-disclosure path, but the safe subset is smaller than the original proposal: clamp only the Hermes-owned snapshot writes and leave process-wide cleanup, user command umask, and concurrent sessions alone. Salvages source PR: https://github.com/NousResearch/hermes-agent/pull/20056 Related issue: https://github.com/NousResearch/hermes-agent/issues/48441 Co-authored-by: Andrew Homeyer <andrew@hndl.app>
This commit is contained in:
parent
4f6313eadc
commit
a1e6ea7d71
2 changed files with 81 additions and 0 deletions
|
|
@ -174,6 +174,32 @@ class TestAtomicSnapshotWrite:
|
|||
assert "$BASHPID" in boot
|
||||
assert ".tmp.$$" not in boot
|
||||
|
||||
def test_snapshot_writes_use_private_umask_after_user_command(self):
|
||||
env = _TestableEnv()
|
||||
env._snapshot_ready = True
|
||||
wrapped = env._wrap_command("echo hi", "/tmp")
|
||||
|
||||
assert "umask 077" in wrapped
|
||||
assert wrapped.index("eval 'echo hi'") < wrapped.index("umask 077")
|
||||
assert wrapped.index("umask 077") < wrapped.index("export -p >")
|
||||
|
||||
def test_init_session_bootstrap_uses_private_umask(self):
|
||||
env = _TestableEnv()
|
||||
captured = {}
|
||||
|
||||
def fake_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
|
||||
captured["cmd"] = cmd_string
|
||||
raise RuntimeError("stop after capture")
|
||||
|
||||
env._run_bash = fake_run_bash # type: ignore[assignment]
|
||||
try:
|
||||
env.init_session()
|
||||
except Exception:
|
||||
pass
|
||||
boot = captured.get("cmd", "")
|
||||
assert "umask 077" in boot
|
||||
assert boot.index("umask 077") < boot.index("export -p >")
|
||||
|
||||
|
||||
class TestAtomicSnapshotConcurrencyBehavioral:
|
||||
"""Behavioral regression for #38249 — actually EXECUTES the generated
|
||||
|
|
@ -250,6 +276,57 @@ class TestAtomicSnapshotConcurrencyBehavioral:
|
|||
assert "export GOOD=1" in out.stdout, "good snapshot was destroyed by a failed export"
|
||||
|
||||
|
||||
class TestSnapshotFileModes:
|
||||
"""Snapshot metadata files are private without changing user command umask."""
|
||||
|
||||
def test_snapshot_and_cwd_files_are_0600(self, tmp_path):
|
||||
import os
|
||||
from pathlib import Path
|
||||
import shutil
|
||||
import stat
|
||||
import subprocess
|
||||
if not shutil.which("bash"):
|
||||
import pytest
|
||||
pytest.skip("bash required")
|
||||
|
||||
class ExecutableEnv(BaseEnvironment):
|
||||
def __init__(self, temp_dir):
|
||||
self._temp_dir = str(temp_dir)
|
||||
super().__init__(cwd=str(temp_dir), timeout=10)
|
||||
|
||||
def get_temp_dir(self):
|
||||
return self._temp_dir
|
||||
|
||||
def _run_bash(self, cmd_string, *, login=False, timeout=120, stdin_data=None):
|
||||
proc = subprocess.Popen(
|
||||
["/bin/bash", "-lc", cmd_string],
|
||||
stdout=subprocess.PIPE,
|
||||
stderr=subprocess.STDOUT,
|
||||
stdin=subprocess.DEVNULL,
|
||||
text=True,
|
||||
cwd=self.cwd,
|
||||
)
|
||||
proc.communicate(timeout=timeout)
|
||||
return proc
|
||||
|
||||
def cleanup(self):
|
||||
pass
|
||||
|
||||
old_umask = os.umask(0o022)
|
||||
try:
|
||||
env = ExecutableEnv(tmp_path)
|
||||
env.init_session()
|
||||
|
||||
user_file = tmp_path / "user-created.txt"
|
||||
env.execute(f"touch {user_file}")
|
||||
|
||||
assert stat.S_IMODE(user_file.stat().st_mode) == 0o644
|
||||
assert stat.S_IMODE(Path(env._snapshot_path).stat().st_mode) == 0o600
|
||||
assert stat.S_IMODE(Path(env._cwd_file).stat().st_mode) == 0o600
|
||||
finally:
|
||||
os.umask(old_umask)
|
||||
|
||||
|
||||
class TestExtractCwdFromOutput:
|
||||
def test_happy_path(self):
|
||||
env = _TestableEnv()
|
||||
|
|
|
|||
|
|
@ -395,6 +395,7 @@ class BaseEnvironment(ABC):
|
|||
# with ``$BASHPID`` left outside the quotes so it still expands.
|
||||
_snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$BASHPID"
|
||||
bootstrap = (
|
||||
f"umask 077\n"
|
||||
f"export -p > {_snap_tmp}\n"
|
||||
# Dump function definitions, filtering out private (``_``-prefixed)
|
||||
# helpers — mainly bash-completion internals (``_git``, ``_make``…)
|
||||
|
|
@ -502,6 +503,9 @@ class BaseEnvironment(ABC):
|
|||
# Run the actual command
|
||||
parts.append(f"eval '{escaped}'")
|
||||
parts.append("__hermes_ec=$?")
|
||||
# Restrict Hermes metadata files without changing the user's command
|
||||
# umask. Snapshot files may contain env-carried secrets.
|
||||
parts.append("umask 077")
|
||||
|
||||
# Re-dump env vars to snapshot (atomic replacement to avoid races).
|
||||
# Chain mv on the export succeeding so a failed/partial dump never
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue