2017513 - Use-After-Free in BoundStorageKeyParent via Dangling PBackgroundParent Pointer

Status: RESOLVED FIXED

(Core :: Storage: Cache API, defect)

Description

[Jason Kratzer [:jkratzer]]

Use-After-Free in BoundStorageKeyParent via Dangling PBackgroundParent Pointer

Vulnerability Type

Heap Use-After-Free (CWE-416)

Bug Class

Use-after-free via dangling raw pointer across independent IPC actor lifetimes

Affected Component

• dom/cache/BoundStorageKeyParent.h (line 46: raw MOZ_NON_OWNING_REF pointer)
• dom/cache/BoundStorageKeyParent.cpp (line 23: use of dangling pointer)
• dom/cache/CacheStorageParent.cpp (line 50: dereference of freed memory)
• ipc/glue/BackgroundParentImpl.cpp (line 1113: creation of BoundStorageKeyParent with raw this pointer)

Summary

BoundStorageKeyParent stores a raw, non-owning pointer (MOZ_NON_OWNING_REF) to PBackgroundParent (line 46 of BoundStorageKeyParent.h). Since PBoundStorageKey is a top-level protocol with its own independent IPC transport (created via Endpoint), its lifetime is completely decoupled from PBackground. A compromised content process can destroy the PBackground connection while keeping the PBoundStorageKey channel alive, then send a PCacheStorage constructor message which causes the parent to dereference the freed PBackgroundParent memory.

Detailed Analysis

Root Cause

In BackgroundParentImpl::RecvCreateBoundStorageKeyParent (BackgroundParentImpl.cpp:1113):

auto* pActor = new dom::cache::BoundStorageKeyParent(this);

The this pointer (which is the BackgroundParentImpl* / PBackgroundParent*) is stored as a raw pointer in BoundStorageKeyParent::mBackgroundParent (BoundStorageKeyParent.h:46):

mozilla::ipc::PBackgroundParent* MOZ_NON_OWNING_REF mBackgroundParent;

The comment on the raw pointer incorrectly states:

"Raw ptr is fine here as PBackground is one of the few top-level that gets created very early on in the content and parent lifecycle and extends the lifetime of this class."

This assumption is wrong because PBoundStorageKey is a top-level protocol with its own independent transport channel, so IPDL provides no lifetime guarantee between PBackground and PBoundStorageKey.

Exploitation Path

1. Content process creates PBackground — standard initialization
2. Content process sends CreateBoundStorageKeyParent — parent creates BoundStorageKeyParent holding raw ptr to PBackgroundParent
3. Content process closes PBackground — PBackgroundParent is destroyed and freed
4. Content process sends PCacheStorage on PBoundStorageKey — parent calls BoundStorageKeyParent::AllocPCacheStorageParent() which passes the dangling mBackgroundParent to CacheStorageParent constructor and PrincipalVerifier::CreateAndDispatch()
5. Parent dereferences freed memory — BackgroundParent::GetContentParentHandle(mBackgroundParent) reads from freed heap, causing heap-use-after-free

ASAN Stack Trace (Crash Point)

READ of size 8 at freed memory (thread T11 - background thread)
    #0 RefPtr<ThreadsafeContentParentHandle>::get()
    #1 ParentImpl::GetContentParentHandle(PBackgroundParent*)  BackgroundImpl.cpp:783
    #2 BackgroundParent::GetContentParentHandle(PBackgroundParent*)  BackgroundImpl.cpp:683
    #3 PrincipalVerifier::PrincipalVerifier(...)  PrincipalVerifier.cpp:63
    #4 PrincipalVerifier::CreateAndDispatch(...)  PrincipalVerifier.cpp:41
    #5 CacheStorageParent::CacheStorageParent(PBackgroundParent*,...)  CacheStorageParent.cpp:50
    #6 AllocPCacheStorageParent(PBackgroundParent*,...)  CacheStorageParent.cpp:31
    #7 BoundStorageKeyParent::AllocPCacheStorageParent(...)  BoundStorageKeyParent.cpp:23

Exploitability

This is a heap-use-after-free where:

• The freed object is a 680-byte ParentImpl (PBackgroundParent implementation)
• The read occurs at offset 648 within the freed object
• The freed memory can be reallocated with attacker-controlled content via heap spray
• An attacker can control what data is read from the freed memory, potentially:
  • Hijacking the ThreadsafeContentParentHandle RefPtr to point to attacker-controlled memory
  • Gaining control of subsequent operations that use the returned handle
  • Escalating to arbitrary code execution in the parent process

Proof of Concept

The exploit modifies the content process to:

1. Obtain the PBackground child actor
2. Create PBoundStorageKey endpoints and send CreateBoundStorageKeyParent to parent
3. Bind the child endpoint locally
4. Call Close() on PBackground, destroying PBackgroundParent in the parent process
5. After a delay, send PCacheStorage constructor on the PBoundStorageKey channel
6. Parent-side BoundStorageKeyParent::AllocPCacheStorageParent() dereferences the freed mBackgroundParent

Recommended Fix

Replace the raw MOZ_NON_OWNING_REF pointer with a RefPtr or WeakPtr, and add a null-check before use:

// In BoundStorageKeyParent.h:
// Replace:
//   mozilla::ipc::PBackgroundParent* MOZ_NON_OWNING_REF mBackgroundParent;
// With:
//   RefPtr<mozilla::ipc::PBackgroundParent> mBackgroundParent;
// Or use a weak reference with null-check in AllocPCacheStorageParent()

Alternatively, clear mBackgroundParent when PBackground is destroyed by registering BoundStorageKeyParent as a destruction observer, or by making PBoundStorageKey a managed sub-protocol of PBackground so IPDL enforces proper lifetime ordering.%

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: exploit_bsk_uaf.html

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: exploit.patch

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: crash_stack.txt

Comment 4

[Frederik Braun [:freddy]]

Triage owner doesn't have access to the bug. CCing jld.
This file hasn't been touched in a while, but most recently by Nika. CCing her as well.

Comment 5

[Andrew McCreight [:mccr8]]

Harveer, could you take a look at this? Thanks.

Comment 6

[Andrew McCreight [:mccr8]]

From skimming the code, I suspect that this dates to the landing of BoundStorageKeyParent in bug 1969120, which would mean neither ESR is affected, but it would be good to have confirmation.

Comment 7

[Harveer Singh]

Yes, the effected code and file in question were not introduced a long ago. I have assigned the task to myself and will be working on it very shortly.

Comment 8

[BugBot [:suhaib / :marco]]

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

Comment 9

[Harveer Singh]

Attachment: (secure)

Comment 10

[BugBot [:suhaib / :marco]]

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

Comment 11

[Donal Meehan [:dmeehan]]

Harveer, the planned Fx148 dot release builds and releases next week.
This would need to land and have beta/release uplift requests this week to be included.

Comment 12

[Harveer Singh]

sure, I will ping reviewers to expedite the review process.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/66bf37cfc55a
https://hg.mozilla.org/integration/autoland/rev/d026b3ea6d95
Replace raw PBackgroundParent pointer with RefPtr in BoundStorageKeyParent r=dom-storage-reviewers,dom-worker-reviewers,edenchuang

Comment 14

[Andrew McCreight [:mccr8]]

This should have gotten a sec-approval flag before landing. Please fill it out even though it is kind of too late already. Thanks.

Comment 15

[Donal Meehan [:dmeehan]]

Sorry if my message in Comment 11 implied otherwise, but as mentioned in Comment 14 and in a message when landing the bug, it needs sec approval

Comment 16

[Harveer Singh]

oh apologies. I thought sec-appovals are only needed when uplifting the patches and not for landing to main. I will try to do that now.

Comment 17

[Harveer Singh]

Comment on attachment 9547647 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's not easy to exploit this bug as it assumes compromised content process.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: ESRs are not effected but beta and release. Yes, it's marked as a regression accordingly
• 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?: It should be pretty trivial to cherry-pick the changes
• How likely is this patch to cause regressions; how much testing does it need?: Existing automated regression tests should cover the new changes
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Unknown

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/d026b3ea6d95

Comment 19

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:hsingh, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox149 to wontfix.

For more information, please visit BugBot documentation.

Comment 20

[Harveer Singh]

Attachment: (secure)

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

Comment 21

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Parent process could lead to crash in case content process gets compromised. The related bug is rated as sec-high.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: There's STR on the bug. It involves assuming compromised content process.
• Risk associated with taking this patch: low
• Explanation of risk level: Patch should apply cleanly and should be covered by existing regression tests.
• String changes made/needed: N/A
• Is Android affected?: no

Comment 22

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined: Parent process could lead to crash in case content process gets compromised. The related bug is rated as sec-high.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: There's STR on the bug. It involves assuming compromised content process.
• Risk associated with taking this patch: low
• Explanation of risk level: Patch should apply cleanly and should be covered by existing regression tests.
• String changes made/needed: N/A
• Is Android affected?: no

Comment 23

[Harveer Singh]

Attachment: (secure)

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

Comment 24

[Brindusa Tot, DTE]

We noticed that the "qe-verify+" label was added. Can this issue be verified manually? If so, could you please provide specific steps for manual verification?”

Comment 25

[Christian Holler (:decoder)]

This issue cannot be verified without creating your own Firefox build, so likely not something QA can easily verify as fixed.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 9547647 [details]
(secure)

sec-approval+ to land now and request uplifts

Comment 27

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/de0bdbae3b6b
https://hg.mozilla.org/releases/mozilla-release/rev/4f6a4faf972b

Comment 28

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/674830de7ced
https://hg.mozilla.org/releases/mozilla-beta/rev/05d49568a798