1473890 - Ensure malicious content processes cannot spoof animation ids for other processes

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

I was doing some code inspection to see what might result in stuck animations (bug 1452165). For the most part the code seems sane, but the CompositorAnimationStorage and WebRenderBridgeParent both use a 64-bit compositor animation id. This animation id is generated using a static counter on the content side. This means that different content processes could generate different animations with the same animation id, and they could get sent to the same CompositorAnimationStorage instance in the compositor, if they are in the same browser window. This seems problematic, although as far as I can see it would only result in animations getting clobbered or terminated early, rather than animations failing to get removed.

Still, something we should fix. I think a reasonable fix would be to take the animation id that arrives in the parent at [1] and combine it with the layers id of the WRBP before using it as the id.

[1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/gfx/layers/wr/WebRenderBridgeParent.cpp#879

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Oh whoops, I misread the code. The id already builds in the process id [1] so we shouldn't get collisions. But this should still be changed because it's a security hole - a malicious content process could send some other process id and clobber animations of other tabs.

[1] https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/gfx/layers/AnimationHelper.cpp#613

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1473890 - Verify that CompositorAnimations come from the content process that they claim. r?nical

MozReview-Commit-ID: AqcezlMQXE4

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 9005944 [details]
Bug 1473890 - Verify that CompositorAnimations come from the content process that they claim. r?nical

Nicolas Silva [:nical] has approved the revision.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ff28f2b711f
Verify that CompositorAnimations come from the content process that they claim. r=nical

Comment 5

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/1ff28f2b711f