1632717 - Potential UaF in MediaEncryptedEvent::Constructor() and MediaKeyMessageEvent::Constructor()

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

MediaEncryptedEvent::Constructor() calls ArrayBuffer::ComputeState() to initialize a pointer for Data() before using JS::NewArrayBuffer(), which presumably can trigger GC, and then using the pointer.

MediaKeyMessageEvent::Constructor() does similarly.

+++ This bug was initially created as a clone of Bug #1626382 +++

Comment 1

[Paul Adenot (:padenot)]

Bryce, for context: https://bugzilla.mozilla.org/show_bug.cgi?id=1626382#c6 (I've cc-ed you).

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Thanks for auditing, the heads up, and the context. That's quite the footgun.

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1632717 - r?karlt,padenot

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 9143252 [details]
Bug 1632717 - r?karlt,padenot

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unclear. The code path being updated involves synthetic events. A savvy attacker may be able to figure out what is being fixed by the patches. However, I'm not clear how easily an attacker can trigger GCs to cause the fault we're concerned about. It's unclear if the actual issue we're worried about can take place based on discussion on the patch, but I'd prefer to be safe.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I think this change should graft cleanly. No higher risk than taking the patch in nightly as the change is small and to code that has not changed in some time.
• How likely is this patch to cause regressions; how much testing does it need?: Low chance -- the change is small. We have a small amount of coverage around the synthetic event construction and more around the non-synthetic version of these events. We have no tests for the specific sec issue we're concerned about here, though I imagine one could be constructed if the issue exists.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 9143252 [details]
Bug 1632717 - r?karlt,padenot

sec-approval to land on nightly. Later on we'll need to uplift this to the ESR branch, but at least a week after the merge.

Comment 6

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

https://hg.mozilla.org/integration/autoland/rev/bbf0b2e8dde29518d3bccb09ee64a9365e401387
https://hg.mozilla.org/mozilla-central/rev/bbf0b2e8dde2

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR68 approval when you get a chance.

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 9143252 [details]
Bug 1632717 - r?karlt,padenot

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec high.
• User impact if declined: Users could be exposed to a UAF if using some synthetic events relating to EME.
• Fix Landed on Version: 77
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is relatively small and has been been landed since 77 without issue.
• String or UUID changes made by this patch: None.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9143252 [details]
Bug 1632717 - r?karlt,padenot

approved for 68.9esr

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/239cd9654b8659b22a751f8018c93e32873f3e62

There were conflicts due to ComputeState vs ComputeLengthAndData, hopefully I didn't mess that up.