1440717 - (CVE-2018-5148) AddressSanitizer: heap-use-after-free [@ mozilla::gl::GLContext::MakeCurrent] with READ of size 8

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: crash.txt

Crash seen while fuzzing m-c 20180223-f7c5598e45c3, but I don't have a testcase that reproduces the issue.

==6735==ERROR: AddressSanitizer: heap-use-after-free on address 0x61e000161918 at pc 0x000109286765 bp 0x70000f46d600 sp 0x70000f46d5f8
READ of size 8 at 0x61e000161918 thread T34
    #0 0x109286764 in mozilla::gl::GLContext::MakeCurrent(bool) const (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x27e6764)
    #1 0x10943dc35 in mozilla::layers::CompositingRenderTargetOGL::~CompositingRenderTargetOGL() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x299dc35)
    #2 0x10943de9d in mozilla::layers::CompositingRenderTargetOGL::~CompositingRenderTargetOGL() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x299de9d)
    #3 0x10969c844 in mozilla::layers::ContainerLayerComposite::~ContainerLayerComposite() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2bfc844)
    #4 0x10969ca6d in mozilla::layers::ContainerLayerComposite::~ContainerLayerComposite() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2bfca6d)
    #5 0x109364301 in mozilla::layers::ContainerLayer::RemoveChild(mozilla::layers::Layer*) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x28c4301)
    #6 0x10969c7b1 in mozilla::layers::ContainerLayerComposite::~ContainerLayerComposite() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2bfc7b1)
    #7 0x10969ca6d in mozilla::layers::ContainerLayerComposite::~ContainerLayerComposite() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2bfca6d)
    #8 0x109364301 in mozilla::layers::ContainerLayer::RemoveChild(mozilla::layers::Layer*) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x28c4301)
    #9 0x10969c7b1 in mozilla::layers::ContainerLayerComposite::~ContainerLayerComposite() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2bfc7b1)
    #10 0x10969ca6d in mozilla::layers::ContainerLayerComposite::~ContainerLayerComposite() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2bfca6d)
    #11 0x10976b77a in mozilla::layers::LayerTransactionParent::~LayerTransactionParent() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2ccb77a)
    #12 0x10976ba94 in non-virtual thunk to mozilla::layers::LayerTransactionParent::~LayerTransactionParent() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2ccba94)
    #13 0x10973ce2e in mozilla::layers::CrossProcessCompositorBridgeParent::DeallocPLayerTransactionParent(mozilla::layers::PLayerTransactionParent*) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2c9ce2e)
    #14 0x10855844f in mozilla::layers::PCompositorBridgeParent::RemoveManagee(int, mozilla::ipc::IProtocol*) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x1ab844f)
    #15 0x107f16c63 in mozilla::layers::PLayerTransactionParent::Send__delete__(mozilla::layers::PLayerTransactionParent*) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x1476c63)
    #16 0x10976c201 in mozilla::layers::LayerTransactionParent::RecvShutdown() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2ccc201)
    #17 0x107f19af6 in mozilla::layers::PLayerTransactionParent::OnMessageReceived(IPC::Message const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x1479af6)

0x61e000161918 is located 1176 bytes inside of 2640-byte region [0x61e000161480,0x61e000161ed0)
freed by thread T34 here:
    #0 0x103223b4d in wrap_free (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/libclang_rt.asan_osx_dynamic.dylib:x86_64+0x58b4d)
    #1 0x1094aca05 in mozilla::layers::TextureImageTextureSourceOGL::~TextureImageTextureSourceOGL() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2a0ca05)
    #2 0x1097053bf in mozilla::layers::ShmemTextureHost::~ShmemTextureHost() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2c653bf)
    #3 0x1097055fd in mozilla::layers::ShmemTextureHost::~ShmemTextureHost() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2c655fd)
    #4 0x1093dae51 in mozilla::AtomicRefCountedWithFinalize<mozilla::layers::TextureHost>::Release() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x293ae51)
    #5 0x1097422cb in mozilla::layers::ParentActor<mozilla::layers::PTextureParent>::RecvDestroy() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2ca22cb)
    #6 0x1081967db in mozilla::layers::PTextureParent::OnMessageReceived(IPC::Message const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x16f67db)
    #7 0x108572598 in mozilla::layers::PCompositorManagerParent::OnMessageReceived(IPC::Message const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x1ad2598)
    #8 0x107c3d8c7 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x119d8c7)

previously allocated by thread T34 here:
    #0 0x103223993 in wrap_malloc (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/libclang_rt.asan_osx_dynamic.dylib:x86_64+0x58993)
    #1 0x1025841bd in moz_xmalloc (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/libmozglue.dylib:x86_64+0x21bd)
    #2 0x109338c07 in mozilla::gl::GLContextProviderCGL::CreateForWindow(nsIWidget*, bool, bool) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2898c07)
    #3 0x10933888a in mozilla::gl::GLContextProviderCGL::CreateForCompositorWidget(mozilla::widget::CompositorWidget*, bool) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x289888a)
    #4 0x109440e7f in mozilla::layers::CompositorOGL::CreateContext() (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x29a0e7f)
    #5 0x109441c13 in mozilla::layers::CompositorOGL::Initialize(nsTString<char>*) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x29a1c13)
    #6 0x10972d5e8 in mozilla::layers::CompositorBridgeParent::NewCompositor(nsTArray<mozilla::layers::LayersBackend> const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2c8d5e8)
    #7 0x10972cb49 in mozilla::layers::CompositorBridgeParent::InitializeLayerManager(nsTArray<mozilla::layers::LayersBackend> const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2c8cb49)
    #8 0x10972dc7f in mozilla::layers::CompositorBridgeParent::AllocPLayerTransactionParent(nsTArray<mozilla::layers::LayersBackend> const&, unsigned long long const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x2c8dc7f)
    #9 0x10855d2a3 in mozilla::layers::PCompositorBridgeParent::OnMessageReceived(IPC::Message const&) (/Users/truber/builds/asan/Nightly.app/Contents/MacOS/XUL:x86_64+0x1abd2a3)

Comment 1

[Daniel Veditz [:dveditz]]

The "see also" bug was duped to a bug fixed in 59, so this is something different if maybe related.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

So, TextureImageTextureSourceOGL has a RefPtr to GL context and deletes it in its destructor because the ref count is zero.  However, CompositingRenderTargetOGL has a raw pointer to GL context, so we get in trouble when it outlives TextureImageTextureSourceOGL - not because it tries to delete it, but because it tries to use it.

If we make a mGL a RefPtr, is there a cycle?  CompositingRenderTargetOGL has a ref ptr to the compositor, but there is a comment that says:
   * There is temporary a cycle between the compositor and the render target,
   * each having a strong ref to the other. The compositor's reference to
   * the target is always cleared at the end of a frame.

Can we just make mGL a RefPtr, or do we need some extra magic?

Comment 3

[Nicolas Silva [:nical]]

> Can we just make mGL a RefPtr [...]?

Yes, I think we should do this.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Use RefPtr to store CompositingRenderTargetOGL instead of a raw pointer.

the render target already holds a compositor which holds a RefPtr for the gl context and there doesn't seem to be any cycle breaking logic in there so I don't think this risks introducing a reference cycle.

We could also just use the compositor's gl context actually.

Comment 5

[Nicolas Silva [:nical]]

try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=09b9853ef5bf1a2246a8078b8f29305649b1d08e

Comment 6

[Nicolas Silva [:nical]]

Another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f0b17ec9e9432e26f934b9de276f091b8018271

The previous one was generated with trychooser but it seems like the macosx64-st-an target isn't valid anymore.

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8956797 [details] [diff] [review]
Use RefPtr to store CompositingRenderTargetOGL instead of a raw pointer.

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

Not easily.

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 of them.

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

They'll be easy to make, but let's first land this on central.

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

Unlikely. This just turns a raw pointer into a reference counted one, it's unlikely to cause a cycle (see my earlier comments).

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Sec-approval to land on March 27, two weeks into the next release cycle. We'll want to backport it to 60 and ESR52 at that time as well so we'll need nominations for patches after it lands.

Comment 9

[Nicolas Silva [:nical]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d2d55223d2aa5cb85bbdf33075d1d38f2a9a30

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b6d2d55223d2

Comment 11

[Al Billings [:abillings - ex-MoCo]]

(In reply to Nicolas Silva [:nical] from comment #9)
>  https://hg.mozilla.org/integration/mozilla-inbound/rev/
> b6d2d55223d2aa5cb85bbdf33075d1d38f2a9a30

You just checked this in 15 days early and before Firefox 59 shipped. I didn't get sec-approval+ until March 27, see comment 8.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Adding Liz and Dan since I've briefed them about this. Since we're already pushing 59 out to CDNs and shipping in 12 hours or so, this is going to have to get fixed in any point release or chemspill of 59.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 59/60. We can keep this in mind for a dot release. 
If we have to chemspill later this week, we should consider taking this too.

Comment 14

[Nicolas Silva [:nical]]

> You just checked this in 15 days early and before Firefox 59 shipped. I didn't get sec-approval+ until March 27, see comment 8.

Oh sorry I misread your comment. We can always back this out from 59 as matter of principle, although I wouldn't worry about this patch.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

(In reply to Nicolas Silva [:nical] from comment #14)
> > You just checked this in 15 days early and before Firefox 59 shipped. I didn't get sec-approval+ until March 27, see comment 8.
> 
> Oh sorry I misread your comment. We can always back this out from 59 as
> matter of principle, although I wouldn't worry about this patch.

Once it goes in, it doesn't matter as the point of sec-approval is to avoid 0daying ourselves by checking security bugs into a public repo. Since it is in a repo, that ship has sailed... There is no reason to back it out now and it may even draw more attention to it.

Comment 16

[Julien Cristau [:jcristau]]

We should get this on beta, please request uplift when you get a chance.

Comment 17

[Nicolas Silva [:nical]]

Comment on attachment 8956797 [details] [diff] [review]
Use RefPtr to store CompositingRenderTargetOGL instead of a raw pointer.

Approval Request Comment
[Feature/Bug causing the regression]: The layers refactoring from 5 years ago.
[User impact if declined]: UAF shutdown crashes.
[Is this code covered by automated tests?]: Not specifically (the code is run in all tests but no test written for this particular piece of code).
[Has the fix been verified in Nightly?]: It has landed and doesn't seem to have made waves.
[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?]: change a raw pointer into a refcounted one. unlikely to cause cycle because the owning object also owns another object that in turns own the target object. The added ref pointer ensures deallocation happens in the right order.
[String changes made/needed]: None.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8956797 [details] [diff] [review]
Use RefPtr to store CompositingRenderTargetOGL instead of a raw pointer.

uaf fix, beta60+

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

We have a few options here, including

- let this ride the train with 60
- wait for a planned dot release (but not a chemspill) and uplift to m-r

I think we can decide between those two options next week. 

Another option would be including this fix in a potential chemspill today. 
I don't want to do that because it can slow down the process if there are test failures (either manual or automated tests)

Dan, do you have an opinion on that?

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ec90964e0bc479412a4da8a61d48b665211736e

Comment 21

[Daniel Veditz [:dveditz]]

I hope there's a point-release we can get this into, but I don't want it in the Pwn2Own chemspill.

Comment 22

[Andrei Vaida [:avaida]]

Setting qe-verify- on this bug since it has automated coverage (although indirect) and Nicolas sees no need for manual testing (Comment 17).

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8956797 [details] [diff] [review]
Use RefPtr to store CompositingRenderTargetOGL instead of a raw pointer.

We're going to want this on ESR52 eventually.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8956797 [details] [diff] [review]
Use RefPtr to store CompositingRenderTargetOGL instead of a raw pointer.

Based on Dan's request to include this in a dot release, A+ for inclusion in 59.0.2 and ESR52.7.3

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c3e447e07077

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/08e0ad236142