Closed Bug 1469309 Opened Last year Closed Last year

Heap-buffer-underflow READ 8 from HalParent::RecvEnableSensorNotifications

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ fixed
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 + fixed
firefox63 + fixed

People

(Reporter: Alex_Gaynor, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1465898 +++

This is basically the same as that one but in RecvEnableSensorNotifications instead:

==64==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60d0000dbd88 at pc 0x7f1a02585f14 bp 0x7ffdee56ffb0 sp 0x7ffdee56ffa8
READ of size 8 at 0x60d0000dbd88 thread T0
SCARINESS: 33 (8-byte-read-heap-buffer-overflow-far-from-bounds)
    #0 0x7f1a02585f13 in Length /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:371:37
    #1 0x7f1a02585f13 in AppendElement<mozilla::Observer<mozilla::hal::SensorData> *&, nsTArrayInfallibleAllocator> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:2289
    #2 0x7f1a02585f13 in AddObserver /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Observer.h:51
    #3 0x7f1a02585f13 in mozilla::hal::RegisterSensorObserver(mozilla::hal::SensorType, mozilla::Observer<mozilla::hal::SensorData>*) /builds/worker/workspace/build/src/hal/Hal.cpp:421
    #4 0x7f1a025995fd in mozilla::hal_sandbox::HalParent::RecvEnableSensorNotifications(mozilla::hal::SensorType const&) /builds/worker/workspace/build/src/hal/sandbox/SandboxHal.cpp:359:5
    #5 0x7f1a01ce632b in mozilla::hal_sandbox::PHalParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PHalParent.cpp:530:20
    #6 0x7f1a01aba106 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentParent.cpp:3501:28
    #7 0x7f1a10592fe8 in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, nsTArray<nsTString<char> > const&) /builds/worker/workspace/build/src/obj-firefox/dist/include/ProtocolFuzzer.h:49:18
    #8 0x7f1a10592b0a in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:33:3
    #9 0x5925dd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #10 0x591e5b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3
    #11 0x59334d in fuzzer::Fuzzer::MutateAndTestOne() /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:650:19
    #12 0x593d05 in fuzzer::Fuzzer::Loop(std::vector<std::string, fuzzer::fuzzer_allocator<std::string> > const&) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:773:5
    #13 0x58b3a5 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #14 0x7f1a0e8525b6 in mozilla::FuzzerRunner::Run(int*, char***) /builds/worker/workspace/build/src/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #15 0x7f1a0e76f81a in XREMain::XRE_mainStartup(bool*) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:3932:35
    #16 0x7f1a0e7843e3 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4876:12
    #17 0x7f1a0e785f5e in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4983:21
    #18 0x4f52dc in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:233:22
    #19 0x4f52dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:311
    #20 0x7f1a2821a1c0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x211c0)
    #21 0x4248bc in _start (/home/worker/firefox/firefox+0x4248bc)

DEDUP_TOKEN: Length
0x60d0000dbd88 is located 8 bytes to the left of 136-byte region [0x60d0000dbd90,0x60d0000dbe18)
allocated by thread T0 here:
    #0 0x4c5003 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x4f636d in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7f1a02585c3f in operator new[] /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:168:12
    #3 0x7f1a02585c3f in GetSensorObservers /builds/worker/workspace/build/src/hal/Hal.cpp:410
    #4 0x7f1a02585c3f in mozilla::hal::RegisterSensorObserver(mozilla::hal::SensorType, mozilla::Observer<mozilla::hal::SensorData>*) /builds/worker/workspace/build/src/hal/Hal.cpp:417
    #5 0x7f1a025995fd in mozilla::hal_sandbox::HalParent::RecvEnableSensorNotifications(mozilla::hal::SensorType const&) /builds/worker/workspace/build/src/hal/sandbox/SandboxHal.cpp:359:5
    #6 0x7f1a01ce632b in mozilla::hal_sandbox::PHalParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PHalParent.cpp:530:20
    #7 0x7f1a01aba106 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentParent.cpp:3501:28
    #8 0x7f1a10592fe8 in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, nsTArray<nsTString<char> > const&) /builds/worker/workspace/build/src/obj-firefox/dist/include/ProtocolFuzzer.h:49:18
    #9 0x7f1a10592b0a in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:33:3
    #10 0x5925dd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #11 0x591e5b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3
    #12 0x59334d in fuzzer::Fuzzer::MutateAndTestOne() /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:650:19
    #13 0x593d05 in fuzzer::Fuzzer::Loop(std::vector<std::string, fuzzer::fuzzer_allocator<std::string> > const&) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:773:5
    #14 0x58b3a5 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #15 0x7f1a0e8525b6 in mozilla::FuzzerRunner::Run(int*, char***) /builds/worker/workspace/build/src/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #16 0x7f1a0e76f81a in XREMain::XRE_mainStartup(bool*) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:3932:35
    #17 0x7f1a0e7843e3 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4876:12
    #18 0x7f1a0e785f5e in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4983:21
    #19 0x4f52dc in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:233:22
    #20 0x4f52dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:311
    #21 0x7f1a2821a1c0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x211c0)

DEDUP_TOKEN: malloc
SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:371:37 in Length
Shadow bytes around the buggy address:
  0x0c1a80013760: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1a80013770: 00 00 00 00 00 fa fa fa fa fa fa fa fa fa 00 00
  0x0c1a80013780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c1a80013790: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1a800137a0: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa
=>0x0c1a800137b0: fa[fa]ac 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1a800137c0: 00 00 00 fa fa fa fa fa fa fa fa fa 00 00 00 00
  0x0c1a800137d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 07 fa fa
  0x0c1a800137e0: fa fa fa fa fa fa 00 00 00 00 00 00 00 00 00 00
  0x0c1a800137f0: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c1a80013800: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
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

Command: ./firefox/firefox -print_pcs=1 -handle_segv=0 -handle_bus=0 -handle_abrt=0 ./corpora/ -handle_ill=0 -handle_fpe=0

==64==ABORTING
Group: core-security → dom-core-security
OK, let's see what's going on. Most likely this code is also unused.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Nope, this is still used and needs a proper fix.
Attachment #8986120 - Flags: review?(agaynor)
It's going to be too late to include this bug in this cycle (we're already building release candidates). This'll need to wait for Fx62/ESR 60.2.
Comment on attachment 8986120 [details] [diff] [review]
[PATCH] Only (un)register valid sensors

Review of attachment 8986120 [details] [diff] [review]:
-----------------------------------------------------------------

Patch will fix this issue.

Do any IPC methods actually allow a SensorType of SENSOR_UNKNOWN? Would it make sense to change the ParamTraits to disallow deserializing SENSOR_UNKNOWN?
Attachment #8986120 - Flags: review?(agaynor) → review+
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
> Do any IPC methods actually allow a SensorType of SENSOR_UNKNOWN? Would it
> make sense to change the ParamTraits to disallow deserializing
> SENSOR_UNKNOWN?

I don't think SENSOR_UNKNOWN is used anywhere else but I'm not sure. Would excluding its value from the serializer solve the issue at hand without the need for an explicit check?
Yes, if the ParamTraits rejects SENSOR_UNKNOWN then it can never flow from IPC to this code.

It looks like SENSOR_UNKNOWN is basically unused entirely, so maybe we can just remove it?
Yeah, let's do that. I'll prepare a new patch.
This is definitely shorter and less error-prone in case we add new IPC calls. Is this good enough?
Attachment #8986190 - Flags: feedback?(agaynor)
Comment on attachment 8986190 [details] [diff] [review]
[PATCH alt] Remove an unused sensor type

Review of attachment 8986190 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Maybe consider adding `SENSOR_FIRST = SENSOR_ORIENTATION` so it's clear why you're using that value, but otherwise shipit!
Attachment #8986190 - Flags: feedback?(agaynor) → feedback+
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #10)
> LGTM. Maybe consider adding `SENSOR_FIRST = SENSOR_ORIENTATION` so it's
> clear why you're using that value, but otherwise shipit!

Sure, I'll do that and then we can land this.
I found an even better solution than declaring a constant for the first member of the enum: just use an enumerated range to iterate over them, it doesn't even need to cast type to an integer and back because it uses the correct type automatically.
Attachment #8986120 - Attachment is obsolete: true
Attachment #8986190 - Attachment is obsolete: true
Attachment #8986564 - Flags: review?(agaynor)
Attachment #8986564 - Flags: review?(agaynor) → review+
Comment on attachment 8986564 [details] [diff] [review]
[PATCH] Remove an unused sensor type

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

It would not be rather obvious, the patch only removes an unused element from an enum while the attack involves first compromising a content process, then sending a specifically crafted IPC message including that value.

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 are affected by this issue.

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

This issue was introduced years ago, in bug 697641.

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

ESR version 52 probably requires a dedicated backport, it shouldn't be hard to write it though.

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

This patch is trivial and shouldn't cause regressions.
Attachment #8986564 - Flags: sec-approval?
Alex, do I need to do anything else to get sec-approval for this patch?
Flags: needinfo?(agaynor)
Nope. Al or Dan will probably be around shortly to grant it; I imagine they were slowed down by the release yesterday (there's a bunch of work that goes into preparing all the security advisories for it).
Flags: needinfo?(agaynor)
OK, thanks!
sec-approval+ for trunk.
We'll want beta and ESR60 patches as well.
Attachment #8986564 - Flags: sec-approval? → sec-approval+
Landed on inbound, the patch applies cleanly to beta and I'll provide a version for ESR60.
Backported patch for ESR60.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd88f4a8884fe4327fc08588dce1fe221c45bcb
https://hg.mozilla.org/mozilla-central/rev/3dd88f4a8884
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(In reply to Gabriele Svelto [:gsvelto] from comment #19)
> Created attachment 8988884 [details] [diff] [review]
> [PATCH ESR60] Remove an unused sensor type
> 
> Backported patch for ESR60.

Hi Gabriele, could you make uplift requests for beta an ESR60 please? Thanks
Flags: needinfo?(gsvelto)
Comment on attachment 8986564 [details] [diff] [review]
[PATCH] Remove an unused sensor type

Approval Request Comment
[Feature/Bug causing the regression]: bug 697641
[User impact if declined]: A compromised content process can do an out-of-bounds read in the parent process memory.
[Is this code covered by automated tests?]: No, this has been manually tested
[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]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It only removes an entry from an enum used to index an array. The entry corresponding with the out-of-bounds read to be precise.
[String changes made/needed]: None
Flags: needinfo?(gsvelto)
Attachment #8986564 - Flags: approval-mozilla-beta?
Comment on attachment 8986564 [details] [diff] [review]
[PATCH] Remove an unused sensor type

See comment above
Attachment #8986564 - Flags: approval-mozilla-esr60?
Comment on attachment 8986564 [details] [diff] [review]
[PATCH] Remove an unused sensor type

Aiming this fix at beta 7.
Attachment #8986564 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Attachment #8986564 - Flags: approval-mozilla-esr60?
Comment on attachment 8988884 [details] [diff] [review]
[PATCH ESR60] Remove an unused sensor type

Fixes a sec issue with an unused sensor type. Approved for ESR 60.2.
Attachment #8988884 - Flags: approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.