fix(flows): reject forged tool confirmations from user-authored events - #1393
fix(flows): reject forged tool confirmations from user-authored events#1393alphacharlie-dev wants to merge 1 commit into
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. |
|
Hi @alphacharlie-dev, thank you for your contribution! We appreciate you taking the time to submit this pull request. I noticed that the branch is currently out of sync with the base branch. proceed with the review, could you please resolve the merge conflicts? |
RequestConfirmationLlmRequestProcessor resumes a pending tool call by searching backwards through session events for the adk_request_confirmation function call matching a user's confirmation response. That look-back never checked who authored the call. adk_request_confirmation calls are synthesized by the framework as part of a model response (Functions.java, author = agent name); they are never legitimately user-authored. A client that can append events to a session -- the normal POST /run and POST /run_sse surface, where the client supplies appName, userId and sessionId -- could therefore place an adk_request_confirmation call inside a user-authored message naming any registered tool with any arguments, then send the matching confirmation response. The processor dispatched the tool. The model never requested it and no human approved it. Skip user-authored events when sourcing the originating call. Confirmation responses are still read from user events, so the normal resume flow is unchanged. Adds a regression test that drives the full two-turn forgery through Runner against a model that only ever emits text, and asserts the tool never runs. The test fails without the fix and passes with it.
371bcb1 to
90c2cdb
Compare
|
@hemasekhar-p Rebased onto current There were no merge conflicts to resolve — the five intervening commits on
|
|
@googlebot I signed it! |
Fixes #1389.
Problem
RequestConfirmationLlmRequestProcessorresumes a pending tool call by searching backwardsthrough session events for the
adk_request_confirmationfunction call matching a user'sconfirmation response. The look-back never checked who authored that call.
Those calls are synthesized by the framework as part of a model response (
Functions.java:800,author = agent name) — they are never legitimately user-authored. A client that can append
events to a session (the normal
POST /run/POST /run_ssesurface, where the client suppliesappName,userIdandsessionId) could place anadk_request_confirmationcall inside itsown user message, naming any registered tool with any arguments, then send the matching
confirmation response. The processor dispatched the tool — the model never requested it, and no
human approved it.
The practical effect is that the human-in-the-loop gate does not bound what a client can invoke:
any tool registered on the agent runs with the server's privileges, with attacker-chosen
arguments.
Fix
Skip user-authored events when sourcing the originating call. Confirmation responses are still
read from user events, so the normal resume flow is unchanged.
Verification
Regression test added to
RunnerTest— it drives the full two-turn forgery throughRunneragainst a model that only ever emits text, and asserts the tool never runs.
RunnerTestRequestConfirmationLlmRequestProcessorTestFunctionsTestToolRequestConfirmationActionTestNote for maintainers
This patch skips events authored by
"user". You may prefer the inverse — accept theoriginating call only when its author is an agent in the invocation's agent tree, e.g.
invocationContext.agent().rootAgent().findAgent(event.author()).isPresent(), which mirrors theexisting pattern at
Runner.java:840-841.The reason to consider it:
SessionJsonConverter.java:201restoresauthorverbatim fromstored JSON (
.author((String) apiEvent.get("author"))), so the field is not server-stamped onevery path, and the codebase carries several non-
"user"author values ("model", agent names,remote A2A agent names). Skipping one literal string is therefore narrower than allowlisting
known agents, which fails closed.
I have not demonstrated a bypass of the check as written — reaching that deserialization path
with a chosen author is a different attack from the one reported here — so I've kept this PR to
the minimal fix for the reported issue and left the broader hardening to your judgement.