fix(file): harden read_file device alias blocking - #34466
Conversation
egilewski
changed the title
bc443b7 to
55ed231
Compare
|
Verified: the
Symlink hop walk with cycle protection: the Final One minor note: the hop walk calls |
alt-glitch
added
type/security
|
My Codex neglected to mention that the initial commit was found to be conflicting after #10133 was merged, and after rebasing reduced in scope to amend what was left behind and close the issues. |
55ed231 to
130b769
Compare
Security-hardening fix for the read_file device guard, not a new sandbox
boundary. The guard already rejects direct device paths and upstream now
has a resolved-path pass for workspace symlinks to blocked devices, but
its concrete-path helper still compared the expanded path before
normalization. That leaves residual alias cases where the dangerous path
is visible before final terminal-specific resolution, for example:
1. /dev/../dev/zero and /dev/./urandom should match the blocked-device
list as concrete paths, not only after final realpath;
2. /dev/stdin-style aliases can disappear once realpath follows them
to /proc/self/fd/0 and then to a tty path;
3. a user symlink to /dev/../dev/stdin exposes the dangerous
intermediate target before final resolution, but not necessarily
after it.
Normalize expanded paths before matching and inspect each symlink hop
before falling back to realpath. This preserves the existing /proc fd and
/proc pseudo-file guards while enforcing the intended security invariant:
model-supplied read paths must not reach blocking or infinite device
streams through spelling, normalization, or symlink-hop tricks.
Classification: security hardening / residual bypass fix for the
read_file device blocklist. This is defensive code at the file-tool
boundary, but it fixes a concrete denial-of-service class tracked as
security in NousResearch#10141 and NousResearch#29158.
Tests:
- normalized /dev/../dev/zero and /dev/./urandom aliases
- symlink to /dev/../dev/stdin blocked before realpath
- existing symlink-to-device and regular-symlink guards still pass
Fixes NousResearch#10141
Fixes NousResearch#29158
The read_file device guard now walks symlink hops before the file operation layer, but that hop walk still interpreted relative paths against the Python process cwd. In sessions where TERMINAL_CWD points at the task workspace, a relative workspace symlink to a blocked alias such as /dev/../dev/stdin could therefore miss the intermediate device target before later task-cwd resolution. Anchor relative device checks to the task base before symlink-hop inspection so the pre-I/O guard sees the same workspace path that read_file would otherwise read. Absolute device paths and the existing final realpath fallback remain unchanged. Refs NousResearch#10141 Refs NousResearch#29158
130b769 to
acee75a
Compare
|
Rebased and force-pushed the original branch onto current The branch now keeps the original hardening commit as its own signed commit and adds the
The follow-up fixes the remaining case where a relative workspace symlink to Validation on the local branch before push: focused device-guard pytest passed, the mocked bypass probe no longer reached the read sink, Signed: GPT-5.5 in Codex |
|
Merged via PR #50221. Your commit was cherry-picked onto current main with your authorship preserved in git log (commit 9078b4b, author: Eugeniusz Gilewski). The per-symlink-hop check closes the genuine residual bypass (a symlink through /dev/stdin that realpath would resolve past to a tty) plus the normpath path-alias class. Thanks for the hardening work! |
4 participants
Summary
realpath/devaliases and a symlink to a blocked aliasClassification
This is a security-hardening residual bypass fix for the
read_filedevice blocklist, not a new sandbox boundary.Upstream already has a resolved-path pass for ordinary workspace symlinks to blocked devices. This PR covers the remaining alias class: paths or symlink targets that are dangerous before final terminal-specific resolution, such as
/dev/../dev/stdin.Why
read_fileis supposed to reject blocking or infinite device streams before file I/O. Comparing only the expanded spelling lets model-supplied aliases depend on finalrealpathbehavior:/dev/../dev/zeroand/dev/./urandomshould match the blocklist as concrete path aliases/dev/stdin-style aliases can disappear afterrealpathfollows them through/proc/self/fd/0to a tty path/dev/../dev/stdin, exposing the blocked target before final resolutionThe fix normalizes paths inside the concrete helper and checks each symlink hop, while preserving the existing
/procfd and pseudo-file protections.Tests
/home/mac/hermes-agent/.venv/bin/python -m pytest tests/tools/test_file_read_guards.py -q-> 41 passed/home/mac/hermes-agent/.venv/bin/python -m pytest tests/tools/test_file_tools.py tests/tools/test_file_tools_live.py tests/tools/test_file_operations.py tests/tools/test_file_operations_edge_cases.py tests/agent/test_file_safety_credentials.py -q-> 214 passed/home/mac/hermes-agent/.venv/bin/python -m py_compile tools/file_tools.py tests/tools/test_file_read_guards.pygit diff --check upstream/main..HEADFixes #10141
Fixes #29158