1920800 - (CVE-2024-10463) Possible early principal change on video stream

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Karl Tomlinson (:karlt)]

When an existing set of images (oldImages) has last frame with ID matching mFrameIDForPendingPrincipalHandle - 1, VideoFrameContainer::SetCurrentFrames() will notify PrincipalHandleChangedForVideoFrameContainer() even when a replacement set of frames (aFrames) contains a frame with the same ID.
This is earlier than the intended "when all FrameIDs prior to aFrameID have been flushed out."

Having replacement frame sets with some of the some frames from the previous set seems plausible because the addition of new frames does not usually directly expire all old frames. Overlapping frame sets are usually expected by VideoFrameContainer and ImageContainer so that the compositor can choose the presentation timing.

Comment 1

[Karl Tomlinson (:karlt)]

This looks like it could provide content access to a cross origin video frame. Perhaps factors such as timestamps on frames might make such access difficult, but content can slow frame rates to give it a better chance.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Bug 1920800 Remove unnecessary ClearCurrentFrame() r?pehrsons

This was never called because SetCurrentFrame() callers all pass non-null
aImage, but the logic is retained.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Bug 1920800 Remove oldImages from check for flushed old principal handle r?pehrsons

The logic should be equivalent when frame IDs on aImages are contiguously
increasing from IDs on previously set frames.

There are currently no callers passing empty aImages, but the logic in this
case would now be as described in the documentation of
UpdatePrincipalHandleForFrameID():

We will notify mElement that aPrincipalHandle has been applied when all
FrameIDs prior to aFrameID have been flushed out.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1920800 Test VideoFrameContainer.PrincipalHandleChangedForVideoFrameContainer() timing r?pehrsons

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a76ec056139f
Remove unnecessary ClearCurrentFrame() r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/91eddc0bb6d9
Remove oldImages from check for flushed old principal handle r=pehrsons

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a76ec056139f
https://hg.mozilla.org/mozilla-central/rev/91eddc0bb6d9

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Bug 1920800 Remove oldImages from check for flushed old principal handle

The logic should be equivalent when frame IDs on aImages are contiguously
increasing from IDs on previously set frames.

There are currently no callers passing empty aImages, but the logic in this
case would now be as described in the documentation of
UpdatePrincipalHandleForFrameID():

We will notify mElement that aPrincipalHandle has been applied when all
FrameIDs prior to aFrameID have been flushed out.

Original Revision: https://phabricator.services.mozilla.com/D223922

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: Bug 1920800 Remove oldImages from check for flushed old principal handle

The logic should be equivalent when frame IDs on aImages are contiguously
increasing from IDs on previously set frames.

There are currently no callers passing empty aImages, but the logic in this
case would now be as described in the documentation of
UpdatePrincipalHandleForFrameID():

We will notify mElement that aPrincipalHandle has been applied when all
FrameIDs prior to aFrameID have been flushed out.

Original Revision: https://phabricator.services.mozilla.com/D223922

Comment 9

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: leak of video content across origins
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: small patch with local influence, affecting only an unusal situation: video content from more than one domain
• String changes made/needed: none
• Is Android affected?: yes

Comment 10

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: leak of video content across origins
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: small patch with local influence, affecting only an unusal situation: video content from more than one domain
• String changes made/needed: none
• Is Android affected?: yes

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/2b43c0cb7337

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/77fb166286ed

Comment 13

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a6ea80fcf22
Test VideoFrameContainer.PrincipalHandleChangedForVideoFrameContainer() timing r=pehrsons

Comment 15

[agoloman]

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