Alex Gaynor [:Alex_Gaynor] Reporter
	Description • 6 years ago

+++ 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

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 1 • 6 years ago

OK, let's see what's going on. Most likely this code is also unused.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 2 • 6 years ago

Nope, this is still used and needs a proper fix.

	Ryan VanderMeulen [:RyanVM]
	Comment 4 • 6 years ago

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.

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 5 • 6 years ago

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?

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 6 • 6 years ago

(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?

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 7 • 6 years ago

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?

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 8 • 6 years ago

Yeah, let's do that. I'll prepare a new patch.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 9 • 6 years ago

This is definitely shorter and less error-prone in case we add new IPC calls. Is this good enough?

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 10 • 6 years ago

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!

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 11 • 6 years ago

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

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 12 • 6 years ago

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.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 13 • 6 years ago

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.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 14 • 6 years ago

Alex, do I need to do anything else to get sec-approval for this patch?

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 15 • 6 years ago

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

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 16 • 6 years ago

OK, thanks!

	Al Billings [:abillings - ex-MoCo]
	Comment 17 • 6 years ago

sec-approval+ for trunk.
We'll want beta and ESR60 patches as well.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 18 • 6 years ago

Landed on inbound, the patch applies cleanly to beta and I'll provide a version for ESR60.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 19 • 6 years ago

Backported patch for ESR60.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 20 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd88f4a8884fe4327fc08588dce1fe221c45bcb
https://hg.mozilla.org/mozilla-central/rev/3dd88f4a8884

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 21 • 6 years ago

(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

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 22 • 6 years ago

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

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 23 • 6 years ago

Comment on attachment 8986564 [details] [diff] [review]
[PATCH] Remove an unused sensor type

See comment above

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 24 • 6 years ago

Comment on attachment 8986564 [details] [diff] [review]
[PATCH] Remove an unused sensor type

Aiming this fix at beta 7.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 25 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/0915e7335401

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/0e6190b701db