mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-07-02 05:38:12 +00:00
fix: never default the path allow-list to the filesystem root
_get_allowed_roots defaulted its primary root to os.getcwd(). In a container started without a WORKDIR, cwd is "/", and since "/" is a parent of every absolute path the deny-by-default allow-list then permitted the entire filesystem -- silently disabling confinement and re-opening arbitrary LLM-controlled file read/write (the exact hole this PR closes). Distinguish an implicitly defaulted primary root (base_dir is None -> os.getcwd()) from operator-provided roots (base_dir, allowed_dirs, CREWAI_TOOLS_ALLOWED_DIRS). When the implicit cwd default resolves to os.sep it is dropped; an explicit "/" is still honored as a deliberate opt-in. If no usable root remains, raise a clear ValueError instead of allowing everything. Addresses the corridor-security review finding on #6248. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -40,23 +40,50 @@ def _get_allowed_roots(
|
||||
is compared by its real location. Empty entries are ignored and duplicates
|
||||
are collapsed while preserving order. The first element is always the
|
||||
primary root used to resolve relative candidate paths.
|
||||
|
||||
The filesystem root (``os.sep``, e.g. ``"/"``) is never accepted as an
|
||||
*implicitly defaulted* root. When ``base_dir`` is not supplied and the
|
||||
current working directory is ``/`` -- common in containers started without
|
||||
a ``WORKDIR`` -- defaulting to it would make every absolute path "within"
|
||||
the allow-list and disable confinement entirely. In that case the cwd
|
||||
default is dropped; an operator who genuinely wants the whole filesystem
|
||||
must opt in explicitly via ``base_dir``, ``allowed_dirs``, or
|
||||
``CREWAI_TOOLS_ALLOWED_DIRS``. If no usable root remains, a ``ValueError``
|
||||
is raised rather than silently allowing everything.
|
||||
"""
|
||||
raw_roots: list[str] = [base_dir if base_dir is not None else os.getcwd()]
|
||||
primary_explicit = base_dir is not None
|
||||
primary = base_dir if base_dir is not None else os.getcwd()
|
||||
|
||||
# (root, is_explicit) -- explicit roots are operator-provided and may
|
||||
# legitimately include the filesystem root as an opt-in.
|
||||
raw_roots: list[tuple[str, bool]] = [(primary, primary_explicit)]
|
||||
|
||||
env_dirs = os.environ.get(_ALLOWED_DIRS_ENV, "")
|
||||
if env_dirs:
|
||||
raw_roots.extend(d for d in env_dirs.split(os.pathsep) if d)
|
||||
raw_roots.extend((d, True) for d in env_dirs.split(os.pathsep) if d)
|
||||
|
||||
if allowed_dirs:
|
||||
raw_roots.extend(d for d in allowed_dirs if d)
|
||||
raw_roots.extend((d, True) for d in allowed_dirs if d)
|
||||
|
||||
resolved: list[str] = []
|
||||
seen: set[str] = set()
|
||||
for root in raw_roots:
|
||||
for root, is_explicit in raw_roots:
|
||||
real = os.path.realpath(root)
|
||||
if real == os.sep and not is_explicit:
|
||||
# Refuse to let an unconfigured cwd of "/" open the whole filesystem.
|
||||
continue
|
||||
if real not in seen:
|
||||
seen.add(real)
|
||||
resolved.append(real)
|
||||
|
||||
if not resolved:
|
||||
raise ValueError(
|
||||
"No safe allowed directory could be determined: the current working "
|
||||
f"directory is the filesystem root ('{os.sep}'). Set "
|
||||
f"{_ALLOWED_DIRS_ENV} to an explicit directory, pass "
|
||||
f"base_dir/allowed_dirs, or set {_UNSAFE_PATHS_ENV}=true to bypass "
|
||||
"path validation."
|
||||
)
|
||||
return resolved
|
||||
|
||||
|
||||
|
||||
@@ -250,3 +250,36 @@ class TestAllowList:
|
||||
assert validate_file_path(str(b / "fb.txt"), base_dir=str(base)) == str(
|
||||
b / "fb.txt"
|
||||
)
|
||||
|
||||
def test_cwd_root_default_is_not_an_allowed_root(self, tmp_path, monkeypatch):
|
||||
"""An unconfigured cwd of '/' must not open the whole filesystem.
|
||||
|
||||
Regression for the deny-by-default allow-list silently defaulting to the
|
||||
filesystem root in containers started without a WORKDIR.
|
||||
"""
|
||||
monkeypatch.delenv("CREWAI_TOOLS_ALLOWED_DIRS", raising=False)
|
||||
monkeypatch.delenv("CREWAI_TOOLS_ALLOW_UNSAFE_PATHS", raising=False)
|
||||
monkeypatch.setattr(os, "getcwd", lambda: os.sep)
|
||||
with pytest.raises(ValueError, match="filesystem root"):
|
||||
validate_file_path("/etc/passwd")
|
||||
|
||||
def test_cwd_root_with_explicit_allowed_dirs_confines(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
"""With cwd '/', confinement falls back to the explicit allow-list."""
|
||||
monkeypatch.delenv("CREWAI_TOOLS_ALLOWED_DIRS", raising=False)
|
||||
monkeypatch.setattr(os, "getcwd", lambda: os.sep)
|
||||
(tmp_path / "data.txt").touch()
|
||||
assert validate_file_path(
|
||||
str(tmp_path / "data.txt"), allowed_dirs=[str(tmp_path)]
|
||||
) == str(tmp_path / "data.txt")
|
||||
with pytest.raises(ValueError, match="outside the allowed directories"):
|
||||
validate_file_path("/etc/passwd", allowed_dirs=[str(tmp_path)])
|
||||
|
||||
def test_explicit_base_dir_root_is_opt_in(self, monkeypatch):
|
||||
"""An explicit base_dir of '/' is honored as a deliberate opt-in."""
|
||||
monkeypatch.delenv("CREWAI_TOOLS_ALLOWED_DIRS", raising=False)
|
||||
monkeypatch.delenv("CREWAI_TOOLS_ALLOW_UNSAFE_PATHS", raising=False)
|
||||
assert validate_file_path("/etc/passwd", base_dir=os.sep) == os.path.realpath(
|
||||
"/etc/passwd"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user