Closed Bug 1768559 Opened 2 years ago Closed 2 years ago

AddressSanitizer: heap-buffer-overflow in [@ mozilla::SipccSdp::GetMediaSection]

Categories

(Core :: WebRTC: Signaling, defect, P2)

defect

Tracking

()

VERIFIED FIXED
102 Branch
Tracking Status
firefox-esr91 101+ fixed
firefox100 --- wontfix
firefox101 + fixed
firefox102 + verified

People

(Reporter: tsmith, Assigned: bwc)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [bugmon:bisected,confirmed][adv-main101+r][adv-esr91.10+r])

Crash Data

Attachments

(2 files, 1 obsolete file)

Found while fuzzing m-c 20220509-23c5e3bc5a1b (--enable-address-sanitizer --enable-fuzzing)

A test case is currently being reduced. It will be attached once reduction is complete.

==23743==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000983f8 at pc 0x7f873586c644 bp 0x7ffeb324d0f0 sp 0x7ffeb324d0e8
READ of size 8 at 0x6020000983f8 thread T0 (Isolated Web Co)
    #0 0x7f873586c643 in get /builds/worker/workspace/obj-build/dist/include/mozilla/UniquePtr.h:290:32
    #1 0x7f873586c643 in operator* /builds/worker/workspace/obj-build/dist/include/mozilla/UniquePtr.h:281:13
    #2 0x7f873586c643 in mozilla::SipccSdp::GetMediaSection(unsigned long) /gecko/dom/media/webrtc/sdp/SipccSdp.cpp:42:10
    #3 0x7f873575e47c in mozilla::JsepSessionImpl::CheckNegotiationNeeded() const /gecko/dom/media/webrtc/jsep/JsepSessionImpl.cpp:2442:35
    #4 0x7f87356c2764 in operator() /gecko/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp:1302:14
    #5 0x7f87356c2764 in mozilla::detail::RunnableFunction<mozilla::PeerConnectionImpl::UpdateNegotiationNeeded()::$_75>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:531:5
    #6 0x7f872eb0c302 in mozilla::RunnableTask::Run() /gecko/xpcom/threads/TaskController.cpp:467:16
    #7 0x7f872ead2355 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /gecko/xpcom/threads/TaskController.cpp:780:26
    #8 0x7f872eacf508 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /gecko/xpcom/threads/TaskController.cpp:612:15
    #9 0x7f872eacfc30 in mozilla::TaskController::ProcessPendingMTTask(bool) /gecko/xpcom/threads/TaskController.cpp:390:36
    #10 0x7f872eb14e51 in operator() /gecko/xpcom/threads/TaskController.cpp:124:37
    #11 0x7f872eb14e51 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:531:5
    #12 0x7f872eaf2cc7 in nsThread::ProcessNextEvent(bool, bool*) /gecko/xpcom/threads/nsThread.cpp:1180:16
    #13 0x7f872eafce2c in NS_ProcessNextEvent(nsIThread*, bool) /gecko/xpcom/threads/nsThreadUtils.cpp:465:10
    #14 0x7f87301fa41f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /gecko/ipc/glue/MessagePump.cpp:85:21
    #15 0x7f8730071f01 in RunInternal /gecko/ipc/chromium/src/base/message_loop.cc:380:10
    #16 0x7f8730071f01 in RunHandler /gecko/ipc/chromium/src/base/message_loop.cc:373:3
    #17 0x7f8730071f01 in MessageLoop::Run() /gecko/ipc/chromium/src/base/message_loop.cc:355:3
    #18 0x7f8736ed2d17 in nsBaseAppShell::Run() /gecko/widget/nsBaseAppShell.cpp:137:27
    #19 0x7f873bd8ca3f in XRE_RunAppShell() /gecko/toolkit/xre/nsEmbedFunctions.cpp:874:20
    #20 0x7f8730071f01 in RunInternal /gecko/ipc/chromium/src/base/message_loop.cc:380:10
    #21 0x7f8730071f01 in RunHandler /gecko/ipc/chromium/src/base/message_loop.cc:373:3
    #22 0x7f8730071f01 in MessageLoop::Run() /gecko/ipc/chromium/src/base/message_loop.cc:355:3
    #23 0x7f873bd8bbeb in XRE_InitChildProcess(int, char**, XREChildData const*) /gecko/toolkit/xre/nsEmbedFunctions.cpp:733:34
    #24 0x563c83269b9d in content_process_main(mozilla::Bootstrap*, int, char**) /gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #25 0x563c83269fd0 in main /gecko/browser/app/nsBrowserApp.cpp:327:18
    #26 0x7f87545ba0b2 in __libc_start_main /build/glibc-sMfBJT/glibc-2.31/csu/../csu/libc-start.c:308:16
    #27 0x563c831a9fe9 in _start (/home/worker/builds/m-c-20220509094710-fuzzing-asan-opt/firefox+0x5efe9) (BuildId: ac054a69eec57b4cf68c807e5f26ffc4a9d13251)

0x6020000983f8 is located 0 bytes to the right of 8-byte region [0x6020000983f0,0x6020000983f8)
allocated by thread T0 (Isolated Web Co) here:
    #0 0x563c8322c50e in __interceptor_malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0x563c8327028d in moz_xmalloc /gecko/memory/mozalloc/mozalloc.cpp:52:15
    #2 0x7f873588f7a6 in operator new /builds/worker/workspace/obj-build/dist/include/mozilla/cxxalloc.h:33:10
    #3 0x7f873588f7a6 in allocate /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/ext/new_allocator.h:111:27
    #4 0x7f873588f7a6 in allocate /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/alloc_traits.h:436:20
    #5 0x7f873588f7a6 in _M_allocate /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/stl_vector.h:172:20
    #6 0x7f873588f7a6 in void std::vector<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> >, std::allocator<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> > > >::_M_realloc_insert<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> > >(__gnu_cxx::__normal_iterator<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> >*, std::vector<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> >, std::allocator<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> > > > >, mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> >&&) /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/vector.tcc:406:33
    #7 0x7f873588f67b in mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> >& std::vector<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> >, std::allocator<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> > > >::emplace_back<mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> > >(mozilla::UniquePtr<mozilla::SipccSdpMediaSection, mozilla::DefaultDelete<mozilla::SipccSdpMediaSection> >&&) /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/vector.tcc:105:4
    #8 0x7f873586d923 in push_back /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/stl_vector.h:954:9
    #9 0x7f873586d923 in mozilla::SipccSdp::Load(sdp_t*, mozilla::SdpParser::InternalResults&) /gecko/dom/media/webrtc/sdp/SipccSdp.cpp:115:20
    #10 0x7f8735882bf9 in mozilla::SipccSdpParser::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /gecko/dom/media/webrtc/sdp/SipccSdpParser.cpp:75:28
    #11 0x7f8735832cdc in mozilla::HybridSdpParser::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /gecko/dom/media/webrtc/sdp/HybridSdpParser.cpp:55:28
    #12 0x7f873573f662 in mozilla::JsepSessionImpl::ParseSdp(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, mozilla::UniquePtr<mozilla::Sdp, mozilla::DefaultDelete<mozilla::Sdp> >*) /gecko/dom/media/webrtc/jsep/JsepSessionImpl.cpp:1355:27
    #13 0x7f873573b6cc in mozilla::JsepSessionImpl::SetLocalDescription(mozilla::JsepSdpType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /gecko/dom/media/webrtc/jsep/JsepSessionImpl.cpp:781:17
    #14 0x7f87356001ca in mozilla::PeerConnectionImpl::SetLocalDescription(int, char const*) /gecko/dom/media/webrtc/jsapi/PeerConnectionImpl.cpp:1512:21
    #15 0x7f8732786e98 in mozilla::PeerConnectionImpl::SetLocalDescription(int, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /gecko/dom/media/webrtc/jsapi/PeerConnectionImpl.h:246:10
    #16 0x7f8732786af2 in mozilla::dom::PeerConnectionImpl_Binding::setLocalDescription(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/PeerConnectionImplBinding.cpp:281:24
    #17 0x7f8733a9f3bd in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /gecko/dom/bindings/BindingUtils.cpp:3271:13
    #18 0x7f873daa31d4 in CallJSNative /gecko/js/src/vm/Interpreter.cpp:420:13
    #19 0x7f873daa31d4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:507:12
    #20 0x7f873da904be in InternalCall /gecko/js/src/vm/Interpreter.cpp:574:10
    #21 0x7f873da904be in CallFromStack /gecko/js/src/vm/Interpreter.cpp:578:10
    #22 0x7f873da904be in Interpret(JSContext*, js::RunState&) /gecko/js/src/vm/Interpreter.cpp:3314:16
    #23 0x7f873da75041 in js::RunScript(JSContext*, js::RunState&) /gecko/js/src/vm/Interpreter.cpp:389:13
    #24 0x7f873daa330f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:539:13
    #25 0x7f873daa4e9a in InternalCall /gecko/js/src/vm/Interpreter.cpp:574:10
    #26 0x7f873daa4e9a in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:605:8
    #27 0x7f873c43554e in js::PromiseObject::create(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool) /gecko/js/src/builtin/Promise.cpp:2869:15
    #28 0x7f873c47ff1e in PromiseConstructor(JSContext*, unsigned int, JS::Value*) /gecko/js/src/builtin/Promise.cpp:2780:7
    #29 0x7f873daa571f in CallJSNative /gecko/js/src/vm/Interpreter.cpp:420:13
    #30 0x7f873daa571f in CallJSNativeConstructor /gecko/js/src/vm/Interpreter.cpp:436:8
    #31 0x7f873daa571f in InternalConstruct(JSContext*, js::AnyConstructArgs const&) /gecko/js/src/vm/Interpreter.cpp:633:14
    #32 0x7f873da9046d in Interpret(JSContext*, js::RunState&) /gecko/js/src/vm/Interpreter.cpp:3304:16
    #33 0x7f873da75041 in js::RunScript(JSContext*, js::RunState&) /gecko/js/src/vm/Interpreter.cpp:389:13
    #34 0x7f873daa330f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:539:13
    #35 0x7f873daa4e9a in InternalCall /gecko/js/src/vm/Interpreter.cpp:574:10
    #36 0x7f873daa4e9a in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:605:8
    #37 0x7f873c570a0c in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /gecko/js/src/vm/SelfHosting.cpp:1590:10
    #38 0x7f873cf6ba44 in js::jit::InterpretResume(JSContext*, JS::Handle<JSObject*>, JS::Value*, JS::MutableHandle<JS::Value>) /gecko/js/src/jit/VMFunctions.cpp:1093:10
    #39 0x7f86a09379df  (<unknown module>)
    #40 0x7f86a093456e  (<unknown module>)
    #41 0x7f873d4ca100 in EnterJit /gecko/js/src/jit/Jit.cpp:107:5
    #42 0x7f873d4ca100 in js::jit::MaybeEnterJit(JSContext*, js::RunState&) /gecko/js/src/jit/Jit.cpp:205:10

Not exactly sure how this happens, but I can add a check that asserts on debug, and does something safe on non-debug, and we can then figure out the root cause.

Assignee: nobody → docfaraday
Severity: S2 → S1
Priority: -- → P1

Per Slack discussion with bwc, moving this back to S2.

Severity: S1 → S2
Priority: P1 → --
Priority: -- → P2

This looks sec-high to me.

Keywords: sec-high
Attached file testcase.html (obsolete) —
Flags: in-testsuite?
Keywords: bugmon, testcase

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220510213954-0a4cbd771ad2.
The bug appears to have been introduced in the following build range:

Start: 11fde2629eacf0f130e7123e3c2984b4ac921bd9 (20210622113140)
End: e11836c2c03d8b50de930730d6c0e62c25db58e4 (20210622140333)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=11fde2629eacf0f130e7123e3c2984b4ac921bd9&tochange=e11836c2c03d8b50de930730d6c0e62c25db58e4

Keywords: regression
Whiteboard: [bugmon:bisected,confirmed]

(In reply to Bugmon [:jkratzer for issues] from comment #7)

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220510213954-0a4cbd771ad2.
The bug appears to have been introduced in the following build range:

Start: 11fde2629eacf0f130e7123e3c2984b4ac921bd9 (20210622113140)
End: e11836c2c03d8b50de930730d6c0e62c25db58e4 (20210622140333)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=11fde2629eacf0f130e7123e3c2984b4ac921bd9&tochange=e11836c2c03d8b50de930730d6c0e62c25db58e4

Testcase uses window.clientInformation, so this is probably a red herring given bug 1717072 in that range. But at least that confirms that ESR is likely also affected.

:bwc, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Whiteboard: [bugmon:bisected,confirmed] → [confirmed]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220511094252-87443e31b7c0.
The bug appears to have been introduced in the following build range:

Start: 11fde2629eacf0f130e7123e3c2984b4ac921bd9 (20210622113140)
End: e11836c2c03d8b50de930730d6c0e62c25db58e4 (20210622140333)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=11fde2629eacf0f130e7123e3c2984b4ac921bd9&tochange=e11836c2c03d8b50de930730d6c0e62c25db58e4

Whiteboard: [confirmed] → [confirmed][bugmon:bisected,confirmed]

sigh Not sure how to prevent bugmon from setting this spurious regression range.

We can just remove bugmon for now.

Keywords: bugmon
Whiteboard: [confirmed][bugmon:bisected,confirmed]

If we want to try getting a better regression range, can we try modifying the testcase to use window.navigator instead?

Flags: needinfo?(twsmith)
Attached file testcase.html

Updated test case use of self.clientInformation.mediaDevices -> self.navigator.mediaDevices.

Attachment #9275920 - Attachment is obsolete: true
Flags: needinfo?(twsmith)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

If we want to try getting a better regression range, can we try modifying the testcase to use window.navigator instead?

Let's see what bugmon says now.

Keywords: bugmon

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220512094957-2f240882d907.
Unable to bisect testcase (Testcase reproduces on start build!):

Start: e2e39e32a588dc6ac32d4d0d40640a820f4d3219 (20210513032648)
End: 23c5e3bc5a1bcf1472a73dc7fa85717770e39333 (20220509094710)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False)

Whiteboard: [bugmon:bisected,confirmed]

Comment on attachment 9275879 [details]
Bug 1768559: Do something safe in this weird corner case. r?mjf

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: It seems like it would be pretty tricky.
  • 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, it seems
  • 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?: Probably pretty easy.
  • How likely is this patch to cause regressions; how much testing does it need?: Extremely unlikely to result in regressions.
  • Is Android affected?: Unknown
Attachment #9275879 - Flags: sec-approval?

The testcase from comment 14 crashes a non-ASAN build with a null deref at [@ mozilla::JsepSessionImpl::CheckNegotiationNeeded ]
bp-f7396bea-5d14-4d8b-9243-6e8be0220516

Crash Signature: [@ mozilla::JsepSessionImpl::CheckNegotiationNeeded ]

Comment on attachment 9275879 [details]
Bug 1768559: Do something safe in this weird corner case. r?mjf

sec-approval = dveditz

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

Marking leave-open until I have cycles to delve into fixing the root cause.

Keywords: leave-open

Assuming we want to uplift the already-landed patch to Beta/ESR (please go ahead and nominate if yes!), it would probably be easier for tracking if we moved the root cause fix into a new bug set as a dependency on this one.

Flags: needinfo?(docfaraday)

Let me verify that the patch is indeed having the desired effect.

I bet that this bug has the same root cause as bug 1766668, and would also be fixed if we fixed bug 1769802.

Flags: needinfo?(docfaraday)

Comment on attachment 9275879 [details]
Bug 1768559: Do something safe in this weird corner case. r?mjf

Beta/Release Uplift Approval Request

  • User impact if declined:
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • 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):
  • String changes made/needed: None
  • Is Android affected?: Yes

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined:
  • Fix Landed on Version: 102
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
Attachment #9275879 - Flags: approval-mozilla-esr91?
Attachment #9275879 - Flags: approval-mozilla-beta?

Per discussion with Byron, we're going to go ahead and close this bug out with the assumption that bug 1769802 will handle any follow-up work.

Group: media-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Depends on: 1769802
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220519093729-ba856ffe455a.
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

Comment on attachment 9275879 [details]
Bug 1768559: Do something safe in this weird corner case. r?mjf

Approved for 101.0b9 and 91.10esr.

Attachment #9275879 - Flags: approval-mozilla-esr91?
Attachment #9275879 - Flags: approval-mozilla-esr91+
Attachment #9275879 - Flags: approval-mozilla-beta?
Attachment #9275879 - Flags: approval-mozilla-beta+
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed][adv-main101+r]
Whiteboard: [bugmon:bisected,confirmed][adv-main101+r] → [bugmon:bisected,confirmed][adv-main101+r][adv-esr91.10+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.