fix(configurable): prevent path traversal in AgentTool config_path resolution - #878
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
…solution Absolute config_path values were accepted unconditionally, and relative paths were joined without boundary validation, allowing traversal outside the agent directory via "../../../etc/passwd" style inputs. Fix: reject absolute paths; verify the resolved path stays within the parent agent's directory using strings.HasPrefix after filepath.Clean.
3352de5 to
2a4876e
Compare
…nfig_path containment check The containment check for AgentTool config_path compared an absolute target path against a parent directory that was only cleaned, not made absolute, so a relative parentPath caused legitimate in-directory references to be rejected. The comparison was also purely lexical, so a symlink inside the agent directory could still resolve outside it and be loaded. Make both sides absolute before comparing and resolve symlinks where the paths exist, falling back to the lexical path when they do not so that a missing file still reports as not found rather than as a traversal. Add regression tests covering absolute paths, parent traversal, symlink escape, references inside the agent directory, and a relative parent path.
|
@wolo-lab @hanorik — could one of you take a look at this when you have a moment, or route it to whoever owns This closes a path traversal in The same issue was fixed in the sibling implementations and both have landed:
adk-go is the last of the three still carrying the original pattern. I have just pushed an update that tightens the check and adds tests:
Status: CLA signed, Happy to adjust the approach to whatever you prefer, or feel free to take the change over and land it yourselves. |
|
Thank you for preparing a fix. @baptmont, please take a look at this PR. |
…solution (#878) (#1277) fix(configurable): prevent path traversal in AgentTool config_path resolution Reject absolute config_path values and verify that a relative one resolves inside the referencing agent's directory, resolving symlinks where the paths exist. Matches the hard rejection adk-python landed in 171ae9e; adk-java (#1218) chose a warn-only deprecation instead. BREAKING: an absolute config_path is no longer accepted. --------- * fix(configurable): prevent path traversal in AgentTool config_path resolution Absolute config_path values were accepted unconditionally, and relative paths were joined without boundary validation, allowing traversal outside the agent directory via "../../../etc/passwd" style inputs. Fix: reject absolute paths; verify the resolved path stays within the parent agent's directory using strings.HasPrefix after filepath.Clean. * fix(configurable): compare absolute, symlink-resolved paths in the config_path containment check The containment check for AgentTool config_path compared an absolute target path against a parent directory that was only cleaned, not made absolute, so a relative parentPath caused legitimate in-directory references to be rejected. The comparison was also purely lexical, so a symlink inside the agent directory could still resolve outside it and be loaded. Make both sides absolute before comparing and resolve symlinks where the paths exist, falling back to the lexical path when they do not so that a missing file still reports as not found rather than as a traversal. Add regression tests covering absolute paths, parent traversal, symlink escape, references inside the agent directory, and a relative parent path. --------- Co-authored-by: João Westerberg <westerberg@google.com> (cherry picked from commit 604dd63) Co-authored-by: Adil Burak Şen <56400880+adilburaksen@users.noreply.github.com>
…erences #878 added a containment check to ResolveAgentReference, but the workflow loader resolves node references on a separate path that never reaches it. resolveNodeLike accepted an absolute reference and joined a relative one without validating the result, so a workflow config could name any .yaml or .yml file on disk. The FunctionNode, JoinNode and ToolNode branches of resolveNodeFromYAML all return before its fall-through call to ResolveAgentReference, so for those node classes an out-of-tree file was read, parsed and instantiated with no containment check at all. Only the agent branch was covered, and by the time it ran the read had already happened. Extract the check into resolveContainedConfigPath and call it from both resolvers, before the file is read and before the node cache is consulted. One shared copy is the point: two independent resolvers is how the gap appeared in the first place. BREAKING: an absolute workflow node reference is no longer accepted, matching the config_path behaviour #878 established.
PR google#878 added a containment check to ResolveAgentReference: an AgentTool config_path may not be absolute, and a relative one must resolve inside the referencing agent's directory. Workflow edge references were left on the pre-google#878 code — absolute refs accepted unconditionally, relative ones joined with no boundary check — and resolveNodeFromYAML reads them straight off disk with os.ReadFile before dispatching on agent_class. A workflow config could therefore load and instantiate a FunctionNode, JoinNode or ToolNode from anywhere on the filesystem. Extract the containment logic into resolveConfigReference and route both ResolveAgentReference and resolveNodeLike through it, so the rule has one implementation rather than one per reference kind. Two fixes to the check itself, both found while covering the shared helper: - The parent directory is now resolved before the reference is joined onto it, so both sides of the comparison are rooted in the same real path. Resolving only the parent side meant that a parent directory reached through a symlink plus a not-yet-existing target — which has no symlinks to resolve and so keeps its unresolved spelling — was reported as a traversal instead of as not found, the case google#878 intended to allow. - A reference carrying a volume name is rejected alongside absolute ones. On Windows a drive-relative reference such as `C:node.yaml` is not absolute yet still escapes, by resolving against the current directory of that drive; filepath.VolumeName also covers UNC paths and is empty on Unix. BREAKING: an absolute config_path is no longer accepted in workflow edge references, matching the google#878 change to agent references. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Summary
ResolveAgentReferenceinconfigurable_utils.goaccepted absoluterefPathvalues unconditionally and joined relative paths viafilepath.Joinwithout boundary enforcement. An attacker-controlledconfig_pathfield in an agent YAML could read arbitrary files accessible to the server process.Vulnerable pattern (before):
Fix
refPathvaluesstrings.HasPrefixon the cleaned pathRelated
Same vulnerability exists in
adk-python(PR: google/adk-python#5826) andadk-java(PR: google/adk-java#1218) — fix pattern is identical across all three SDKs.