Closed Bug 1673526 Opened 1 year ago Closed 11 months ago

Assertion failure: aTrack->GraphImpl() == GraphImpl(), at /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998

Categories

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

defect

Tracking

()

VERIFIED FIXED
86 Branch
Tracking Status
firefox-esr78 85+ fixed
firefox83 --- wontfix
firefox84 --- wontfix
firefox85 + fixed
firefox86 --- fixed

People

(Reporter: jkratzer, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, testcase, Whiteboard: [bugmon:bisected,confirmed][no-nag][adv-main85+r][adv-esr78.7+r])

Attachments

(3 files)

Attached file testcase.html

Testcase found while fuzzing mozilla-central rev b1a74943bc51 (built with --enable-debug).

Assertion failure: aTrack->GraphImpl() == GraphImpl(), at /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998

==1506437==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f1fdd8778f2 bp 0x7ffce9342720 sp 0x7ffce93426d0 T1506437)
==1506437==The signal is caused by a WRITE memory access.
==1506437==Hint: address points to the zero page.
    #0 0x7f1fdd8778f2 in mozilla::ProcessedMediaTrack::AllocateInputPort(mozilla::MediaTrack*, unsigned short, unsigned short) /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998:5
    #1 0x7f1fddd6abff in mozilla::MediaPipelineTransmit::SetTrack(RefPtr<mozilla::dom::MediaStreamTrack>) /builds/worker/checkouts/gecko/dom/media/webrtc/transportbridge/MediaPipeline.cpp:1043:31
    #2 0x7f1fddc6a7b0 in mozilla::TransceiverImpl::UpdateSendTrack(mozilla::dom::MediaStreamTrack*) /builds/worker/checkouts/gecko/dom/media/webrtc/jsapi/TransceiverImpl.cpp:182:29
    #3 0x7f1fddc6a505 in mozilla::PeerConnectionImpl::ReplaceTrackNoRenegotiation(mozilla::TransceiverImpl&, mozilla::dom::MediaStreamTrack*) /builds/worker/checkouts/gecko/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp:1835:30
    #4 0x7f1fdc39a622 in ReplaceTrackNoRenegotiation /builds/worker/checkouts/gecko/dom/media/webrtc/jsapi/PeerConnectionImpl.h:291:10
    #5 0x7f1fdc39a622 in mozilla::dom::PeerConnectionImpl_Binding::replaceTrackNoRenegotiation(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/PeerConnectionImplBinding.cpp:588:24
    #6 0x7f1fdd05a78a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3229:13
    #7 0x7f1fdffea001 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:506:13
    #8 0x7f1fdffe9718 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:598:12
    #9 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #10 0x7f1fdffdf083 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:667:10
    #11 0x7f1fdffdf083 in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3336:16
    #12 0x7f1fdffd61f4 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:476:13
    #13 0x7f1fdffe96e9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:635:13
    #14 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #15 0x7f1fdffeb51f in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:680:8
    #16 0x7f1fe05d690b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jsapi.cpp:2829:10
    #17 0x7f1fdc472b8e in mozilla::dom::RTCRtpSenderJSImpl::SetTrack(mozilla::dom::MediaStreamTrack*, mozilla::ErrorResult&, JS::Realm*) /builds/worker/workspace/obj-build/dom/bindings/RTCRtpSenderBinding.cpp:3435:8
    #18 0x7f1fdc4e631b in SetTrack /builds/worker/workspace/obj-build/dom/bindings/RTCRtpSenderBinding.cpp:3757:17
    #19 0x7f1fdc4e631b in mozilla::dom::RTCRtpSender_Binding::setTrack(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/RTCRtpSenderBinding.cpp:2451:24
    #20 0x7f1fdd05a78a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3229:13
    #21 0x7f1fdffea001 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:506:13
    #22 0x7f1fdffe9718 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:598:12
    #23 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #24 0x7f1fdffdf083 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:667:10
    #25 0x7f1fdffdf083 in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3336:16
    #26 0x7f1fdffd61f4 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:476:13
    #27 0x7f1fdffe96e9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:635:13
    #28 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #29 0x7f1fdffeb51f in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:680:8
    #30 0x7f1fe05d690b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jsapi.cpp:2829:10
    #31 0x7f1fdc44ad93 in mozilla::dom::RTCPeerConnectionJSImpl::AddTrack(mozilla::dom::MediaStreamTrack&, nsTArray<mozilla::OwningNonNull<mozilla::DOMMediaStream> > const&, mozilla::ErrorResult&, JS::Realm*) /builds/worker/workspace/obj-build/dom/bindings/RTCPeerConnectionBinding.cpp:7480:8
    #32 0x7f1fdc4cb0ef in AddTrack /builds/worker/workspace/obj-build/dom/bindings/RTCPeerConnectionBinding.cpp:10000:17
    #33 0x7f1fdc4cb0ef in mozilla::dom::RTCPeerConnection_Binding::addTrack(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/RTCPeerConnectionBinding.cpp:3592:79
    #34 0x7f1fdd05a78a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3229:13
    #35 0x7f1fdffea001 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:506:13
    #36 0x7f1fdffe9718 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:598:12
    #37 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #38 0x7f1fdffdf083 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:667:10
    #39 0x7f1fdffdf083 in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3336:16
    #40 0x7f1fdffd61f4 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:476:13
    #41 0x7f1fdffe96e9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:635:13
    #42 0x7f1fdffeb2e3 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:663:10
    #43 0x7f1fe09b5365 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jit/BaselineIC.cpp:2997:10
    #44 0x123ef89b3582  (<unknown module>)

UndefinedBehaviorSanitizer can not provide additional info.
SUMMARY: UndefinedBehaviorSanitizer: SEGV /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:2998:5 in mozilla::ProcessedMediaTrack::AllocateInputPort(mozilla::MediaTrack*, unsigned short, unsigned short)
Flags: in-testsuite?

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20201027044126-46a0e993f8bb.
The bug appears to have been introduced in the following build range:

Start: f02f44b9f7a51da2123fdf7e892fe4aa6cbd92b6 (20191212223426)
End: 172a5c2edd6e46ad5863ca043d7c6af2bdd92e20 (20191212225453)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f02f44b9f7a51da2123fdf7e892fe4aa6cbd92b6&tochange=172a5c2edd6e46ad5863ca043d7c6af2bdd92e20

Whiteboard: [bugmon:confirm] → [bugmon:bisected,confirmed]
Component: Audio/Video → Audio/Video: MediaStreamGraph

A Pernosco session is available here: https://pernos.co/debug/LqQGv-en1n3BifwswlU3mQ/index.html

Flags: needinfo?(apehrson)
Group: core-security, media-core-security
Severity: normal → S2
Keywords: csectype-uaf
Priority: -- → P1
Group: core-security
Keywords: sec-high

While it would be possible to make tracks work cross-frame, we probably just want a safe failure for now.

Assignee: nobody → apehrson

This is a regression from bug 1604746.

In MediaPipelineTransmit, we check whether mDomTrack and aDomTrack are both live and reside in different graphs, and if they are we re-create mSendTrack. Then we set up an input port from aDomTrack to mSendTrack.

mDomTrack and mSendTrack are not set in sync. For instance, replaceTrack(null) will unset mDomTrack but keep mSendTrack around, presumably so it can continue sending silence.

We should in the cross-graph check check the graphs of aDomTrack and mSendTrack. mDomTrack is irrelevant.

Flags: needinfo?(apehrson)
Status: NEW → ASSIGNED

I filed bug 1680007 to do some followup cleaning of things I've seen during this investigation.

(In reply to Andreas Pehrson [:pehrsons] from comment #4)

This is a regression from bug 1604746.

I should add some nuance here.

This is not a regression from bug 1604746 per se. Bug 1604746 fixed a larger issue of connecting tracks across graphs but missed a corner case that is showing up here.

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: From this patch, it would be a serious effort of backtracking code, but plausible. The tests make it obvious how to trigger the assertion failure in debug builds, but I haven't been able to make them cause a UAF in non-debug builds. As they stand they're racy, but likely need more massaging to trigger a UAF.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • 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?: They go clean, modulo a minimal conflict in the crashtest manifest for esr78.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely. No threading changes; no timing changes.
Attachment #9190593 - Flags: sec-approval?
Attachment #9190592 - Flags: sec-approval?

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

sec-approved to land the patch on Jan 4

Attachment #9190593 - Flags: sec-approval? → sec-approval+

Comment on attachment 9190592 [details]
Bug 1673526 - Add crashtests. r?bwc

sec-approved to land the test after March 1

Attachment #9190592 - Flags: sec-approval? → sec-approval+
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed][land after 2021-01-04]
Whiteboard: [bugmon:bisected,confirmed][land after 2021-01-04] → [bugmon:bisected,confirmed][land after 2021-01-04][no-nag]
Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

Beta/Release Uplift Approval Request

  • User impact if declined: There's a risk for a UAF.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simple change only affecting logic.
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: There's a risk for a UAF.
  • Fix Landed on Version: 86
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simple change only affecting logic.
  • String or UUID changes made by this patch:
Attachment #9190593 - Flags: approval-mozilla-esr78?
Attachment #9190593 - Flags: approval-mozilla-beta?

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

approved for 85.0b7

Attachment #9190593 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9190593 [details]
Bug 1673526 - Simplify send track recreation condition. r?bwc

Approved for 78.7esr.

Attachment #9190593 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Whiteboard: [bugmon:bisected,confirmed][land after 2021-01-04][no-nag] → [bugmon:bisected,confirmed][no-nag]
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [bugmon:bisected,confirmed][no-nag] → [bugmon:bisected,confirmed][no-nag][adv-main85+r]
Whiteboard: [bugmon:bisected,confirmed][no-nag][adv-main85+r] → [bugmon:bisected,confirmed][no-nag][adv-main85+r][adv-esr78.7+r]

Would be great to get the crashtests patch into beta and ESR78, thanks.

Flags: in-testsuite? → in-testsuite+
Whiteboard: [bugmon:bisected,confirmed][no-nag][adv-main85+r][adv-esr78.7+r] → [bugmon:bisected,confirmed][no-nag][adv-main85+r][adv-esr78.7+r][checkin-needed-beta]
Whiteboard: [bugmon:bisected,confirmed][no-nag][adv-main85+r][adv-esr78.7+r][checkin-needed-beta] → [bugmon:bisected,confirmed][no-nag][adv-main85+r][adv-esr78.7+r]
Group: core-security-release

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210107163634-81e34ce5a390.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.