1913382 - Count how common it is for SVG STYLE/XMP/etc. elements to contain children in the real world

Status: RESOLVED FIXED

(Core :: DOM: Security, task, P5)

Description

[Frederik Braun [:freddy]]

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)

Comment 1

[Frederik Braun [:freddy]]

Attachment: Bug 1913382 - count the rate of SVG elements with surprising children r=hsivonen

Comment 2

[Frederik Braun [:freddy]]

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.

Comment 3

[Frederik Braun [:freddy]]

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)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bec9af94668
count the rate of SVG elements with surprising children r=hsivonen

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0bec9af94668

Comment 6

[Frederik Braun [:freddy]]

Attachment: Bug 1913382 - counting SVG unusual child elements r=hsivonen

Comment 7

[Frederik Braun [:freddy]]

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

[Cristian Tuns]

Backed out because of the performance regression introduced Bug 1917623
Backout link: https://hg.mozilla.org/integration/autoland/rev/0379200384baa7de0caeb0658acd412f3f6979fa

Comment 9

[Henri Sivonen (:hsivonen)]

(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 in MOZ_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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c273d940e794
counting SVG unusual child elements r=hsivonen

Comment 11

[Florian Quèze [:florian]]

(In reply to Pulsebot from comment #10)

Pushed by [e-mail]:
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

[Florian Quèze [:florian]]

Attachment: Bug 1913382 - reorder toolkit/components/glean/metrics_index.py to have parser/html/metrics.yaml at the correct position, r=freddyb.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d0d5e032337
reorder toolkit/components/glean/metrics_index.py to have parser/html/metrics.yaml at the correct position, r=freddyb.

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/c273d940e794

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/6d0d5e032337

Comment 18

[Intermittent Failures Robot]

5 failures in 4448 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 5

Platform and build breakdown:

• windows11-64: 5
  • opt: 5

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1913382&startday=2024-09-09&endday=2024-09-15&tree=all

Comment 19

[Florin Bilt]

(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.