fix(security): extend /proc read block to maps siblings + auxv + pagemap - #226
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
PR adds /proc pseudo-file read guards and search result filtering but introduces three issues: an incomplete tenki backend removal that breaks file operations, a /proc/*/stat omission in the ASLR blocklist, and a search result filter that doesn't apply the /proc device blocklist. Files Reviewed (2 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (72/100) — 2 high findings, 1 medium · 126 LOC across 2 files
High Severity
-
Incomplete tenki removal (
tools/file_tools.py:987): The PR removes tenki backend support fromfile_tools.pyonly, whileterminal_tool.pyandcode_execution_tool.pystill carry full tenki support (image branches,container_configkeys,_CONTAINER_BACKENDSmembership). Any tenki user triggering file operations will getimage=""andcontainer_config=None, causingTenkiEnvironmentto fail at connection time. The five other callers are unaffected because their code paths are unchanged. -
ASLR bypass via
/proc/*/stat(tools/file_tools.py:374-386): The blocklist expansion coversmaps,smaps,smaps_rollup,numa_maps,mem,auxv, andpagemapbut omits/proc/<pid>/stat, which exposesstart_code,end_code, andstart_stack(fields 26-28) perproc(5)— the same ASLR-sensitive addresses asmaps. An attacker/model blocked from reading/mapscan read/statinstead.
Medium Severity
- search_tool
/procfilter asymmetry (tools/file_tools.py:446-477):read_file_toolapplies dual guards (_is_blocked_device+get_read_block_error), but the new search filter applies onlyget_read_block_error./procpseudo-file path metadata leaks through search results even thoughread_file_toolblocks those same paths. Content is redacted but file path metadata (including line numbers) persists in results.
Impact
This is a security-hardening PR with three gaps: one functional regression (tenki), one genuine bypass (stat omission), and one defense-in-depth inconsistency (search asymmetry). The tenki issue will break existing tenki-backed deployments; the other two reduce the effectiveness of the intended hardening.
|
|
||
| container_config = None | ||
| if env_type in _CONTAINER_BACKENDS: | ||
| if env_type in {"docker", "singularity", "modal", "daytona"}: |
There was a problem hiding this comment.
🟠 Incomplete tenki removal: file_tools.py strips tenki config while terminal_tool.py and code_execution_tool.py still expect it (bug)
The PR removes tenki backend support from file_tools.py only: it deletes the elif env_type == "tenki": image-resolution branch, changes line 987 from if env_type in _CONTAINER_BACKENDS to a hardcoded set {"docker", "singularity", "modal", "daytona"} that excludes tenki, and removes all tenki_* keys from container_config. However, two other files in the same codebase still carry full tenki support: terminal_tool.py (line 1218: _CONTAINER_BACKENDS includes "tenki"; lines 1522-1544: _create_environment() reads tenki_api_endpoint, tenki_workspace_id, tenki_project_id, etc. from container_config and passes them to TenkiEnvironment) and code_execution_tool.py (lines 655-681: has the env_type == "tenki" image branch, puts tenki in the container set, and includes all tenki_* keys in container_config). As a result, any user with env_type = "tenki" who triggers file operations (read_file, write_file, search_files — via any of the 6 call sites of _get_file_ops at lines 1080, 1198, 1517, 1538, 1671, 1809) will get image="" and container_config=None, causing TenkiEnvironment to be constructed with blank credentials that will fail at connection time. Terminal operations and code execution remain unaffected because their respective code paths are unchanged.
💡 Suggestion: The tenki removal must be applied consistently across all three files. Either (a) restore tenki support in file_tools.py to match terminal_tool.py and code_execution_tool.py, including the image branch, container_config keys, and the _CONTAINER_BACKENDS guard; or (b) complete the tenki removal by also removing tenki from terminal_tool.py _CONTAINER_BACKENDS (line 1218), the TenkiEnvironment creation block (lines 1522-1544), and code_execution_tool.py (lines 655-656, 663, 671-680). The current partial removal leaves the code in an inconsistent state.
📋 Prompt for AI Agents
To complete the tenki removal consistently: (1) In tools/terminal_tool.py line 1218, remove "tenki" from the CONTAINER_BACKENDS frozenset. (2) In tools/terminal_tool.py, remove lines 1522-1544 (the elif env_type == "tenki": block that creates TenkiEnvironment). (3) In tools/code_execution_tool.py line 655-656, remove the elif env_type == "tenki": image branch. (4) In tools/code_execution_tool.py line 663, remove "tenki" from the hardcoded set. (5) In tools/code_execution_tool.py lines 671-680, remove the tenki* config keys from container_config. (6) In tools/file_tools.py line 987, restore _CONTAINER_BACKENDS instead of the hardcoded set (so it stays consistent after tenki is removed from the frozenset).
| if normalized.startswith("/proc/") and normalized.endswith( | ||
| ("/environ", "/cmdline", "/maps") | ||
| ( | ||
| "/environ", | ||
| "/cmdline", | ||
| "/maps", | ||
| "/smaps", | ||
| "/smaps_rollup", | ||
| "/numa_maps", | ||
| "/mem", | ||
| "/auxv", | ||
| "/pagemap", | ||
| ) | ||
| ): |
There was a problem hiding this comment.
🟠 /proc/*/stat missing from blocklist enables ASLR oracle bypass of maps/smaps/auxv blocks (security)
The _is_blocked_device_path function blocks /proc/*/maps, /smaps, /smaps_rollup, /numa_maps, /mem, /auxv, and /pagemap to prevent ASLR bypass and memory disclosure. However, /proc/<pid>/stat is NOT blocked, despite exposing start_code (text-segment base), end_code (text-segment end), and start_stack (stack base) — fields 26–28 per the Linux proc(5) man page. These are the same ASLR-sensitive addresses that maps would reveal. An attacker or model that cannot read /proc/self/maps can read /proc/self/stat and extract equivalent address information, defeating the block. The endswith-based check would naturally cover both /proc/<pid>/stat and /proc/<pid>/task/<tid>/stat if /stat were added to the blocked suffix tuple.
💡 Suggestion: Add /stat to the blocked suffix tuple in _is_blocked_device_path so the ASLR oracle is blocked alongside maps/smaps/auxv/pagemap. The endswith check already covers /proc/<pid>/stat and /proc/<pid>/task/<tid>/stat automatically.
📋 Prompt for AI Agents
In tools/file_tools.py, in function _is_blocked_device_path around lines 374-386, add /stat to the endswith tuple alongside the existing blocked /proc suffixes. The tuple currently contains ('/environ', '/cmdline', '/maps', '/smaps', '/smaps_rollup', '/numa_maps', '/mem', '/auxv', '/pagemap'). Insert '/stat' into the tuple. Also update the test fixtures in tests/tools/test_file_read_guards.py to add test cases for /proc/self/stat, /proc/12345/stat, and /proc/self/task/1234/stat in the existing test_proc_sensitive_pseudo_files_blocked and test_proc_task_thread_sensitive_files_blocked tests.
| def _filter_read_blocked_search_results(result, task_id: str = "default") -> int: | ||
| """Remove credential/cache/env paths from a SearchResult in-place.""" | ||
| omitted = 0 | ||
|
|
||
| if hasattr(result, "matches") and result.matches: | ||
| allowed_matches = [] | ||
| for match in result.matches: | ||
| if _search_result_read_block_error(match.path, task_id): | ||
| omitted += 1 | ||
| continue | ||
| allowed_matches.append(match) | ||
| result.matches = allowed_matches | ||
|
|
||
| if hasattr(result, "files") and result.files: | ||
| allowed_files = [] | ||
| for file_path in result.files: | ||
| if _search_result_read_block_error(file_path, task_id): | ||
| omitted += 1 | ||
| continue | ||
| allowed_files.append(file_path) | ||
| result.files = allowed_files | ||
|
|
||
| if hasattr(result, "counts") and result.counts: | ||
| allowed_counts = {} | ||
| for file_path, count in result.counts.items(): | ||
| if _search_result_read_block_error(file_path, task_id): | ||
| omitted += 1 | ||
| continue | ||
| allowed_counts[file_path] = count | ||
| result.counts = allowed_counts | ||
|
|
||
| return omitted |
There was a problem hiding this comment.
🟡 search_tool results bypass /proc pseudo-file blocklist applied by read_file_tool (security)
read_file_tool blocks /proc pseudo-files (maps, mem, environ, auxv, pagemap, stat, etc.) via _is_blocked_device at line 1059. However, the new search result filtering in _filter_read_blocked_search_results (lines 446-477) and the search directory guard (line 1805) apply only get_read_block_error — which blocks credential stores, .env files, and Hermes cache paths, but NOT /proc pseudo-files. This means: (1) a search rooted in /proc/self/ is not blocked by the directory guard, and (2) search results containing paths like /proc/self/maps or /proc/12345/environ pass through the result filter. While content matches are redacted via redact_sensitive_text (line 1818), the file path metadata (including line numbers) remains in results. This creates an asymmetry between read_file_tool (dual guards) and search_tool (single guard), potentially leaking blocked-path metadata through search enumeration.
💡 Suggestion: Extend _filter_read_blocked_search_results to also call _is_blocked_device (or _is_blocked_device_path) for each search result path, mirroring the dual-guard pattern used in read_file_tool. Additionally, apply _is_blocked_device to the search directory in search_tool (alongside the existing get_read_block_error guard at line 1805) to block searches rooted in /proc/ directories.
📋 Prompt for AI Agents
In tools/file_tools.py: (1) In _search_result_read_block_error (lines 431-443), add a call to _is_blocked_device_path(str(resolved)) alongside the existing get_read_block_error(str(resolved)) call — return the first non-None error from either guard, so /proc pseudo-files are filtered from search results. (2) In search_tool around line 1805, add a call to _is_blocked_device(str(resolved_path)) for the search directory path alongside the existing get_read_block_error check, so searches rooted in /proc/self/ or similar sensitive directories are blocked before performing I/O.
1 participant
Summary
read_filenow blocks six more/proc/*pseudo-files that leaked the same ASLR layout/proc/*/mapswas hidden to protect (PR NousResearch#4609). Previously_is_blocked_device_path()only matchedendswith(("/environ", "/cmdline", "/maps")), so every sibling below slipped through./proc/*/smaps/proc/*/smaps_rollup/proc/*/numa_maps/proc/*/mem/proc/*/auxv/proc/*/pagemapread_file("/proc/self/smaps")or/proc/self/auxvrecovered the exact address layout NousResearch#4609 set out to hide.Changes
tools/file_tools.py: extend theendswithtuple in_is_blocked_device_path()to cover smaps / smaps_rollup / numa_maps / mem (@AhmetArif0's commit) plus auxv / pagemap.endswithmatches both/proc/<pid>/Xand the per-thread/proc/<pid>/task/<tid>/Xalias.tests/tools/test_file_read_guards.py: regression assertions for all six new paths + a new test for the/proc/<pid>/task/<tid>/*thread-alias form.Same suffix-tuple pattern as NousResearch#4609. A regex/set refactor across the full
/procleak surface (stack, syscall, wchan, kallsyms, …) is a worthwhile follow-up but out of scope for closing this immediate gap.Validation
/proc/self/{smaps,smaps_rollup,numa_maps,mem,auxv,pagemap}/proc/self/task/<tid>/{maps,auxv,pagemap,…}/proc/{cpuinfo,meminfo,version,uptime,status}tests/tools/test_file_read_guards.py: 43/43 pass.HERMES_HOME: all 10 sensitive paths refused at both the_is_blocked_deviceguard and theread_filesurface; all 5 legit/procpaths still readable.Salvage of NousResearch#32238 (@AhmetArif0, commit cherry-picked with authorship preserved). Closes NousResearch#34430, closes NousResearch#32238.
Infographic
Mirror-of: NousResearch#56219
NousResearch#56219