mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-30 06:41:51 +00:00
fix(security): guard os.chmod(parent) against / and top-level dirs
Five call sites do os.chmod(path.parent, 0o700) without checking that the parent resolves to a safe directory. If HERMES_HOME or another path env var resolves to /, the chmod strips traversal permission from the root inode and bricks the entire host. Add secure_parent_dir() to hermes_constants.py that refuses to chmod / or any top-level directory (depth < 2). Replace all 5 call sites with this helper. Fixes #25821
This commit is contained in:
parent
3bbe980115
commit
4ead464f97
5 changed files with 127 additions and 22 deletions
|
|
@ -59,7 +59,7 @@ from dataclasses import dataclass
|
|||
from pathlib import Path
|
||||
from typing import Any, Dict, Optional, Tuple
|
||||
|
||||
from hermes_constants import get_hermes_home
|
||||
from hermes_constants import get_hermes_home, secure_parent_dir
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
|
@ -491,10 +491,8 @@ def save_credentials(creds: GoogleCredentials) -> Path:
|
|||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
# Tighten parent dir to 0o700 so siblings can't traverse to the creds file.
|
||||
# On Windows this is a no-op (POSIX mode bits aren't enforced); ignore failures.
|
||||
try:
|
||||
os.chmod(path.parent, 0o700)
|
||||
except OSError:
|
||||
pass
|
||||
# secure_parent_dir refuses to chmod / or top-level dirs (#25821).
|
||||
secure_parent_dir(path)
|
||||
payload = json.dumps(creds.to_dict(), indent=2, sort_keys=True) + "\n"
|
||||
|
||||
with _credentials_lock():
|
||||
|
|
|
|||
|
|
@ -48,7 +48,7 @@ import httpx
|
|||
import yaml
|
||||
|
||||
from hermes_cli.config import get_hermes_home, get_config_path, read_raw_config
|
||||
from hermes_constants import OPENROUTER_BASE_URL
|
||||
from hermes_constants import OPENROUTER_BASE_URL, secure_parent_dir
|
||||
from utils import atomic_replace, atomic_yaml_write, is_truthy_value
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
|
@ -1030,10 +1030,8 @@ def _save_auth_store(auth_store: Dict[str, Any]) -> Path:
|
|||
auth_file.parent.mkdir(parents=True, exist_ok=True)
|
||||
# Tighten parent dir to 0o700 so siblings can't traverse to creds.
|
||||
# No-op on Windows (POSIX mode bits not enforced); ignore failures.
|
||||
try:
|
||||
os.chmod(auth_file.parent, 0o700)
|
||||
except OSError:
|
||||
pass
|
||||
# secure_parent_dir refuses to chmod / or top-level dirs (#25821).
|
||||
secure_parent_dir(auth_file)
|
||||
auth_store["version"] = AUTH_STORE_VERSION
|
||||
auth_store["updated_at"] = datetime.now(timezone.utc).isoformat()
|
||||
payload = json.dumps(auth_store, indent=2) + "\n"
|
||||
|
|
@ -1863,10 +1861,8 @@ def _read_qwen_cli_tokens() -> Dict[str, Any]:
|
|||
def _save_qwen_cli_tokens(tokens: Dict[str, Any]) -> Path:
|
||||
auth_path = _qwen_cli_auth_path()
|
||||
auth_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
os.chmod(auth_path.parent, 0o700)
|
||||
except OSError:
|
||||
pass
|
||||
# secure_parent_dir refuses to chmod / or top-level dirs (#25821).
|
||||
secure_parent_dir(auth_path)
|
||||
# Per-process random temp suffix avoids collisions between concurrent
|
||||
# writers and stale leftovers from a crashed prior write.
|
||||
tmp_path = auth_path.with_name(f"{auth_path.name}.tmp.{os.getpid()}.{uuid.uuid4().hex}")
|
||||
|
|
@ -4168,10 +4164,8 @@ def _write_shared_nous_state(state: Dict[str, Any]) -> None:
|
|||
with _nous_shared_store_lock():
|
||||
path = _nous_shared_store_path()
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
os.chmod(path.parent, 0o700)
|
||||
except OSError:
|
||||
pass
|
||||
# secure_parent_dir refuses to chmod / or top-level dirs (#25821).
|
||||
secure_parent_dir(path)
|
||||
tmp = path.with_name(f"{path.name}.tmp.{os.getpid()}.{uuid.uuid4().hex}")
|
||||
# Create with 0o600 atomically via os.open(O_EXCL) — closes the TOCTOU
|
||||
# window where write_text() + post-write chmod briefly exposed Nous
|
||||
|
|
|
|||
|
|
@ -235,6 +235,27 @@ def display_hermes_home() -> str:
|
|||
return str(home)
|
||||
|
||||
|
||||
|
||||
|
||||
def secure_parent_dir(path: Path) -> None:
|
||||
"""Chmod ``0o700`` on the parent directory of *path*, but only if safe.
|
||||
|
||||
Refuses to chmod ``/`` or any top-level directory (depth < 2) to
|
||||
prevent catastrophic host bricking when ``HERMES_HOME`` or other
|
||||
path env vars resolve to an unexpected location.
|
||||
|
||||
See https://github.com/NousResearch/hermes-agent/issues/25821.
|
||||
"""
|
||||
parent = path.parent.resolve()
|
||||
# Refuse root and its direct children (/usr, /home, /var, /tmp, …).
|
||||
if parent == Path("/") or len(parent.parts) < 3:
|
||||
return
|
||||
try:
|
||||
os.chmod(parent, 0o700)
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
|
||||
def get_subprocess_home() -> str | None:
|
||||
"""Return a per-profile HOME directory for subprocesses, or None.
|
||||
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ from hermes_constants import (
|
|||
get_default_hermes_root,
|
||||
is_container,
|
||||
parse_reasoning_effort,
|
||||
secure_parent_dir,
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -171,3 +172,95 @@ class TestParseReasoningEffort:
|
|||
"""
|
||||
documented = {"minimal", "low", "medium", "high", "xhigh"}
|
||||
assert documented.issubset(set(VALID_REASONING_EFFORTS))
|
||||
|
||||
|
||||
class TestSecureParentDir:
|
||||
"""Tests for secure_parent_dir() — prevents chmod on / or top-level dirs."""
|
||||
|
||||
def test_safe_path_calls_chmod(self, tmp_path, monkeypatch):
|
||||
"""Normal nested path (depth >= 3) should call os.chmod."""
|
||||
safe_dir = tmp_path / "home" / "user" / ".hermes"
|
||||
safe_dir.mkdir(parents=True)
|
||||
target = safe_dir / "auth.json"
|
||||
target.touch()
|
||||
|
||||
called_with = []
|
||||
monkeypatch.setattr(os, "chmod", lambda p, m: called_with.append((str(p), m)))
|
||||
|
||||
secure_parent_dir(target)
|
||||
assert len(called_with) == 1
|
||||
assert called_with[0] == (str(safe_dir), 0o700)
|
||||
|
||||
def test_root_dir_skipped(self, monkeypatch):
|
||||
"""Parent resolving to / must NOT be chmod'd."""
|
||||
called_with = []
|
||||
monkeypatch.setattr(os, "chmod", lambda p, m: called_with.append((str(p), m)))
|
||||
|
||||
# Path("/foo").parent == Path("/")
|
||||
secure_parent_dir(Path("/foo"))
|
||||
assert called_with == []
|
||||
|
||||
def test_top_level_dir_skipped(self, monkeypatch):
|
||||
"""Parent resolving to a top-level dir (depth 2) must NOT be chmod'd."""
|
||||
called_with = []
|
||||
monkeypatch.setattr(os, "chmod", lambda p, m: called_with.append((str(p), m)))
|
||||
|
||||
# Path("/usr/foo").parent == Path("/usr") — depth 2
|
||||
secure_parent_dir(Path("/usr/foo"))
|
||||
assert called_with == []
|
||||
|
||||
def test_two_component_path_skipped(self, monkeypatch):
|
||||
"""Parent with < 3 resolved parts must NOT be chmod'd.
|
||||
|
||||
Uses monkeypatch to avoid macOS firmlink resolution of /home.
|
||||
"""
|
||||
called_with = []
|
||||
monkeypatch.setattr(os, "chmod", lambda p, m: called_with.append((str(p), m)))
|
||||
|
||||
# Mock Path.resolve to return a short path regardless of OS quirks
|
||||
original_resolve = Path.resolve
|
||||
def mock_resolve(self):
|
||||
if str(self) == "/x/y":
|
||||
return Path("/x")
|
||||
return original_resolve(self)
|
||||
monkeypatch.setattr(Path, "resolve", mock_resolve)
|
||||
|
||||
secure_parent_dir(Path("/x/y"))
|
||||
assert called_with == []
|
||||
|
||||
def test_oserror_suppressed(self, tmp_path, monkeypatch):
|
||||
"""OSError from chmod should be silently caught."""
|
||||
safe_dir = tmp_path / "a" / "b" / "c"
|
||||
safe_dir.mkdir(parents=True)
|
||||
target = safe_dir / "file.json"
|
||||
target.touch()
|
||||
|
||||
def raise_oserror(p, m):
|
||||
raise OSError("permission denied")
|
||||
|
||||
monkeypatch.setattr(os, "chmod", raise_oserror)
|
||||
# Should not raise
|
||||
secure_parent_dir(target)
|
||||
|
||||
def test_symlink_resolved(self, tmp_path, monkeypatch):
|
||||
"""Symlinks should be resolved before checking depth."""
|
||||
real_dir = tmp_path / "a" / "b"
|
||||
real_dir.mkdir(parents=True)
|
||||
target = real_dir / "file.json"
|
||||
target.touch()
|
||||
|
||||
# Create a symlink with fewer path components
|
||||
link = tmp_path / "link"
|
||||
link.symlink_to(real_dir)
|
||||
link_target = link / "file.json"
|
||||
|
||||
called_with = []
|
||||
monkeypatch.setattr(os, "chmod", lambda p, m: called_with.append((str(p), m)))
|
||||
|
||||
# Even though /tmp/link has only 3 parts, the resolved path has 4
|
||||
# The resolved parent (real_dir) has depth 4, so it should be chmod'd
|
||||
secure_parent_dir(link_target)
|
||||
assert len(called_with) == 1
|
||||
assert called_with[0] == (str(real_dir), 0o700)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -48,6 +48,7 @@ from http.server import BaseHTTPRequestHandler, HTTPServer
|
|||
from pathlib import Path
|
||||
from typing import Any
|
||||
from urllib.parse import parse_qs, urlparse
|
||||
from hermes_constants import secure_parent_dir
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
|
@ -175,10 +176,8 @@ def _write_json(path: Path, data: dict) -> None:
|
|||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
# Tighten parent dir to 0o700 so siblings can't traverse to the creds.
|
||||
# No-op on Windows (POSIX mode bits aren't enforced); ignore failures.
|
||||
try:
|
||||
os.chmod(path.parent, 0o700)
|
||||
except OSError:
|
||||
pass
|
||||
# secure_parent_dir refuses to chmod / or top-level dirs (#25821).
|
||||
secure_parent_dir(path)
|
||||
# Per-process random suffix avoids collisions between concurrent
|
||||
# writers and stale leftovers from a prior crashed write.
|
||||
tmp = path.with_suffix(f".tmp.{os.getpid()}.{secrets.token_hex(4)}")
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue