Closed Bug 1614971 (CVE-2020-6807) Opened 4 years ago Closed 4 years ago

Fix heap-use-after-free errors found by AddressSanitizer in cubeb-coreaudio

Categories

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

Product:
Core
Component:
Audio/Video: cubeb
Platform:
Desktop
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla75
Tracking Flags:
Tracking Status
firefox-esr68 74+ fixed
firefox73 --- wontfix
firefox74 + fixed
firefox75 + fixed

People

(Reporter: chunmin, Assigned: chunmin)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main74+][adv-esr68.6+])

Attachments

(6 files, 2 obsolete files)

UAF-demo.diff
3.93 KB, patch
Details | Diff | Splinter Review
UAF-demo-c.diff
2.66 KB, patch
Details | Diff | Splinter Review
Bug 1614971 - Revise messages in test_switch_device. r?padenot
47 bytes, text/x-phabricator-request
tjr
: sec-approval-
Details | Review
Bug 1614971 - Fix the UAF. r?padenot
47 bytes, text/x-phabricator-request
tjr
: approval-mozilla-esr68+
tjr
: sec-approval+
Details | Review
Bug 1614971 - Update cubeb-coreaudio to 4acd802. r?padenot
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review
advisory.txt
284 bytes, text/plain
Details
Blocks: 1530713
Blocks: 1594426
Blocks: 1595457
Group: media-core-security
Keywords: csectype-uaf

The errors are found by running RUSTFLAGS="-Z sanitizer=address" sh run_tests.sh:
There are 4 errors in total. The error happens when running:

  • test_switch_device
  • test_destroy_duplex_stream_after_unplugging_a_nondefault_input_device
  • test_destroy_duplex_stream_after_unplugging_a_default_input_device
  • test_destroy_duplex_stream_after_unplugging_a_default_output_device

test_switch_device

Run RUSTFLAGS="-Z sanitizer=address" cargo test test_switch_device -- --ignored --nocapture to see

...

test backend::tests::device_change::test_switch_device ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 183 filtered out

==4253==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000129db at pc 0x00010a5c5793 bp 0x7000004b91c0 sp 0x7000004b91b8
READ of size 1 at 0x6160000129db thread T6

test_destroy_duplex_stream_after_unplugging_a_nondefault_input_device

Run RUSTFLAGS="-Z sanitizer=address" cargo test test_destroy_duplex_stream_after_unplugging_a_nondefault_input_device -- --ignored --nocapture to see

...

running 1 test
Device change callback. data @ 0x603000011ff0
=================================================================
test backend::tests::device_change::test_destroy_duplex_stream_after_unplugging_a_nondefault_input_device ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 183 filtered out

==4268==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000010bdb at pc 0x000103ffe793 bp 0x700007cee1c0 sp 0x700007cee1b8
READ of size 1 at 0x616000010bdb thread T5

test_destroy_duplex_stream_after_unplugging_a_default_input_device

Run RUSTFLAGS="-Z sanitizer=address" cargo test test_destroy_duplex_stream_after_unplugging_a_default_input_device -- --ignored --nocapture to see:

...

running 1 test
Device change callback. data @ 0x603000013820
=================================================================
test backend::tests::device_change::test_destroy_duplex_stream_after_unplugging_a_default_input_device ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 183 filtered out

==4292==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000010bdb at pc 0x00010cc31793 bp 0x70000cd961c0 sp 0x70000cd961b8
READ of size 1 at 0x616000010bdb thread T6

test_destroy_duplex_stream_after_unplugging_a_default_output_device

Run RUSTFLAGS="-Z sanitizer=address" cargo test test_destroy_duplex_stream_after_unplugging_a_default_output_device -- --ignored --nocapture to see:

...

running 1 test
Device change callback. data @ 0x603000012110
=================================================================
test backend::tests::device_change::test_destroy_duplex_stream_after_unplugging_a_default_output_device ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 183 filtered out

==4315==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000010bdb at pc 0x0001082cf793 bp 0x7000031ef1c0 sp 0x7000031ef1b8
READ of size 1 at 0x616000010bdb thread T6

Got a more detail log for running RUSTFLAGS="-Z sanitizer=address" cargo test test_switch_device -- --ignored --nocapture with adding a delay on the end of the test (otherwise the stacktraces won't be printed, don't know why)

running 1 test

Switch default device for Output while the stream is working.

Output devices
--------------------
 204: AppleGFXHDAEngineOutputDP:10001:0:{C315-2684-05BF0E12}
	uid: EV2750
 194: AppleHDAEngineOutput:1F,3,0,1,1:0
	uid: Headphones
  41: SoundflowerEngine:0
	uid: Soundflower (2ch)
  52: SoundflowerEngine:1
	uid: Soundflower (64ch)

Switch device for Output: 194 -> 41
Switch device for Output: 41 -> 52
Switch device for Output: 52 -> 204
Switch device for Output: 204 -> 194
=================================================================
==7236==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000010bdb at pc 0x000106410723 bp 0x7000029fa1c0 sp 0x7000029fa1b8
READ of size 1 at 0x616000010bdb thread T6
    #0 0x106410722 in core::sync::atomic::atomic_load::h4dc415c6a3ce79a6 atomic.rs:2277
    #1 0x10647185c in core::sync::atomic::AtomicBool::load::hd6d654f0922c62d7 atomic.rs:404
    #2 0x1063fb16c in cubeb_coreaudio::backend::AudioUnitStream::reinit_async::_$u7b$$u7b$closure$u7d$$u7d$::hbea024c63054739e mod.rs:3270
    #3 0x1063082e2 in coreaudio_sys_utils::dispatch::create_closure_and_executor::closure_executer::haff5ead75c17e7a3 dispatch.rs:59
    #4 0x106c87441 in asan_dispatch_call_block_and_release (librustc_rt.asan.dylib:x86_64+0x41441)
    #5 0x7fff6e3f850d in _dispatch_client_callout (libdispatch.dylib:x86_64+0x350d)
    #6 0x7fff6e3fdacd in _dispatch_lane_serial_drain (libdispatch.dylib:x86_64+0x8acd)
    #7 0x7fff6e3fe451 in _dispatch_lane_invoke (libdispatch.dylib:x86_64+0x9451)
    #8 0x7fff6e407a9d in _dispatch_workloop_worker_thread (libdispatch.dylib:x86_64+0x12a9d)
    #9 0x7fff6e6526fb in _pthread_wqthread (libsystem_pthread.dylib:x86_64+0x26fb)
    #10 0x7fff6e651826 in start_wqthread (libsystem_pthread.dylib:x86_64+0x1826)

0x616000010bdb is located 91 bytes inside of 608-byte region [0x616000010b80,0x616000010de0)
freed by thread T1 here:
    #0 0x106c88ad6 in wrap_free (librustc_rt.asan.dylib:x86_64+0x42ad6)
    #1 0x10669c57a in alloc::alloc::dealloc::h2a46682b3fd1fcd4 alloc.rs:103
    #2 0x1063cbbc0 in alloc::alloc::box_free::h85e777566b08c76d alloc.rs:217
    #3 0x1062af623 in core::ptr::drop_in_place::he4a20f3e32dba16b mod.rs:174
    #4 0x10643c62b in cubeb_backend::capi::capi_stream_destroy::h504649dfea752dec capi.rs:173
    #5 0x1064e0a09 in cubeb_coreaudio::backend::tests::utils::test_ops_stream_operation::_$u7b$$u7b$closure$u7d$$u7d$::hbc15cf636f0b150a utils.rs:1091
    #6 0x1064c795a in cubeb_coreaudio::backend::tests::utils::test_ops_context_operation::hfab4167c5af0e787 utils.rs:1035
    #7 0x1064cd09f in cubeb_coreaudio::backend::tests::utils::test_ops_stream_operation::haaacc9c4ff9c81b0 utils.rs:1056
    #8 0x1061f95ca in cubeb_coreaudio::backend::tests::device_change::test_get_started_stream_in_scope::hcd049a2a814dc689 device_change.rs:165
    #9 0x1064049cc in cubeb_coreaudio::backend::tests::device_change::test_switch_device_in_scope::hb0894f5959915bd4 device_change.rs:60
    #10 0x10640dfa8 in cubeb_coreaudio::backend::tests::device_change::test_switch_device::hf2026957a4208330 device_change.rs:31
    #11 0x1061ffa70 in cubeb_coreaudio::backend::tests::device_change::test_switch_device::_$u7b$$u7b$closure$u7d$$u7d$::h7e363590f88f7d2c device_change.rs:29
    #12 0x10627e643 in core::ops::function::FnOnce::call_once::h0da49dbfb7e04f06 function.rs:232
    #13 0x10676d5ed in _$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h09f25e49c6b2dde5 boxed.rs:1016
    #14 0x1068627ea in __rust_maybe_catch_panic lib.rs:86
    #15 0x106787363 in test::run_test::run_test_inner::_$u7b$$u7b$closure$u7d$$u7d$::he7e26757f8f77360 lib.rs:539
    #16 0x10676058a in std::sys_common::backtrace::__rust_begin_short_backtrace::h1ec69ba5fcb050d2 backtrace.rs:129
    #17 0x1067652da in std::panicking::try::do_call::h3f0071cc5ab7da00 panicking.rs:303
    #18 0x1068627ea in __rust_maybe_catch_panic lib.rs:86
    #19 0x106765df5 in core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::h059274226e6540b3 mod.rs:474
    #20 0x10684fd7d in _$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h9d68ab0c0b6f49ed boxed.rs:1016
    #21 0x10686212d in std::sys::unix::thread::Thread::new::thread_start::h18be3f46590c0640 boxed.rs:1016
    #22 0x7fff6e655e64 in _pthread_start (libsystem_pthread.dylib:x86_64+0x5e64)
    #23 0x7fff6e65183a in thread_start (libsystem_pthread.dylib:x86_64+0x183a)

previously allocated by thread T1 here:
    #0 0x106c8898d in wrap_malloc (librustc_rt.asan.dylib:x86_64+0x4298d)
    #1 0x10669c3c4 in alloc::alloc::alloc::hf6ad4692150492f5 alloc.rs:81
    #2 0x10669c16b in alloc::alloc::exchange_malloc::h873d61ec2e163377 alloc.rs:203
    #3 0x10654c166 in _$LT$cubeb_coreaudio..backend..AudioUnitContext$u20$as$u20$cubeb_backend..traits..ContextOps$GT$::stream_init::h174f9c68d49e1f3b mod.rs:2159
    #4 0x10643bbef in cubeb_backend::capi::capi_stream_init::h354e09e23cca1513 capi.rs:155
    #5 0x1064e0163 in cubeb_coreaudio::backend::tests::utils::test_ops_stream_operation::_$u7b$$u7b$closure$u7d$$u7d$::hbc15cf636f0b150a utils.rs:1072
    #6 0x1064c795a in cubeb_coreaudio::backend::tests::utils::test_ops_context_operation::hfab4167c5af0e787 utils.rs:1035
    #7 0x1064cd09f in cubeb_coreaudio::backend::tests::utils::test_ops_stream_operation::haaacc9c4ff9c81b0 utils.rs:1056
    #8 0x1061f95ca in cubeb_coreaudio::backend::tests::device_change::test_get_started_stream_in_scope::hcd049a2a814dc689 device_change.rs:165
    #9 0x1064049cc in cubeb_coreaudio::backend::tests::device_change::test_switch_device_in_scope::hb0894f5959915bd4 device_change.rs:60
    #10 0x10640dfa8 in cubeb_coreaudio::backend::tests::device_change::test_switch_device::hf2026957a4208330 device_change.rs:31
    #11 0x1061ffa70 in cubeb_coreaudio::backend::tests::device_change::test_switch_device::_$u7b$$u7b$closure$u7d$$u7d$::h7e363590f88f7d2c device_change.rs:29
    #12 0x10627e643 in core::ops::function::FnOnce::call_once::h0da49dbfb7e04f06 function.rs:232
    #13 0x10676d5ed in _$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h09f25e49c6b2dde5 boxed.rs:1016
    #14 0x1068627ea in __rust_maybe_catch_panic lib.rs:86
    #15 0x106787363 in test::run_test::run_test_inner::_$u7b$$u7b$closure$u7d$$u7d$::he7e26757f8f77360 lib.rs:539
    #16 0x10676058a in std::sys_common::backtrace::__rust_begin_short_backtrace::h1ec69ba5fcb050d2 backtrace.rs:129
    #17 0x1067652da in std::panicking::try::do_call::h3f0071cc5ab7da00 panicking.rs:303
    #18 0x1068627ea in __rust_maybe_catch_panic lib.rs:86
    #19 0x106765df5 in core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::h059274226e6540b3 mod.rs:474
    #20 0x10684fd7d in _$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h9d68ab0c0b6f49ed boxed.rs:1016
    #21 0x10686212d in std::sys::unix::thread::Thread::new::thread_start::h18be3f46590c0640 boxed.rs:1016
    #22 0x7fff6e655e64 in _pthread_start (libsystem_pthread.dylib:x86_64+0x5e64)
    #23 0x7fff6e65183a in thread_start (libsystem_pthread.dylib:x86_64+0x183a)

Thread T6 created by T4 here:
    <empty stack>

Thread T4 created by T1 here:
    #0 0x106c82b8a in wrap_pthread_create (librustc_rt.asan.dylib:x86_64+0x3cb8a)
    #1 0x7fff6e402742 in _dispatch_root_queue_poke_slow (libdispatch.dylib:x86_64+0xd742)
    #2 0x7fff366deaf5 in HALB_DispatchQueue::InstallMIGServer(unsigned int, unsigned int, unsigned int (*)(mach_msg_header_t*, mach_msg_header_t*)) (CoreAudio:x86_64+0x3f9af5)
    #3 0x7fff3660e26a in HALC_ProxySystem::HALC_ProxySystem() (CoreAudio:x86_64+0x32926a)
    #4 0x7fff3661e0af in HALC_ProxyObjectMap::_CreateSystemObject() (CoreAudio:x86_64+0x3390af)
    #5 0x7fff3661d48a in HALC_ProxyObjectMap::CopyObjectByObjectID(unsigned int) (CoreAudio:x86_64+0x33848a)
    #6 0x7fff362e7812 in HALSystem::InitializeDevices() (CoreAudio:x86_64+0x2812)
    #7 0x7fff362e686c in HALSystem::CheckOutInstance() (CoreAudio:x86_64+0x186c)
    #8 0x7fff362ec390 in AudioObjectGetPropertyDataSize (CoreAudio:x86_64+0x7390)
    #9 0x106619685 in cubeb_coreaudio::backend::tests::utils::test_get_all_devices::hfc2293ff551a6aa8 utils.rs:255
    #10 0x10661a9b1 in cubeb_coreaudio::backend::tests::utils::test_get_devices_in_scope::hc73873ced910c1db utils.rs:291
    #11 0x1064046b0 in cubeb_coreaudio::backend::tests::device_change::test_switch_device_in_scope::hb0894f5959915bd4 device_change.rs:41
    #12 0x10640dfa8 in cubeb_coreaudio::backend::tests::device_change::test_switch_device::hf2026957a4208330 device_change.rs:31
    #13 0x1061ffa70 in cubeb_coreaudio::backend::tests::device_change::test_switch_device::_$u7b$$u7b$closure$u7d$$u7d$::h7e363590f88f7d2c device_change.rs:29
    #14 0x10627e643 in core::ops::function::FnOnce::call_once::h0da49dbfb7e04f06 function.rs:232
    #15 0x10676d5ed in _$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h09f25e49c6b2dde5 boxed.rs:1016
    #16 0x1068627ea in __rust_maybe_catch_panic lib.rs:86
    #17 0x106787363 in test::run_test::run_test_inner::_$u7b$$u7b$closure$u7d$$u7d$::he7e26757f8f77360 lib.rs:539
    #18 0x10676058a in std::sys_common::backtrace::__rust_begin_short_backtrace::h1ec69ba5fcb050d2 backtrace.rs:129
    #19 0x1067652da in std::panicking::try::do_call::h3f0071cc5ab7da00 panicking.rs:303
    #20 0x1068627ea in __rust_maybe_catch_panic lib.rs:86
    #21 0x106765df5 in core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::h059274226e6540b3 mod.rs:474
    #22 0x10684fd7d in _$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h9d68ab0c0b6f49ed boxed.rs:1016
    #23 0x10686212d in std::sys::unix::thread::Thread::new::thread_start::h18be3f46590c0640 boxed.rs:1016
    #24 0x7fff6e655e64 in _pthread_start (libsystem_pthread.dylib:x86_64+0x5e64)
    #25 0x7fff6e65183a in thread_start (libsystem_pthread.dylib:x86_64+0x183a)

Thread T1 created by T0 here:
    #0 0x106c82b8a in wrap_pthread_create (librustc_rt.asan.dylib:x86_64+0x3cb8a)
    #1 0x106861dd7 in std::sys::unix::thread::Thread::new::h586ae1a3fbb68ccd thread.rs:68
    #2 0x106786cba in test::run_test::run_test_inner::h41a17006bde78306 mod.rs:492
    #3 0x1067852dd in test::run_test::hc2b0fa225f1cdfde lib.rs:503
    #4 0x10678127c in test::run_tests::hf9fe773408722b8d lib.rs:301
    #5 0x1067733a5 in test::console::run_tests_console::h257d38a897830738 console.rs:280
    #6 0x10677e9a2 in test::test_main::h0ece35b24fdccd5b lib.rs:122
    #7 0x10677f9ae in test::test_main_static::h03d43955d3e6f66b lib.rs:141
    #8 0x106733dd7 in cubeb_coreaudio::main::ha80bf2604bb83103 (cubeb_coreaudio-8efcc4bcc1a06d9a:x86_64+0x100564dd7)
    #9 0x1066c5a1d in std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h46ca11651ad8e411 rt.rs:67
    #10 0x10685bc07 in std::panicking::try::do_call::hd7d5ebb621afe41a panicking.rs:303
    #11 0x1068627ea in __rust_maybe_catch_panic lib.rs:86
    #12 0x10685c4fb in std::rt::lang_start_internal::hf660b1f8d8c56d9b panicking.rs:281
    #13 0x1066c5985 in std::rt::lang_start::hdcbaaa8cfc30d74d rt.rs:67
    #14 0x106733e01 in main (cubeb_coreaudio-8efcc4bcc1a06d9a:x86_64+0x100564e01)
    #15 0x7fff6e4517fc in start (libdyld.dylib:x86_64+0x1a7fc)

SUMMARY: AddressSanitizer: heap-use-after-free atomic.rs:2277 in core::sync::atomic::atomic_load::h4dc415c6a3ce79a6
Shadow bytes around the buggy address:
  0x1c2c00002120: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2c00002130: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2c00002140: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2c00002150: fd fd fd fd fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2c00002160: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x1c2c00002170: fd fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd
  0x1c2c00002180: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2c00002190: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2c000021a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2c000021b0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x1c2c000021c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==7236==ABORTING
error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/Users/cchang/Work/cubeb-coreaudio-rs/target/debug/deps/cubeb_coreaudio-8efcc4bcc1a06d9a test_switch_device --ignored --nocapture` (signal: 6, SIGABRT: process abort signal)

Got a more detail log for running RUSTFLAGS="-Z sanitizer=address" cargo test test_switch_device -- --ignored --nocapture with adding a delay on the end of the test (otherwise the stacktraces won't be printed, don't know why)

running 1 test

Switch default device for Output while the stream is working.

Output devices
--------------------
 204: AppleGFXHDAEngineOutputDP:10001:0:{C315-2684-05BF0E12}
	uid: EV2750
 194: AppleHDAEngineOutput:1F,3,0,1,1:0
	uid: Headphones
  41: SoundflowerEngine:0
	uid: Soundflower (2ch)
  52: SoundflowerEngine:1
	uid: Soundflower (64ch)

Switch device for Output: 194 -> 41
Switch device for Output: 41 -> 52
Switch device for Output: 52 -> 204
Switch device for Output: 204 -> 194
=================================================================
==7236==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000010bdb at pc 0x000106410723 bp 0x7000029fa1c0 sp 0x7000029fa1b8
READ of size 1 at 0x616000010bdb thread T6
    #0 0x106410722 in core::sync::atomic::atomic_load::h4dc415c6a3ce79a6 atomic.rs:2277
    #1 0x10647185c in core::sync::atomic::AtomicBool::load::hd6d654f0922c62d7 atomic.rs:404
    #2 0x1063fb16c in cubeb_coreaudio::backend::AudioUnitStream::reinit_async::_$u7b$$u7b$closure$u7d$$u7d$::hbea024c63054739e mod.rs:3270
    #3 0x1063082e2 in coreaudio_sys_utils::dispatch::create_closure_and_executor::closure_executer::haff5ead75c17e7a3 dispatch.rs:59
    #4 0x106c87441 in asan_dispatch_call_block_and_release (librustc_rt.asan.dylib:x86_64+0x41441)
    #5 0x7fff6e3f850d in _dispatch_client_callout (libdispatch.dylib:x86_64+0x350d)
    #6 0x7fff6e3fdacd in _dispatch_lane_serial_drain (libdispatch.dylib:x86_64+0x8acd)
    #7 0x7fff6e3fe451 in _dispatch_lane_invoke (libdispatch.dylib:x86_64+0x9451)
    #8 0x7fff6e407a9d in _dispatch_workloop_worker_thread (libdispatch.dylib:x86_64+0x12a9d)
    #9 0x7fff6e6526fb in _pthread_wqthread (libsystem_pthread.dylib:x86_64+0x26fb)
    #10 0x7fff6e651826 in start_wqthread (libsystem_pthread.dylib:x86_64+0x1826)

I dig a bit deeper today. The 0x616000010bdb here is destroy_pending: AtomicBool in AudioUnitStream at mod.rs:3270
https://github.com/ChunMinChang/cubeb-coreaudio-rs/blob/7fe03b4201160f84aad1c12536affe6b05f8839f/src/backend/mod.rs#L3270

I suspect there is a stream-reinit-task posted to the global task queue in the cubeb context (aka AudioUnitContext::serial_queue) while stream-destroy-task is being executed in the global task queue, or cubeb_stream_destroy and audiounit_property_listener_callback get called at the same time and stream-reinit-task is posted after stream-destroy-task is posted.

Suppose there are 3 different threads

  • M: the Main thread running the test_switch_device test
  • T: the thread running the task in the global task queue (AudioUnitContext::serial_queue)
  • D: the device-changed callback thread (whereaudiounit_property_listener_callback is on)

The timeline should be like this

  1. M: Start running test_switch_device
  2. M: Initialize cubeb context
  3. M: Initialize cubeb stream
  4. M: Start cubeb stream
  5. M: Change the default output device
  6. The default device is changed: The following works might be run at the same time
    • M: Get notification of the device changed
      • Do 5 again until it's repeated X times
    • D: If the stream-reinit-task is pending/being run, skip this event. Otherwise, post an async stream-reinit-task to the global task queue
      • T: Start executing the stream-reinit-task
  7. The following works might be run at the same time:
    • After the default output device is changed X times, the test is about to be finished, so
      M start destroying cubeb stream, which posts a sync stream-destroy-task to the the global task queue. Then M is blocked until the stream-destroy-task is done.
    • While M is executing, the D is fired for the last device-changed event and then posts an async stream-reinit-task
  8. If stream-destroy-task is posted before stream-reinit-task, T will execute stream-destroy-task first
  9. T: Finishing the stream-destroy-task
  10. The following works might be run at the same time:
    • M: Destroy cubeb context
    • T: Pick and run the stream-reinit-task after the cubeb stream is destroyed
      • T: Touching destroy_pending leads to a crash

At step 10, touching the destroy_pending in a destroyed cubeb stream causes the crash.

Got some suggestions from Alex:

In addition to what you have, you could use the if stm_guard.destroy_pending.load(Ordering::SeqCst) inside the main body of reinit_async() in a way that we don't async_dispatch another reinit closure if this flag is true. In this case, you would also need synchronization between destroy() and reinit_async().

If the solution is the above I guess the c-api is not protected by this problem, probably we haven't tried the same test.

Attached patch UAF-demo.diff — — Splinter Review

I can make this happen manually with this patch in version 7fe03b4, by running cargo test test_stream_tester -- --ignored --nocapture, with switching device manually and dropping the cubeb context, stream by the test command, at the proper times.

This patch adds delays to force the context-switch at a proper time to demonstrate what happens and use dbg! to touch the memory that is free after cubeb context and stream is destroyed.

The device can be switched by clicking the sound device icon on the main UI bar then select one device on the system. To drop the cubeb context and stream, it needs to manually enter the correct command in the test_stream_tester test.

The whole log for the demo (cargo test test_stream_tester -- --ignored --nocapture) is:

running 1 test
commands:
	'q': quit
	'c': create a stream
	'd': destroy a stream
	's': start the created stream
	't': stop the created stream
c
Select stream type:
1) Input 2) Output 3) In-Out Duplex 4) Back
2 // Create a output stream
Stream 0x7fed3a9122b0 created.
commands:
	'q': quit
	'c': create a stream
	'd': destroy a stream
	's': start the created stream
	't': stop the created stream
d // destroy the created output stream
1) <AudioUnitStream> destroy: Force context-switch. Please fire a device-change event now!
// Please manually change the default output device here!
2) <AudioUnitStream> reinit_async: Sneak here when destroy_pending is false. Then force context-switch
// Force to back to destroy-task, from reinit-task
3) <AudioUnitStream> destroy: Executing destroy-task.
4) <AudioUnitStream> destroy: destroy-task is done.
5) <AudioUnitStream> reinit_async: Before executing reinit-task,
	force context-switch to terminate the stream and context.
	(back to test-main thread, press 'q')
Stream 0x7f9bd2e18500 destroyed.
commands:
	'q': quit
	'c': create a stream
	'd': destroy a stream
	's': start the created stream
	't': stop the created stream
q // cubeb context will be dropped
Quit.
No need to destroy stream.
6) <AudioUnitContext> drop: context is about to be dropped.
Wait 10 seconds before ending this manual test.
7) <AudioUnitStream> reinit_async: Executing reinit-task. (after stream is destroyed?)
[src/backend/mod.rs:3289] &stm_guard = AudioUnitStream {
    context: AudioUnitContext {
        _ops: error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/Users/cchang/Work/cubeb-coreaudio-rs/target/debug/deps/cubeb_coreaudio-dc2fede9d64566bd test_stream_tester --ignored --nocapture` (signal: 11, SIGSEGV: invalid memory reference)
Attached patch UAF-demo-c.diff — — Splinter Review

This is reproducible in C backend as well. By applying this patch that adds some delays at the proper time, the UAF can be reproduced via cubeb-test. The delays are added in the same place exactly as those in the Attachment 9126802 [details] [diff].

% ./cubeb-test   
Log level is DISABLED
Init cubeb backend: audiounit
collection device type is UNKNOWN
stream_init succeed
state is CUBEB_STATE_STARTED
stream_start succeed
press `q` to abort or `h` for help
q
state is CUBEB_STATE_STOPPED
1) <audiounit_stream_destroy> Force context-switch. Please fire a device-change event now!
// Need to manually change the default output device
2) <audiounit_reinit_stream_async> Sneak here when destroy_pending is 0. Then force context-switch.
3) <audiounit_stream_destroy> Executing destroy-task.
4) <audiounit_stream_destroy> destroy-task is done.
5) <audiounit_reinit_stream_async> Before executing reinit-task,
	force context-switch to terminate the stream and context.
6) <audiounit_destroy> context is dropped.
Wait 10 seconds before ending this manual test.
7) <audiounit_reinit_stream_async> Executing reinit-task. (after stream is destroyed?)
8) touch stm->output_unit: 0x0 // This is actually UAF!

(In reply to C.M.Chang[:chunmin] from comment #4)

I guess the c-api is not protected by this problem, probably we haven't tried the same test.

Yes, this is reproducible in both C and Rust backend. The steps-to-reproduce are in comment 5 and comment 6.

In addition to what you have, you could use the if stm_guard.destroy_pending.load(Ordering::SeqCst) inside the main body of reinit_async() in a way that we don't async_dispatch another reinit closure if this flag is true. In this case, you would also need synchronization between destroy() and reinit_async().

Actually, I've tried this at first. But it doesn't seem to work (Not sure if I understand the suggestion correctly though). Since the reinit-task and destroy-task are racing, destroy_pending might be false even if destroy is called.

Another way I could think is to move the ownership of the task queue to the cubeb stream, instead of the cubeb context. The task will be created in the stream, instead of the cubeb context. Then when the stream is being destroyed, we can cancel all the pending tasks. However, there is Dispatch Queue API that can do that. We may need to create a task-queue class by our own (and it needs to make sure it won't regress the data race fixed in bug 1598413).

Move the test log to the first line of the switch-device test. It will
be clearer about what's going on when the error log is printed.

The UAF is actually caused by a data race between the stream-reinit task
on the device-changed callback thread, and the stream-destroy task on
the main audio thread. When the device is changed upon the stream is
about to be destroyed, the stream-reinit task can be posted to the
global task queue just right after the stream-destroy task in that task
queue. As a result, the stream-reinit task will be executed after the
stream is destroyed and cause a used-after-free error.

The solution here is to unregister the device-changed callback before
the stream is destroyed, on the main audio thread. The calls for
unregistering device-changed callbacks will block the current thread
until all the pending device-changed callbacks finish, and stop firing
any callbacks in the future. This makes sure the following things

  • No device-changed callback thread will run while the stream is ready
    to be destroyed.
  • No stream-reinit task can be appended to the task queue

As a result,

  • There is no device-changed callback thread that races with the task
    thread running the stream-destroy task
  • There is no stream-reinit task appended after the stream-destroy task.

(Note that the running data callbacks will be stopped in the
stream-destroy task, by stopping the running AudioUnits, before
destroying the stream.)

Depends on D63368

Comment on attachment 9127642 [details]
Bug 1614971 - Fix the UAF. r?padenot

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: It's not hard to infer when the data race causing an UAF can happen by reading this patch carefully.
  • 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?: For new Rust backend, it's FF74. For C backend, it exists for a long time.
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: For Rust cubeb backend, this patch can be uplifted to the FF74 easily.
    For C cubeb backend, it's easy to import this change from the Rust backend to C backend.

However, the cubeb code are in public github repos.

The update will be submitted in github repos instead of being updated in the gecko directly. I am not sure how to hide the security vulnerability properly when updating the code. (One thing I can do is to update the code without commit messages or comments. The comments can be added in other separated commits anyway.)

  • How likely is this patch to cause regressions; how much testing does it need?: It's a small change without doing anything that can change the behavior. The change here is to destroy the audio stream properly to prevent system callback from being executed after the stream is destroyed.
Attachment #9127642 - Flags: sec-approval?
Attachment #9127641 - Flags: sec-approval?
Keywords: sec-high

Comment on attachment 9127642 [details]
Bug 1614971 - Fix the UAF. r?padenot

sec-approved to land AFTER

  1. Change the commit message's title to something other than 'Fix the UAF'. perhaps 'Ensure object is fully deleted' or something
  2. Remove the rest of the commit message (it's preserved in this bug's comments)
  3. Remove the comments from the commit
  4. Do not land the test change until mid-way through next cycle

I don't see a backport for esr68 but you said you had one, so please ensure esr68 gets it.

Attachment #9127642 - Flags: sec-approval?
Attachment #9127642 - Flags: sec-approval+
Attachment #9127642 - Flags: approval-mozilla-esr68+
Attachment #9127642 - Flags: approval-mozilla-beta+

Comment on attachment 9127641 [details]
Bug 1614971 - Revise messages in test_switch_device. r?padenot

I'm minus-ing the test change because it should not land until ~3/25

Attachment #9127641 - Flags: sec-approval? → sec-approval-

Pick commits:

  • 4acd802: Destroy the stream properly
  • 54d950a: Run the tests in the subcrate (#51)
  • 132d209: Clean up clippy warnings and errors (#49)

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #11)

Comment on attachment 9127642 [details]
Bug 1614971 - Fix the UAF. r?padenot

sec-approved to land AFTER

  1. Change the commit message's title to something other than 'Fix the UAF'. perhaps 'Ensure object is fully deleted' or something
  2. Remove the rest of the commit message (it's preserved in this bug's comments)
  3. Remove the comments from the commit

The code will be imported in D63725, instead of D63369. D63369 is created to hide the review in the public git repo, since the code is in a third-party library developed by Mozilla.

The commit message is simplied to Destroy the stream properly without any explanation. The title of D63725, Update cubeb-coreaudio to 4acd802, is a regular update title, as what we used to have in the past.

Can I carry the sec-approval from D63369 to D63725? Or I need to ask a sec-approval again?

  1. Do not land the test change until mid-way through next cycle

Sure.

I don't see a backport for esr68 but you said you had one, so please ensure esr68 gets it.

The fix is under review in another github repo: https://github.com/kinetiknz/cubeb/pull/574. It will be imported to gecko once it's merge in the repo's master branch. It should be similar to what we have in D63725. I'll ask a sec-approval again then.

Flags: needinfo?(tom)

No, you don't need to ask for sec-approval again for either.

Flags: needinfo?(tom)

Is this ready to land? 74 goes to RC next week.

Flags: needinfo?(cchang)

Comment on attachment 9128247 [details]
Bug 1614971 - Update cubeb-coreaudio to 4acd802. r?padenot

Beta/Release Uplift Approval Request

  • User impact if declined: The users could get an used-after-free error.
  • Is this code covered by automated tests?: No
  • 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): The risk is low. It's a small change without doing anything that can change the behavior. The change here is to destroy the audio stream properly to prevent system callback from being executed after the stream is destroyed.
  • String changes made/needed:
Flags: needinfo?(cchang)
Attachment #9128247 - Flags: approval-mozilla-beta?

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

Is this ready to land? 74 goes to RC next week.

Yes, I am going to land it.

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #11)

Comment on attachment 9127642 [details]
Bug 1614971 - Fix the UAF. r?padenot
...
I don't see a backport for esr68 but you said you had one, so please ensure esr68 gets it.

Paul, can you help me to merge https://github.com/kinetiknz/cubeb/pull/574 ? I don't have right to do so if the fix should be uplifted to esr 68.

Flags: needinfo?(padenot)

https://hg.mozilla.org/integration/autoland/rev/5106e7c7b90b18b84865cbb64993c17d7a7fb84a
https://hg.mozilla.org/mozilla-central/rev/5106e7c7b90b

Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox75: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

(In reply to C.M.Chang[:chunmin] from comment #19)

Paul, can you help me to merge https://github.com/kinetiknz/cubeb/pull/574 ? I don't have right to do so if the fix should be uplifted to esr 68.

I've merged it now. Paul had already reviewed it, so it was just waiting on the merge.

It looks like I sent you an invite for cubeb repo access a while ago, but it must've gone missing. I'll PM you a new invite link.

Flags: needinfo?(padenot)

Comment on attachment 9127642 [details]
Bug 1614971 - Fix the UAF. r?padenot

moving approval to the right patch.

Attachment #9127642 - Flags: approval-mozilla-beta+
Attachment #9128247 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

https://hg.mozilla.org/releases/mozilla-beta/rev/40b874cbfe8b

What shall land for ESR68? The cubeb-coreaudio update will fail to apply, the patch with approval for ESR68 mentions the UAF in the commit message

status-firefox74: affected → fixed
Flags: needinfo?(cchang)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

I am not sure if this bug blocks bug 863846.

This errors are found by running Asan in the test of a third-party audio library used and maintained by Mozilla. The error happens when users unplug or switch the current output audio device. I don't think those tests are run in Firefox's test, and this won't be detected in the build process. Is there a better category, instead of asan build, to track this bug ?

Flags: needinfo?(sledru)

ok, makes sense!

No longer blocks: asan-maintenance
Flags: needinfo?(sledru)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main74+][adv-esr68.6+]
Attached file advisory.txt (obsolete) —
Attachment #9131375 - Attachment is obsolete: true
Attached file advisory.txt —
Attachment #9131380 - Attachment is obsolete: true
Alias: CVE-2020-6807
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: