Closed
Bug 1469309
Opened 6 years ago
Closed 6 years ago
Heap-buffer-underflow READ 8 from HalParent::RecvEnableSensorNotifications
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
People
(Reporter: Alex_Gaynor, Assigned: gsvelto)
References
(Blocks 1 open bug)
Details
(Keywords: csectype-bounds, csectype-sandbox-escape, sec-high, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])
Attachments
(2 files, 2 obsolete files)
3.65 KB,
patch
|
Alex_Gaynor
:
review+
lizzard
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.66 KB,
patch
|
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
+++ 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
Updated•6 years ago
|
Group: core-security → dom-core-security
Assignee | ||
Comment 1•6 years ago
|
||
OK, let's see what's going on. Most likely this code is also unused.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
Nope, this is still used and needs a proper fix.
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8986120 -
Flags: review?(agaynor)
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.
status-firefox60:
--- → wontfix
status-firefox61:
--- → wontfix
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → affected
tracking-firefox62:
--- → +
tracking-firefox-esr60:
--- → 62+
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?
Attachment #8986120 -
Flags: review?(agaynor) → review+
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?
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?
Assignee | ||
Comment 8•6 years ago
|
||
Yeah, let's do that. I'll prepare a new patch.
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?
Attachment #8986190 -
Flags: feedback?(agaynor)
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!
Attachment #8986190 -
Flags: feedback?(agaynor) → feedback+
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.
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.
Attachment #8986120 -
Attachment is obsolete: true
Attachment #8986190 -
Attachment is obsolete: true
Attachment #8986564 -
Flags: review?(agaynor)
Reporter | ||
Updated•6 years ago
|
Attachment #8986564 -
Flags: review?(agaynor) → review+
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.
Attachment #8986564 -
Flags: sec-approval?
Assignee | ||
Comment 14•6 years ago
|
||
Alex, do I need to do anything else to get sec-approval for this patch?
Flags: needinfo?(agaynor)
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).
Flags: needinfo?(agaynor)
Assignee | ||
Comment 16•6 years ago
|
||
OK, thanks!
Comment 17•6 years ago
|
||
sec-approval+ for trunk.
We'll want beta and ESR60 patches as well.
status-firefox63:
--- → affected
tracking-firefox63:
--- → +
Updated•6 years ago
|
Attachment #8986564 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 18•6 years ago
|
||
Landed on inbound, the patch applies cleanly to beta and I'll provide a version for ESR60.
Assignee | ||
Comment 19•6 years ago
|
||
Backported patch for ESR60.
Comment 20•6 years ago
|
||
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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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
Flags: needinfo?(gsvelto)
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
Flags: needinfo?(gsvelto)
Attachment #8986564 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 23•6 years ago
|
||
Comment on attachment 8986564 [details] [diff] [review]
[PATCH] Remove an unused sensor type
See comment above
Attachment #8986564 -
Flags: approval-mozilla-esr60?
Comment 24•6 years ago
|
||
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+
Comment 25•6 years ago
|
||
uplift |
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•6 years ago
|
Attachment #8986564 -
Flags: approval-mozilla-esr60?
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.
Attachment #8988884 -
Flags: approval-mozilla-esr60+
Comment 27•6 years ago
|
||
uplift |
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Updated•5 years ago
|
Group: core-security-release
Updated•3 years ago
|
Keywords: csectype-sandbox-escape
You need to log in
before you can comment on or make changes to this bug.
Description
•