Repository navigation
ReadFileTool shells out on ranged reads and interpolates untrusted path #5530
Description
ReadFileTool shells out for ranged reads (start_line > 1 or end_line is set) instead of reading the file directly.
adk-python/src/google/adk/tools/environment/_tools.py
Lines 190 to 191 in 3e282d2
As a result, the ranged-read path is not a pure file-read operation, and paths containing shell-sensitive characters can be interpreted unexpectedly.
Steps to Reproduce:
- Install ADK.
- Run the minimal reproduction below.
- Call
ReadFileToolwithstart_line=2and a path containing a single quote. - Observe that the ranged-read path invokes shell execution instead of direct file I/O.
Expected Behavior:
ReadFileTool should always read file contents directly and should not invoke shell commands based on the requested line range.
Observed Behavior:
When start_line or end_line is provided, ReadFileTool builds and runs a shell command using the provided path.
Environment Details:
- ADK Library Version (pip show google-adk): 1.31.0
- Desktop OS:** Linux
- Python Version (python -V): Python 3.14.4
Model Information:
- Are you using LiteLLM: No
- Which model is being used: N/A
🟡 Optional Information
Regression:
Unknown / N/A
Logs:
text file.
No special logs required. The issue is reproducible from the current
ReadFileTool implementation.
Screenshots / Video:
N/A
Additional Context:
This looks fixable by removing the shell-based ranged-read path and handling line slicing entirely in Python.
Minimal Reproduction Code:
Please provide a code snippet or a link to a Gist/repo that isolates the issue.
import asyncio
from pathlib import Path
import tempfile
from google.adk.environment._local_environment import LocalEnvironment
from google.adk.tools.environment._tools import ReadFileTool
async def main():
with tempfile.TemporaryDirectory() as td:
env = LocalEnvironment(working_dir=Path(td))
await env.initialize()
target = Path(td) / "sample.txt"
target.write_text("line1\nline2\nline3\n", encoding="utf-8")
marker = Path(td) / "marker.txt"
# ReadFileTool is expected to treat `path` as data, not as shell syntax.
# This payload closes the quoted filename used by the ranged-read path,
# injects an extra command, and reopens the quote to keep the shell happy.
injected_path = f"sample.txt'; touch {marker}; echo '"
tool = ReadFileTool(env)
result = await tool.run_async(
# `start_line=2` is important because it triggers the code path that
# shells out through `cat ... | sed ...` instead of doing pure Python
# file I/O.
args={"path": injected_path, "start_line": 2},
tool_context=None,
)
print(result)
# If this prints True, a file-read API caused an unintended side effect.
# That demonstrates the bug: the path was interpreted by the shell.
print("marker exists:", marker.exists())
await env.close()
asyncio.run(main())
How often has this issue occurred?:
- Always (100%)
Activity
surajksharma07 commentedon Apr 30, 2026
@failuresmith The cat -n '{path}' | sed -n ... path is interpolating the untrusted path directly into a shell command whenever start_line > 1 or end_line is set.
The pure-Python fallback below it already handles ranged reads correctly, so the fix is straightforward: remove the sed-based block entirely and let all reads go through self._environment.read_file(path).
PR #5537 has been linked and looks like the right direction — could you give it a review and test once it lands and confirm it resolves the injection on your side?
Please do thorough testing before we push it through!
milaforge commentedon Apr 30, 2026
@surajksharma07 I verified this blocks the original
path-based PoC, but I don’t think it fully fixes the issue. The ranged-read path still shells out, andend_lineis still interpolated into thesedcommand; I was able to reproduce command execution through that path after this patch.I think the right fix is to remove the
cat | sedbranch entirely and always useread_file()plus the existing Python slicing logic. That eliminates the injection surface and also fixes the current missing-file behavior for ranged reads.I’ve put together a follow-up patch with tests here.
surajksharma07 commentedon May 1, 2026
@failuresmith PR #5537 only fixes the path vector.
The end_line injection is still open because sed_range is built as f'{start},{end_line}' and dropped directly into the shell command unquoted: sed -n '{sed_range}p'.
Since end_line has no type validation before it hits execute(), passing something like end_line="999; touch /tmp/pwned" still executes.
Quoting path shifted the attack surface — it didn't close it.
PR #5558 is the correct fix.
Removing the shell branch entirely means there's nothing left to inject into.
The pure-Python path with read_file() + list slicing already handles all the same cases: start_line, end_line, range validation, line numbering, and missing-file errors.
The end_line integer check you added on top is the right defensive layer — returning 'end_line must be an integer if provided' before any execution path is reached is the cleanest way to close that last ambiguity.
The test coverage in #5558 also looks solid — regression for normal ranged reads, correct missing-file behavior (which the sed path was silently swallowing by returning ok with empty content), and the non-integer end_line input all covered.
Worth flagging: this is still unpatched as of v1.32.0 — neither #5537 nor #5558 has landed yet.
Since this is arbitrary command execution reachable through a core tool, #5558 should take priority.
Team is looking more into it.
Thanks !