[SVG] (1) UKM filters applied to embedded content [chromium/src : main]
Ari Chivukula (Gerrit)
Ari Chivukula added 5 comments![]( "Open in Gerrit")
Philip RogersCould use an initial look + pointers on why I'm failing to count plugins properly (it seems like the layout view object is missing when paint is called).
Ari ChivukulaCan you try copying `third_party/blink/web_tests/ppapi/plugins/plugin-paint-test.html`?
Done
[SVG] Count filters applied to embedded contentAri ChivukulaImagine the following case:
local.com/index.html:
```
<div with complex svg filter>
<iframe src=remote.com/iframe.html">
</div>
```remote.com/iframe.html:
```
<div>
<plugin>
</div>
```In this case, I think we should only record that a remote frame is filtered, and not that a plugin is filtered. It would be complex and error-prone (think: timing issues) to track this information across processes. Can you test this case?
Done
WebFeature::kSvgFilterPaintedOnLocalFrame,Ari ChivukulaWhat about a same-origin frame with CSP: sandbox? Does that create a remote frame? I think it does, but please check this. If it does not, then we might need to use different terms than Local and Remote.
it likely does create a remote frame, as the distinction isn't just same or cross origin. That said, I think the risk we care about is the risk between two non-local frames, I don't think it's more important that they are or aren't cross-origin.
if (target->StyleRef().HasReferenceFilter()) {Ari ChivukulaThis will be true if target itself has a reference filter, but we also need to check if any ancestor of target has a reference filter.
An approach is to pass this information down, rather than looking up. This can be done using PaintInfo.
Are you saying the current PaintInfo has this context, or that I could modify it to pass that sort of info down? I've been cautioned against adding more data to any critical objects like that, and iframes/plugins should be rare enough that a quick ancestor scan (as I'm doing here) seems reasonable.
<feGaussianBlur stdDeviation="1" />Ari ChivukulaWe want to allow some svg filters and not others, right? I think svg blur is one that we actually want to allow? feTile is an example that we do not want to allow. Can you adjust the tests, usecounter name, and implementation to reflect this?
Just throwing out an idea, maybe we could go with kComplexSvgFilterOnLocalFrame, kComplexSvgFilterPaintedOnRemoteFrame, and kComplexSvgFilterOnPlugin?
I don't have a specific desire to allow some SVG filters and not others. I could see adding UMA after this that breaks out more detailed information like *why* two frames are remote (not same origin, CSP, etc) as well as the contents of the filter itself. For now, the goal is just to get some high level counts to get an idea of the scope of the issue.
Related details
- Philip Rogers
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. |
Sun Yueru (Gerrit)
Sun Yueru voted Code-Review+1![]( "Open in Gerrit")
- Ari Chivukula
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
if (target->StyleRef().HasReferenceFilter()) {Ari ChivukulaThis will be true if target itself has a reference filter, but we also need to check if any ancestor of target has a reference filter.
An approach is to pass this information down, rather than looking up. This can be done using PaintInfo.
Are you saying the current PaintInfo has this context, or that I could modify it to pass that sort of info down? I've been cautioned against adding more data to any critical objects like that, and iframes/plugins should be rare enough that a quick ancestor scan (as I'm doing here) seems reasonable.
I was suggesting using PaintInfo but I agree with your points here: this is rare enough that the simpler approach makes sense.
<feGaussianBlur stdDeviation="1" />Ari ChivukulaWe want to allow some svg filters and not others, right? I think svg blur is one that we actually want to allow? feTile is an example that we do not want to allow. Can you adjust the tests, usecounter name, and implementation to reflect this?
Just throwing out an idea, maybe we could go with kComplexSvgFilterOnLocalFrame, kComplexSvgFilterPaintedOnRemoteFrame, and kComplexSvgFilterOnPlugin?
I don't have a specific desire to allow some SVG filters and not others. I could see adding UMA after this that breaks out more detailed information like *why* two frames are remote (not same origin, CSP, etc) as well as the contents of the filter itself. For now, the goal is just to get some high level counts to get an idea of the scope of the issue.
Safari only disallows some SVG filters. If we deviate from Safari, that risks web compat issues, since we would be the first browser to disallow svg fe gaussian blur on iframes.
https://docs.google.com/document/d/11t372cAaXVT53JwAfmt6mfpPk8M9XIo7oXHT95e42Ks/edit?usp=sharing is a summary of the public usecounter data of the svg filter subtypes, and fe gaussian blur is pretty popular at 3% of page views (https://chromestatus.com/metrics/feature/timeline/popularity/5756).
Does this change your opinion here?
await waitForRender(1000);waitForRender is typically requestAnimationFrame + setTimeout(0). Why do we use 1000 here?
Related details
- Ari Chivukula
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. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
await waitForRender(1000);waitForRender is typically requestAnimationFrame + setTimeout(0). Why do we use 1000 here?
that's an old version, once I resolved the plugin issue it because unneeded
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
<feGaussianBlur stdDeviation="1" />Ari ChivukulaWe want to allow some svg filters and not others, right? I think svg blur is one that we actually want to allow? feTile is an example that we do not want to allow. Can you adjust the tests, usecounter name, and implementation to reflect this?
Just throwing out an idea, maybe we could go with kComplexSvgFilterOnLocalFrame, kComplexSvgFilterPaintedOnRemoteFrame, and kComplexSvgFilterOnPlugin?
Philip RogersI don't have a specific desire to allow some SVG filters and not others. I could see adding UMA after this that breaks out more detailed information like *why* two frames are remote (not same origin, CSP, etc) as well as the contents of the filter itself. For now, the goal is just to get some high level counts to get an idea of the scope of the issue.
Safari only disallows some SVG filters. If we deviate from Safari, that risks web compat issues, since we would be the first browser to disallow svg fe gaussian blur on iframes.
https://docs.google.com/document/d/11t372cAaXVT53JwAfmt6mfpPk8M9XIo7oXHT95e42Ks/edit?usp=sharing is a summary of the public usecounter data of the svg filter subtypes, and fe gaussian blur is pretty popular at 3% of page views (https://chromestatus.com/metrics/feature/timeline/popularity/5756).
Does this change your opinion here?
Are they actually only blocking some filters in practice? I know they mentioned that intent, but I think they also block any reference filter: https://github.com/WebKit/WebKit/blob/0ada01f2e44580f5b584121ad770c8cf07d9a847/Source/WebCore/style/values/filter-effects/StyleFilterReference.h#L56
Lyra Rebane (Gerrit)
Lyra Rebane added 1 comment![]( "Open in Gerrit")
<feGaussianBlur stdDeviation="1" />Ari ChivukulaWe want to allow some svg filters and not others, right? I think svg blur is one that we actually want to allow? feTile is an example that we do not want to allow. Can you adjust the tests, usecounter name, and implementation to reflect this?
Just throwing out an idea, maybe we could go with kComplexSvgFilterOnLocalFrame, kComplexSvgFilterPaintedOnRemoteFrame, and kComplexSvgFilterOnPlugin?
Philip RogersI don't have a specific desire to allow some SVG filters and not others. I could see adding UMA after this that breaks out more detailed information like *why* two frames are remote (not same origin, CSP, etc) as well as the contents of the filter itself. For now, the goal is just to get some high level counts to get an idea of the scope of the issue.
Ari ChivukulaSafari only disallows some SVG filters. If we deviate from Safari, that risks web compat issues, since we would be the first browser to disallow svg fe gaussian blur on iframes.
https://docs.google.com/document/d/11t372cAaXVT53JwAfmt6mfpPk8M9XIo7oXHT95e42Ks/edit?usp=sharing is a summary of the public usecounter data of the svg filter subtypes, and fe gaussian blur is pretty popular at 3% of page views (https://chromestatus.com/metrics/feature/timeline/popularity/5756).
Does this change your opinion here?
Are they actually only blocking some filters in practice? I know they mentioned that intent, but I think they also block any reference filter: https://github.com/WebKit/WebKit/blob/0ada01f2e44580f5b584121ad770c8cf07d9a847/Source/WebCore/style/values/filter-effects/StyleFilterReference.h#L56
Tested on Safari and can confirm that feGaussianBlur is disallowed.
Related details
- Ari Chivukula
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
<feGaussianBlur stdDeviation="1" />Ari ChivukulaWe want to allow some svg filters and not others, right? I think svg blur is one that we actually want to allow? feTile is an example that we do not want to allow. Can you adjust the tests, usecounter name, and implementation to reflect this?
Just throwing out an idea, maybe we could go with kComplexSvgFilterOnLocalFrame, kComplexSvgFilterPaintedOnRemoteFrame, and kComplexSvgFilterOnPlugin?
Philip RogersI don't have a specific desire to allow some SVG filters and not others. I could see adding UMA after this that breaks out more detailed information like *why* two frames are remote (not same origin, CSP, etc) as well as the contents of the filter itself. For now, the goal is just to get some high level counts to get an idea of the scope of the issue.
Ari ChivukulaSafari only disallows some SVG filters. If we deviate from Safari, that risks web compat issues, since we would be the first browser to disallow svg fe gaussian blur on iframes.
https://docs.google.com/document/d/11t372cAaXVT53JwAfmt6mfpPk8M9XIo7oXHT95e42Ks/edit?usp=sharing is a summary of the public usecounter data of the svg filter subtypes, and fe gaussian blur is pretty popular at 3% of page views (https://chromestatus.com/metrics/feature/timeline/popularity/5756).
Does this change your opinion here?
Lyra RebaneAre they actually only blocking some filters in practice? I know they mentioned that intent, but I think they also block any reference filter: https://github.com/WebKit/WebKit/blob/0ada01f2e44580f5b584121ad770c8cf07d9a847/Source/WebCore/style/values/filter-effects/StyleFilterReference.h#L56
Tested on Safari and can confirm that feGaussianBlur is disallowed.
Matching Safari SGTM! Sorry, I thought they allowed blur for some reason.
Related details
- Ari Chivukula
- Lyra Rebane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
const LayoutObject* target = this;Mind adding a small note to help future us?
```
// This is an iteration of all parents on every paint, but frames are rare
// enough that we do not expect this to be a problem.
```
<!DOCTYPE html>Can you add one additional test where you have an existing css filter and then you change it to a svg filter and check that the usecounter fires? Something like:
```
promise_test(async (t) => {
await waitForLoad();
reset();
await waitForRender();
expectCounters(/*local=*/false, /*remote=*/false, /*plugin=*/false, 'Initial load.');
// 1. Start with a CSS filter on the iframe
document.getElementById('local_iframe').className = 'css-blur';
await waitForRender();
expectCounters(/*local=*/false, /*remote=*/false, /*plugin=*/false, 'Initial CSS filter.');
// 2. Switch directly to an SVG filter WITHOUT calling reset()
document.getElementById('local_iframe').className = 'svg-blur';
await waitForRender();
expectCounters(/*local=*/true, /*remote=*/false, /*plugin=*/false, 'Switched to SVG filter on iframe.');
reset();
await waitForRender();
// 3. Start with a CSS filter on an ancestor
document.getElementById('local_outer').className = 'css-blur';
await waitForRender();
expectCounters(/*local=*/false, /*remote=*/false, /*plugin=*/false, 'Initial CSS filter on ancestor.');
// 4. Switch directly to an SVG filter WITHOUT calling reset()
document.getElementById('local_outer').className = 'svg-blur';
await waitForRender();
expectCounters(/*local=*/true, /*remote=*/false, /*plugin=*/false, 'Switched to SVG filter on ancestor.');
}, 'Switching from CSS filter to SVG filter triggers counter');
```
expectCounters(/*local=*/false, /*remote=*/false, /*plugin=*/false, 'Inital load.');nit: Inital -> Initial
Related details
- Ari Chivukula
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. |
Ari Chivukula (Gerrit)
Ari Chivukula added 3 comments![]( "Open in Gerrit")
Mind adding a small note to help future us?
```
// This is an iteration of all parents on every paint, but frames are rare
// enough that we do not expect this to be a problem.
```
Done
Done
expectCounters(/*local=*/false, /*remote=*/false, /*plugin=*/false, 'Inital load.');Ari Chivukulanit: Inital -> Initial
Done
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula voted Commit-Queue+2![]( "Open in Gerrit")
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![]( "Open in Gerrit")
Change information
[SVG] (1) UKM filters applied to embedded content
As outlined in:
https://lyra.horse/blog/2025/12/svg-clickjacking/
SVG filters applied to frames/plugins contain a risk cross-origin
information extraction.
This CL adds metrics on which URLs are impacted by such filters,
but stops sport of gathering detailed information on the filters
themselves or the nature of the frames/plugins. That will be left for
the follow-up UMA counter which will gather more detailed information.
This CL is part of a series of CLs:
(1) UKM filters applied to embedded content
(2) UMA filters applied to embedded content
(3) Add flag to block filters applied to embedded content
- M components/page_load_metrics/browser/observers/use_counter/ukm_features.cc
- M third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
- M third_party/blink/renderer/core/exported/web_plugin_container_impl.cc
- M third_party/blink/renderer/core/exported/web_plugin_container_impl.h
- M third_party/blink/renderer/core/frame/embedded_content_view.h
- M third_party/blink/renderer/core/frame/local_frame_view.cc
- M third_party/blink/renderer/core/frame/local_frame_view.h
- M third_party/blink/renderer/core/frame/remote_frame_view.cc
- M third_party/blink/renderer/core/frame/remote_frame_view.h
- M third_party/blink/renderer/core/layout/layout_embedded_content.cc
- M third_party/blink/renderer/core/layout/layout_embedded_content.h
- M third_party/blink/renderer/core/layout/layout_embedded_object.cc
- A third_party/blink/web_tests/flag-specific/disable-site-isolation-trials/wpt_internal/svg/use-count-svg-filter-on-embedded-content.sub.https-expected.txt
- A third_party/blink/web_tests/wpt_internal/svg/resources/empty.html
- A third_party/blink/web_tests/wpt_internal/svg/resources/nested.sub.html
- A third_party/blink/web_tests/wpt_internal/svg/use-count-svg-filter-on-embedded-content.sub.https.html
- M tools/metrics/histograms/metadata/blink/enums.xml
Code-Review: +1 by Philip Rogers
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |