1671382 - (CVE-2020-26972) AddressSanitizer: heap-use-after-free [@ GetLifecycleProxy] through [@ mozilla::ClientWebGLContext::GetFrontBufferSnapshot] with READ of size 8

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 83.0a1-20201008094950-https://hg.mozilla.org/mozilla-central/rev/e88890094825db821835b2f173e1e16eae3f51af.

For detailed crash information, see attachment.

This was already reported on October 8, so if this is on file already somewhere else, please dup :)

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Andrew McCreight [:mccr8]]

I think the use is child on the last line here:

    const auto& child = mNotLost->outOfProcess->mWebGLChild;
    child->FlushPendingCmds();
    webgl::FrontBufferSnapshotIpc res;
    if (!child->SendGetFrontBufferSnapshot(&res)) {
      res = {};
    }
    const auto& surfSize = res.surfSize;
    const webgl::RaiiShmem shmem{child, res.shmem};

We're taking a weak stack reference to the actor, then doing some IPC stuff, then using the reference, which looks concerning to me, but the free stack doesn't make it look like it is running there, so I'm not sure.

Comment 4

[Kelsey Gilbert [:jgilbert]]

This UAF is for a CompositorManagerChild's inherited IProtocol::mLifecycleProxy. Everything here looks main-thread, so that's good.

  RaiiShmem(mozilla::ipc::IProtocol* const allocator,
            const mozilla::ipc::Shmem& shmem)
      : mWeakRef(allocator->ToplevelProtocol()->GetLifecycleProxy()), <<< UAF

I'm guessing CompositorManagerChild is the top-level here, but is being freed before our PWebGL.

I feel like IToplevelProtocol* IProtocol::mToplevel should be a weak-ptr.

Interestingly, ToplevelProtocol() is almost unused:
https://searchfox.org/mozilla-central/search?path=&q=ToplevelProtocol()

Comment 5

[Kelsey Gilbert [:jgilbert]]

This is really more of an IPC question than a WebGL question.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Nika, what do you think?

Comment 7

[Nika Layzell [:nika] (ni? for response)]

This bug appears to be being caused by a RaiiShmem being created at a bad time. For backwards compatibility, there's nothing which prevents a manager actor from being freed before a managed actor after the actors have been closed, so this code is almost certainly occurring after the actor has already been closed.

In general, calling almost any method on IProtocol when the actor has been destroyed is very unsafe, and this code is currently not performing any checks to make sure that the actor has not already been destroyed. The root of the issue here is that RaiiShmem is being passed a dead actor by pointer, but the implementation assumes that the actor is still alive.

This can probably be fixed by changing the code to do something more like the following:

RaiiShmem(mozilla::ipc::IProtocol* aAllocator,
          const mozilla::ipc::Shmem& aShmem) {
  // Check that the allocator is alive. If it isn't, we must've been passed an invalid shmem, and should remain in the invalid state.
  // We can avoid holding a reference to the allocator when passed an invalid shmem.
  if (!aAllocator || !aAllocator->CanSend() || !mShmem.IsReadable()) {
    MOZ_DIAGNOSTIC_ASSERT(!mShmem.IsReadable(), "valid shmem with null or dead allocator");
    return;
  }

  mShmem = aShmem;
  mAllocator = aAllocator->ToplevelProtocol()->GetLifecycleProxy();
  MOZ_DIAGNOSTIC_ASSERT(mAllocator, "No lifecycle proxy for non-dead allocator?");
}

Does that seem like the desired behaviour in this sort of situation behaviour to you?

Comment 8

[Kelsey Gilbert [:jgilbert]]

I think that gives me the info I need, though I want the fallibility of RaiiShmem surfaced at creation time, instead of requiring checking by each user.
Thanks!

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1671382 - reset() RaiiShmem if its allocator is invalid.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9186431 [details]
Bug 1671382 - reset() RaiiShmem if its allocator is invalid.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Tricky: Attacker needs to cause the top level proto to die while we're still manipulating shmems. (from enqueue commands?)
• 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?: 83
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should rebase cleanly
• How likely is this patch to cause regressions; how much testing does it need?: Pretty low chance of causing problems given our CI.

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9186431 [details]
Bug 1671382 - reset() RaiiShmem if its allocator is invalid.

Approved to land and uplift

Comment 12

[Tom Ritter [:tjr]]

Setting tracking flags

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9186431 [details]
Bug 1671382 - reset() RaiiShmem if its allocator is invalid.

Actually, esr78 is unaffected

Comment 14

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

https://hg.mozilla.org/integration/autoland/rev/fb54693d6b4c88df07d11f80773dea749d3cb66b
https://hg.mozilla.org/mozilla-central/rev/fb54693d6b4c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d99cd517549

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Survey submitted.

Comment 18

[Tom Ritter [:tjr]]

Attachment: advisory.txt