diff --git a/strix/config/subscription_store.py b/strix/config/subscription_store.py index 148e8871..cfc9d745 100644 --- a/strix/config/subscription_store.py +++ b/strix/config/subscription_store.py @@ -18,14 +18,15 @@ from __future__ import annotations import contextlib import json import os +import tempfile import threading +from pathlib import Path from typing import TYPE_CHECKING, Any if TYPE_CHECKING: from collections.abc import Iterator from io import TextIOWrapper - from pathlib import Path class StoreLockError(RuntimeError): @@ -46,18 +47,23 @@ def read(path: Path) -> dict[str, Any]: def write(path: Path, data: dict[str, Any]) -> None: - """Atomically replace the store, owner-only from creation.""" + """Atomically replace the store, owner-only from creation. + + The temp file is created with a random name via ``mkstemp`` (mode 0600, no + symlink following), so a local attacker can't pre-plant a symlink at a + predictable path to divert the token write. + """ path.parent.mkdir(parents=True, exist_ok=True) - tmp = path.with_suffix(".json.tmp") - fd = os.open(str(tmp), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + fd, tmp_name = tempfile.mkstemp(dir=path.parent, prefix=path.name, suffix=".tmp") + tmp = Path(tmp_name) try: with os.fdopen(fd, "w", encoding="utf-8") as handle: json.dump(data, handle, indent=2) + tmp.replace(path) except BaseException: with contextlib.suppress(OSError): tmp.unlink() raise - tmp.replace(path) with contextlib.suppress(OSError): path.chmod(0o600) diff --git a/tests/test_subscription_store.py b/tests/test_subscription_store.py index b6b83b1f..6f8a8be7 100644 --- a/tests/test_subscription_store.py +++ b/tests/test_subscription_store.py @@ -23,6 +23,22 @@ def test_write_creates_owner_only_file(tmp_path: Path) -> None: assert not path.with_suffix(".json.tmp").exists() +def test_write_does_not_follow_a_symlink_at_target(tmp_path: Path) -> None: + store_dir = tmp_path / ".strix" + store_dir.mkdir() + outside = tmp_path / "attacker-target.json" + path = store_dir / "subscription-auth.json" + path.symlink_to(outside) # attacker pre-plants a symlink at the store path + + subscription_store.write(path, {"grok": {"type": "oauth", "access": "a", "refresh": "r"}}) + + # The atomic rename replaced the symlink with a real file; nothing was + # written through it to the attacker-chosen location. + assert not path.is_symlink() + assert not outside.exists() + assert subscription_store.read(path)["grok"]["access"] == "a" + + def test_providers_share_store_without_clobbering( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: