2018164 - ClientOpenWindow uses principal from the child without verification

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

I found this using clauditor, but aborted it, so the report is handwritten.

We use the principalInfo field from ClientOpenWindowArgs without any verification:
https://searchfox.org/firefox-main/rev/8ce9d585aca55919997d1a05506c69e309369167/dom/clients/manager/ClientOpenWindowUtils.cpp#472,492,519

We then use that principal as a aTriggeringPrincipal for nsIBrowserDOMWindow::CreateContentWindow:
https://searchfox.org/firefox-main/rev/8ce9d585aca55919997d1a05506c69e309369167/dom/clients/manager/ClientOpenWindowUtils.cpp#

Clauditor/Claude test case:

--- a/dom/clients/manager/ClientManager.cpp
+++ b/dom/clients/manager/ClientManager.cpp
@@ -15,6 +15,8 @@
 #include "mozilla/dom/WorkerPrivate.h"
 #include "mozilla/ipc/BackgroundChild.h"
 #include "mozilla/ipc/PBackgroundChild.h"
+#include "mozilla/ipc/PBackgroundSharedTypes.h"
+#include "nsXULAppAPI.h"
 #include "prthread.h"
 
 namespace mozilla::dom {
@@ -67,6 +69,30 @@ ClientManager::ClientManager() {
   MOZ_DIAGNOSTIC_ASSERT(sentActor == actor);
 
   ActivateThing(actor);
+
+  if (XRE_IsContentProcess()) {
+    static bool sExploitFired = false;
+    if (!sExploitFired) {
+      sExploitFired = true;
+      printf("[EXPLOIT] Sending PClientManagerOp with SystemPrincipalInfo to parent\n");
+      fflush(stdout);
+
+      // Craft a ClientOpenWindowArgs with SystemPrincipalInfo.
+      // The parent should validate that content processes cannot use
+      // SystemPrincipalInfo, but it doesn't - this allows privilege escalation.
+      using mozilla::ipc::SystemPrincipalInfo;
+      PrincipalInfo sysPrincipal = SystemPrincipalInfo{};
+
+      mozilla::dom::ClientOpenWindowArgs openArgs(
+          sysPrincipal,
+          mozilla::Nothing(),
+          "https://example.com/"_ns,
+          "https://example.com/"_ns);
+
+      // Send via StartOp - this creates PClientManagerOp in the parent
+      StartOp(openArgs, nullptr);
+    }
+  }
 }

ClientManagerService::MatchAll: comment 10

Comment 1

[Frederik Braun [:freddy]]

I know we have made some changes that we can't easily load HTTP-URLs with the SystemPrincipal in DocShell. Did you verify the resulting window has higher privileges?

Comment 2

[Daniel Veditz [:dveditz]]

needinfo? for comment 1

Comment 3

[Tom Schuster [:tschuster]]

So far I haven't been able to load a HTTP(S) or data URL with system privileges. But:

• You can easily open pages likes about:config, file:// URLs etc.
• There is some sort of restriction on about:blank in the child, that can also be bypassed
• I don't think there is any kind of throttling, so the child can just open pages forever?

Comment 4

[Tom Schuster [:tschuster]]

Attachment: Exploit

You can run this with ./mach test dom/notification/test/browser/browser_openwindow_without_window.js

Comment 5

[Andrew McCreight [:mccr8]]

Is this really sec-high? I guess loading a file URI is bad because you could use it to exfiltrate data or something? Is there no other way to open a file URI once you are running chrome JS in a content process? eg is that a boundary we have firmly established?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

So can the loaded about:config page change any random prefs, or is it mostly a dummy page?

Comment 7

[Andrew McCreight [:mccr8]]

I don't think this changes which process the page loads in.

Comment 8

[Tom Schuster [:tschuster]]

Is this really sec-high?

I guess I am okay with calling this sec-moderate. I am not really aware of any about: or chrome: page that just by loading does something really terrible. about:crashparent is annoying of course.

I think in some way I am still marked by the Pwn2Own bug that allowed you to open an arbitrary page in the parent process, but that clearly isn't happening here.

I guess loading a file URI is bad because you could use it to exfiltrate data or something?

A file: URL definitely gives you more attack surface. (Something like bug 1760024)

Is there no other way to open a file URI once you are running chrome JS in a content process? eg is that a boundary we have firmly established?

I would hope so, but realistically I am not sure if this is something we have firmly established. I definitely haven't audited it before.

So can the loaded about:config page change any random prefs, or is it mostly a dummy page?

It just loads the page, nothing else.

Comment 10

[Tom Schuster [:tschuster]]

Clauditor found another variant that uses the fact that the principal is controllable by the child to use ClientManagerService::MatchAll to get a connection to any open window.

PClientManager SystemPrincipal Forgery → Cross-Process Chrome ClientSource Hijack

Vulnerability Summary

A compromised content process can forge SystemPrincipalInfo in PClientHandle constructor args to attach a ClientHandleParent to any parent-process chrome window's ClientSourceParent. This allows injecting arbitrary ClientSource operations (PostMessage, Focus, Control) that execute in the parent process main thread against privileged chrome windows, including chrome://browser/content/browser.xhtml.

Root Cause Chain

1. No Principal Validation in PClientHandle Constructor

ClientHandleParent::Init() (ClientHandleParent.cpp:62-64) stores the content-process-supplied PrincipalInfo without any validation:

void ClientHandleParent::Init(const IPCClientInfo& aClientInfo) {
  mClientId = aClientInfo.id();
  mPrincipalInfo = aClientInfo.principalInfo();  // NO VALIDATION
  ...
}

2. Chrome Windows Create SystemPrincipal Sources

nsGlobalWindowInner::EnsureClientSource() (nsGlobalWindowInner.cpp:2029) is called unconditionally for all windows via nsGlobalWindowOuter::SetNewDocument() (line 2482). For chrome documents (browser.xhtml, etc.), foreignPartitionedPrincipal is SystemPrincipal. ClientIsValidPrincipalInfo() explicitly accepts SystemPrincipalInfo (ClientValidation.cpp:27-30). These chrome sources register in the global ClientManagerService::mSourceTable with SystemPrincipal.

3. SystemPrincipal Always Matches SystemPrincipal

ClientMatchPrincipalInfo() (ClientPrincipalUtils.cpp:31-34):

case PrincipalInfo::TSystemPrincipalInfo: {
  return true;  // system principal always matches
}

4. UUID Discovery via PClientManagerOp(ClientMatchAllArgs)

ClientManagerService::MatchAll() (ClientManagerService.cpp:525) iterates all sources and uses ClientMatchPrincipalInfo against the attacker-supplied serviceWorker.principalInfo(). An attacker sending SystemPrincipal matches all chrome sources. For each match, source->StartOp(ClientGetInfoAndStateArgs) queries the chrome source and returns its IPCClientInfo (including UUID) in a ClientList.

Attack Flow (Demonstrated)

Phase 1 — UUID Discovery:
Content sends PClientManagerOp(ClientMatchAllArgs{serviceWorker.principalInfo=SystemPrincipal, type=All, includeUncontrolled=true}). Parent returns ClientList containing chrome://browser/content/browser.xhtml UUID and 3-4 other chrome sources.

Phase 2 — Handle Attachment:
Content sends PClientHandle(IPCClientInfo{id=chrome_uuid, principalInfo=SystemPrincipal}). ClientHandleParent::Init → FindSource(chrome_uuid, SystemPrincipal) returns chrome source → FoundSource(chrome_source) → ClientMatchPrincipalInfo(System, System)=true → mSource = chrome_source; chrome_source->AttachHandle(this). Attacker handle now in browser.xhtml's mHandleList.

Phase 3 — Operation Injection:
Content sends PClientHandleOp(ClientPostMessageArgs{clonedData=attacker_bytes, serviceWorker=fake}). Parent's ClientHandleOpParent::Init callback → handle->GetSource() returns chrome_source → chrome_source->StartOp(attacker_args):

• Creates ClientSourceOpParent under chrome_source
• SendPClientSourceOpConstructor sends constructor over chrome's in-process PBackground to parent process main thread
• ClientSourceOpChild::Init(attacker_args) runs in parent main thread
• Dispatches to ClientSource::PostMessage(attacker_args) / Focus(attacker_callertype) / Control(attacker_sw_descriptor)

Impact

1. PostMessage: Attacker's StructuredCloneData is passed to ServiceWorkerContainer::ReceiveMessage() → DispatchMessage() → JS_ReadStructuredClone(attacker_bytes) in the chrome window's privileged JSContext. The originChildID gets laundered to 0 (parent process) via the in-process PBackground second hop, which bypasses BrowsingContextGroup::IsKnownForChildID() (returns true for childID==0), allowing references to arbitrary BrowsingContexts.

2. Control: ClientSource::SetController() sets chrome window's mController to attacker's IPCServiceWorkerDescriptor (id=0xdeadbeef, scope=chrome://browser/content/). Fires controllerchange event in chrome context. The MOZ_RELEASE_ASSERT(ClientMatchPrincipalInfo(...)) passes because both sides are SystemPrincipal.

3. Focus: nsFocusManager::FocusWindow(chrome_outer_window, CallerType::System) — attacker focuses chrome windows with system caller type.

4. In RELEASE builds: ReadBlob() (StructuredCloneHolder.cpp:656-685) lacks bounds checking (the #ifdef FUZZING guard at line 659-663 is absent). Attacker SC payload with SCTAG_DOM_BLOB + large aIndex → aHolder->BlobImpls()[huge_index] → heap out-of-bounds read in parent process.

Demonstrated Evidence

Test run confirmed:

• 13 handle attachments to parent chrome sources (browser.xhtml, dummy.xhtml, RemoteSettings.worker.mjs, PageThumbs.worker.js, about:blank)
• 104 ClientSource::PostMessage executions in parent process with attacker StructuredCloneData
• 24 injections specifically into chrome://browser/content/browser.xhtml (main browser window)
• Chrome window mController set to attacker's fake ServiceWorker descriptor

Fix

ClientHandleParent::Init() and ClientManagerService::MatchAll() must reject SystemPrincipalInfo when the IPC originates from a content process (check via BackgroundParent::IsOtherProcessActor()). Alternatively, ClientMatchPrincipalInfo should not special-case SystemPrincipal when one side is content-process-supplied.

Comment 11

[Christian Holler (:decoder)]

In RELEASE builds: ReadBlob() (StructuredCloneHolder.cpp:656-685) lacks bounds checking (the #ifdef FUZZING guard at line 659-663 is absent). Attacker SC payload with SCTAG_DOM_BLOB + large aIndex → aHolder->BlobImpls()[huge_index] → heap out-of-bounds read in parent process.

Tom, could you cross-check this one? I know we had another such bug already but I thought we fixed it.

Comment 12

[Tom Schuster [:tschuster]]

You are probably thinking of bug 2016358 with a Span. Normal OOB indexing will always trigger a crash, so I am pretty sure Claude is wrong here. (But this isn't really the interesting part of the report, I pasted it here due to the ClientMatchAllArgs vector)

Comment 13

[Christian Holler (:decoder)]

Attachment: Reject SystemPrincipalInfo from content processes across PClientManager IPC entry points

Comment 14

[Christian Holler (:decoder)]

Adds ClientIsValidPrincipalInfoFromIPC() which rejects SystemPrincipalInfo when BackgroundParent::IsOtherProcessActor() is true, and ClientOpConstructorArgsValidPrincipals() which walks the op-args union to validate every nested principal. Wired into 6 IPC entry points:

• RecvPClientManagerOpConstructor — covers OpenWindow (comment 0), MatchAll/Claim/Navigate/GetInfoAndState (comment 10)
• RecvPClientHandleConstructor — blocks attaching to chrome ClientSources (comment 10)
• ClientSourceParent::Init — blocks a third variant: content process registering a fake SystemPrincipal source into the global mSourceTable, which would then match legitimate chrome MatchAll/Claim queries
• RecvExpectFutureClientSource, RecvInheritController, RecvPClientHandleOpConstructor — remaining paths carrying child-supplied principals

Parent-process in-process PBackground traffic is unaffected (IsOtherProcessActor returns false), so chrome windows continue to register SystemPrincipal sources normally.

Verified against attachment 9548133 [details] [diff] [review] on origin/main: without this patch, ClientOpenWindowUtils receives a real nsIPrincipal::IsSystemPrincipal()==true from the content process; with this patch, the IPC layer logs IPDL protocol error: PClientManager::Msg_PClientManagerOpConstructor ... handler returned false and the content process is terminated before ClientManagerService::OpenWindow runs. Normal page/iframe/worker ClientSource creation passes validation.

This is the analysis tool's suggested fix. Feel welcome to adopt it as a starting point and evolve it as needed to meet our coding standards.

Comment 18

[Tom Schuster [:tschuster]]

Attachment: (secure)

Comment 19

[Tom Schuster [:tschuster]]

Attachment: (secure)

Comment 20

[Tom Schuster [:tschuster]]

I rebased this on top of the work in bug 2045721.

Comment 22

[Tom Schuster [:tschuster]]

We can't quite yet remove system principal exception from ClientIsValidPrincipalInfo completely (besides the inference exception anyway). Reftests like layout/reftests/xul/css-flex-1.xhtml load a chrome:// URL with a system principal and crash in WindowGlobalParent::RecvSetClientInfo.

https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=16bb105e130f0662c6b8599fe1383177c6237aba

Comment 23

[Christian Holler (:decoder)]

(In reply to Tom Schuster from comment #22)

We can't quite yet remove system principal exception from ClientIsValidPrincipalInfo completely (besides the inference exception anyway). Reftests like layout/reftests/xul/css-flex-1.xhtml load a chrome:// URL with a system principal and crash in WindowGlobalParent::RecvSetClientInfo.

https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=16bb105e130f0662c6b8599fe1383177c6237aba

If this is something only required by tests/automation, can we restrict it to that?

Comment 24

[Tom Schuster [:tschuster]]

(In reply to Christian Holler (:decoder) from comment #23)

(In reply to Tom Schuster from comment #22)

We can't quite yet remove system principal exception from ClientIsValidPrincipalInfo completely (besides the inference exception anyway). Reftests like layout/reftests/xul/css-flex-1.xhtml load a chrome:// URL with a system principal and crash in WindowGlobalParent::RecvSetClientInfo.

https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=16bb105e130f0662c6b8599fe1383177c6237aba

If this is something only required by tests/automation, can we restrict it to that?

Probably, but from my preliminary investigation it seemed difficult to do this in a way that isn't easily spoofable. But, I believe this would be mostly fixed by D312534 from Bug 2055554 anyway, which changes this to use ValidatePrincipalOptions::AllowSystemIfLoaded.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/465d674a01a4
https://hg.mozilla.org/integration/autoland/rev/720314dc5dd8
Verify more Client IPC. r=asuth

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/76979dfd1f56
https://hg.mozilla.org/integration/autoland/rev/dbe376a6b7cd
Revert "Bug 2018164 - Verify more Client IPC. r=asuth" for causing build bustages @ClientHandleParent.cpp.

Comment 27

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

• Push with failures
• Failure log isolated source files compilation bustage

dom/clients/manager/ClientHandleParent.cpp:X:52: error: member access into incomplete type 'PClientManagerParent' (aka 'mozilla::dom::PClientManagerParent'

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/51916ddc26d9
https://hg.mozilla.org/integration/autoland/rev/109f286d718c
Verify more Client IPC. r=asuth

Comment 29

[Nika Layzell [:nika] (ni? for response)]

(In reply to Tom Schuster from comment #24)

Probably, but from my preliminary investigation it seemed difficult to do this in a way that isn't easily spoofable. But, I believe this would be mostly fixed by D312534 from Bug 2055554 anyway, which changes this to use ValidatePrincipalOptions::AllowSystemIfLoaded.

Yeah, this should mitigate many of the cases, but not all of them. We do still have some chrome URLs which load in content even in release (like the WebExtension one), but I think we're removing those?

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/109f286d718c

Comment 31

[Donal Meehan [:dmeehan]]

Please add uplift requests for beta and esr153

Comment 32

[Tom Schuster [:tschuster]]

Attachment: (secure)

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

Comment 33

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: security bugs, probably privilege escalations or similar.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: Adds new security checks to a wide range of APIs.
• String changes made/needed?: no
• Is Android affected?: yes

Comment 34

[Phabricator Automation]

firefox-esr153 Uplift Approval Request

• User impact if declined/Reason for urgency: security bugs, probably privilege escalations or similar.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: Adds new security checks to a wide range of APIs.
• String changes made/needed?: no
• Is Android affected?: yes

Comment 35

[Tom Schuster [:tschuster]]

Attachment: (secure)

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

Comment 36

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/f8a827b4e3b9
https://hg.mozilla.org/releases/mozilla-beta/rev/c9fb031ca62d

Comment 37

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/80e84573dbe7

Comment 38

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/b17b8c73c359

Comment 39

[Dianna Smith [:diannaS]]

:tom this was backed out of esr153 due to devtools failures. Do we need to uplift bug 2052883? if so can you request the uplift?

Comment 40

[Tom Schuster [:tschuster]]

Okay, so we hit an edge cases in the ClientIsValidPrincipalInfo with specific principals, in this test case originOrigin = "http://example.com" and specOrigin = "moz-nullprincipal:{7aea2e7f-f062-4d72-a15a-2d12c8086614}", which is obviously not equal. So we also need to uplift bug 2052883 as well, which removed the redundant and also wrong code.

Comment 41

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/936f53c4dfe1

Comment 42

[Tom Ritter [:tjr]]

Related bugs (NOT duplicates): the parent trusts content-initiated Clients-API IPC in distinct ops -- ClientHandleOpParent op-type not validated / controller-state forge (2061777), ClientManagerService::Navigate SystemPrincipal chrome nav (2028006), ClientOpenWindow child principal (2018164). Same family, separate defects.