1895579 - (CVE-2024-5695) Access-violation read on MozJemallocPHC::moz_arena_realloc -> AllocInfo::Get (after malloc failure on MaybePageRealloc)

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: PHC-MaybePageRealloc.patch

When run JetStream2 WSL benchmark on low memory condition, I notice the Firefox tab able to crash on access-violation on jemalloc AllocInfo::Get, after some debugging I found it occur after newPtr = MozJemalloc::moz_arena_malloc failure on PHC.cpp MaybePageRealloc function.

Steps to reproduce:

1. Apply PHC-MaybePageRealloc.patch
2. After Firefox was compiled
3. Visit https://browserbench.org/JetStream2.0/
4. After loading benchmark is completed
5. Click Start Test
6. On WSL benchmark, firefox tab crashed with access-violation on AllocInfo::Get

(If the tab is not crashed, reload the JetStream page, then re-click the Start Test button)

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: log_minidump_00.txt

Comment 2

[Andrew McCreight [:mccr8]]

Paul, could you take a look? Thanks.

Comment 3

[Irvan Kurniawan [:sourc7]]

https://searchfox.org/mozilla-central/source/memory/build/PHC.cpp#1464

Comment 4

[Irvan Kurniawan [:sourc7]]

Attachment: log_minidump_00_archlinux.txt

Comment 5

[Mike Hommey [:glandium]]

The patch attached in comment 0 is in utf-16 for some reason, and what it does is essentially make MaybePageRealloc return null when it would normally call moz_arena_malloc for a PHC alloc to normal alloc transition.

Here it is in pernosco: https://pernos.co/debug/5emXEVpOBwBE8q3_tAN9fg/index.html

After returning null, what happens is that PageRealloc does this:

  return ptr ? ptr
             : (aArenaId.isSome() ? MozJemalloc::moz_arena_realloc(
                                        *aArenaId, aOldPtr, aNewSize)
                                  : MozJemalloc::realloc(aOldPtr, aNewSize));

So it tries to realloc the PHC allocation pointer with mozjemalloc, which is what ends up failing, since it's not handled by mozjemalloc.

Comment 6

[Mike Hommey [:glandium]]

The fundamental problem is that MaybePageRealloc's return value doesn't distinguish between "I did something and the result is a legitimate null" and "Not my allocation, deal with it".

Comment 7

[Mike Hommey [:glandium]]

As for how dangerous this is: from the PHC allocation pointer, mozjemalloc will get the address of the chunk it thinks corresponds to it, which, because of how things are setup, will end up in the first PHC allocation.

(Ignoring MOZ_DIAGNOSTIC_ASSERTS below, as they only affect nightly and devedition, but they could catch something earlier)

If the PHC allocation that is being reallocated is the first, then mozjemalloc will think it's a huge allocation, look it up, and fail to find it, at which point one of two things can happen:

• it will crash on https://searchfox.org/mozilla-central/rev/729361e481cf63c8d2b5617a6ff589f53e302520/memory/build/mozjemalloc.cpp#4803
• it will crash on https://searchfox.org/mozilla-central/rev/729361e481cf63c8d2b5617a6ff589f53e302520/memory/build/mozjemalloc.cpp#4122

If the PHC allocation that is being reallocated is not the first one, then it will read the first PHC allocation as if it were a chunk header and make a mess in https://searchfox.org/mozilla-central/rev/729361e481cf63c8d2b5617a6ff589f53e302520/memory/build/mozjemalloc.cpp#3691, either crashing because what it thinks is a pointer is that fake chunk header points to unmapped memory, or returning completely bogus size information for the allocation.

Then depending on the size, it might crash on https://searchfox.org/mozilla-central/rev/729361e481cf63c8d2b5617a6ff589f53e302520/memory/build/mozjemalloc.cpp#3744

If things line up right, you can end up in https://searchfox.org/mozilla-central/rev/729361e481cf63c8d2b5617a6ff589f53e302520/memory/build/mozjemalloc.cpp#4070

Before bug 1850008, if the size is larger than a page, we'd be crashing on MaybePoison hitting a PHC guard page, but that won't happen anymore.

After that, if things line up perfectly, you could theoretically end up with mozjemalloc thinking it's reallocating something properly.

So, if an attacker somehow manages to get a hand on two PHC allocations, one of which is would have to be the first one, and craft them to go through the right pinholes, it would be possible to trick the allocator into returning a controlled pointer as the result of the realloc this all started from. And I'm not sure what could be gained from that. I'm not even sure an attacker could hammer the allocator enough to be given out PHC allocations they'd have control over twice.

That is to say, there's definitely a bug, but I don't think it's meaningfully exploitable, but you never know.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1895579 - Better distinguish between MaybePageRealloc return values.

Allowing caller to distinguish between PHC not handling the realloc, and
OOM.

Comment 9

[Mike Hommey [:glandium]]

With the patch, I get this in the web console:

Uncaught out of memory

or

Uncaught (in promise) uncaught exception: out of memory

Comment 10

[Andrew McCreight [:mccr8]]

I'll mark this as sec-moderate. Maybe that's a bit too high, but having a deeply confused allocator like this sounds potentially dangerous.

Comment 11

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

Set release status flags based on info from the regressing bug 1854550

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 9400610 [details]
Bug 1895579 - Better distinguish between MaybePageRealloc return values.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Cf. comment 7, I think it's far from trivial, and might not even lead to something useful.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: beta, release
• 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?: The patch should be cherry-pickable. The last change to the function that is being modified is, in fact, the change that introduced the bug, AFAICT.
• How likely is this patch to cause regressions; how much testing does it need?: Low. There is virtually no logic change, just a "clarification" of behavior.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 13

[Mike Hommey [:glandium]]

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown

Let me clarify on this, because the form doesn't really allow to elaborate: my answer would both yes and no. The patch paints a bulls-eye on reallocations involving PHC being a problem in some way. The fact that the bug is a security bug makes it more noticeable than if it were in a normal bug that just talked about some MOZ_RELEASE_ASSERT being triggered on OOM. But the conditions necessary to make it a security bug are not immediately obvious, and it's not obvious how it can be weaponized, especially since potentially exploiting this requires getting TWO PHC allocations under your control, one of which needs to be at a very specific location, and PHC allocations happen infrequently and randomly. I'm not even sure how often they can be directly or indirectly writable by attacker-controlled code, or by code that might write something that could look like a proper chunk header. Oh, and one of the allocations would need to be page-sized (or close to that), something smaller would just trigger one of the many code paths that just crash in a controlled manner.

Comment 14

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

Set release status flags based on info from the regressing bug 1854550

Comment 15

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 9400610 [details]
Bug 1895579 - Better distinguish between MaybePageRealloc return values.

Approved to land and uplift - although now I see it's a moderate; moderates don't need sec-approval.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Bug 1895579 - Better distinguish between MaybePageRealloc return values.

Allowing caller to distinguish between PHC not handling the realloc, and
OOM.

Original Revision: https://phabricator.services.mozilla.com/D209764

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/982b58476954
Better distinguish between MaybePageRealloc return values. r=pbone

Comment 19

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Unexpected crashes
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: There is virtually no logic change, just a "clarification" of behavior.
• String changes made/needed: N/A
• Is Android affected?: no

Comment 20

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

Copying crash signatures from duplicate bugs.

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/982b58476954

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/02088deda7f1

Comment 23

[Tom Ritter [:tjr] (OOTO until 9/2)]

Attachment: advisory.txt