Harden built-in file_read directory listing against symlink TOCTOU (CWE-367) #126

New issue

Open

opened 2026-08-09 21:49:57 +00:00 by CoreRasurae · 0 comments

CoreRasurae commented

2026-08-09 21:49:57 +00:00

Member

Copy link

Metadata

Commit Message: feat(agents): symlink-safe directory listing in file_read tool
Branch: feature/mN-symlink-safe-directory-listing (provisional — align the milestone number with the milestone assigned at triage)

Background and context

ADR-2035 D-9 (#105, PR !120) closed a time-of-check/time-of-use race (CWE-367) in FileAccessCore.read_window/write by routing every file open through a new SymlinkSafeOpener, which walks the resolved path one component at a time with O_NOFOLLOW set, so a symlink swapped into any component after path validation but before the open is refused instead of silently followed.

That hardening covers file opens only. ToolAgent._file_read_tool's directory-listing branch (cleveractors.agents.tool) — used when file_read is pointed at a directory instead of a file — enumerates the directory with plain os.listdir/os.path.isdir/os.path.getsize, a call chain that never reaches FileAccessCore and therefore never reaches SymlinkSafeOpener. This gap was flagged during PR !120's review (minor issue #3) and deliberately left out of that PR: it is pre-existing (not introduced by D-9), and closing it means extending the component-wise hardening to a structurally different filesystem primitive (directory enumeration, not open()) — a distinct unit of work from #105's file-open scope.

Current behavior

_file_read_tool performs its own safe-mode / .. / absolute-path / symlink-escape checks on filepath (existing SandboxRootPolicy().is_within(os.path.realpath(filepath)) check).
If os.path.isdir(filepath) is true, it lists the directory directly: os.listdir(filepath), then per entry os.path.isdir(entry_path) and os.path.getsize(entry_path) — all plain, symlink-following calls.
Between the tool's own path check and this enumeration, an attacker who already has write access to a directory the path passes through can swap a path component — the final one or an intermediate parent — for a symlink pointing outside the sandbox root. The subsequent os.listdir/os.path.isdir/os.path.getsize calls re-resolve symlinks in every component and will silently follow the swap, leaking the outside directory's entry names and sizes (not file content) back to the caller.

Expected behavior

Directory listing is confined so that no path component can be a symlink that escapes the sandbox root at enumeration time, closing the same class of race D-9 closed for file opens. Candidate approach: extend SymlinkSafeOpener (or add a sibling helper) to open the target directory itself via a component-wise O_NOFOLLOW/O_DIRECTORY walk, yielding a verified dir_fd, then enumerate via that descriptor (e.g. os.listdir(dir_fd), os.stat(entry, dir_fd=dir_fd)) instead of by path string.

Legitimate in-root directory listings MUST continue to behave exactly as today (same output format, same entries, same reported sizes).

Acceptance criteria

A directory listing whose final path component is a symlink to a target outside the sandbox root fails without listing any entries, even when the symlink is created after the tool's own path checks have validated the path.
A directory listing where an intermediate parent component is swapped to an out-of-root symlink after those checks likewise fails without listing any entries.
Legitimate in-root directory listings continue to succeed with unchanged output (same [FILE_READ_SUCCESS]📁 Directory: ... format, entry names, markers, and reported sizes).
The hardening does not change file_read's normal (non-directory) read behavior.
Coverage remains >= 97% via nox -s coverage_report.

Supporting information

Weakness class: CWE-367 (TOCTOU) — same class as #105/ADR-2035 D-9.
Origin: flagged as minor issue #3 in the code review of PR !120 (which implemented #105); deliberately deferred rather than folded into that PR, and deliberately not documented in ADR-2035 itself (an ADR records the decision it makes and that decision's consequences, not an inventory of every unhardened call site elsewhere in the codebase — this is tracked as its own issue instead).
Introduced/handled in: src/cleveractors/agents/tool.py (ToolAgent._file_read_tool, the os.path.isdir(filepath) branch), src/cleveractors/agents/file_access.py (SymlinkSafeOpener, the collaborator to extend or pair with a sibling).
Related: docs/adr/ADR-2035-inline-code-sandboxed-file-access.md (D-9) for the file-open hardening this issue extends the same technique to.
If the enumeration strategy changes any normative filesystem-boundary semantics of docs/index.md §13.3/§4.5.4, an ADR + a spec revision (§21 of docs/index.md) is required before implementation; the current expectation is that it will not, since — like D-9 — this only hardens enforcement of an already-existing boundary, not the set of paths admitted or rejected.

Subtasks

Decide the symlink-safe directory-enumeration strategy (extend SymlinkSafeOpener vs. a sibling helper), documenting it as an ADR-2035 revision if the mechanism is architecturally significant.
Implement the symlink-safe directory listing in ToolAgent._file_read_tool, replacing the plain os.listdir/os.path.isdir/os.path.getsize calls on the directory branch.
Tests (Behave): post-validation symlink swap on a directory listing's final and intermediate components is refused; legitimate directory listing still succeeds with unchanged output.
Tests (Robot): integration exercising a real concurrent symlink-swap attempt against directory listing is refused end to end.
Update CHANGELOG with a user-facing entry.
Verify coverage >= 97% via nox -s coverage_report.
Run nox (all default sessions), fix any errors.

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line matches the Commit Message in Metadata exactly.
The commit is pushed to the branch matching the Branch in Metadata exactly.
The commit is submitted as a PR to master, reviewed, and merged.

## Metadata - **Commit Message:** `feat(agents): symlink-safe directory listing in file_read tool` - **Branch:** `feature/mN-symlink-safe-directory-listing` (provisional — align the milestone number with the milestone assigned at triage) ## Background and context ADR-2035 D-9 (#105, PR !120) closed a time-of-check/time-of-use race (CWE-367) in `FileAccessCore.read_window`/`write` by routing every file open through a new `SymlinkSafeOpener`, which walks the resolved path one component at a time with `O_NOFOLLOW` set, so a symlink swapped into any component after path validation but before the open is refused instead of silently followed. That hardening covers file opens only. `ToolAgent._file_read_tool`'s directory-listing branch (`cleveractors.agents.tool`) — used when `file_read` is pointed at a directory instead of a file — enumerates the directory with plain `os.listdir`/`os.path.isdir`/`os.path.getsize`, a call chain that never reaches `FileAccessCore` and therefore never reaches `SymlinkSafeOpener`. This gap was flagged during PR !120's review (minor issue #3) and deliberately left out of that PR: it is pre-existing (not introduced by D-9), and closing it means extending the component-wise hardening to a structurally different filesystem primitive (directory enumeration, not `open()`) — a distinct unit of work from #105's file-open scope. ## Current behavior - `_file_read_tool` performs its own safe-mode / `..` / absolute-path / symlink-escape checks on `filepath` (existing `SandboxRootPolicy().is_within(os.path.realpath(filepath))` check). - If `os.path.isdir(filepath)` is true, it lists the directory directly: `os.listdir(filepath)`, then per entry `os.path.isdir(entry_path)` and `os.path.getsize(entry_path)` — all plain, symlink-following calls. - Between the tool's own path check and this enumeration, an attacker who already has write access to a directory the path passes through can swap a path component — the final one or an intermediate parent — for a symlink pointing outside the sandbox root. The subsequent `os.listdir`/`os.path.isdir`/`os.path.getsize` calls re-resolve symlinks in every component and will silently follow the swap, leaking the outside directory's entry names and sizes (not file content) back to the caller. ## Expected behavior Directory listing is confined so that no path component can be a symlink that escapes the sandbox root **at enumeration time**, closing the same class of race D-9 closed for file opens. Candidate approach: extend `SymlinkSafeOpener` (or add a sibling helper) to open the target directory itself via a component-wise `O_NOFOLLOW`/`O_DIRECTORY` walk, yielding a verified `dir_fd`, then enumerate via that descriptor (e.g. `os.listdir(dir_fd)`, `os.stat(entry, dir_fd=dir_fd)`) instead of by path string. Legitimate in-root directory listings MUST continue to behave exactly as today (same output format, same entries, same reported sizes). ## Acceptance criteria - [ ] A directory listing whose **final** path component is a symlink to a target outside the sandbox root fails without listing any entries, even when the symlink is created after the tool's own path checks have validated the path. - [ ] A directory listing where an **intermediate** parent component is swapped to an out-of-root symlink after those checks likewise fails without listing any entries. - [ ] Legitimate in-root directory listings continue to succeed with unchanged output (same `[FILE_READ_SUCCESS]📁 Directory: ...` format, entry names, markers, and reported sizes). - [ ] The hardening does not change `file_read`'s normal (non-directory) read behavior. - [ ] Coverage remains >= 97% via `nox -s coverage_report`. ## Supporting information - Weakness class: CWE-367 (TOCTOU) — same class as #105/ADR-2035 D-9. - Origin: flagged as minor issue #3 in the code review of PR !120 (which implemented #105); deliberately deferred rather than folded into that PR, and deliberately not documented in ADR-2035 itself (an ADR records the decision it makes and that decision's consequences, not an inventory of every unhardened call site elsewhere in the codebase — this is tracked as its own issue instead). - Introduced/handled in: `src/cleveractors/agents/tool.py` (`ToolAgent._file_read_tool`, the `os.path.isdir(filepath)` branch), `src/cleveractors/agents/file_access.py` (`SymlinkSafeOpener`, the collaborator to extend or pair with a sibling). - Related: `docs/adr/ADR-2035-inline-code-sandboxed-file-access.md` (D-9) for the file-open hardening this issue extends the same technique to. - If the enumeration strategy changes any normative filesystem-boundary semantics of `docs/index.md` §13.3/§4.5.4, an ADR + a spec revision (§21 of `docs/index.md`) is required before implementation; the current expectation is that it will not, since — like D-9 — this only hardens *enforcement* of an already-existing boundary, not the set of paths admitted or rejected. ## Subtasks - [ ] Decide the symlink-safe directory-enumeration strategy (extend `SymlinkSafeOpener` vs. a sibling helper), documenting it as an ADR-2035 revision if the mechanism is architecturally significant. - [ ] Implement the symlink-safe directory listing in `ToolAgent._file_read_tool`, replacing the plain `os.listdir`/`os.path.isdir`/`os.path.getsize` calls on the directory branch. - [ ] Tests (Behave): post-validation symlink swap on a directory listing's final and intermediate components is refused; legitimate directory listing still succeeds with unchanged output. - [ ] Tests (Robot): integration exercising a real concurrent symlink-swap attempt against directory listing is refused end to end. - [ ] Update CHANGELOG with a user-facing entry. - [ ] Verify coverage >= 97% via `nox -s coverage_report`. - [ ] Run `nox` (all default sessions), fix any errors. ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the first line matches the Commit Message in Metadata exactly. - The commit is pushed to the branch matching the Branch in Metadata exactly. - The commit is submitted as a PR to `master`, reviewed, and merged.

CoreRasurae added the

labels

2026-08-09 21:49:57 +00:00

CoreRasurae added a new dependency

2026-08-09 21:50:15 +00:00

#77 Epic: LLM Agent Runtime Stabilization — reliability, resource enforcement & correctness hardening