Count how common it is for SVG STYLE/XMP/etc. elements to contain children in the real world
Categories
(Core :: DOM: Security, task, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox132 | --- | fixed |
People
(Reporter: freddy, Assigned: freddy)
References
Details
(Keywords: perf-alert, Whiteboard: [domsecurity-active])
Attachments
(3 files)
The hypothesis is that a lot of mXSS attacks are based on the facts that SVG STYLE elements can have children. What if that would just not be the case?
I am implementing a glean counter to see how much this comes up in the real world (thanks hsivonen for the many various tips on how to do that)
Assignee | ||
Comment 1•2 months ago
|
||
Assignee | ||
Comment 2•2 months ago
|
||
Here's my patch for early feedback and some questions that I have not yet been able to answer for myself:
Do we want to count this as a rate
metric or would just numbers be OK? If we keep this as a rate, should I count "per parsing" or "per svg"? I currently use all parsing as the denominator.
For local testing and reproduction, I can easily get the code to be hit where isInSVGStyle
is true by opening a new tab of data:text/html,<svg><style><animate>
. Something with Glean seems a bit delayed. When I go to about:glean
and query the probe in DevTools console and type in Glean.parsing.svgStyleWithChildren.testGetValue()
with I do not immediately get the numerator above 0
. However, it shows up after some time. This is likely fine.
Assignee | ||
Comment 3•2 months ago
|
||
After conversations with Michał Bentkowski, it would be best if we would also check the behavior for the elements ['style', 'xmp', 'iframe', 'noembed', 'noframes', 'noscript', 'script']
, which might complicate the performance characteristics a bit more (though untested). I will look into it, once my build is no longer broken (bug 1913928)
Updated•2 months ago
|
Assignee | ||
Updated•2 months ago
|
Updated•2 months ago
|
Comment 5•2 months ago
|
||
bugherder |
Assignee | ||
Comment 6•2 months ago
|
||
Assignee | ||
Comment 7•2 months ago
|
||
With the 2% perf regression, I suppose this is going to be hard to ship.
How about we put this check behind a default-off pref wrapped in MOZ_UNLIKELY
and then I gather that data through an experiment.
Would that work for you, Henri?
Comment 8•2 months ago
|
||
Backed out because of the performance regression introduced Bug 1917623
Backout link: https://hg.mozilla.org/integration/autoland/rev/0379200384baa7de0caeb0658acd412f3f6979fa
Comment 9•2 months ago
|
||
(In reply to Frederik Braun [:freddy] from comment #7)
With the 2% perf regression, I suppose this is going to be hard to ship.
How about we put this check behind a default-off pref wrapped inMOZ_UNLIKELY
and then I gather that data through an experiment.Would that work for you, Henri?
Works for me, but I think it's worthwhile to check the perf impact of applying the namespace check that was missing and putting MOZ_UNLIKELY
around the namespace check and around taking action on the counter introduced here being non-zero. After all, the missing namespace check caused multiple pointer comparisons.
Updated•2 months ago
|
Comment 10•2 months ago
|
||
Assignee | ||
Updated•2 months ago
|
Comment 11•1 months ago
|
||
(In reply to Pulsebot from comment #10)
Pushed by fbraun@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c273d940e794
counting SVG unusual child elements r=hsivonen
This is causing a test failure ("toolkit/components/glean/tests/pytest/test_yaml_indices.py::test_yamls_sorted TEST-UNEXPECTED-FAIL") that was mistakenly stared against bug 1916532.
Comment 12•1 months ago
|
||
Comment 13•1 months ago
|
||
Comment 14•1 months ago
|
||
bugherder |
Comment 15•1 month ago
|
||
bugherder |
Comment hidden (Intermittent Failures Robot) |
Updated•1 month ago
|
Updated•1 month ago
|
Comment 19•1 month ago
|
||
(In reply to Cristian Tuns from comment #8)
Backed out because of the performance regression introduced Bug 1917623
Backout link: https://hg.mozilla.org/integration/autoland/rev/0379200384baa7de0caeb0658acd412f3f6979fa
Perfherder has detected a browsertime performance change from push 0379200384baa7de0caeb0658acd412f3f6979fa.
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) | Performance Profiles |
---|---|---|---|---|---|
11% | speedometer jQuery-TodoMVC/CompletingAllItems/Async | windows11-64-shippable-qr | fission webrender | 2.14 -> 1.91 | Before/After |
11% | speedometer jQuery-TodoMVC/CompletingAllItems/Async | windows11-64-nightlyasrelease-qr | fission webrender | 2.07 -> 1.85 | Before/After |
6% | speedometer jQuery-TodoMVC/DeletingAllItems/Async | windows11-64-shippable-qr | fission webrender | 0.54 -> 0.50 | Before/After |
4% | speedometer VanillaJS-TodoMVC/Adding100Items/Sync | windows11-64-nightlyasrelease-qr | fission webrender | 15.82 -> 15.13 | Before/After |
4% | speedometer jQuery-TodoMVC/CompletingAllItems/Async | macosx1400-64-shippable-qr | fission webrender | 1.19 -> 1.14 | Before/After |
... | ... | ... | ... | ... | ... |
2% | speedometer jQuery-TodoMVC/DeletingAllItems/Sync | windows11-64-nightlyasrelease-qr | fission webrender | 35.21 -> 34.50 | Before/After |
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.
If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.
You can run these tests on try with ./mach try perf --alert 2110
For more information on performance sheriffing please see our FAQ.
Description
•