Closed Bug 1561484 Opened 3 years ago Closed 3 years ago

Benchmark code doesn't keep MediaInfo object alive until execution ends.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 69+ fixed
firefox68 --- wontfix
firefox69 + fixed
firefox70 + fixed

People

(Reporter: tsmith, Assigned: jya)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main69+][adv-esr68.1+])

Attachments

(3 files)

Attached file full_log.txt

This crash was found using decoders new libfuzzer media fuzzing interface (bug 1465407)

==125399==ERROR: AddressSanitizer: heap-use-after-free on address 0x6110000c02c8 at pc 0x7fba744556df bp 0x7fb9ce593850 sp 0x7fb9ce593848
READ of size 8 at 0x6110000c02c8 thread T18 (MediaPD~oder #1)
    #0 0x7fba744556de in get objdir-ff-fuzzing/dist/include/mozilla/RefPtr.h:268:27
    #1 0x7fba744556de in operator-> objdir-ff-fuzzing/dist/include/mozilla/RefPtr.h:298
    #2 0x7fba744556de in mozilla::OpusDataDecoder::Init() dom/media/platforms/agnostic/OpusDecoder.cpp:62
    #3 0x7fba744ba790 in operator() dom/media/platforms/wrappers/AudioTrimmer.cpp:24:56
    #4 0x7fba744ba790 in mozilla::detail::ProxyFunctionRunnable<mozilla::AudioTrimmer::Init()::$_34, mozilla::MozPromise<mozilla::TrackInfo::TrackType, mozilla::MediaResult, true> >::Run() objdir-ff-fuzzing/dist/include/mozilla/MozPromise.h:1420
    #5 0x7fba6b00c7ab in mozilla::TaskQueue::Runner::Run() xpcom/threads/TaskQueue.cpp:199:12
    #6 0x7fba6b0500f4 in nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:244:14
    #7 0x7fba6b0510a4 in non-virtual thunk to nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp
    #8 0x7fba6b043d40 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1225:14
    #9 0x7fba6b04b644 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:486:10
    #10 0x7fba6c69379e in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:303:20
    #11 0x7fba6c502b9e in RunInternal ipc/chromium/src/base/message_loop.cc:315:10
    #12 0x7fba6c502b9e in RunHandler ipc/chromium/src/base/message_loop.cc:308
    #13 0x7fba6c502b9e in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:290
    #14 0x7fba6b03b718 in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:459:11
    #15 0x7fba92635f48 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:201:5
    #16 0x7fba922666da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
    #17 0x7fba9124488e in clone /build/glibc-OTsEL5/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Attached video testcase.webm

This file will likely not reproduce the crash on a regular build. The libfuzzer fuzzing interface should be used to verify this bug. Feel free to ping me if needed.

The allocation and free stacks look the same as bug 1561490 -- could be same underlying bug?

Keywords: sec-high

In case these are related as per comment #2 it probably makes sense for you Alastor to look into this one as well.

Assignee: nobody → alwu
Flags: needinfo?(alwu)
Priority: -- → P1

Hi, Tyson,
As I couldn't reproduce this crash on my own build, may I know how to reproduce it and the crash in bug 1561490?
Thank you.

Flags: needinfo?(alwu) → needinfo?(twsmith)

Hey Alastor, this uses the libfuzzer fuzzing interface[1]. To repro apply the patches from bug 1465407 and bug 1561256 and then follow the fuzzing build instructions[1]. Once you have the build to repro run MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=MediaOgg ./firefox <testcase>

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Fuzzing_Interface

Flags: needinfo?(twsmith)

(In reply to Daniel Veditz [:dveditz] from comment #2)

The allocation and free stacks look the same as bug 1561490 -- could be same underlying bug?

I believe it is.

Incorrect usage of the MediaDataDecoder.

I think the issue is in the Benchmark code.

I lower the priority however as this is not a problem that can occur in the wild.

Assignee: alwu → jyavenard
Priority: P1 → P3
Duplicate of this bug: 1561490
Summary: heap-use-after-free in [@ mozilla::OpusDataDecoder::Init] → Benchmark code doesn't keep MediaInfo object alive until execution ends.

Comment on attachment 9075300 [details]
Bug 1561484 - Keep MediaInfo object for entire benchmark. r?alwu

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: hard to tell. This code is typically not used in such fashion. It's only used with a set, static content that we control.

The new fuzzer access the underlying C++ code directly.

  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: 65
  • 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?: should apply for all branches.
  • How likely is this patch to cause regressions; how much testing does it need?: no possibility of regression
Attachment #9075300 - Flags: sec-approval?

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Not a regression, but a problem caused by enabling an internal interface for fuzz testing only.

Keywords: regression
Attachment #9075300 - Flags: sec-approval? → sec-approval+

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Priority: P3 → P1

(In reply to Nils Ohlmeier [:drno] from comment #11)

Not a regression, but a problem caused by enabling an internal interface for fuzz testing only.

Actually, the bug exists all the time. It's just quite hard to exploit as we only use it with our own static content (the VP9 sample).

Group: media-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Please request Beta and ESR68 approval on this when you get a chance. It grafts cleanly to both as-landed.

Flags: needinfo?(jyavenard)

Comment on attachment 9075300 [details]
Bug 1561484 - Keep MediaInfo object for entire benchmark. r?alwu

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: data race leading to use after free.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We keep the object alive until it's no longer used
  • String or UUID changes made by this patch: none

Beta/Release Uplift Approval Request

  • User impact if declined: data race leading to use after free.
  • 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): We keep the object alive until it's no longer used
  • String changes made/needed: none
Flags: needinfo?(jyavenard)
Attachment #9075300 - Flags: approval-mozilla-esr68?
Attachment #9075300 - Flags: approval-mozilla-beta?

Comment on attachment 9075300 [details]
Bug 1561484 - Keep MediaInfo object for entire benchmark. r?alwu

Fixes a sec bug. Approved for 69.0b5 and 68.1esr.

Attachment #9075300 - Flags: approval-mozilla-esr68?
Attachment #9075300 - Flags: approval-mozilla-esr68+
Attachment #9075300 - Flags: approval-mozilla-beta?
Attachment #9075300 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main69+][adv-esr68.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.