Closed Bug 1671382 (CVE-2020-26972) Opened 11 months ago Closed 10 months ago

AddressSanitizer: heap-use-after-free [@ GetLifecycleProxy] through [@ mozilla::ClientWebGLContext::GetFrontBufferSnapshot] with READ of size 8

Categories

(Core :: IPC, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
85 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox82 --- unaffected
firefox83 - wontfix
firefox84 + fixed
firefox85 + fixed

People

(Reporter: decoder, Assigned: jgilbert)

Details

(4 keywords, Whiteboard: [sec-survey][adv-main84+])

Attachments

(3 files)

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 :)

Flags: sec-bounty?
Group: core-security → gfx-core-security
Component: Graphics → Canvas: WebGL

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.

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()

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

Component: Canvas: WebGL → IPC
Group: gfx-core-security → dom-core-security

Nika, what do you think?

Flags: needinfo?(nika)

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?

Flags: needinfo?(nika) → needinfo?(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!

Flags: needinfo?(jgilbert)
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED

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.
Attachment #9186431 - Flags: sec-approval?

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

Approved to land and uplift

Attachment #9186431 - Flags: sec-approval?
Attachment #9186431 - Flags: sec-approval+
Attachment #9186431 - Flags: approval-mozilla-esr78+
Attachment #9186431 - Flags: approval-mozilla-beta+

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

Actually, esr78 is unaffected

Attachment #9186431 - Flags: approval-mozilla-esr78+ → approval-mozilla-esr78-
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
Flags: qe-verify-

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.

Flags: needinfo?(jgilbert)
Whiteboard: [sec-survey]
Flags: needinfo?(jgilbert)

Survey submitted.

Flags: sec-bounty? → sec-bounty+
Whiteboard: [sec-survey] → [sec-survey][adv-main84+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.