1789371 - (CVE-2022-46882) SEGV on unknown address 0xe5e5e5f5 in mozilla::ClientWebGLContext::GetExtension

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Irvan Kurniawan (:sourc7)]

Attachment: crash.html

I found in Firefox 32-bit after filling with a lot of memory (approximate 3000MB) then create a lot new Worker construction afterward call webgl getExtension('OES_vertex_array_object') repeatedly Firefox able to crash on 0xe5e5e5f5 address in mozilla::ClientWebGLContext::GetExtension.

I observe on stdout before Firefox crash it show WARNING: Call to mmap failed: Operation not permitted and Failed to allocate internal command buffer.. It's look like after Firefox freed the memory, but it fail to allocate new memory on the target address due to low address spaces, so when getExtension('OES_vertex_array_object') is called it still point to freed memory which leads to UAF.

Reproducible on:

• Firefox Nightly 106.0a1 (2022-09-05) (32-bit) on Arch Linux using AMD Ryzen 7 5700G
• Firefox Nightly 106.0a1 (2022-09-05) (32-bit) on Arch Linux using Intel i5-1035G1
• Firefox 104.0.1 (32-bit) on Arch Linux using AMD Ryzen 7 5700G
• Firefox 104.0.1 (32-bit) on Arch Linux using Intel i5-1035G1

Steps to reproduce:

1. Open Firefox 32-bit
2. Visit attached crash.html
3. Firefox crashed in mozilla::ClientWebGLContext::GetExtension on address 0xe5e5e5f5

Comment 1

[Irvan Kurniawan (:sourc7)]

Attachment: log_minidump_01.txt

Comment 2

[Irvan Kurniawan (:sourc7)]

Attachment: gdb.txt

Comment 3

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

The severity field is not set for this bug.
:jgilbert, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Ashley Hale [:ahale]]

I'm trying to understand whether this is a bug in RefPtr with regards to address space exhaustion, or a bug in WebGL, my reading leans toward a bug in RefPtr, which seems like it would make for a truly massive attack surface for UAF vulnerabilities.

Ideally I think we should be crashing the WebGL process when address space is nearly exhausted, or on the first allocation failure which more or less indicates the same thing, because proceeding past an allocation failure is highly unsafe in code written with these assumptions.

Comment 5

[Ashley Hale [:ahale]]

NeedInfo - I'm confused by the error messages quoted in comment #1, which don't match what I expect to see from jemalloc if mmap failed, which should have aborted the process. Can you elaborate or quote a larger section of stdout/stderr? ( https://searchfox.org/mozilla-central/rev/f118dae98073bc17efb8604a32abfcb7b4e05880/memory/build/mozjemalloc.cpp#1502 https://man.archlinux.org/man/mmap.2.en )

Comment 6

[Irvan Kurniawan (:sourc7)]

Attachment: stdout.txt

(In reply to Ashley Hale from comment #5)

NeedInfo - I'm confused by the error messages quoted in comment #1, which don't match what I expect to see from jemalloc if mmap failed, which should have aborted the process. Can you elaborate or quote a larger section of stdout/stderr? ( https://searchfox.org/mozilla-central/rev/f118dae98073bc17efb8604a32abfcb7b4e05880/memory/build/mozjemalloc.cpp#1502 https://man.archlinux.org/man/mmap.2.en )

When I run the crash.html the stdout show as attached, I notice before the crash this message often appear on stdout:

[Child 2471958, Main Thread] WARNING: Call to mmap failed: Operation not permitted: file /home/sourc7/git/gecko-dev/ipc/chromium/src/base/shared_memory_posix.cc:515
JavaScript warning: http://127.0.0.1:8000/crash.html, line 1277: Failed to allocate internal command buffer.
JavaScript warning: http://127.0.0.1:8000/crash.html, line 1277: WebGL context was lost.

Comment 7

[Kelsey Gilbert [:jgilbert]]

This sure would have been prevented by a borrow-checker. :)
I think we just need a const auto keepAlive = mNotLost; in GetExtension:

+  const auto keepAlive = mNotLost;
  auto& extSlot = mNotLost->extensions[UnderlyingValue(ext)];
[...]
    MOZ_ASSERT(extSlot);
    RequestExtension(ext); [// this can cause context-loss, which nulls mNotLost]
  }

  return extSlot;

I can just take this real quick.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1789371 - Hold a strong-ref to mNotLost in GetExtension.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9298541 [details]
Bug 1789371 - Hold a strong-ref to mNotLost in GetExtension.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't think it's feasible to exploit this, since this is a UAF while coming back down the stack.
• 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
• 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?: Easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to regress.
• Is Android affected?: Yes

Comment 10

[Kelsey Gilbert [:jgilbert]]

I think this is relatively non-exploitable. This is a case where we do this:

AttackerControlledCaller():
  auto& danglingRef = mStrongRef->foo;
  auto mem = malloc();
  if (!mem) {
    mStrongRef = nullptr;
  }
  return danglingRef;
}

I don't think there's an opportunity on this thread to fill the UAF memory with anything useful.
You'd have to, on another thread, race to get some other allocation wedged in there.
It's unsound, but I don't think there's enough attacker-leverageable branches/tools lying around in this code to make an exploit out of it.

I'm tempted to say sec-low, but we err on the safe side and go with sec-moderate.

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9298541 [details]
Bug 1789371 - Hold a strong-ref to mNotLost in GetExtension.

Approved to land and uplift after beta branches

Comment 12

[Kelsey Gilbert [:jgilbert]]

Bug 1795372 (which was in response to a different-but-similar non-sec issue) actually covers this, and that made it into 107.
Given that this isn't really a chemspill candidate, that leaves only esr102 vulnerable.
I think instead we should uplift bug 1795372's fix instead, either here or in that bug.
I think I can make the argument in the other bug that we should uplift to ESR for robustness reasons rather than calling it out as a sec-fix.

@tjr: What do you think?

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Sounds good to me; you can let relman know out-of-band from the uplift request about this backstory.

Comment 14

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jgilbert, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Still waiting on an ESR102 approval request in bug 1795372. Targeting the 102.6esr release at this point. FYI, it grafts cleanly.

Comment 16

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt