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

This was discovered by the IPC fuzzer. Here's one example sequence of events leading to a UAF, this example uses WakeLocks, but you can get basically the same result with any of the other observer types too.

1) Allocate a HalParent via ContentParent.
2) Send EnableWakeLockNotifications message, this registers the HalParent as an observer: https://searchfox.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#390
3) Send EnableWakeLockNotifications message again, this registers the HalParent a second time
4) Deallocate the HalParent by sending __delete__. This will unregister the HalParent, however this only removes _one_ instance of it from the observer array: https://searchfox.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#239
5) Allocate a new HalParent
6) Do something to trigger notification on the observer array, such as send a ModifyWakeLock message: https://searchfox.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#382

At this point the array will contain a reference to the now deallocated origina HalParent, and try to invoke the Notify vtable method on it, UAF.

Same sequence of steps works for any of the observer array types. I suspect this pattern likely exists in other places in our code, particularly IPC.

Some options for fixing this:

1) HalParent could track what it's registered for and not re-register if it gets a duplicate IPC message
2) the observer list could de-duplicate on registration
3) the observer list could remove all instances on de-registration

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

Forgot to include, here's what an ASAN error looks like (probably not reported as a UAF because too many allocations in between):

==1444==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700044f7b0 at pc 0x7f96cae8c6a9 bp 0x7fff9bda99d0 sp 0x7fff9bda99c8
READ of size 8 at 0x60700044f7b0 thread T0
SCARINESS: 33 (8-byte-read-heap-buffer-overflow-far-from-bounds)
    #0 0x7f96cae8c6a8 in mozilla::ObserverList<mozilla::hal::WakeLockInformation>::Broadcast(mozilla::hal::WakeLockInformation const&) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Observer.h:90:28
    #1 0x7f96cae8d8b3 in mozilla::hal_impl::ModifyWakeLock(nsTSubstring<char16_t> const&, mozilla::hal::WakeLockControl, mozilla::hal::WakeLockControl, unsigned long) /builds/worker/workspace/build/src/hal/HalWakeLock.cpp:255:5
    #2 0x7f96cae993fe in mozilla::hal_sandbox::HalParent::RecvModifyWakeLock(nsTString<char16_t> const&, mozilla::hal::WakeLockControl const&, mozilla::hal::WakeLockControl const&, unsigned long const&) /builds/worker/workspace/build/src/hal/sandbox/SandboxHal.cpp:382:5
    #3 0x7f96ca5e7cc0 in mozilla::hal_sandbox::PHalParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PHalParent.cpp:403:20
    #4 0x7f96ca3ba106 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentParent.cpp:3501:28
    #5 0x7f96d8e92fe8 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
    #6 0x7f96d8e92b0a in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:33:3
    #7 0x5925dd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #8 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
    #9 0x59334d in fuzzer::Fuzzer::MutateAndTestOne() /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:650:19
    #10 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
    #11 0x58b3a5 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #12 0x7f96d71525b6 in mozilla::FuzzerRunner::Run(int*, char***) /builds/worker/workspace/build/src/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #13 0x7f96d706f81a in XREMain::XRE_mainStartup(bool*) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:3932:35
    #14 0x7f96d70843e3 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4876:12
    #15 0x7f96d7085f5e in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4983:21
    #16 0x4f52dc in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:233:22
    #17 0x4f52dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:311
    #18 0x7f96f0ada1c0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x211c0)
    #19 0x4248bc in _start (/home/worker/firefox/firefox+0x4248bc)

DEDUP_TOKEN: mozilla::ObserverList<mozilla::hal::WakeLockInformation>::Broadcast(mozilla::hal::WakeLockInformation const&)
Address 0x60700044f7b0 is a wild pointer.
SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Observer.h:90:28 in mozilla::ObserverList<mozilla::hal::WakeLockInformation>::Broadcast(mozilla::hal::WakeLockInformation const&)
Shadow bytes around the buggy address:
  0x0c0e80081ea0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80081eb0: fa fa fa fa fa fa 00 00 00 00 00 00 00 00 01 fa
  0x0c0e80081ec0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80081ed0: fa fa 00 00 00 00 00 00 00 00 00 04 fa fa fa fa
  0x0c0e80081ee0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c0e80081ef0: fa fa fa fa fa fa[fa]fa fa fa fa fa fa fa fa fa
  0x0c0e80081f00: fa fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
  0x0c0e80081f10: 00 00 01 fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80081f20: fa fa fa fa fa fa 00 00 00 00 00 00 00 00 00 fa
  0x0c0e80081f30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80081f40: fa fa fa fa fa fa fa fa 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

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

==1444==ABORTING

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

I believe that the best course of action here would be to switch this code to use nsTObserverArray instead of the ObserverList so that we can use AppendElementUnlessExists() to ensure that the same observer is not added twice. nsTObserverArray has all the functionality of ObserverList plus it plays nice with adding and removing observers while iterating on the array. Since ObserverList is only used within the HAL code this means we could also get rid of it and trim our codebase a bit.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 3 • 6 years ago

This patch replaces the ObserverList implementation with one based on nsTObserverArray which has the ability to add unique observers. This is used to prevent the same observer from being registered twice with the HAL. Since nsTObserverArray also provides stable iteration I took the chance to trim down the ObserverList implementation and remove some unneeded dependencies.

	Nathan Froyd [:froydnj]
	Comment 4 • 6 years ago

Comment on attachment 8986939 [details] [diff] [review]
[PATCH] Prevent the HAL from registering duplicate observers

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

Thank you!

	Gabriele Svelto [:gsvelto] Assignee
	Comment 5 • 6 years ago

Comment on attachment 8986939 [details] [diff] [review]
[PATCH] Prevent the HAL from registering duplicate observers

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

Not easily, the patch doesn't mention that it's fixing an UAF issue and the logic required to trigger it is outside of the code that's being changed.

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?

This applies to all branches

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

This was introduced over 7 years ago in bug 678694

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

Backporting the patch is required and is a rather simple endeavor.

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

I manually tested the patch; the patch slightly changes the behavior of the affected code but it isn't likely to cause regressions.

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

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

	Gabriele Svelto [:gsvelto] Assignee
	Comment 7 • 6 years ago

I've just finished testing this locally on Windows and Mac, will land soon.

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

Landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/cd857f86810ce8178354841fbe99c8dc0eaf7492

Backed out for causing leaks on multiple tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/42d7dee84c8166701533e1af59f15fe3f1da7c28

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cd857f86810ce8178354841fbe99c8dc0eaf7492&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=185910725&repo=mozilla-inbound
After a mochitest headless dom/smil/test/ run:
[task 2018-07-02T09:54:24.228Z]     INFO - Stopping ssltunnel
[task 2018-07-02T09:54:24.249Z]     INFO - TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes
[task 2018-07-02T09:54:24.250Z]     INFO - TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes
[task 2018-07-02T09:54:24.251Z]     INFO - TEST-INFO | leakcheck | tab process: leak threshold set at 0 bytes
[task 2018-07-02T09:54:24.252Z]     INFO - TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes
[task 2018-07-02T09:54:24.253Z]     INFO - TEST-INFO | leakcheck | gpu process: leak threshold set at 0 bytes
[task 2018-07-02T09:54:24.254Z]     INFO - 
[task 2018-07-02T09:54:24.255Z]     INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 1074
[task 2018-07-02T09:54:24.256Z]     INFO - 
[task 2018-07-02T09:54:24.258Z]     INFO -      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
[task 2018-07-02T09:54:24.259Z]     INFO -      |                                      | Per-Inst   Leaked|   Total      Rem|
[task 2018-07-02T09:54:24.260Z]     INFO -    0 |TOTAL                                 |       39        8| 7832369        1|
[task 2018-07-02T09:54:24.264Z]     INFO - 1073 |nsTArray_base                         |        8        8| 4020422        1|
[task 2018-07-02T09:54:24.265Z]     INFO - 
[task 2018-07-02T09:54:24.266Z]     INFO - nsTraceRefcnt::DumpStatistics: 1140 entries
[task 2018-07-02T09:54:24.267Z]     INFO - TEST-INFO | leakcheck | tab process: leaked 1 nsTArray_base
[task 2018-07-02T09:54:24.268Z]    ERROR - TEST-UNEXPECTED-FAIL | leakcheck | tab process: 8 bytes leaked (nsTArray_base)

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

This is quite annoying, the leaks which I hadn't noticed during local testing are caused by statically allocated objects that are now storing an nsTArray instance directly. Ironically enough we weren't leaking because unregistering an observer detected the situation in which no observers were registered anymore and manually delete'd the array. Locally I'm now detecting four leaks but I could only fix two. If I can't find them soon enough I'll just modify the patch to more closely match the original code even though I don't like the manual allocation of the list one bit.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 10 • 6 years ago

I've identified what's causing the leaks and it's complicated... basically the service is often deregistering listeners deep during shutdown, so the observer lists can be re-created after they've already been destroyed (e.g. after ClearOnShutdown phase has executed). The original code special-cased this scenario by not allowing deregistrations when the list had already been destroyed.

To minimize risk here I've restored the original code into my patch so that I can land it. I will file a follow up to handle this more gracefully because it really isn't pretty.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6dea408b0ee63ec1c675b8b0293c0ee2d100dd
https://hg.mozilla.org/mozilla-central/rev/8e6dea408b0e

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

Please request Beta/ESR60 approval on this when you get a chance.

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

Comment on attachment 8986939 [details] [diff] [review]
[PATCH] Prevent the HAL from registering duplicate observers

Approval Request Comment
[Feature/Bug causing the regression]: bug 678694
[User impact if declined]: A compromised content process can force the parent process to read already freed memory
[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]: None
[Is the change risky?]: Slightly
[Why is the change risky/not risky?]: While landing a previous version of this patch we hit some (small) memory leaks, see comment 8. The updated version should be problem-free but some of this code gets called very late during shutdown which makes verifying corner cases tricky.
[String changes made/needed]:

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

Comment on attachment 8986939 [details] [diff] [review]
[PATCH] Prevent the HAL from registering duplicate observers

See comment above

	Gabriele Svelto [:gsvelto] Assignee
	Comment 15 • 6 years ago

The patch applies cleanly to both beta and esr60 using hg graft.

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

Comment on attachment 8986939 [details] [diff] [review]
[PATCH] Prevent the HAL from registering duplicate observers

Fix for a crash, has sec-approval, ok for uplift to beta 7 
We can be on the lookout for memory leaks/ new shutdown issues.

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

https://hg.mozilla.org/releases/mozilla-beta/rev/3da147dac512

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

Comment on attachment 8986939 [details] [diff] [review]
[PATCH] Prevent the HAL from registering duplicate observers

Fixes a sec-high. Approved for ESR 60.2.

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/f00c89b679b2