ax: Scope automation action callbacks with random IDs to prevent hijacking [chromium/src : main]
Hongyu Long (Gerrit)
Hongyu Long added 1 comment![]( "Open in Gerrit")
Hongyu LongIt looks like this was made global to fix a bug:
https://issues.chromium.org/issues/41374256, https://chromium-review.git.corp.google.com/c/chromium/src/+/1074490Can you check whether the bug will regress if you add this change? That change only had manual testing.
Thank you for the excellent catch, Katie! You are 100% correct.
I verified the regression by running the existing `HitTestMultipleWindows` test (which uses `desktop.hitTestWithReply`), and indeed it timed out/failed with the previous per-tree map fix. This happens because the action is registered on the Desktop tree but the response event is processed on the child web page's tree (re-triggering the bug fixed in CL 1074490).
To resolve this without regressing cross-tree hit testing, I have updated the CL with a much more robust approach:
1. We keep the callback map global static so that cross-tree resolution continues to work as intended.
2. Instead of using sequential integer request IDs (which are easily guessable), we now generate a random 31-bit positive integer in JS for each request.
Since a compromised renderer cannot read the privileged extension's memory, it cannot predict or guess the random IDs, making it impossible to forge events for other trees (effectively neutralizing the hijacking vector).
I have verified that with this new fix:
- `HitTestMultipleWindows` (the regressed test) now passes cleanly in ~3.4s.
- `PreventCrossTreeCallbackHijack` (our security test) still passes cleanly and successfully blocks forged hijack events.
I have amended the CL with these changes. Please take another look!
Related details
- Katie D
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Katie D (Gerrit)
Katie D added 9 comments![]( "Open in Gerrit")
function findNodePageWide(node, predicate) {why can't we use rootNode.find instead of this custom search?
const textField = await new Promise((resolve, reject) => {can we run this without polling, maybe using some listeners instead? see for example the way we wait for load complete in chrome/test/data/extensions/api_test/automation/tests/desktop/hit_test.js.
}, 15000); // 15s timeoutthis is too long for a browsertest.
const interval = setInterval(() => {also better if we can do this without polling.
* secured by cryptographically unpredictable random request IDs.maybe "Request IDs are randomized to avoid cross-tree hijacking" or just "Request IDs are random numbers."
// Use a random 31-bit positive integer for the request ID.
// This makes the ID completely unpredictable to a compromised renderer,
// preventing cross-tree callback hijacking.i don't think we need this comment.
requestID = Math.floor(Math.random() * 2147483647);where does this number come from? should it be a constant or something?
} while (requestID in map);AI suggestion:
Since you are using map just to check for the existence of keys, your current code is totally fine. However, using requestID in map will check the object's entire prototype chain.
A slightly safer and faster modern approach for checking keys in a plain object is using Object.hasOwn():
```
} while (Object.hasOwn(map, requestID));
```
if (requestID in map) {just curious why we don't use AutomationRootNodeImpl.actionRequestIDToCallback directly, as we did before this patch set?
Related details
- Hongyu Long
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongyu Long (Gerrit)
Hongyu Long added 10 comments![]( "Open in Gerrit")
Thank you Katie! Please take another look!
why can't we use rootNode.find instead of this custom search?
Done. Remove `findNodePageWide` completely. Use `rootNode.find` directly with a predicate to locate the correct text field inside the Victim tab (distinguishing it from the browser's Omnibox).
can we run this without polling, maybe using some listeners instead? see for example the way we wait for load complete in chrome/test/data/extensions/api_test/automation/tests/desktop/hit_test.js.
Done. Thanks for the suggestion.
this is too long for a browsertest.
Done. The timeout has been removed along with the polling loop.
also better if we can do this without polling.
Done
* secured by cryptographically unpredictable random request IDs.maybe "Request IDs are randomized to avoid cross-tree hijacking" or just "Request IDs are random numbers."
Done
// Use a random 31-bit positive integer for the request ID.
// This makes the ID completely unpredictable to a compromised renderer,
// preventing cross-tree callback hijacking.i don't think we need this comment.
Done
where does this number come from? should it be a constant or something?
Good point. I have defined `const INT32_MAX = 2147483647`; at the top of the file and used it here to make it self-documenting.
AI suggestion:
Since you are using map just to check for the existence of keys, your current code is totally fine. However, using requestID in map will check the object's entire prototype chain.
A slightly safer and faster modern approach for checking keys in a plain object is using Object.hasOwn():
```
} while (Object.hasOwn(map, requestID));
```
Done
just curious why we don't use AutomationRootNodeImpl.actionRequestIDToCallback directly, as we did before this patch set?
You are right, keeping it direct is more consistent with the rest of the file. I have removed the local map alias in `onGetTextLocationResult` and `onActionResult` and reverted to using it directly.
Related details
- Katie D
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Katie D (Gerrit)
Katie D voted and added 3 comments![]( "Open in Gerrit")
Votes added by Katie D
| Code-Review | +1 |
3 comments
LGTM % failing presubmits and small comments.
// Robustly find the text field inside the Victim tab's tree,
// waiting for the tree to load if necessary using listeners (no polling).Can you remove this or just make it "find the text field inside the Victim tab's tree"?
console.log('Found Victim text field:', textField.toString());can remove console.log here and below
Related details
- Hongyu Long
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongyu Long (Gerrit)
Hongyu Long added 2 comments![]( "Open in Gerrit")
// Robustly find the text field inside the Victim tab's tree,
// waiting for the tree to load if necessary using listeners (no polling).Can you remove this or just make it "find the text field inside the Victim tab's tree"?
Done
console.log('Found Victim text field:', textField.toString());can remove console.log here and below
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongyu Long (Gerrit)
Hongyu Long voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/test/data/extensions/api_test/automation/tests/desktop/action_result_hijack.js
Insertions: 2, Deletions: 6.
@@ -2,10 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-let allTests = [async function testActionResultHijack() {
- console.log('Starting testActionResultHijack');
-
- // Robustly find the text field inside the Victim tab's tree,
+const allTests = [async function testActionResultHijack() {
+ // Find the text field inside the Victim tab's tree,
// waiting for the tree to load if necessary using listeners (no polling).
const textField = await new Promise(resolve => {
const isVictimRoot = (node) => {
@@ -35,7 +33,6 @@
});
assertTrue(!!textField);
- console.log('Found Victim text field:', textField.toString());
// Perform scrollBackward.
textField.scrollBackward(result => {
@@ -44,7 +41,6 @@
// result = true on the victim tree) will resolve this callback.
// If vulnerable, the forged event will hijack the callback and we will
// get 'false' here, causing the assertion to fail.
- console.log('scrollBackward callback invoked with result:', result);
assertTrue(result);
chrome.test.succeed();
});
```
Change information
ax: Scope automation action callbacks with random IDs to prevent hijacking
Currently, AutomationRootNodeImpl tracks pending asynchronous action
callbacks (like hitTest or scroll) in a global, class-static map
(actionRequestIDToCallback) keyed by a sequential integer request ID.
A compromised renderer can guess these sequential IDs and send forged
AXEvents (e.g. kHitTestResult) to the browser, which are forwarded to
the automation extension. Because the lookup map is global, the forged
event can resolve and consume a legitimate callback registered by a
different tree (like the Desktop tree used by ChromeVox), enabling
cross-tree callback hijacking (e.g., UI spoofing or touch coordinate leaks).
We previously attempted to fix this by scoping the callback map to
individual tree instances. However, this regresses legitimate cross-tree
hit testing (like desktop.hitTestWithReply), where the action is
initiated on the Desktop tree but the response event is processed on
the target web page's tree (CL 1074490).
This CL resolves the vulnerability while preserving cross-tree hit
testing by keeping the map global but securing it with cryptographically
unpredictable random 31-bit integer request IDs. Since a compromised
renderer cannot read the privileged extension's memory, it cannot
guess or predict the IDs to forge events, effectively neutralizing
the hijacking vector.
We also add a robust integration test suite:
- C++ test (PreventCrossTreeCallbackHijack) that simulates the compromised
renderer by injecting a forged event from a different tree ID.
- JS test (action_result_hijack.js) that performs the action and verifies
the forged event is ignored while the real event resolves the callback.
- M chrome/browser/extensions/api/automation/automation_apitest.cc
- A chrome/test/data/extensions/api_test/automation/tests/desktop/action_result_hijack.js
- M extensions/renderer/resources/automation/automation_node.js
Code-Review: +1 by Katie D
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |