1598543 - stack-buffer-overflow in [@ mozilla::media::LambdaRunnable]

Status: VERIFIED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found with m-c 20191116-05f4ac2d6dd6

==20092==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x0049159fed00 at pc 0x7ffd1c84df9a bp 0x0049159fe220 sp 0x0049159fe268
READ of size 135 at 0x0049159fed00 thread T93
    #0 0x7ffd1c84dfc2 in __asan_wrap_strlen Z:\task_1573467939\fetches\llvm-project\llvm\projects\compiler-rt\lib\sanitizer_common\sanitizer_common_interceptors.inc:365
    #1 0x7ffd060528f5 in nsTSubstring<char>::Assign \src\xpcom\string\nsTSubstring.cpp:401
    #2 0x7ffd06045c57 in nsTSubstring<char>::Assign \src\xpcom\string\nsTSubstring.cpp:380
    #3 0x7ffd0e50bb42 in mozilla::media::LambdaRunnable<`lambda at z:/build/build/src/dom/media/systemservices/CamerasParent.cpp:592:54'>::Run \src\dom\media\systemservices\MediaUtils.h:73
    #4 0x7ffd0749f303 in MessageLoop::DeferOrRunPendingTask \src\ipc\chromium\src\base\message_loop.cc:450
    #5 0x7ffd074a0d3e in MessageLoop::DoWork \src\ipc\chromium\src\base\message_loop.cc:523
    #6 0x7ffd0755f72d in mozilla::ipc::MessagePumpForNonMainUIThreads::DoRunLoop \src\ipc\glue\MessagePump.cpp:376
    #7 0x7ffd074722c9 in base::MessagePumpWin::Run \src\ipc\chromium\src\base\message_pump_win.h:79
    #8 0x7ffd0749e09e in MessageLoop::RunHandler \src\ipc\chromium\src\base\message_loop.cc:308
    #9 0x7ffd074b0521 in base::Thread::ThreadMain \src\ipc\chromium\src\base\thread.cc:192
    #10 0x7ffd07473adf in `anonymous namespace'::ThreadFunc \src\ipc\chromium\src\base\platform_thread_win.cc:19
    #11 0x7ffd1c85f838 in __asan::AsanThread::ThreadStart Z:\task_1573467939\fetches\llvm-project\llvm\projects\compiler-rt\lib\asan\asan_thread.cc:262
    #12 0x7ffd4c097bd3 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180017bd3)
    #13 0x7ffd1e6d4d8b in patched_BaseThreadInitThunk \src\mozglue\dllservices\WindowsDllBlocklist.cpp:564
    #14 0x7ffd4c1ecee0 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x18006cee0)

Address 0x0049159fed00 is located in stack of thread T64 at offset 192 in frame
    #0 0x7ffd0e50b66f in mozilla::media::LambdaRunnable<`lambda at z:/build/build/src/dom/media/systemservices/CamerasParent.cpp:592:54'>::Run \src\dom\media\systemservices\MediaUtils.h:73

  This frame has 8 object(s):
    [32, 40) 'agg.tmp.i.i'
    [64, 192) 'deviceName.i'
    [224, 480) 'deviceUniqueId.i' <== Memory access at offset 192 partially underflows this variable
    [544, 560) 'name.i'
    [576, 592) 'uniqueId.i'
    [608, 612) 'devicePid.i'
    [624, 640) 'devInfo.i'
    [656, 712) 'ref.tmp16.i'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp, SEH and C++ exceptions *are* supported)
Thread T64 created by T27 here:
    #0 0x7ffd1c86095c in __asan_wrap_CreateThread Z:\task_1573467939\fetches\llvm-project\llvm\projects\compiler-rt\lib\asan\asan_win.cc:146
    #1 0x7ffd4941d8d6 in beginthreadex+0x56 (C:\Windows\System32\ucrtbase.dll+0x18001d8d6)
    #2 0x7ffd1c41721d in _PR_MD_CREATE_THREAD \src\nsprpub\pr\src\md\windows\w95thred.c:153
    #3 0x7ffd1c4482ec in _PR_NativeCreateThread \src\nsprpub\pr\src\threads\combined\pruthr.c:1058
    #4 0x7ffd1c448c95 in _PR_CreateThread \src\nsprpub\pr\src\threads\combined\pruthr.c:1184
    #5 0x7ffd1c43b6ef in PR_CreateThread \src\nsprpub\pr\src\threads\combined\pruthr.c:1404
    #6 0x7ffd062efc37 in nsThread::Init \src\xpcom\threads\nsThread.cpp:673
    #7 0x7ffd062fd757 in nsThreadManager::NewNamedThread \src\xpcom\threads\nsThreadManager.cpp:550
    #8 0x7ffd06301f40 in NS_NewNamedThread \src\xpcom\threads\nsThreadUtils.cpp:139
    #9 0x7ffd0edd47e4 in mozilla::dom::indexedDB::`anonymous namespace'::ConnectionPool::ScheduleTransaction \src\dom\indexedDB\ActorsParent.cpp:11403
    #10 0x7ffd0edfcdc0 in mozilla::dom::indexedDB::`anonymous namespace'::TransactionDatabaseOperationBase::StartOnConnectionPool \src\dom\indexedDB\ActorsParent.cpp:21997
    #11 0x7ffd0edc2fc3 in mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::DispatchToWorkThread \src\dom\indexedDB\ActorsParent.cpp:21054
    #12 0x7ffd0edb3043 in mozilla::dom::indexedDB::`anonymous namespace'::FactoryOp::Run \src\dom\indexedDB\ActorsParent.cpp
    #13 0x7ffd0edca944 in mozilla::dom::indexedDB::`anonymous namespace'::WaitForTransactionsHelper::MaybeWaitForFileHandles \src\dom\indexedDB\ActorsParent.cpp:12589
    #14 0x7ffd0edc99fe in mozilla::dom::indexedDB::`anonymous namespace'::WaitForTransactionsHelper::Run \src\dom\indexedDB\ActorsParent.cpp:12617
    #15 0x7ffd0edc9c7e in mozilla::dom::indexedDB::`anonymous namespace'::WaitForTransactionsHelper::Run \src\dom\indexedDB\ActorsParent.cpp:12613
    #16 0x7ffd0edc1805 in mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::BeginVersionChange \src\dom\indexedDB\ActorsParent.cpp:20943
    #17 0x7ffd0edb3043 in mozilla::dom::indexedDB::`anonymous namespace'::FactoryOp::Run \src\dom\indexedDB\ActorsParent.cpp
    #18 0x7ffd062f4c47 in nsThread::ProcessNextEvent \src\xpcom\threads\nsThread.cpp:1250
    #19 0x7ffd062febb8 in NS_ProcessNextEvent \src\xpcom\threads\nsThreadUtils.cpp:486
    #20 0x7ffd0755f12c in mozilla::ipc::MessagePumpForNonMainThreads::Run \src\ipc\glue\MessagePump.cpp:333
    #21 0x7ffd0749e09e in MessageLoop::RunHandler \src\ipc\chromium\src\base\message_loop.cc:308
    #22 0x7ffd0749de35 in MessageLoop::Run \src\ipc\chromium\src\base\message_loop.cc:290
    #23 0x7ffd062ecd26 in nsThread::ThreadFunc \src\xpcom\threads\nsThread.cpp:458
    #24 0x7ffd1c4473dd in _PR_NativeRunThread \src\nsprpub\pr\src\threads\combined\pruthr.c:399
    #25 0x7ffd1c4173f4 in pr_root \src\nsprpub\pr\src\md\windows\w95thred.c:139
    #26 0x7ffd4941d9f1 in o_strncat_s+0x71 (C:\Windows\System32\ucrtbase.dll+0x18001d9f1)
    #27 0x7ffd1c85f838 in __asan::AsanThread::ThreadStart Z:\task_1573467939\fetches\llvm-project\llvm\projects\compiler-rt\lib\asan\asan_thread.cc:262
    #28 0x7ffd4c097bd3 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180017bd3)
    #29 0x7ffd1e6d4d8b in patched_BaseThreadInitThunk \src\mozglue\dllservices\WindowsDllBlocklist.cpp:564
    #30 0x7ffd4c1ecee0 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x18006cee0)

Thread T27 created by T0 here:
    #0 0x7ffd1c86095c in __asan_wrap_CreateThread Z:\task_1573467939\fetches\llvm-project\llvm\projects\compiler-rt\lib\asan\asan_win.cc:146
    #1 0x7ffd4941d8d6 in beginthreadex+0x56 (C:\Windows\System32\ucrtbase.dll+0x18001d8d6)
    #2 0x7ffd1c41721d in _PR_MD_CREATE_THREAD \src\nsprpub\pr\src\md\windows\w95thred.c:153
    #3 0x7ffd1c4482ec in _PR_NativeCreateThread \src\nsprpub\pr\src\threads\combined\pruthr.c:1058
    #4 0x7ffd1c448c95 in _PR_CreateThread \src\nsprpub\pr\src\threads\combined\pruthr.c:1184
    #5 0x7ffd1c43b6ef in PR_CreateThread \src\nsprpub\pr\src\threads\combined\pruthr.c:1404
    #6 0x7ffd062efc37 in nsThread::Init \src\xpcom\threads\nsThread.cpp:673
    #7 0x7ffd062fd757 in nsThreadManager::NewNamedThread \src\xpcom\threads\nsThreadManager.cpp:550
    #8 0x7ffd06301f40 in NS_NewNamedThread \src\xpcom\threads\nsThreadUtils.cpp:139
    #9 0x7ffd075141ee in `anonymous namespace'::ParentImpl::CreateBackgroundThread \src\ipc\glue\BackgroundImpl.cpp:944
    #10 0x7ffd0751a7bf in `anonymous namespace'::ParentImpl::CreateActorHelper::Run \src\ipc\glue\BackgroundImpl.cpp:1263
    #11 0x7ffd062f4c47 in nsThread::ProcessNextEvent \src\xpcom\threads\nsThread.cpp:1250
    #12 0x7ffd062febb8 in NS_ProcessNextEvent \src\xpcom\threads\nsThreadUtils.cpp:486
    #13 0x7ffd062fe0f5 in nsThreadManager::SpinEventLoopUntilInternal \src\xpcom\threads\nsThreadManager.cpp:624
    #14 0x7ffd16e449f1 in XPTC__InvokebyIndex+0x71 (C:\Users\Administrator\builds\asan\xul.dll+0x190e949f1)
    #15 0x7ffd083b0a51 in XPCWrappedNative::CallMethod \src\js\xpconnect\src\XPCWrappedNative.cpp:1149
    #16 0x7ffd083b831e in XPC_WN_CallMethod \src\js\xpconnect\src\XPCWrappedNativeJSOps.cpp:946
    #17 0x7ffd141203e4 in js::InternalCallOrConstruct \src\js\src\vm\Interpreter.cpp:548
    #18 0x7ffd141235da in InternalCall \src\js\src\vm\Interpreter.cpp:617
    #19 0x7ffd141046e4 in Interpret \src\js\src\vm\Interpreter.cpp:3119
    #20 0x7ffd140e6f6a in js::RunScript \src\js\src\vm\Interpreter.cpp:423
    #21 0x7ffd14120cb5 in js::InternalCallOrConstruct \src\js\src\vm\Interpreter.cpp:589
    #22 0x7ffd141235da in InternalCall \src\js\src\vm\Interpreter.cpp:617
    #23 0x7ffd14123812 in js::Call \src\js\src\vm\Interpreter.cpp:634
    #24 0x7ffd14771e0f in js::fun_apply \src\js\src\vm\JSFunction.cpp:1190
    #25 0x7ffd141203e4 in js::InternalCallOrConstruct \src\js\src\vm\Interpreter.cpp:548
    #26 0x7ffd141235da in InternalCall \src\js\src\vm\Interpreter.cpp:617
    #27 0x7ffd141046e4 in Interpret \src\js\src\vm\Interpreter.cpp:3119
    #28 0x7ffd140e6f6a in js::RunScript \src\js\src\vm\Interpreter.cpp:423
    #29 0x7ffd14120cb5 in js::InternalCallOrConstruct \src\js\src\vm\Interpreter.cpp:589
    #30 0x7ffd141235da in InternalCall \src\js\src\vm\Interpreter.cpp:617
    #31 0x7ffd14123812 in js::Call \src\js\src\vm\Interpreter.cpp:634
    #32 0x7ffd14300566 in JS_CallFunctionValue \src\js\src\jsapi.cpp:2655
    #33 0x7ffd0839e934 in nsXPCWrappedJS::CallMethod \src\js\xpconnect\src\XPCWrappedJSClass.cpp:956
    #34 0x7ffd0632e966 in PrepareAndDispatch \src\xpcom\reflect\xptcall\md\win32\xptcstubs_x86_64.cpp:181
    #35 0x7ffd16e44a48 in SharedStub+0x48 (C:\Users\Administrator\builds\asan\xul.dll+0x190e94a48)
    #36 0x7ffd13e6d039 in nsXREDirProvider::DoStartup \src\toolkit\xre\nsXREDirProvider.cpp:952
    #37 0x7ffd13e1aa61 in XREMain::XRE_mainRun \src\toolkit\xre\nsAppRunner.cpp:4397
    #38 0x7ffd13e1f42d in XREMain::XRE_main \src\toolkit\xre\nsAppRunner.cpp:4721
    #39 0x7ffd13e212b8 in XRE_main \src\toolkit\xre\nsAppRunner.cpp:4802
    #40 0x7ff7e3d9234e in NS_internal_main \src\browser\app\nsBrowserApp.cpp:308
    #41 0x7ff7e3d91501 in wmain \src\toolkit\xre\nsWindowsWMain.cpp:131
    #42 0x7ff7e3e8da67 in __scrt_common_main_seh f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #43 0x7ffd4c097bd3 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180017bd3)
    #44 0x7ffd4c1ecee0 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x18006cee0)

SUMMARY: AddressSanitizer: stack-buffer-overflow Z:\task_1573467939\fetches\llvm-project\llvm\projects\compiler-rt\lib\sanitizer_common\sanitizer_common_interceptors.inc:365 in __asan_wrap_strlen
Shadow bytes around the buggy address:
  0x01c4f2f3fd50: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x01c4f2f3fd60: f8 f8 f2 f2 f8 f3 f3 f3 00 00 00 00 00 00 00 00
  0x01c4f2f3fd70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x01c4f2f3fd80: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 f8 f2 f2 f2
  0x01c4f2f3fd90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x01c4f2f3fda0:[f2]f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00
  0x01c4f2f3fdb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x01c4f2f3fdc0: 00 00 00 00 f2 f2 f2 f2 f2 f2 f2 f2 00 00 f2 f2
  0x01c4f2f3fdd0: 00 00 f2 f2 04 f2 f8 f8 f2 f2 f8 f8 f8 f8 f8 f8
  0x01c4f2f3fde0: f8 f3 f3 f3 f3 f3 f3 f3 00 00 00 00 00 00 00 00
  0x01c4f2f3fdf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
Thread T93 created by T0 here:
    #0 0x7ffd1c86095c in __asan_wrap_CreateThread Z:\task_1573467939\fetches\llvm-project\llvm\projects\compiler-rt\lib\asan\asan_win.cc:146
    #1 0x7ffd07473a7c in PlatformThread::Create \src\ipc\chromium\src\base\platform_thread_win.cc:57
    #2 0x7ffd074afb0b in base::Thread::StartWithOptions \src\ipc\chromium\src\base\thread.cc:97
    #3 0x7ffd0e5187de in mozilla::media::LambdaRunnable<`lambda at z:/build/build/src/dom/media/systemservices/CamerasParent.cpp:1069:43'>::Run \src\dom\media\systemservices\MediaUtils.h:73
    #4 0x7ffd062f4c47 in nsThread::ProcessNextEvent \src\xpcom\threads\nsThread.cpp:1250
    #5 0x7ffd062febb8 in NS_ProcessNextEvent \src\xpcom\threads\nsThreadUtils.cpp:486
    #6 0x7ffd0755dd4f in mozilla::ipc::MessagePump::Run \src\ipc\glue\MessagePump.cpp:88
    #7 0x7ffd0749e09e in MessageLoop::RunHandler \src\ipc\chromium\src\base\message_loop.cc:308
    #8 0x7ffd0749de35 in MessageLoop::Run \src\ipc\chromium\src\base\message_loop.cc:290
    #9 0x7ffd0fad46ca in nsBaseAppShell::Run \src\widget\nsBaseAppShell.cpp:137
    #10 0x7ffd0fc6e858 in nsAppShell::Run \src\widget\windows\nsAppShell.cpp:406
    #11 0x7ffd13b47095 in nsAppStartup::Run \src\toolkit\components\startup\nsAppStartup.cpp:276
    #12 0x7ffd13e1b4fa in XREMain::XRE_mainRun \src\toolkit\xre\nsAppRunner.cpp:4586
    #13 0x7ffd13e1f42d in XREMain::XRE_main \src\toolkit\xre\nsAppRunner.cpp:4721
    #14 0x7ffd13e212b8 in XRE_main \src\toolkit\xre\nsAppRunner.cpp:4802
    #15 0x7ff7e3d9234e in NS_internal_main \src\browser\app\nsBrowserApp.cpp:308
    #16 0x7ff7e3d91501 in wmain \src\toolkit\xre\nsWindowsWMain.cpp:131
    #17 0x7ff7e3e8da67 in __scrt_common_main_seh f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #18 0x7ffd4c097bd3 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180017bd3)
    #19 0x7ffd4c1ecee0 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x18006cee0)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Required to reproduce with testcase.html

Comment 2

[Andrew McCreight [:mccr8]]

This looks like some kind of string handling issue in a runnable in CamerasParent::RecvGetCaptureDevice.

Comment 3

[Nils Ohlmeier [:drno]]

P1 based on sec-high rating.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1598543 - Use size instead of length.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1598543 - Cleanup upstream constants to also use size instead of length.

Depends on D58144

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The bug is in a string pass-out pattern inherited from upstream where "length" sometimes refers to string length, sometimes buffer length.

Code was removed upstream long ago, and we're maintaining it in dom/media/systemservices. (We're not sure where the reciprocal upstream code for screen capture lives now, likely moved to chromium).

Two patches. The first one fixes the faulty desktop capture code responsible for comment 0. This is the significant vulnerability, since the string is under attacker control.

Still, it's not clear to me how en exploit can be made from it. Getters are copying at most one extra byte, causing the stack string buffer holding the result to lose its termination, but we're not overwriting any information. This way, it may be possible to expose part of the stack in a JS observable-string. Would like an opinion from security folks on how serious this is.

The second patch fixes similar patterns in the camera code. That code exists upstream still, and should probably be reported/upstreamed. However, since an attacker is unlikely to be able to affect names of hardware cameras, this would seem hard to exploit.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Forgot to mention I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1038559 9 days ago.

It's marked Security_Severity-Medium there based on patch two (they haven't yet verified whether their moved screen sharing code is similarly affected). But unlike Firefox, Chrome does not expose document titles in the label of screen sharing tracks, so they would not be vulnerable to the test case anyway.

I pinged them for a timeline so we can unblock this.

Comment 8

[Maire Reavy [:mreavy]]

Hi Jan-Ivar, Have you gotten a reply? If so, what are they planning to do? If not, do you need help getting movement from Chrome? (I and others can help if you do.) And thanks for digging in on this!

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

They seem to have difficulty locating owners for their native WebRTC capture stack. :-( The issue first got transferred to guidou who just bounced it back to the person I think is the triager yesterday.

I left another comment just now that this is timing sensitive, that we're holding landing security patches in Firefox over this, and at some point we may not wait anymore and land them, and hope their screen sharing code (which apparently got moved to chromium somewhere) is not similarly affected, since we've gotten no confirmation from them that it has this problem (the camera/mic version of this that we've already pointed out seems hard to exploit).

I also suggested they ask juberti. Hoping this will unstuck it.

They may in fact not be urgently affected, since Chrome does not appear to use the window title or document title in the track.label when sharing a window at least (I didn't try tab-sharing), so it may not get to JS in that case, leaving the problem to rare camera/microphone devices with long names.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Update on the upstream bug in comment 7: Guido "checked the screen capture code (which is used in Chromium) in third_party/webrtc/modules/desktop_capture and found no strlen() calls. memcpy() calls there looked safe". So Chrome should be good.

But, he then closed it and duped it into a new issue on webrtc.org https://bugs.chromium.org/p/webrtc/issues/detail?id=11290 opened on Friday 10:19 AM EST.

However, It looks like the Security label on that new issue was added 8 minutes after it was opened, and the Restrict-View-SecurityTeam label a day later. Does that mean there was exposure? I added a comment asking that.

Since the new issue dups the text of the original issue, it contains a detailed explanation including the reproduction steps in testcase.html and the words "We discovered this in Firefox".

How do we manage this risk? Should we still wait for upstream or go ahead and fix Firefox now?

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Dminor, I think we're the only major browser still using the upstream capture code, but I'm not sure. Do you know?

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also is sec-high still right for an OOB read? See comment 6 for details.

Comment 13

[Dan Minor [:dminor]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #11)

Dminor, I think we're the only major browser still using the upstream capture code, but I'm not sure. Do you know?

For video capture, as far as I know that is the case. The affected screen capture code isn't part of webrtc.org anymore, and I was not able to find anything similar to the affected screen capture code in the chromium source. I'm not that familiar with chromium so its possible I missed something.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Hi, is this waiting on a sec-approval request before landing? We're low on time to land this patch for this cycle still.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I personally don't think this OOB read needs to be sec-high, but I'll add a request anyway.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9117568 [details]
Bug 1598543 - Use size instead of length.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Medium. Patch does some cleanup (renaming) that maybe obscures a little bit that the old code didn't allow for the string terminator in its buffer copy. But to get from that to an OOB read exploit requires the aha that in screen capture of browser windows the device name is modifiable by modifying your own page's dom title.
• 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?: 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?: This is upstream code that should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Got confirmation that upstream bug was unrestricted for ~21 hours unfortunately, so I think we should land this.

Comment 18

[Daniel Veditz [:dveditz]]

How do we manage this risk? Should we still wait for upstream or go ahead and fix Firefox now?

kind of moot, but yeah we should fix it and not wait for upstream :-)

Also is sec-high still right for an OOB read? See comment 6 for details.

Generically yes, without knowing any mitigating details. Heartbleed was an OOB read. How would the forbidden data be reflected back into content so it could be used by an attacker?

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Part of the stack after buffer(s) in the master process here, up to the nearest 0000 probably (modulo utf8 weirdness) would be exposed to js in the track.label string.

The two buffers deviceName and deviceUniqueId are back to back, and only the string that goes into the first one is JS modifiable using the dom title trick, I believe, so I think if the first one is exploited it will merely run into the second buffer and be terminated there.

I don't see how the termination of the second buffer could be overridden, since it seems to be set from an OS id in all platforms.

Comment 20

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9117592 [details]
Bug 1598543 - Cleanup upstream constants to also use size instead of length.

Approved to land and request uplift.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/266640dee34b48f7b57f2397ec06cf37e96aef8e
https://hg.mozilla.org/integration/autoland/rev/b133c39528ada1fed399bdd57e69ea62acefcc89
https://hg.mozilla.org/mozilla-central/rev/266640dee34b
https://hg.mozilla.org/mozilla-central/rev/b133c39528ad

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(comment 19 made assumptions about order of stack variables, so I've obsoleted it)

In trying to make repro steps for this, I've confirmed the OOB read happens, but I appear to have assumed that the result made it all the way back to JS, when it appears to get truncated to 128 characters again here, which effectively removes the OOB data before JS sees it. Gotta love old string handling code.

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9117568 [details]
Bug 1598543 - Use size instead of length.

Beta/Release Uplift Approval Request

• User impact if declined: OOB stack read in master process found by AddressSanitizer, and observable in debugger, but no exposure of the data to JS has been observed (see comment 23). Request is mostly for hygiene and in case we missed some other codepath that uses this code, in case folks go digging for it.
• 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): Simple string length test fixes to account for \0 terminator, masked by some trivial renaming.
• 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: See above.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above.
• String or UUID changes made by this patch:

Comment 25

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9117568 [details]
Bug 1598543 - Use size instead of length.

sec fix, approved for 73.0b11 and 68.5esr

Comment 26

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e07c1d22ec2f
https://hg.mozilla.org/releases/mozilla-beta/rev/6cbc2116abb4

Comment 27

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/f754db38b851
https://hg.mozilla.org/releases/mozilla-esr68/rev/231b92b1db0a

Comment 28

[Cristian Baica [:cbaica], Release Desktop QA]

@Jan-Ivar Could you please provide the necessary repro steps for this?

I've downloaded an affected asan build (on windows and ubuntu 18.04) and loaded the testcase.html in a profile containing the prefs.js file, but didn't get anything similar to the log you posted.

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The original log in comment 0 is from Tyler. Unfortunately I don't have a linux box, so I reproed locally using the debugger on macos instead.

Tyler, can you help verify this is fixed?

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

FYI I had to adjust the length of the string to repro in nightly on macOS. I think this is because the window's title (which is what gets used) may vary by os and firefox cut (e.g. nightly on my mac appends " - Nightly").

E.g. the following:
<html>
<meta charset=utf-8>
<div id="div"></div>
<button id="button">User gesture</button><br>
<video id="video" width="320" height="200" autoplay></video><br>
<script>

const console = { log: msg => div.innerHTML += msg + "<br>" };
const wait = ms => new Promise(resolve => setTimeout(resolve, ms));

button.onclick = async () => {
  try {
    document.title = '12345678901ó §·ó ®ˆð«©³Ùªážó ¦œ-ó Š¯ä¼”+ó ¸/=ðŸ—€ó €…ð¯»˜ì½¬ð©²Û°B05';
    await wait(200);
    const stream = await navigator.mediaDevices.getDisplayMedia();
    const [track] = stream.getTracks();
    console.log(track.label);
  } catch (e) {
    console.log(e);
  }
};

</script>
</html>

... outputs 12345678901ó §·ó ®ˆð«©³Ùªážó ¦œ-ó Š¯ä¼”+ó ¸/=ðŸ—€ó €…ð¯»˜ì½¬ð©²Û°B05 - Nightl for me (128 bytes including null-terminator), which doesn't actually verify the fix because of comment 23, but shows that the string is long enough (missing the y character).

Hope that helps repro it.

Comment 31

[Cristian Baica [:cbaica], Release Desktop QA]

I have tried reproducing the issue on Fx 74.0a1 ASAN buildID: 20200131013147 using the newly provided html, but without any success.
The full name is displayed on both Ubuntu 18.04 x64 and on Windows 10 x64.
If Tyler could help us out in verifying the issue, it would be greatly appreciated.

Comment 32

[Tyson Smith [:tsmith]]

There is no Tyler :)

Verified with m-b (73) 20200130-14ac73797dd8 and m-c (74) 20200204-b95921676bb4

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry Tyson! 🙏 Thanks for doing that.