Closed Bug 1632717 Opened 9 months ago Closed 9 months ago

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


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




Tracking Status
firefox-esr68 77+ fixed
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 + fixed


(Reporter: karlt, Assigned: bryce)



(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main77+r][adv-esr68.9+r])


(1 file)

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 +++

Component: Web Audio → Audio/Video: Playback

Bryce, for context: (I've cc-ed you).

Flags: needinfo?(bvandyk)
Assignee: nobody → bvandyk
Flags: needinfo?(bvandyk)

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

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.
Attachment #9143252 - Flags: sec-approval?

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.

Attachment #9143252 - Flags: sec-approval? → sec-approval+
Group: media-core-security → core-security-release
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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

Flags: needinfo?(bvandyk)

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.
Flags: needinfo?(bvandyk)
Attachment #9143252 - Flags: approval-mozilla-esr68?

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

approved for 68.9esr

Attachment #9143252 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main77+r]
Whiteboard: [post-critsmash-triage][adv-main77+r] → [post-critsmash-triage][adv-main77+r][adv-esr68.9+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.