Skip to content

Commit e0f736d

Browse files
committed
fix(security): close TOCTOU window in hermes_cli/auth.py credential writers
`_save_auth_store`, `_save_qwen_cli_tokens`, and `_write_shared_nous_state` all created the temp file via `Path.open('w')` / `Path.write_text` and only tightened permissions to 0o600 afterward. Between create and chmod the file existed at the process umask (commonly 0o644 = world-readable on multi-user hosts), briefly exposing OAuth access/refresh tokens for Nous, Codex, Copilot, Claude, Qwen, Gemini, and every other native OAuth provider that flows through auth.json. Switch all three to `os.open(O_WRONLY|O_CREAT|O_EXCL, 0o600)` + `os.fdopen` + `fsync` so the file is atomic at 0o600 on creation. Tighten each parent directory (`~/.hermes/`, Qwen auth dir, Nous shared auth dir) to 0o700 so siblings can't traverse to the creds. `_save_auth_store` also gains a per-process random temp suffix to match `agent/google_oauth.py` (#19673) and `tools/mcp_oauth.py` (#21148). Adds `tests/hermes_cli/test_auth_toctou_file_modes.py` asserting final file mode 0o600 and parent dir mode 0o700 across all three writers, plus an explicit `os.open(flags, mode)` check on the main auth.json writer that would fail if anyone reintroduces the `Path.open('w')` pattern. POSIX-only (mode bits skipped on Windows).
1 parent 2e00bca commit e0f736d

2 files changed

Lines changed: 263 additions & 9 deletions

File tree

hermes_cli/auth.py

Lines changed: 65 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -985,12 +985,27 @@ def _load_auth_store(auth_file: Optional[Path] = None) -> Dict[str, Any]:
985985
def _save_auth_store(auth_store: Dict[str, Any]) -> Path:
986986
auth_file = _auth_file_path()
987987
auth_file.parent.mkdir(parents=True, exist_ok=True)
988+
# Tighten parent dir to 0o700 so siblings can't traverse to creds.
989+
# No-op on Windows (POSIX mode bits not enforced); ignore failures.
990+
try:
991+
os.chmod(auth_file.parent, 0o700)
992+
except OSError:
993+
pass
988994
auth_store["version"] = AUTH_STORE_VERSION
989995
auth_store["updated_at"] = datetime.now(timezone.utc).isoformat()
990996
payload = json.dumps(auth_store, indent=2) + "\n"
991997
tmp_path = auth_file.with_name(f"{auth_file.name}.tmp.{os.getpid()}.{uuid.uuid4().hex}")
992998
try:
993-
with tmp_path.open("w", encoding="utf-8") as handle:
999+
# Create with 0o600 atomically via os.open(O_EXCL) + fdopen to close
1000+
# the TOCTOU window where default umask (often 0o644) briefly exposed
1001+
# OAuth tokens to other local users between open() and chmod().
1002+
# Mirrors agent/google_oauth.py (#19673) and tools/mcp_oauth.py (#21148).
1003+
fd = os.open(
1004+
str(tmp_path),
1005+
os.O_WRONLY | os.O_CREAT | os.O_EXCL,
1006+
stat.S_IRUSR | stat.S_IWUSR,
1007+
)
1008+
with os.fdopen(fd, "w", encoding="utf-8") as handle:
9941009
handle.write(payload)
9951010
handle.flush()
9961011
os.fsync(handle.fileno())
@@ -1554,10 +1569,33 @@ def _read_qwen_cli_tokens() -> Dict[str, Any]:
15541569
def _save_qwen_cli_tokens(tokens: Dict[str, Any]) -> Path:
15551570
auth_path = _qwen_cli_auth_path()
15561571
auth_path.parent.mkdir(parents=True, exist_ok=True)
1557-
tmp_path = auth_path.with_suffix(".tmp")
1558-
tmp_path.write_text(json.dumps(tokens, indent=2, sort_keys=True) + "\n", encoding="utf-8")
1559-
os.chmod(tmp_path, stat.S_IRUSR | stat.S_IWUSR)
1560-
tmp_path.replace(auth_path)
1572+
try:
1573+
os.chmod(auth_path.parent, 0o700)
1574+
except OSError:
1575+
pass
1576+
# Per-process random temp suffix avoids collisions between concurrent
1577+
# writers and stale leftovers from a crashed prior write.
1578+
tmp_path = auth_path.with_name(f"{auth_path.name}.tmp.{os.getpid()}.{uuid.uuid4().hex}")
1579+
# Create with 0o600 atomically via os.open(O_EXCL) — closes the TOCTOU
1580+
# window where write_text() + post-write chmod briefly exposed tokens
1581+
# at process umask (typically 0o644). See #19673, #21148.
1582+
fd = os.open(
1583+
str(tmp_path),
1584+
os.O_WRONLY | os.O_CREAT | os.O_EXCL,
1585+
stat.S_IRUSR | stat.S_IWUSR,
1586+
)
1587+
try:
1588+
with os.fdopen(fd, "w", encoding="utf-8") as fh:
1589+
fh.write(json.dumps(tokens, indent=2, sort_keys=True) + "\n")
1590+
fh.flush()
1591+
os.fsync(fh.fileno())
1592+
atomic_replace(tmp_path, auth_path)
1593+
finally:
1594+
try:
1595+
if tmp_path.exists():
1596+
tmp_path.unlink()
1597+
except OSError:
1598+
pass
15611599
return auth_path
15621600

15631601

@@ -2938,13 +2976,31 @@ def _write_shared_nous_state(state: Dict[str, Any]) -> None:
29382976
with _nous_shared_store_lock():
29392977
path = _nous_shared_store_path()
29402978
path.parent.mkdir(parents=True, exist_ok=True)
2941-
tmp = path.with_suffix(path.suffix + ".tmp")
2942-
tmp.write_text(json.dumps(shared, indent=2, sort_keys=True))
29432979
try:
2944-
os.chmod(tmp, 0o600)
2980+
os.chmod(path.parent, 0o700)
29452981
except OSError:
29462982
pass
2947-
os.replace(tmp, path)
2983+
tmp = path.with_name(f"{path.name}.tmp.{os.getpid()}.{uuid.uuid4().hex}")
2984+
# Create with 0o600 atomically via os.open(O_EXCL) — closes the TOCTOU
2985+
# window where write_text() + post-write chmod briefly exposed Nous
2986+
# refresh_token at process umask. See #19673, #21148.
2987+
fd = os.open(
2988+
str(tmp),
2989+
os.O_WRONLY | os.O_CREAT | os.O_EXCL,
2990+
stat.S_IRUSR | stat.S_IWUSR,
2991+
)
2992+
try:
2993+
with os.fdopen(fd, "w", encoding="utf-8") as fh:
2994+
fh.write(json.dumps(shared, indent=2, sort_keys=True))
2995+
fh.flush()
2996+
os.fsync(fh.fileno())
2997+
os.replace(tmp, path)
2998+
finally:
2999+
try:
3000+
if tmp.exists():
3001+
tmp.unlink()
3002+
except OSError:
3003+
pass
29483004
_oauth_trace(
29493005
"nous_shared_store_written",
29503006
path=str(path),
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
"""Regression tests for TOCTOU-safe credential file writers in ``hermes_cli.auth``.
2+
3+
Background
4+
==========
5+
The three writers below used to create a temp file via ``Path.write_text`` /
6+
``Path.open('w')`` and only ``chmod``'d it to ``0o600`` afterward. Between
7+
create and chmod the file existed at the process umask (typically ``0o644``),
8+
briefly exposing OAuth tokens to other local users on multi-user hosts. The
9+
fix switches them to ``os.open(O_EXCL, mode=0o600)`` + ``os.fdopen`` +
10+
``fsync`` so the file is atomic at ``0o600`` on creation. Mirrors the fixes
11+
shipped for ``agent/google_oauth.py`` (#19673) and ``tools/mcp_oauth.py``
12+
(#21148).
13+
14+
These tests stay green only while the token file and its parent directory
15+
end up at ``0o600`` / ``0o700`` after every write. POSIX-only — the mode-bit
16+
enforcement does not exist on Windows.
17+
"""
18+
19+
from __future__ import annotations
20+
21+
import json
22+
import os
23+
import stat
24+
import sys
25+
from unittest.mock import patch
26+
27+
import pytest
28+
29+
30+
pytestmark = pytest.mark.skipif(
31+
sys.platform.startswith("win"),
32+
reason="POSIX mode bits not enforced on Windows",
33+
)
34+
35+
36+
# ---------------------------------------------------------------------------
37+
# _save_auth_store (~/.hermes/auth.json — every native OAuth provider)
38+
# ---------------------------------------------------------------------------
39+
40+
41+
def test_save_auth_store_writes_0o600_with_0o700_parent(tmp_path, monkeypatch):
42+
"""``_save_auth_store`` must land ``auth.json`` at 0o600 and parent at 0o700."""
43+
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
44+
old_umask = os.umask(0o022) # make the race observable if it regresses
45+
try:
46+
from hermes_cli import auth as auth_mod
47+
48+
auth_store = {
49+
"version": auth_mod.AUTH_STORE_VERSION,
50+
"providers": {"openai-codex": {"tokens": {"access_token": "secret-x"}}},
51+
"active_provider": "openai-codex",
52+
}
53+
auth_path = auth_mod._save_auth_store(auth_store)
54+
finally:
55+
os.umask(old_umask)
56+
57+
mode = stat.S_IMODE(auth_path.stat().st_mode)
58+
parent_mode = stat.S_IMODE(auth_path.parent.stat().st_mode)
59+
60+
assert mode == 0o600, (
61+
f"auth.json mode 0o{mode:o} != 0o600 — TOCTOU race regressed"
62+
)
63+
assert parent_mode == 0o700, (
64+
f"auth.json parent dir mode 0o{parent_mode:o} != 0o700 — siblings can traverse"
65+
)
66+
67+
# Content survived the rewrite
68+
data = json.loads(auth_path.read_text())
69+
assert data["providers"]["openai-codex"]["tokens"]["access_token"] == "secret-x"
70+
71+
72+
# ---------------------------------------------------------------------------
73+
# _save_qwen_cli_tokens (Qwen CLI OAuth tokens)
74+
# ---------------------------------------------------------------------------
75+
76+
77+
def test_save_qwen_cli_tokens_writes_0o600_with_0o700_parent(tmp_path, monkeypatch):
78+
"""``_save_qwen_cli_tokens`` must land the token file at 0o600 and parent at 0o700."""
79+
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
80+
# The Qwen CLI auth path lives under $HOME/.qwen by default — isolate it.
81+
monkeypatch.setenv("HOME", str(tmp_path))
82+
old_umask = os.umask(0o022)
83+
try:
84+
from hermes_cli import auth as auth_mod
85+
86+
tokens = {
87+
"access_token": "qwen-secret",
88+
"refresh_token": "qwen-refresh",
89+
"token_type": "Bearer",
90+
"expiry_date": 123,
91+
}
92+
auth_path = auth_mod._save_qwen_cli_tokens(tokens)
93+
finally:
94+
os.umask(old_umask)
95+
96+
mode = stat.S_IMODE(auth_path.stat().st_mode)
97+
parent_mode = stat.S_IMODE(auth_path.parent.stat().st_mode)
98+
99+
assert mode == 0o600, (
100+
f"Qwen token file mode 0o{mode:o} != 0o600 — TOCTOU race regressed"
101+
)
102+
assert parent_mode == 0o700, (
103+
f"Qwen token parent dir mode 0o{parent_mode:o} != 0o700"
104+
)
105+
106+
data = json.loads(auth_path.read_text())
107+
assert data["access_token"] == "qwen-secret"
108+
109+
110+
# ---------------------------------------------------------------------------
111+
# Nous shared-credential store write (inside _write_shared_nous_state)
112+
# ---------------------------------------------------------------------------
113+
114+
115+
def test_shared_nous_store_writes_0o600_with_0o700_parent(tmp_path, monkeypatch):
116+
"""The Nous shared-credential store must land at 0o600 / parent 0o700."""
117+
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
118+
# _nous_shared_store_path() refuses to touch the real shared store during
119+
# pytest runs; redirect it into tmp_path explicitly.
120+
monkeypatch.setenv("HERMES_SHARED_AUTH_DIR", str(tmp_path / "shared"))
121+
old_umask = os.umask(0o022)
122+
try:
123+
from hermes_cli import auth as auth_mod
124+
125+
state = {
126+
"access_token": "nous-access-xxx",
127+
"refresh_token": "nous-refresh-xxx",
128+
"token_type": "Bearer",
129+
"scope": "openid profile",
130+
"client_id": "test-client",
131+
"obtained_at": "2026-01-01T00:00:00Z",
132+
"expires_at": "2026-01-01T01:00:00Z",
133+
}
134+
auth_mod._write_shared_nous_state(state)
135+
path = auth_mod._nous_shared_store_path()
136+
finally:
137+
os.umask(old_umask)
138+
139+
assert path.exists(), "shared Nous store was not written"
140+
mode = stat.S_IMODE(path.stat().st_mode)
141+
parent_mode = stat.S_IMODE(path.parent.stat().st_mode)
142+
143+
assert mode == 0o600, (
144+
f"Nous shared store mode 0o{mode:o} != 0o600 — TOCTOU race regressed"
145+
)
146+
assert parent_mode == 0o700, (
147+
f"Nous shared store parent dir mode 0o{parent_mode:o} != 0o700"
148+
)
149+
150+
data = json.loads(path.read_text())
151+
assert data["refresh_token"] == "nous-refresh-xxx"
152+
153+
154+
# ---------------------------------------------------------------------------
155+
# Atomicity: verify ``os.open`` is called with an explicit 0o600 mode.
156+
# ---------------------------------------------------------------------------
157+
158+
159+
def test_save_auth_store_uses_os_open_with_0o600_mode(tmp_path, monkeypatch):
160+
"""Regression: the writer must call ``os.open`` with an explicit restricted
161+
mode so the file is created at 0o600 atomically — closing the TOCTOU
162+
window the previous ``Path.open('w')`` left open (fd inherited process
163+
umask and was briefly 0o644 before post-write chmod)."""
164+
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
165+
166+
observed_opens: list[tuple[str, int, int]] = []
167+
real_os_open = os.open
168+
169+
def spying_os_open(path, flags, mode=0o777, *args, **kwargs):
170+
observed_opens.append((str(path), flags, mode))
171+
return real_os_open(path, flags, mode, *args, **kwargs)
172+
173+
with patch.object(os, "open", spying_os_open):
174+
from hermes_cli import auth as auth_mod
175+
176+
auth_mod._save_auth_store(
177+
{"version": auth_mod.AUTH_STORE_VERSION, "providers": {}}
178+
)
179+
180+
auth_tmp_opens = [
181+
(p, fl, m) for (p, fl, m) in observed_opens if "auth.json.tmp" in p
182+
]
183+
assert auth_tmp_opens, (
184+
f"os.open was never called for the auth.json temp file; "
185+
f"observed={observed_opens!r}"
186+
)
187+
for path, flags, mode in auth_tmp_opens:
188+
assert flags & os.O_CREAT, f"auth.json temp open missing O_CREAT: path={path}"
189+
assert flags & os.O_EXCL, (
190+
f"auth.json temp open missing O_EXCL — TOCTOU-safe pattern regressed: "
191+
f"path={path}, flags={flags}"
192+
)
193+
# Must be exactly S_IRUSR | S_IWUSR (0o600) — no group/other bits.
194+
expected = stat.S_IRUSR | stat.S_IWUSR
195+
assert mode == expected, (
196+
f"auth.json temp open mode 0o{mode:o} != 0o{expected:o} — "
197+
f"umask would apply and potentially expose tokens"
198+
)

0 commit comments

Comments
 (0)