From cb978a570d128ba8b8460292ad0b6f4d3c6ab7a4 Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Wed, 15 Jul 2026 12:07:51 -0700 Subject: [PATCH] fix(cli): close TOCTOU window in lite up's settings backup write write_backup wrote the backup (which can embed the original apiKeyHelper/settings content) with plain open() + a chmod call after the fact -- the same permissive-until-corrected window already fixed for autoroute's config.yaml and Claude settings writes, and missed entirely when the backup file already exists with broader permissions. Moves secure_create (atomic-enough 0600 via fchmod before any content is written) to up.py, the module both lite up and lite autoroute share, and has autoroute/process.py import it from there instead of keeping its own copy. --- .../client/cli/commands/autoroute/process.py | 25 ++--------------- litellm/proxy/client/cli/commands/up.py | 27 ++++++++++++++++--- .../proxy/client/cli/test_up_commands.py | 16 +++++++++++ 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/litellm/proxy/client/cli/commands/autoroute/process.py b/litellm/proxy/client/cli/commands/autoroute/process.py index 000ccf49623..d1b798c40cf 100644 --- a/litellm/proxy/client/cli/commands/autoroute/process.py +++ b/litellm/proxy/client/cli/commands/autoroute/process.py @@ -9,40 +9,19 @@ import threading import time from dataclasses import dataclass from pathlib import Path -from typing import IO, Iterator import click import requests from pydantic import TypeAdapter +from ..up import secure_create + AUTOROUTE_DIR = Path.home() / ".litellm" / "autorouter" CONFIG_PATH = AUTOROUTE_DIR / "config.yaml" LOG_PATH = AUTOROUTE_DIR / "proxy.log" PID_RECORD_PATH = AUTOROUTE_DIR / "proxy.pid.json" -@contextlib.contextmanager -def secure_create(path: Path) -> Iterator[IO[str]]: - """Open path for writing with mode 0600 fixed up before any content is written. - - A plain `open(path, "w")` creates a *new* file at the umask-derived default (commonly 0644) - and leaves it world- or group-readable until a later `chmod` call catches up -- a real window - in which a file holding a credential (a proxy master key, a Claude Code auth token) is readable - by another local account. Passing the mode to `os.open` closes that window for a brand-new - file, but `O_CREAT`'s mode argument is only applied on creation: if the file already exists - (the common case for `~/.claude/settings.json`, which normally predates `lite autoroute up`) - its old, broader permissions carry over untouched. `os.fchmod` right after opening -- before a - single byte of the new content is written -- covers both cases. - """ - fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) - os.fchmod(fd, 0o600) - f: IO[str] = os.fdopen(fd, "w") - try: - yield f - finally: - f.close() - - class ProcessLaunchError(Exception): """Raised when the ephemeral proxy subprocess fails to come up healthy.""" diff --git a/litellm/proxy/client/cli/commands/up.py b/litellm/proxy/client/cli/commands/up.py index 23cce90992c..edf5a7acc05 100644 --- a/litellm/proxy/client/cli/commands/up.py +++ b/litellm/proxy/client/cli/commands/up.py @@ -1,4 +1,5 @@ import atexit +import contextlib import json import os import shlex @@ -9,7 +10,7 @@ import threading from dataclasses import dataclass from pathlib import Path from types import FrameType -from typing import Mapping +from typing import IO, Iterator, Mapping import click from pydantic import JsonValue, TypeAdapter @@ -71,12 +72,32 @@ def merge_claude_settings( return {**settings, ENV_KEY: env, API_KEY_HELPER_KEY: api_key_helper} +@contextlib.contextmanager +def secure_create(path: Path) -> Iterator[IO[str]]: + """Open path for writing with mode 0600 fixed up before any content is written. + + A plain `open(path, "w")` creates a *new* file at the umask-derived default (commonly 0644) + and leaves it world- or group-readable until a later `chmod` call catches up -- a real window + in which a file holding a credential is readable by another local account. Passing the mode to + `os.open` closes that window for a brand-new file, but `O_CREAT`'s mode argument is only + applied on creation: if the file already exists its old, broader permissions carry over + untouched. `os.fchmod` right after opening -- before a single byte of the new content is + written -- covers both cases. + """ + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + os.fchmod(fd, 0o600) + f: IO[str] = os.fdopen(fd, "w") + try: + yield f + finally: + f.close() + + def write_backup(record: BackupRecord, backup_path: Path | None = None) -> None: path = backup_path if backup_path is not None else BACKUP_PATH path.parent.mkdir(exist_ok=True) - with open(path, "w") as f: + with secure_create(path) as f: json.dump({"existed": record.existed, "content": record.content}, f, indent=2) - os.chmod(path, 0o600) def read_backup(backup_path: Path | None = None) -> BackupRecord | None: diff --git a/tests/test_litellm/proxy/client/cli/test_up_commands.py b/tests/test_litellm/proxy/client/cli/test_up_commands.py index 63f1da8facf..49508c22f47 100644 --- a/tests/test_litellm/proxy/client/cli/test_up_commands.py +++ b/tests/test_litellm/proxy/client/cli/test_up_commands.py @@ -1,5 +1,6 @@ import json import shutil +import stat import sys from unittest.mock import patch @@ -129,6 +130,21 @@ class TestBackupRoundTrip: _patch_paths(monkeypatch, tmp_path) assert read_backup() is None + def test_write_backup_restricts_permissions_for_a_new_file(self, monkeypatch, tmp_path): + _settings_path, backup_path = _patch_paths(monkeypatch, tmp_path) + write_backup(BackupRecord(existed=True, content={"a": 1})) + assert stat.S_IMODE(backup_path.stat().st_mode) == 0o600 + + def test_write_backup_restricts_permissions_of_a_preexisting_permissive_file(self, monkeypatch, tmp_path): + _settings_path, backup_path = _patch_paths(monkeypatch, tmp_path) + backup_path.parent.mkdir(parents=True, exist_ok=True) + backup_path.write_text("{}") + backup_path.chmod(0o644) + + write_backup(BackupRecord(existed=True, content={"a": 1})) + + assert stat.S_IMODE(backup_path.stat().st_mode) == 0o600 + def test_backup_file_always_removed_after_restore(self, monkeypatch, tmp_path): _settings_path, backup_path = _patch_paths(monkeypatch, tmp_path) write_backup(BackupRecord(existed=False, content=None))