diff --git a/agent/google_oauth.py b/agent/google_oauth.py index ede64251e29..6f45c370f6c 100644 --- a/agent/google_oauth.py +++ b/agent/google_oauth.py @@ -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(): diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index f21ada7db8b..59daa2c5d40 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -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 diff --git a/hermes_constants.py b/hermes_constants.py index a988fc5fda5..a81441e2080 100644 --- a/hermes_constants.py +++ b/hermes_constants.py @@ -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. diff --git a/tests/test_hermes_constants.py b/tests/test_hermes_constants.py index a3ffc0dcc14..edbb4eb7b84 100644 --- a/tests/test_hermes_constants.py +++ b/tests/test_hermes_constants.py @@ -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) + + diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index 8d48eedf0e8..53b4615000f 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -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)}")