cleveragents/cleveragents-core

Fork 3

UAT: `InlineToolExecutor._validate_paths()` uses heuristic key-name matching — sandbox path restriction bypassed via non-standard input key names #4120

New issue

Open

opened 2026-04-06 10:28:38 +00:00 by freemo · 1 comment

freemo commented

2026-04-06 10:28:38 +00:00

Owner

Metadata

Branch: fix/security-inline-executor-path-validation
Commit Message: fix(security): strengthen _validate_paths() to check all string values, not just heuristic key names
Milestone: (none — backlog)
Parent Epic: #362

Bug Report

What Was Tested

Path containment security in the InlineToolExecutor._validate_paths() method.

Expected Behavior (from spec)

Per docs/specification.md §Security Model — Sandbox Isolation:

The sandbox path is derived deterministically from the plan ULID, preventing path traversal.

The InlineToolExecutor docstring states:

Path restriction: Any file-path values in input_data are validated to reside within the sandbox root returned by SkillContext.get_sandbox_path().

The spec requires that all file path values in input_data are validated against the sandbox root, not just those with specific key names.

Actual Behavior

_validate_paths() in src/cleveragents/skills/inline_executor.py uses a heuristic key-name check to decide which values to validate:

def _validate_paths(
    self,
    input_data: dict[str, Any],
    sandbox_path: Path,
) -> str | None:
    for key, value in input_data.items():
        if not isinstance(value, str):
            continue
        # Heuristic: treat values that look like file paths
        if key.endswith("_path") or key == "path" or key.endswith("_file"):
            try:
                resolved = Path(value).resolve()
                sandbox_resolved = sandbox_path.resolve()
                if not str(resolved).startswith(str(sandbox_resolved)):  # ← prefix-collision bug too
                    return (
                        f"Path '{value}' for key '{key}' escapes sandbox "
                        f"root '{sandbox_path}'"
                    )
            except (OSError, ValueError):
                return f"Invalid path '{value}' for key '{key}'"
    return None

This check only validates keys that:

End with _path (e.g., file_path, source_path, dest_path)
Are exactly path
End with _file (e.g., input_file, output_file)

Attack Scenario

An inline tool that accepts file paths under any other key name completely bypasses sandbox validation:

# These key names BYPASS path validation:
input_data = {
    "filename": "/etc/passwd",           # ← NOT checked (doesn't end with _path/_file)
    "source": "/etc/shadow",             # ← NOT checked
    "destination": "/tmp/evil",          # ← NOT checked
    "directory": "/etc",                 # ← NOT checked
    "dir": "/etc",                       # ← NOT checked
    "location": "/etc/passwd",           # ← NOT checked
    "target": "/etc/passwd",             # ← NOT checked
    "output": "/etc/passwd",             # ← NOT checked
    "input": "/etc/passwd",              # ← NOT checked
    "src": "/etc/passwd",                # ← NOT checked
    "dst": "/etc/passwd",                # ← NOT checked
    "filepath": "/etc/passwd",           # ← NOT checked (no underscore)
}

# Only these key names ARE checked:
input_data = {
    "path": "/etc/passwd",               # ← checked
    "file_path": "/etc/passwd",          # ← checked
    "source_path": "/etc/passwd",        # ← checked
    "input_file": "/etc/passwd",         # ← checked
}

An inline tool author (or a compromised skill YAML) can trivially bypass the sandbox restriction by using a non-standard key name for file path arguments.

Secondary Issue: Prefix-Collision Bug

The same function also contains the prefix-collision bypass bug (already reported in #3960):

if not str(resolved).startswith(str(sandbox_resolved)):  # ← BUG: no os.sep suffix

This is the same bug as in file_tools.py and file_ops.py.

Code Location

src/cleveragents/skills/inline_executor.py — _validate_paths() method

Fix Required

Option 1 — Validate ALL string values that look like absolute paths:

def _validate_paths(
    self,
    input_data: dict[str, Any],
    sandbox_path: Path,
) -> str | None:
    sandbox_resolved = sandbox_path.resolve()
    for key, value in input_data.items():
        if not isinstance(value, str):
            continue
        # Check ALL string values that look like absolute paths
        # (not just heuristic key names)
        if value.startswith("/") or value.startswith("\\"):
            try:
                resolved = Path(value).resolve()
                # Use is_relative_to() for correct path containment check
                if not resolved.is_relative_to(sandbox_resolved):
                    return (
                        f"Path '{value}' for key '{key}' escapes sandbox "
                        f"root '{sandbox_path}'"
                    )
            except (OSError, ValueError):
                return f"Invalid path '{value}' for key '{key}'"
    return None

Option 2 — Expand the heuristic to cover more key names:

_PATH_KEY_PATTERNS = (
    "_path", "_file", "_dir", "_directory", "_location",
    "path", "file", "dir", "directory", "location",
    "source", "destination", "target", "output", "input",
    "src", "dst", "filename", "filepath",
)

if any(key == p or key.endswith(f"_{p}") or key.startswith(f"{p}_")
       for p in _PATH_KEY_PATTERNS):
    # validate...

The recommended fix is Option 1 — checking all absolute path strings is more robust than maintaining an ever-growing list of key name patterns.

Also fix the prefix-collision bug in the same function (replace startswith(str(root)) with is_relative_to(root) or startswith(str(root) + os.sep)).

Impact

Confidentiality: An inline tool can read files outside the sandbox by using a non-standard key name for file path arguments.
Integrity: An inline tool can write files outside the sandbox.
OWASP Category: A01:2021 — Broken Access Control
Severity: High — breaks the core sandbox isolation guarantee for inline tools

Subtasks

Replace heuristic key-name matching with absolute-path detection in _validate_paths()
Also fix the prefix-collision bug in _validate_paths() (use is_relative_to() instead of startswith())
Add BDD scenario verifying that paths under non-standard key names are validated
Verify nox -e unit_tests passes

Definition of Done

_validate_paths() validates all absolute path strings, not just heuristic key names
_validate_paths() uses is_relative_to() for correct path containment check
A BDD scenario exists verifying the bypass is blocked
nox -e unit_tests and nox -e typecheck pass
All nox stages pass
Coverage >= 97%

Backlog note: This issue was discovered during autonomous UAT security audit.
It does not block milestone completion and has been placed in the backlog
for human review and future milestone assignment.

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-uat-tester

## Metadata - **Branch**: `fix/security-inline-executor-path-validation` - **Commit Message**: `fix(security): strengthen _validate_paths() to check all string values, not just heuristic key names` - **Milestone**: _(none — backlog)_ - **Parent Epic**: #362 ## Bug Report ### What Was Tested Path containment security in the `InlineToolExecutor._validate_paths()` method. ### Expected Behavior (from spec) Per `docs/specification.md` §Security Model — Sandbox Isolation: > The sandbox path is derived deterministically from the plan ULID, **preventing path traversal**. The `InlineToolExecutor` docstring states: > **Path restriction**: Any file-path values in *input_data* are validated to reside within the sandbox root returned by `SkillContext.get_sandbox_path()`. The spec requires that **all** file path values in `input_data` are validated against the sandbox root, not just those with specific key names. ### Actual Behavior `_validate_paths()` in `src/cleveragents/skills/inline_executor.py` uses a **heuristic key-name check** to decide which values to validate: ```python def _validate_paths( self, input_data: dict[str, Any], sandbox_path: Path, ) -> str | None: for key, value in input_data.items(): if not isinstance(value, str): continue # Heuristic: treat values that look like file paths if key.endswith("_path") or key == "path" or key.endswith("_file"): try: resolved = Path(value).resolve() sandbox_resolved = sandbox_path.resolve() if not str(resolved).startswith(str(sandbox_resolved)): # ← prefix-collision bug too return ( f"Path '{value}' for key '{key}' escapes sandbox " f"root '{sandbox_path}'" ) except (OSError, ValueError): return f"Invalid path '{value}' for key '{key}'" return None ``` This check **only validates** keys that: - End with `_path` (e.g., `file_path`, `source_path`, `dest_path`) - Are exactly `path` - End with `_file` (e.g., `input_file`, `output_file`) ### Attack Scenario An inline tool that accepts file paths under any other key name completely bypasses sandbox validation: ```python # These key names BYPASS path validation: input_data = { "filename": "/etc/passwd", # ← NOT checked (doesn't end with _path/_file) "source": "/etc/shadow", # ← NOT checked "destination": "/tmp/evil", # ← NOT checked "directory": "/etc", # ← NOT checked "dir": "/etc", # ← NOT checked "location": "/etc/passwd", # ← NOT checked "target": "/etc/passwd", # ← NOT checked "output": "/etc/passwd", # ← NOT checked "input": "/etc/passwd", # ← NOT checked "src": "/etc/passwd", # ← NOT checked "dst": "/etc/passwd", # ← NOT checked "filepath": "/etc/passwd", # ← NOT checked (no underscore) } # Only these key names ARE checked: input_data = { "path": "/etc/passwd", # ← checked "file_path": "/etc/passwd", # ← checked "source_path": "/etc/passwd", # ← checked "input_file": "/etc/passwd", # ← checked } ``` An inline tool author (or a compromised skill YAML) can trivially bypass the sandbox restriction by using a non-standard key name for file path arguments. ### Secondary Issue: Prefix-Collision Bug The same function also contains the prefix-collision bypass bug (already reported in #3960): ```python if not str(resolved).startswith(str(sandbox_resolved)): # ← BUG: no os.sep suffix ``` This is the same bug as in `file_tools.py` and `file_ops.py`. ### Code Location - `src/cleveragents/skills/inline_executor.py` — `_validate_paths()` method ### Fix Required **Option 1 — Validate ALL string values that look like absolute paths**: ```python def _validate_paths( self, input_data: dict[str, Any], sandbox_path: Path, ) -> str | None: sandbox_resolved = sandbox_path.resolve() for key, value in input_data.items(): if not isinstance(value, str): continue # Check ALL string values that look like absolute paths # (not just heuristic key names) if value.startswith("/") or value.startswith("\\"): try: resolved = Path(value).resolve() # Use is_relative_to() for correct path containment check if not resolved.is_relative_to(sandbox_resolved): return ( f"Path '{value}' for key '{key}' escapes sandbox " f"root '{sandbox_path}'" ) except (OSError, ValueError): return f"Invalid path '{value}' for key '{key}'" return None ``` **Option 2 — Expand the heuristic to cover more key names**: ```python _PATH_KEY_PATTERNS = ( "_path", "_file", "_dir", "_directory", "_location", "path", "file", "dir", "directory", "location", "source", "destination", "target", "output", "input", "src", "dst", "filename", "filepath", ) if any(key == p or key.endswith(f"_{p}") or key.startswith(f"{p}_") for p in _PATH_KEY_PATTERNS): # validate... ``` The recommended fix is Option 1 — checking all absolute path strings is more robust than maintaining an ever-growing list of key name patterns. Also fix the prefix-collision bug in the same function (replace `startswith(str(root))` with `is_relative_to(root)` or `startswith(str(root) + os.sep)`). ### Impact - **Confidentiality**: An inline tool can read files outside the sandbox by using a non-standard key name for file path arguments. - **Integrity**: An inline tool can write files outside the sandbox. - **OWASP Category**: A01:2021 — Broken Access Control - **Severity**: High — breaks the core sandbox isolation guarantee for inline tools ## Subtasks - [ ] Replace heuristic key-name matching with absolute-path detection in `_validate_paths()` - [ ] Also fix the prefix-collision bug in `_validate_paths()` (use `is_relative_to()` instead of `startswith()`) - [ ] Add BDD scenario verifying that paths under non-standard key names are validated - [ ] Verify `nox -e unit_tests` passes ## Definition of Done - [ ] `_validate_paths()` validates all absolute path strings, not just heuristic key names - [ ] `_validate_paths()` uses `is_relative_to()` for correct path containment check - [ ] A BDD scenario exists verifying the bypass is blocked - [ ] `nox -e unit_tests` and `nox -e typecheck` pass - All nox stages pass - Coverage >= 97% > **Backlog note:** This issue was discovered during autonomous UAT security audit. > It does not block milestone completion and has been placed in the backlog > for human review and future milestone assignment. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-uat-tester

freemo referenced this issue

2026-04-06 10:29:10 +00:00

[Automated] CleverAgents Build Session - 2026-04-06 #3775

freemo referenced this issue

2026-04-06 10:35:53 +00:00

[Automated] CleverAgents Build Session - 2026-04-06 #3775

freemo added this to the v3.3.0 milestone

2026-04-06 18:06:36 +00:00

freemo commented

2026-04-06 18:50:11 +00:00

Author

Owner

Milestone Triage Decision: Moved to Backlog

This security logging issue has been moved out of v3.3.0 during aggressive milestone triage. While important for security, it does not relate to the core focus of Corrections + Subplans + Checkpoints.

Reasoning:

v3.3.0 focus: Essential corrections, subplan management, and checkpoint functionality
This issue: Security logging enhancement - important but not milestone-blocking
Impact: Security observability improvement, not core corrections/subplans/checkpoints functionality

Will be addressed in a future milestone focused on security hardening and observability.

**Milestone Triage Decision: Moved to Backlog** This security logging issue has been moved out of v3.3.0 during aggressive milestone triage. While important for security, it does not relate to the core focus of Corrections + Subplans + Checkpoints. **Reasoning:** - v3.3.0 focus: Essential corrections, subplan management, and checkpoint functionality - This issue: Security logging enhancement - important but not milestone-blocking - Impact: Security observability improvement, not core corrections/subplans/checkpoints functionality Will be addressed in a future milestone focused on security hardening and observability.

freemo removed this from the v3.3.0 milestone

2026-04-06 20:42:50 +00:00