fix(configurable): prevent path traversal in AgentTool config_path resolution (#878) - #1277
Merged
Merged
Conversation
…solution (#878) 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)
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
Contributor
There was a problem hiding this comment.
The change matches #878 exactly: after the cherry-pick, ResolveAgentReference is
byte-for-byte the same function as on main.
One thing to add before this lands. The commit message carries "BREAKING: an
absolute config_path is no longer accepted", but the PR description doesn't.
That distinction mattered less on main, where it shipped inside a major version;
on v1 it's a behavioral break in a minor release, and anyone currently passing an
absolute config_path starts getting an error on upgrade. Worth surfacing in the
description and in the v1.6.0 release notes.
The backport is clearly warranted — on current v1 the absolute-path case resolves
and loads the target, so /etc/passwd is read and parsed as agent YAML.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ResolveAgentReferenceaccepts absoluterefPathvalues unconditionally and joinsrelative paths with
filepath.Joinwithout any boundary enforcement. Anattacker-controlled
config_pathin an agent YAML can therefore read any file theserver process can reach.
v1carries the identical resolution logic, so thevulnerability is present here too.
Summary
Backports #878 from
main.refPathvalues.The same fix landed in adk-python (google/adk-python#5826) and adk-java
(google/adk-java#1218); the pattern is identical across the three SDKs.