mirror of
https://github.com/usestrix/strix.git
synced 2026-08-18 09:49:17 +02:00
fix(llm): write credential store via mkstemp to defeat symlink attacks
Third review pass (security): the store temp file used a predictable subscription-auth.json.tmp name, so a local attacker could pre-plant a symlink there and divert the OAuth token write. Create it with tempfile.mkstemp (random name, mode 0600, no symlink following) in the same directory, then atomically rename over the target.
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user