2019112 - Use-After-Free in PContentPermissionRequest via Double prompt() IPC Message

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Bug Description

A use-after-free vulnerability exists in Firefox's PContentPermissionRequest IPC protocol handler. The ContentPermissionRequestParent::Recvprompt() method creates a new nsContentPermissionRequestProxy object each time a prompt() message is received, storing it in the mProxy member field. When a second prompt() message is received, the previous proxy object (proxy1) is replaced but may still be referenced by the browser's PopupNotifications system via a stored JavaScript callback. When the actor is subsequently destroyed via Destroy(), only the current proxy's mParent pointer is nullified via ActorDestroy(), while the orphaned proxy1 retains a dangling raw pointer to the freed ContentPermissionRequestParent actor. When the PopupNotification's event callback fires (e.g., on notification removal), it calls methods on proxy1 that dereference the dangling mParent pointer, resulting in a heap-use-after-free in the parent (browser) process.

Vulnerability Details

• Type: Use-After-Free (CWE-416)
• Component: PContentPermissionRequest IPC protocol / nsContentPermissionHelper.cpp
• Process: Parent (browser) process
• Trigger: Compromised content process sends malicious IPC message sequence
• Severity: Critical (sandbox escape - parent process memory corruption)

Root Cause

The vulnerability is in ContentPermissionRequestParent::Recvprompt() at dom/base/nsContentPermissionHelper.cpp:94-101:

mozilla::ipc::IPCResult ContentPermissionRequestParent::Recvprompt() {
  mProxy = new nsContentPermissionRequestProxy(this);  // Overwrites previous proxy!
  if (NS_FAILED(mProxy->Init(mRequests))) {
    RefPtr<nsContentPermissionRequestProxy> proxy(mProxy);
    proxy->Cancel();
  }
  return IPC_OK();
}

And in ContentPermissionRequestParent::ActorDestroy() at line 108-112:

void ContentPermissionRequestParent::ActorDestroy(ActorDestroyReason why) {
  if (mProxy) {
    mProxy->OnParentDestroyed();  // Only nullifies CURRENT proxy's mParent
  }
}

The nsContentPermissionRequestProxy class holds a non-owning raw pointer (mParent) to its parent ContentPermissionRequestParent actor (declared at line 183 of nsContentPermissionHelper.h):

// Non-owning pointer to the ContentPermissionRequestParent object which owns this proxy.
ContentPermissionRequestParent* mParent;

There are two critical design flaws:

1. No duplicate message guard: Recvprompt() does not check if mProxy is already set. Each call unconditionally creates a new proxy and overwrites the previous one.

2. Incomplete cleanup on actor destruction: ActorDestroy() only nullifies the mParent pointer on the current mProxy. Any previously created proxies that are still alive (held by the PopupNotifications system) retain their dangling mParent pointers.

Attack Sequence

A compromised content process executes the following IPC message sequence:

1. SendPContentPermissionRequestConstructor(): Creates the actor with a valid permission type (e.g., "desktop-notification")
2. Sendprompt() (1st): Parent creates proxy1, stores it in mProxy. The permission prompt service creates a PopupNotification that holds a reference to proxy1 via JavaScript.
3. Sendprompt() (2nd): Parent creates proxy2, overwrites mProxy. proxy1 is released from mProxy but the PopupNotification still holds a reference to it.
4. SendDestroy(): Parent calls Send__delete__(this) → ActorDestroy() nullifies proxy2->mParent → actor memory is freed via DeallocPContentPermissionRequestParent(). proxy1->mParent is never nullified — it now points to freed heap memory.
5. PopupNotification callback fires: When the notification is removed (tab close, shutdown, etc.), the JS callback calls proxy1->Cancel() or accesses proxy1->GetPrincipal(), which dereferences the dangling mParent pointer → heap-use-after-free.

ASAN Crash Trace

==249785==ERROR: AddressSanitizer: heap-use-after-free on address 0x71781115f748
READ of size 8 at 0x71781115f748 thread T0
    #0 nsContentPermissionRequestProxy::GetPrincipal() nsContentPermissionHelper.cpp:736
    #1 PermissionDelegateHandler::GetDelegatePrincipal() PermissionDelegateHandler.cpp
    ...
freed by thread T0:
    #1 ContentParent::DeallocPContentPermissionRequestParent() ContentParent.cpp:5129
    ...
previously allocated by thread T0:
    #3 nsContentPermissionUtils::CreateContentPermissionRequestParent() nsContentPermissionHelper.cpp:247

Exploitability

This vulnerability is highly exploitable for sandbox escape:

• Attacker-controlled timing: The compromised content process fully controls when the actor is created, when duplicate prompts are sent, and when destruction occurs.
• Heap-use-after-free: The freed ContentPermissionRequestParent object (104 bytes) can be replaced with attacker-controlled data via heap spraying from the content process. When proxy1->mParent->mPrincipal is subsequently dereferenced, the attacker controls the virtual table pointer and data, enabling arbitrary code execution.
• Parent process context: The crash occurs in the parent (browser) process main thread, which has full system privileges outside the content sandbox.
• No IPDL state machine protection: The protocol definition has no state machine restricting duplicate prompt() messages, making this attack trivially reliable.

Suggested Fix

In Recvprompt(), either:

1. Check if mProxy is already set and reject duplicate prompts, or
2. Properly clean up the previous proxy before creating a new one:

mozilla::ipc::IPCResult ContentPermissionRequestParent::Recvprompt() {
  if (mProxy) {
    // Prevent duplicate prompt - reject or clean up previous proxy
    mProxy->OnParentDestroyed();  // Nullify the old proxy's mParent
  }
  mProxy = new nsContentPermissionRequestProxy(this);
  if (NS_FAILED(mProxy->Init(mRequests))) {
    RefPtr<nsContentPermissionRequestProxy> proxy(mProxy);
    proxy->Cancel();
  }
  return IPC_OK();
}

Alternatively, add a state check to reject duplicate prompt() messages entirely:

mozilla::ipc::IPCResult ContentPermissionRequestParent::Recvprompt() {
  if (mProxy) {
    return IPC_FAIL(this, "duplicate prompt message");
  }
  mProxy = new nsContentPermissionRequestProxy(this);
  ...
}

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: exploit.patch

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: crash_stack.txt

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: test_perm_uaf.html

Comment 4

[Andrew McCreight [:mccr8]]

This looks pretty nasty.

Comment 5

[Andrew McCreight [:mccr8]]

AFAICT this dates back to the initial geolocation e10s work in 2010.

Comment 6

[Andrew McCreight [:mccr8]]

Nika pointed out that the prompt message is pointless because we always send it exactly once after the constructor, and fixing that would avoid this problem. Funnily enough, not long after this bug was landed, a proposal to do that got filed as an "optimization", in bug 596015.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: (secure)

Fold the "prompt" message into the constructor, as we only ever send it
once, right after we create the actor. Also modernize the initialization.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: (secure)

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 9548407 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The fix here consolidates two messages, so it isn't obviously a security problem. There's even an old bug on file about making this change, bug 596015 (fun fact: this sandbox escapes dates back to 2010, before we even shipped e10s). However, it probably isn't too hard to figure out where the security issue is once you think about why we aren't leaving a separate message. eg the problem is either going to happen when you send that message 0 times or more than once.
• 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?: 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?: Shouldn't be an issue, aside from rebase issues in adjacent code. This code hasn't changed for around 5 years.
• How likely is this patch to cause regressions; how much testing does it need?: Hopefully it won't be too bad. We just do some stuff earlier. We do have some tests for this code.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 10

[Andrew McCreight [:mccr8]]

(my approval also applies to the second patch, but it is just a cleanup with no security properties)

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 9548407 [details]
(secure)

sec-approval+ to land now and request uplifts

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4fe219b19630
https://hg.mozilla.org/integration/autoland/rev/477114802b13
Fold the prompt message into ContentPermissionRequest construction. r=nika
https://github.com/mozilla-firefox/firefox/commit/3da33505e5fa
https://hg.mozilla.org/integration/autoland/rev/2158d234f858
Modernize the initializers for ContentPermissionRequestParent. r=nika

Comment 13

[Andrew McCreight [:mccr8]]

Bug 596015 should get duped to this whenever this bug is opened.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/477114802b13
https://hg-edge.mozilla.org/mozilla-central/rev/2158d234f858

Comment 15

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

The patch landed in nightly and beta is affected.
:mccr8, 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 16

[Andrew McCreight [:mccr8]]

This bug has two patches, but only the "Fold the prompt message" is needed to fix the security issue. I'm not sure whether I requested to uplift them both or not.

Comment 17

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: very old security issue
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: This combines 2 messages into 1, which should be simple, but maybe making the code run at a slightly different time could cause problems. I'm not an expert on this code.
• String changes made/needed: none
• Is Android affected?: yes

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: (secure)

Instead of sending a separate message, immediately do the initialization after the
actor is created and registered, via a new override of RecvPContentPermissionRequestConstructor.
I didn't move the initialization inside AllocPContentPermissionRequestParent because
the Init() method runs JS so it seems like a bad idea to do when the actor isn't fully set up.

aRequests is now set inside the Init() method instead of the constructor, which allows us
to avoid a copy of this array at the low cost of making the diff gross.

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

Comment 19

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined: very old security issue
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: This combines 2 messages into 1, which should be simple, but maybe making the code run at a slightly different time could cause problems. I'm not an expert on this code.
• String changes made/needed: none
• Is Android affected?: yes

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: (secure)

Instead of sending a separate message, immediately do the initialization after the
actor is created and registered, via a new override of RecvPContentPermissionRequestConstructor.
I didn't move the initialization inside AllocPContentPermissionRequestParent because
the Init() method runs JS so it seems like a bad idea to do when the actor isn't fully set up.

aRequests is now set inside the Init() method instead of the constructor, which allows us
to avoid a copy of this array at the low cost of making the diff gross.

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

Comment 21

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: very old security issue
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: This combines 2 messages into 1, which should be simple, but maybe making the code run at a slightly different time could cause problems. I'm not an expert on this code.
• String changes made/needed: none
• Is Android affected?: yes

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: (secure)

Instead of sending a separate message, immediately do the initialization after the
actor is created and registered, via a new override of RecvPContentPermissionRequestConstructor.
I didn't move the initialization inside AllocPContentPermissionRequestParent because
the Init() method runs JS so it seems like a bad idea to do when the actor isn't fully set up.

aRequests is now set inside the Init() method instead of the constructor, which allows us
to avoid a copy of this array at the low cost of making the diff gross.

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

Comment 23

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/dd03f7d7a35c
https://hg.mozilla.org/releases/mozilla-beta/rev/7fb61138232a

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/a8f9579566b9

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/3874c659e209