Use side-effect-free prototype chain lookup for named property telemetry [chromium/src : main]
Jacques Newman (Gerrit)
Jacques Newman added 2 comments![]( "Open in Gerrit")
Can you cc me on the bug linked? It may be that the v8::TryCatch addition is sufficient even with the moved side-effect.
// Use the side-effect-free *Attributes* variant so that this telemetryThis statement is not accurate, if you compare the result of the below script with and without your change you can observe that it is still possible for script to have a side-effect, see:
```html
<pre id="log"></pre>
<script>
const out = document.getElementById('log');
const log = (m) => { out.innerHTML += m + '\n'; };
const original = Object.getPrototypeOf(HTMLFormElement.prototype);
const proxy = new Proxy(original, {
get(target, key, receiver) {
log('get trap fired for: ' + String(key));
return Reflect.get(target, key, receiver);
},
getOwnPropertyDescriptor(target, key) {
log('getOwnPropertyDescriptor trap fired for: ' + String(key));
return Reflect.getOwnPropertyDescriptor(target, key);
},
});
Object.setPrototypeOf(HTMLFormElement.prototype, proxy);
// Build a form with a named input.
const form = document.createElement('form');
const input = document.createElement('input');
input.name = 'foo';
form.appendChild(input);
log('About to access form.foo ...');
const result = form.foo;
log('form.foo === input : ' + (result === input));
</script>
```#### Without this change (tested on stable):
```
get trap fired for: appendChild
About to access form.foo ...
get trap fired for: foo
form.foo === input : true
```
#### With this change:
```
get trap fired for: appendChild
About to access form.foo ...
getOwnPropertyDescriptor trap fired for: foo
form.foo === input : true
```
Related details
- Dan Clark
- Leo Lee
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. |
Leo Lee (Gerrit)
Leo Lee added 3 comments![]( "Open in Gerrit")
Can you cc me on the bug linked? It may be that the v8::TryCatch addition is sufficient even with the moved side-effect.
I've CC'ed you on the bug. Re: whether TryCatch alone is sufficient — it prevents exception leakage but accessor getters would still execute synchronously, allowing DOM mutation mid-interceptor (the re-entrancy concern from the bug report). The Attributes variant blocks that path, which is why I kept it alongside TryCatch.
// Use the side-effect-free *Attributes* variant so that this telemetryGood catch. You're right that GetRealNamedPropertyAttributesInPrototypeChain still triggers the Proxy getOwnPropertyDescriptor trap. I've updated the comment to be accurate about this.
Related details
- Dan Clark
- Jacques Newman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman added 1 comment![]( "Open in Gerrit")
// Use the side-effect-free *Attributes* variant so that this telemetryWith this in mind, haven't we only moved the script reentrancy problem? If not, what makes this version of script reentrancy safer than the existing behavior?
Related details
- Dan Clark
- Leo Lee
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. |
Leo Lee (Gerrit)
Leo Lee added 1 comment![]( "Open in Gerrit")
// Use the side-effect-free *Attributes* variant so that this telemetryQ1 — "Haven't we only moved the reentrancy problem?"
For a JS Proxy in the chain, yes — [[Get]] -> [[GetOwnProperty]] just shifts the
invoked handler from the `get` trap to the `getOwnPropertyDescriptor` trap, same
synchronous window. For every other case it's genuinely eliminated:
[[GetOwnProperty]] returns an ordinary accessor's descriptor without ever
invoking its getter. In practice the Proxy case is a negligible slice of traffic —
the common lookups are either no-match or a native accessor, and the
native-accessor case is exactly where this removes a real getter invocation.
Q2 — "What makes this version safer than the existing behavior?"
The old [[Get]] path ran any accessor getter in the chain — including legitimate
DOM/JS getters, the realistic bug-513819232 vector — whereas the new path never
calls those getters. The only remaining vector is a deliberately-installed Proxy
trap — author-controlled script the page can already invoke via reflection APIs
like `Object.getOwnPropertyDescriptor`, so this grants no new capability.
Summary: it's equal-or-better in every realistic case — get-only proxies now run no
handler script, exceptions no longer leak, and the existence bucket is actually
more accurate.
Related details
- Dan Clark
- Jacques Newman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman voted and added 4 comments![]( "Open in Gerrit")
Votes added by Jacques Newman
| Code-Review | +1 |
4 comments
lgtm with comments addressed.
This definitely moves things in the right direction, thanks for the fix!
which checks existence without invoking JS, and wrap each in anit: update to reflect that this still can invoke JS, just not the getter (traps are still possible as demonstrated in my other comment.)
nit: stray newline in the middle of the sentence.
local_window_proxy.cc, restructure so the side-effect-free variant isnit: likewise caveat that this isn't quite side-effect free, but we have moved the potential side-effect to a much less common code path.
Related details
- Dan Clark
- Leo Lee
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. |
Jacques Newman (Gerrit)
Jacques Newman added 1 comment![]( "Open in Gerrit")
I'd also like to see tests, but won't block on it.
Leo Lee (Gerrit)
Leo Lee added 5 comments![]( "Open in Gerrit")
I'd also like to see tests, but won't block on it.
Done
which checks existence without invoking JS, and wrap each in anit: update to reflect that this still can invoke JS, just not the getter (traps are still possible as demonstrated in my other comment.)
Done
nit: stray newline in the middle of the sentence.
Done
local_window_proxy.cc, restructure so the side-effect-free variant isnit: likewise caveat that this isn't quite side-effect free, but we have moved the potential side-effect to a much less common code path.
- Dan Clark
- Jacques Newman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jacques Newman
| Code-Review | +1 |
- Dan Clark
- Leo Lee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Clark (Gerrit)
Dan Clark added 2 comments![]( "Open in Gerrit")
// Use the side-effect-free *Attributes* variant so that this telemetryI think we need to be clear on whether we're considering the reentrancy to be a security issue or just an interoperability bug. The bot that filed the bug seems to think it could be a security issue because "This can be used as a re-entrancy gadget to manipulate DOM state during property lookups". If that's true, then the fact that "Proxy case is a negligible slice of traffic" doesn't matter; there's still a vulnerability that needs to be patched.
That said, I think this is just an interop issue rather than a security problem. We can run page script by-design in the fallback path when the named property doesn't exist.
I do think it's worth it to write a test for the Proxy case to document what we expect to happen (script running but exceptions not propagated). Contrary to my suggestion about the existing tests being WPTs, this one probably shouldn't be because I don't think we expect that behavior to be interoperable (right?).
Adding Mike West to cc in case Mike has thoughts on this as a reviewer of https://chromium-review.googlesource.com/c/chromium/src/+/5317665 where this was added.
// without observably interacting with the prototype chain (crbug.com/513819232).Both of these new tests are something that we should also expect other browsers to pass, and this seems like a mistake that could conceivably happen in other engines. What do you think about removing the crbug reference and adding these to `web_tests/external/wpt` instead?
Related details
- Leo Lee
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. |
Leo Lee (Gerrit)
Leo Lee added 2 comments![]( "Open in Gerrit")
@mk...@chromium.org, I'd like to get your thoughts on whether this is really a security issue.
@mk...@chromium.org, thoughts on whether this is more of an interop issue than a security one?
Related details
- Dan Clark
- Jacques Newman
- Mike West
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. |