Closed Bug 1387659 Opened 7 years ago Closed 7 years ago

heap-use-after-free in [@ GetLayerManager]

Categories

(Core :: Graphics, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: tsmith, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [adv-main56+])

Attachments

(2 files, 2 obsolete files)

Attached file log.txt
This popped up while fuzzing video. I am working on collecting a test case.

==11736==ERROR: AddressSanitizer: heap-use-after-free on address 0x619003741098 at pc 0x7f61420db42e bp 0x7f6123cf2ad0 sp 0x7f6123cf2ac8
READ of size 8 at 0x619003741098 thread T34 (Compositor)
    #0 0x7f61420db42d in Manager /home/worker/workspace/build/src/obj-firefox/dist/include/Layers.h:852:36
    #1 0x7f61420db42d in GetLayerManager /home/worker/workspace/build/src/gfx/layers/composite/CompositableHost.cpp:171
    #2 0x7f61420db42d in mozilla::layers::ImageHost::UseTextureHost(nsTArray<mozilla::layers::CompositableHost::TimedTexture> const&) /home/worker/workspace/build/src/gfx/layers/composite/ImageHost.cpp:77
    #3 0x7f614213cbce in mozilla::layers::CompositableParentManager::ReceiveCompositableUpdate(mozilla::layers::CompositableOperation const&) /home/worker/workspace/build/src/gfx/layers/ipc/CompositableTransactionParent.cpp:168:23
    #4 0x7f614218ba58 in mozilla::layers::ImageBridgeParent::RecvUpdate(nsTArray<mozilla::layers::CompositableOperation>&&, nsTArray<mozilla::layers::OpDestroy>&&, unsigned long const&) /home/worker/workspace/build/src/gfx/layers/ipc/ImageBridgeParent.cpp:197:10
    #5 0x7f6140de861d in mozilla::layers::PImageBridgeParent::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PImageBridgeParent.cpp:296:20
    #6 0x7f6140bfe7ee in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2092:25
    #7 0x7f6140bfb934 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2018:17
    #8 0x7f6140bfd214 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1887:5
    #9 0x7f6140bfd7f8 in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1920:15
    #10 0x7f6140b6aeb3 in RunTask /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:452:9
    #11 0x7f6140b6aeb3 in DeferOrRunPendingTask /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:460
    #12 0x7f6140b6aeb3 in MessageLoop::DoWork() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:535
    #13 0x7f6140b6cb09 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/chromium/src/base/message_pump_default.cc:36:31
    #14 0x7f6140b68a8b in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #15 0x7f6140b68a8b in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #16 0x7f6140b68a8b in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #17 0x7f6140b86c39 in base::Thread::ThreadMain() /home/worker/workspace/build/src/ipc/chromium/src/base/thread.cc:181:16
    #18 0x7f6140b76f4c in ThreadFunc(void*) /home/worker/workspace/build/src/ipc/chromium/src/base/platform_thread_posix.cc:38:13
    #19 0x7f615dcd26b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #20 0x7f615cd5b3dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Milan: Tyson's not going to be able to capture a testcase here. Is the log at all useful in finding a bug? If not we can close this as incomplete
Flags: needinfo?(milan)
Sotaro, it looks lije the LayerManager has been deleted (not just in the destroyed state), can you think of a way the above stack helps us?
Flags: needinfo?(milan) → needinfo?(sotaro.ikeda.g)
It seems that there is a case that is not detached correctly.
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(sotaro.ikeda.g)
:tsmith, is there a specific STR to reproduce the problem?
Flags: needinfo?(twsmith)
It seems better to address the problem similar way to Bug 1307458.
Attachment #8897742 - Flags: review?(nical.bugzilla)
Comment on attachment 8897742 [details] [diff] [review]
patch - Fix ClearCachedResources() calling

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

I don't have access to Bug 1307458, so I am missing context about the issue and the approach you chose to fix it. Could you provide some more details here?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +1439,5 @@
>  LayerComposite::Destroy()
>  {
>    if (!mDestroyed) {
>      mDestroyed = true;
> +    //CleanupResources();

Is this intentional?
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> :tsmith, is there a specific STR to reproduce the problem?

No sorry nothing that I found will reproduce the issue. I was opening a new tab playing a video and closing the tab repeatedly.
Flags: needinfo?(twsmith)
Attachment #8897742 - Flags: review?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #8)
> Comment on attachment 8897742 [details] [diff] [review]
> patch - Fix ClearCachedResources() calling
> 
> Review of attachment 8897742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have access to Bug 1307458, so I am missing context about the issue
> and the approach you chose to fix it. Could you provide some more details
> here?
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +1439,5 @@
> >  LayerComposite::Destroy()
> >  {
> >    if (!mDestroyed) {
> >      mDestroyed = true;
> > +    //CleanupResources();
> 
> Is this intentional?

When I created the patch, I was going to remove the call, but I did not conclude it. I need to re-think about it.
Attachment #8897742 - Attachment is obsolete: true
From attachment 8894034 [details], it seems that layer manager was already destructed. LayerManagerComposite was destructed only by  CompositorBridgeParent::StopAndClearResources(). Therefore, from it, there seemed to be existed a layer that hold the destructed pointer of LayerManagerComposite even after CompositorBridgeParent::StopAndClearResources() call.

   https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#433

From it, from it I thought there is a layer that did not clean up its resources that belonged to CompositorBridgeParent. Current implementation tried to clean up layer's tree resources by layer tree traversal. To make clear that all belonged layers clean its resources, it seems better to clean up the resources by mLayerMap.Iter() instead of layer tree traversal. Similar thing was done by Bug 1307458.
But during looking into Bug 1390731, I found another route to cause holding LayerManagerComposite pointer after the CompositorBridgeParent::StopAndClearResources(). LayerState destruction does not force to release LayerManagerComposite pointer, and LayerTransactionParent and its layers could exist until its destruction.

LayerState destruction sequence was like the following.

RenderFrameParent::ActorDestroy(ActorDestroyReason why)
->GPUProcessManager::UnmapLayerTreeId()
->mGPUChild->SendRemoveLayerTreeIdMapping();
->GPUParent::RecvRemoveLayerTreeIdMapping()
->CompositorBridgeParent::DeallocateLayerTreeId();
->CompositorLoop()->PostTask(NewRunnableFunction(&EraseLayerState, aId));
->EraseLayerState(uint64_t aId)
attachment 8898654 [details] [diff] [review] changed a way of fixing the problem. attachment 8897742 [details] [diff] [review] tried to clear all layers resources. Instread attachment 8898654 [details] [diff] [review] tried to address the problem by clearing LayerManager pointer of all layers when the pointer becomes unnecessary to avoid stale pointer.
attachment 8898654 [details] [diff] [review] tried to address the root cause.
From attachment 8894034 [details], pointer of Layer that was held by ImageHost was invalid. The pointer should be cleaned befpre deleting ImageLayerComposite. LayerTransactionParent::RecvReleaseLayer() actually do it by calling ImageLayerComposite::Disconnect().
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayerTransactionParent.cpp#1035

By attachment 8894034 [details] the ImageLayerComposite was deleted in ContainerLayerComposite::~ContainerLayerComposite(). It means that LayerTransactionParent already released a ref of ImageLayerComposite. From it, the ImageLayerComposite seemed not get ImageLayerComposite::Disconnect() called.

It means there is a route that LayerTransactionParent released the ImageLayerComposite without calling ImageLayerComposite::Disconnect(). LayerTransactionParent::RecvReleaseLayer() seemed not called for the ImageLayerComposite.
Attachment #8898654 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> 
> It means there is a route that LayerTransactionParent released the
> ImageLayerComposite without calling ImageLayerComposite::Disconnect().
> LayerTransactionParent::RecvReleaseLayer() seemed not called for the
> ImageLayerComposite.

When LayerTransactionParent is destructed, it does not call Layer::Disconnect() for layers in mLayerMap.
Attachment #8899053 - Flags: review?(nical.bugzilla)
Attachment #8899053 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/b1fdab6aa8ef
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: gfx-core-security → core-security-release
What other branches does this affect? Any idea on a severity rating? We probably should have had those answers before landing this :\
https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(sotaro.ikeda.g)
Comment on attachment 8899053 [details] [diff] [review]
patch - Add Layer::Disconnect() calling in LayerTransactionParent::Destroy()

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
 > It is not easy.

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?
 > It exist since firefox 53

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  >  It is easy to back port.


How likely is this patch to cause regressions; how much testing does it need?
 > It is simple fix. And it just add function calls as before. It seems not likely to cause the regression.
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8899053 - Flags: sec-approval?
I think you can just go ahead and nominate for Beta approval too while you're at it :)
Blocks: 1323539
Comment on attachment 8899053 [details] [diff] [review]
patch - Add Layer::Disconnect() calling in LayerTransactionParent::Destroy()

Approval Request Comment
[Feature/Bug causing the regression]: bug 1323539
[User impact if declined]: It might rarely cause a crash during closing a Tab.
[Is this code covered by automated tests?]: Yes.
[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?]: Low risk.
[Why is the change risky/not risky?]: It is simple fix. And it just add function calls like before.
[String changes made/needed]: None.
Attachment #8899053 - Flags: approval-mozilla-beta?
Sec-approval+ to land on trunk. I'll approve Beta as well.
Keywords: sec-high
Attachment #8899053 - Flags: sec-approval?
Attachment #8899053 - Flags: sec-approval+
Attachment #8899053 - Flags: approval-mozilla-beta?
Attachment #8899053 - Flags: approval-mozilla-beta+
(In reply to Sotaro Ikeda [:sotaro  PTO 1/Sep - 13/Sep] from comment #26)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Sotaro Ikeda's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Whiteboard: [adv-main56+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.