Skip to content

Commit 750684c

Browse files
theCyberTechclaude
andcommitted
fix: enforce owner-only permissions on credential files
Credentials stored at rest were left world-readable on multi-user hosts: - TokenManager._get_secure_storage_path() documented its credential dir as mode 0o700 but created it via mkdir() with default perms (0o755), leaving the Fernet secret.key and encrypted tokens.enc in a traversable dir. - Settings.dump() persisted tool_repository_password (plaintext) to settings.json via open("w"), producing a 0o644 file, and created the config dir at 0o755 — despite the sibling token_manager already writing secrets atomically at 0o600. Fixes: - TokenManager: chmod the credential dir to 0o700 after mkdir (robust against umask and pre-existing dirs). - Settings: write settings.json atomically at 0o600 (mkstemp + chmod + os.replace) and chmod the dedicated config dir to 0o700. The /tmp and cwd fallback parents are deliberately not chmod'd; the 0o600 file mode protects the credential there. Adds regression tests asserting 0o600 files and 0o700 dirs, and that shared fallback dirs are not globally tightened. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 854c67d commit 750684c

4 files changed

Lines changed: 144 additions & 2 deletions

File tree

lib/cli/tests/test_config.py

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import json
2+
import os
23
import shutil
4+
import stat
5+
import sys
36
import tempfile
47
import unittest
58
from datetime import datetime, timedelta
@@ -146,3 +149,55 @@ def test_empty_config_file(self):
146149

147150
settings = Settings(config_path=self.config_path)
148151
self.assertIsNone(settings.tool_repository_username)
152+
153+
154+
class TestSettingsFilePermissions(unittest.TestCase):
155+
"""Regression tests: credentials in settings.json must not be world-readable."""
156+
157+
def setUp(self):
158+
self.test_dir = Path(tempfile.mkdtemp())
159+
160+
def tearDown(self):
161+
shutil.rmtree(self.test_dir, ignore_errors=True)
162+
163+
@unittest.skipIf(sys.platform == "win32", "POSIX permission semantics")
164+
def test_dump_writes_owner_only_file(self):
165+
config_path = self.test_dir / "settings.json"
166+
old_umask = os.umask(0o022)
167+
try:
168+
settings = Settings(
169+
config_path=config_path, tool_repository_password="hunter2"
170+
)
171+
settings.dump()
172+
finally:
173+
os.umask(old_umask)
174+
175+
mode = stat.S_IMODE(config_path.stat().st_mode)
176+
self.assertEqual(mode, 0o600, f"expected 0o600, got {oct(mode)}")
177+
178+
@unittest.skipIf(sys.platform == "win32", "POSIX permission semantics")
179+
def test_dedicated_config_dir_is_owner_only(self):
180+
config_path = self.test_dir / "crewai" / "settings.json"
181+
old_umask = os.umask(0o022)
182+
try:
183+
Settings(config_path=config_path, tool_repository_username="u")
184+
finally:
185+
os.umask(old_umask)
186+
187+
mode = stat.S_IMODE(config_path.parent.stat().st_mode)
188+
self.assertEqual(mode, 0o700, f"expected 0o700, got {oct(mode)}")
189+
190+
@unittest.skipIf(sys.platform == "win32", "POSIX permission semantics")
191+
def test_shared_fallback_dir_is_not_chmodded(self):
192+
"""The system temp dir (a fallback parent) must never be globally chmod'd."""
193+
from crewai_core.settings import _ensure_dir_mode
194+
195+
tmp_root = Path(tempfile.gettempdir())
196+
before = stat.S_IMODE(tmp_root.stat().st_mode)
197+
_ensure_dir_mode(tmp_root)
198+
after = stat.S_IMODE(tmp_root.stat().st_mode)
199+
self.assertEqual(before, after)
200+
201+
202+
if __name__ == "__main__":
203+
unittest.main()

lib/cli/tests/test_token_manager.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
"""Tests for TokenManager with atomic file operations."""
22

33
import json
4+
import os
5+
import stat
6+
import sys
47
import tempfile
58
import unittest
69
from datetime import datetime, timedelta
@@ -285,5 +288,50 @@ def test_delete_secure_file_not_exists(
285288
tm._delete_secure_file("nonexistent.txt")
286289

287290

291+
class TestSecureStoragePathPermissions(unittest.TestCase):
292+
"""Test that the credential directory is created with restrictive permissions."""
293+
294+
@unittest.skipIf(sys.platform == "win32", "POSIX permission semantics")
295+
def test_storage_path_is_owner_only(self) -> None:
296+
"""The credential directory must be mode 0o700 even under a permissive umask."""
297+
with tempfile.TemporaryDirectory() as base:
298+
old_umask = os.umask(0o022)
299+
try:
300+
with (
301+
patch("crewai_core.token_manager.sys.platform", "linux"),
302+
patch(
303+
"crewai_core.token_manager.os.path.expanduser",
304+
return_value=base,
305+
),
306+
):
307+
storage_path = TokenManager._get_secure_storage_path()
308+
finally:
309+
os.umask(old_umask)
310+
311+
self.assertTrue(storage_path.is_dir())
312+
mode = stat.S_IMODE(storage_path.stat().st_mode)
313+
self.assertEqual(mode, 0o700, f"expected 0o700, got {oct(mode)}")
314+
315+
@unittest.skipIf(sys.platform == "win32", "POSIX permission semantics")
316+
def test_existing_loose_dir_is_tightened(self) -> None:
317+
"""A pre-existing world-traversable directory is corrected to 0o700."""
318+
with tempfile.TemporaryDirectory() as base:
319+
loose = Path(base) / "crewai" / "credentials"
320+
loose.mkdir(parents=True)
321+
loose.chmod(0o755)
322+
323+
with (
324+
patch("crewai_core.token_manager.sys.platform", "linux"),
325+
patch(
326+
"crewai_core.token_manager.os.path.expanduser",
327+
return_value=base,
328+
),
329+
):
330+
storage_path = TokenManager._get_secure_storage_path()
331+
332+
mode = stat.S_IMODE(storage_path.stat().st_mode)
333+
self.assertEqual(mode, 0o700, f"expected 0o700, got {oct(mode)}")
334+
335+
288336
if __name__ == "__main__":
289337
unittest.main()

lib/crewai-core/src/crewai_core/settings.py

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import json
66
from logging import getLogger
7+
import os
78
from pathlib import Path
89
import tempfile
910
from typing import Any
@@ -25,6 +26,37 @@
2526
DEFAULT_CONFIG_PATH = Path.home() / ".config" / "crewai" / "settings.json"
2627

2728

29+
def _ensure_dir_mode(directory: Path) -> None:
30+
"""Tighten a dedicated config directory to 0o700.
31+
32+
Skips directories shared with other users or content (the system temp dir
33+
and the current working directory), which are used as best-effort fallbacks
34+
by :func:`get_writable_config_path` and must not be globally chmod'd. Secret
35+
files written there are still protected by their own 0o600 mode.
36+
"""
37+
try:
38+
shared = {Path(tempfile.gettempdir()).resolve(), Path.cwd().resolve()}
39+
if directory.resolve() in shared:
40+
return
41+
directory.chmod(0o700)
42+
except OSError:
43+
pass
44+
45+
46+
def _write_secure_json(path: Path, data: dict[str, Any]) -> None:
47+
"""Atomically write ``data`` as JSON to ``path`` with owner-only (0o600) mode."""
48+
fd, tmp = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.")
49+
try:
50+
with os.fdopen(fd, "w") as f:
51+
json.dump(data, f, indent=4)
52+
os.chmod(tmp, 0o600)
53+
os.replace(tmp, path)
54+
except BaseException:
55+
if os.path.exists(tmp):
56+
os.unlink(tmp)
57+
raise
58+
59+
2860
def get_writable_config_path() -> Path | None:
2961
"""Find a writable location for the config file with fallback options.
3062
@@ -43,6 +75,7 @@ def get_writable_config_path() -> Path | None:
4375
for config_path in fallback_paths:
4476
try:
4577
config_path.parent.mkdir(parents=True, exist_ok=True)
78+
_ensure_dir_mode(config_path.parent)
4679
test_file = config_path.parent / ".crewai_write_test"
4780
try:
4881
test_file.write_text("test")
@@ -153,6 +186,7 @@ def __init__(self, config_path: Path | None = None, **data: dict[str, Any]) -> N
153186

154187
try:
155188
config_path.parent.mkdir(parents=True, exist_ok=True)
189+
_ensure_dir_mode(config_path.parent)
156190
except Exception:
157191
merged_data = {**data}
158192
super().__init__(config_path=Path("/dev/null"), **merged_data)
@@ -194,8 +228,7 @@ def dump(self) -> None:
194228
existing_data = {}
195229

196230
updated_data = {**existing_data, **self.model_dump(exclude_unset=True)}
197-
with self.config_path.open("w") as f:
198-
json.dump(updated_data, f, indent=4)
231+
_write_secure_json(self.config_path, updated_data)
199232

200233
except Exception: # noqa: S110
201234
pass

lib/crewai-core/src/crewai_core/token_manager.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,12 @@ def _get_secure_storage_path() -> Path:
9595
storage_path = Path(base_path) / app_name
9696

9797
storage_path.mkdir(parents=True, exist_ok=True)
98+
# Enforce the documented 0o700 mode: mkdir is subject to umask and does
99+
# not adjust the mode of a pre-existing directory, so chmod explicitly.
100+
try:
101+
storage_path.chmod(0o700)
102+
except OSError:
103+
pass
98104

99105
return storage_path
100106

0 commit comments

Comments
 (0)