1465898 - Heap-buffer-underflow READ 8 from HalParent::RecvEnableSwitchNotifications

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

This is an IPC security issue.

The callstack looks roughly like:

  => https://searchfox.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#434
  => https://searchfox.org/mozilla-central/source/hal/Hal.cpp#631
  => https://searchfox.org/mozilla-central/source/hal/Hal.cpp#612

The ParamTraits for SwitchDevice allows it to be SWITCH_DEVICE_UNKNOWN, which has a value of -1:

  => https://searchfox.org/mozilla-central/source/hal/HalTypes.h#112-118
  => https://searchfox.org/mozilla-central/source/hal/HalTypes.h#26

The indexing in GetSwitchObserverList indexes sSwitchObserverLists with -1, leading to an invalid value:


==30762==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6040014f0848 at pc 0x7f6c8e19e492 bp 0x7ffda741b090 sp 0x7ffda741b088
READ of size 8 at 0x6040014f0848 thread T0
    #0 0x7f6c8e19e491 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length() const /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:371:37
    #1 0x7f6c8fb7a5cf in mozilla::Observer<mozilla::hal::SwitchEvent>** nsTArray_Impl<mozilla::Observer<mozilla::hal::SwitchEvent>*, nsTArrayInfallibleAllocator>::AppendElement<mozilla::Observer<mozilla::hal::SwitchEvent>*&, nsTArrayInfallibleAllocator>(mozilla::Observer<mozilla::hal::SwitchEvent>*&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2289:10
    #2 0x7f6c8fb73ca5 in mozilla::ObserverList<mozilla::hal::SwitchEvent>::AddObserver(mozilla::Observer<mozilla::hal::SwitchEvent>*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Observer.h:51:16
    #3 0x7f6c8fb716ad in mozilla::hal::RegisterSwitchObserver(mozilla::hal::SwitchDevice, mozilla::Observer<mozilla::hal::SwitchEvent>*) /home/osboxes/mozilla-central/hal/Hal.cpp:632:12
    #4 0x7f6c8fb8369d in mozilla::hal_sandbox::HalParent::RecvEnableSwitchNotifications(mozilla::hal::SwitchDevice const&) /home/osboxes/mozilla-central/hal/sandbox/SandboxHal.cpp:434:5
    #5 0x7f6c8f58fb30 in mozilla::hal_sandbox::PHalParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PHalParent.cpp:553:20
    #6 0x7f6c8f407857 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:3501:28
    #7 0x7f6c98c91d52 in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, nsTArray<nsTString<char> > const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:49:18
    #8 0x7f6c98c918ea in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:33:3
    #9 0x5897b0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #10 0x58943b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3
    #11 0x589ec1 in fuzzer::Fuzzer::MutateAndTestOne() /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:650:19
    #12 0x58a487 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:773:5
    #13 0x57d35c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #14 0x7f6c97e6d6a2 in mozilla::FuzzerRunner::Run(int*, char***) /home/osboxes/mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #15 0x7f6c97dab796 in XREMain::XRE_mainStartup(bool*) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:3923:35
    #16 0x7f6c97db72cb in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4863:12
    #17 0x7f6c97db8166 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4970:21
    #18 0x531cfb in do_main(int, char**, char**) /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:233:22
    #19 0x53157a in main /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:306:16
    #20 0x7f6cafb21b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #21 0x4349d9 in _start (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4349d9)

0x6040014f0848 is located 8 bytes to the left of 40-byte region [0x6040014f0850,0x6040014f0878)
allocated by thread T0 here:
    #0 0x4f4890 in __interceptor_malloc (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4f4890)
    #1 0x53343d in moz_xmalloc /home/osboxes/mozilla-central/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7f6c8fb71742 in operator new[](unsigned long) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:168:12
    #3 0x7f6c8fb71742 in mozilla::hal::GetSwitchObserverList(mozilla::hal::SwitchDevice) /home/osboxes/mozilla-central/hal/Hal.cpp:610
    #4 0x7f6c8fb7169f in mozilla::hal::RegisterSwitchObserver(mozilla::hal::SwitchDevice, mozilla::Observer<mozilla::hal::SwitchEvent>*) /home/osboxes/mozilla-central/hal/Hal.cpp:631:34
    #5 0x7f6c8fb8369d in mozilla::hal_sandbox::HalParent::RecvEnableSwitchNotifications(mozilla::hal::SwitchDevice const&) /home/osboxes/mozilla-central/hal/sandbox/SandboxHal.cpp:434:5
    #6 0x7f6c8f58fb30 in mozilla::hal_sandbox::PHalParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PHalParent.cpp:553:20
    #7 0x7f6c8f407857 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:3501:28
    #8 0x7f6c98c91d52 in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, nsTArray<nsTString<char> > const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:49:18
    #9 0x7f6c98c918ea in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:33:3
    #10 0x5897b0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #11 0x58943b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3
    #12 0x589ec1 in fuzzer::Fuzzer::MutateAndTestOne() /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:650:19
    #13 0x58a487 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:773:5
    #14 0x57d35c in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #15 0x7f6c97e6d6a2 in mozilla::FuzzerRunner::Run(int*, char***) /home/osboxes/mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #16 0x7f6c97dab796 in XREMain::XRE_mainStartup(bool*) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:3923:35
    #17 0x7f6c97db72cb in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4863:12
    #18 0x7f6c97db8166 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4970:21
    #19 0x531cfb in do_main(int, char**, char**) /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:233:22
    #20 0x53157a in main /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:306:16
    #21 0x7f6cafb21b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:371:37 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length() const
Shadow bytes around the buggy address:
  0x0c08802960b0: fa fa fa fa fa fa fa fa fa fa fd fd fd fd fd fa
  0x0c08802960c0: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fa
  0x0c08802960d0: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fa
  0x0c08802960e0: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00 fa
  0x0c08802960f0: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd fd fd
=>0x0c0880296100: fa fa 00 00 00 00 00 fa fa[fa]ac 00 00 00 00 fa
  0x0c0880296110: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00 fa
  0x0c0880296120: fa fa fa fa fa fa fa fa fa fa fd fd fd fd fd fa
  0x0c0880296130: fa fa fa fa fa fa fa fa fa fa 00 00 00 00 00 fa
  0x0c0880296140: fa fa 00 00 00 00 00 03 fa fa 00 00 00 00 00 00
  0x0c0880296150: fa fa 00 00 00 00 00 03 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
==30762==ABORTING
MS: 2 PersAutoDict-ChangeBit- DE: "\x00\x01-4"-; base unit: c55e8e0af533f3b238b3f01600ad1857e9f66ec6
0x20,0x0,0x0,0x0,0xff,0xff,0xff,0xff,0x1a,0x0,0x45,0x0,0x0,0x4d,0x69,0x6e,0xff,0x6,0x0,0x45,0x0,0x20,0x4d,0xff,0xff,0xff,0xff,0xff,0xff,0x9f,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x0,0x1,0x2d,0x34,0xff,0xff,0xff,0x2d,0x0,0x4e,0x0,0x0,0x0,0x56,0x6,0xff,0xff,0xff,0xf7,0xff,0xff,0xff,0xff,0xff,0xff,0x6b,0x6e,0x50,0x42,
 \x00\x00\x00\xff\xff\xff\xff\x1a\x00E\x00\x00Min\xff\x06\x00E\x00 M\xff\xff\xff\xff\xff\xff\x9f\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\x00\x01-4\xff\xff\xff-\x00N\x00\x00\x00V\x06\xff\xff\xff\xf7\xff\xff\xff\xff\xff\xffknPB
artifact_prefix='/home/osboxes/content-parent-artifacts/'; Test unit written to /home/osboxes/content-parent-artifacts/crash-8de26ad2671e041794f6fee857b17fc98d803cc7
Base64: IAAAAP////8aAEUAAE1pbv8GAEUAIE3///////+f////////////////AAEtNP///y0ATgAAAFYG////9////////2tuUEI=


Sadly I don't have a clean minimal reproducer, but this is hopefully straightforward to follow.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

gsvelto, do you know who a good owner for this might be?

Comment 2

[Gabriele Svelto [:gsvelto]]

That would be me because I also know what that stuff is for. Or more specifically, what it _was_ for: audio management on phones with Firefox OS. The only consumers AFAIR were gonk's audio channel manager and the FM radio DOM interface. Both have been removed so I think we can safely get rid of this.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Cool. I love deleting code as a resolution for security issues! :D

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Remove unused code for managing physical audio devices

This removes all that dead code. The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0287c934ee91f67ee08a62a3e6e889fa3fa8938

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8983197 [details] [diff] [review]
[PATCH] Remove unused code for managing physical audio devices

I don't know much about this code, but I can confirm this definitely deletes the vulnerable code!

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
> Comment on attachment 8983197 [details] [diff] [review]
> [PATCH] Remove unused code for managing physical audio devices
> 
> I don't know much about this code, but I can confirm this definitely deletes
> the vulnerable code!

HAL is mostly leftover stuff from the B2G / Firefox OS days. We haven't added any code to it in ages and we'll probably fold it into another component sooner or later. I'm basically the only reviewer left for the component so I have to ask someone else when I cut some code out ;)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2fa6b2a4ead

This is a sec-high affecting every supported branch AFAICT. Shouldn't this have gotten sec-approval first?

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/f2fa6b2a4ead
> 
> This is a sec-high affecting every supported branch AFAICT. Shouldn't this
> have gotten sec-approval first?

I believe the security rating was based on the fact that the code was presumed to be in use, but the affected code has been dead for multiple versions and was never enabled on Firefox desktop anyway.

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

The threat model for this report was sandbox escapes; I believe a compromised child process would have been able to reach this code?

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #9)
> The threat model for this report was sandbox escapes; I believe a
> compromised child process would have been able to reach this code?

I don't know, but if that's the case then I apologize for not having followed the procedure correctly.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please fill out a sec-approval request on this patch. Please also request Beta/ESR60/ESR52 approval on the patch.

Comment 12

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8983197 [details] [diff] [review]
[PATCH] Remove unused code for managing physical audio devices

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily, the patch removes a large amount of code including the vulnerable one but doesn't call it out specifically.

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 branches.

If not all supported branches, which bug introduced the flaw?

N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This code has not been touched at least since version 48 so backporting the change to older branches should be trivial.

How likely is this patch to cause regressions; how much testing does it need?

This patch only removes unused code so it's unlikely to cause regressions.

Comment 13

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8983197 [details] [diff] [review]
[PATCH] Remove unused code for managing physical audio devices

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: A compromised content process could abuse the IPC code to read memory outside of an array held by the main process.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: None at all
[Why is the change risky/not risky?]: It only removes dead code
[String changes made/needed]: None

Comment 14

[Al Billings [:abillings - ex-MoCo]]

(In reply to Gabriele Svelto [:gsvelto] from comment #12)

> Which older supported branches are affected by this flaw?
> 
> All branches.

But if it is unused code, how does it affect the branches?

Comment 15

[Daniel Veditz [:dveditz]]

Assuming another exploit loose in a sandboxed process, it could call this code to trigger this bug as a sandbox escape.

How good was the sandbox on ESR-52? Might have been OK on Windows in which case this fix might be meaningful there.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8983197 [details] [diff] [review]
[PATCH] Remove unused code for managing physical audio devices

Approvals given.

Comment 17

[Gabriele Svelto [:gsvelto]]

(In reply to Al Billings [:abillings] from comment #14)
> But if it is unused code, how does it affect the branches?

The code was present in all branches but only ever used in Firefox OS builds. Still it was included into desktop builds and because of that a compromised content process might exploit it by sending a malicious IPC message to the parent.

Comment 18

[Alex Gaynor [:Alex_Gaynor]]

The sandbox on ESR52 was not particularly good; I would probably not have described it as a strong security boundary. That said, we've backported other IPC security fixes.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c12ddf5d3f7
https://hg.mozilla.org/releases/mozilla-esr60/rev/ccd106658ca2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for ESR52.

Comment 21

[Gabriele Svelto [:gsvelto]]

The rebased patch needs some extra testing, it'll be ready shortly.

Comment 22

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH esr52] Remove unused code for managing physical audio devices

Here's the backported patch, sorry that it took so long.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e8e9e1ef79f2