1851989 - Add checks to see if socket process creates disallowed IPC actors

Status: RESOLVED FIXED

(Core :: IPC, task)

Description

[Kershaw Chang [:kershaw]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1758155#c27
We'd like to add some checks for ESR, if we don't want to uplift patches in bug 1758155.

Comment 1

[Kershaw Chang [:kershaw]]

One idea would be adding MOZ_RELEASE_ASSERT(!XRE_IsSocketProcess()) in SendPBackground*Constructor methods to prevent socket process from creating those IPC actors. However, those methods are automatically generated, so I am not sure if it's easy to do.

Nika, what do you think?
Thanks.

Comment 2

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

(In reply to Kershaw Chang [:kershaw] from comment #1)

One idea would be adding MOZ_RELEASE_ASSERT(!XRE_IsSocketProcess()) in SendPBackground*Constructor methods to prevent socket process from creating those IPC actors. However, those methods are automatically generated, so I am not sure if it's easy to do.

Nika, what do you think?
Thanks.

In order to do the assertions in an effective way, we'd want to have the checks in the Recv methods rather than the Send methods.

We'd perhaps want to add some methods which act as helpful checks in BackgroundParentImpl.cpp. We don't have the best tools for figuring out which type of actor each PBackgroundParent actor is right now, so we'd have to be somewhat inferring it, but I think we could do it like this:

// A PBackgroundParent is a normal "content" actor if both the parent
// side lives in the parent process and it either connects in-process, or
// to a content process.
static bool IsContentActor(PBackgroundParent* aActor) {
  return XRE_IsParentProcess() && 
      (!BackgroundParent::IsOtherProcessActor(aActor) ||
       BackgroundParent::GetContentParentHandle(aActor));
}

// The parent side of socket bridge actors is always in the socket process.
static bool IsSocketBridgeActor(PBackgroundParent* aActor) {
  return XRE_IsSocketProcess();
}

// The parent side of socket actors is the parent process, and it must
// not have a content process.
static bool IsSocketActor(PBackgroundParent* aActor) {
  return XRE_IsParentProcess() && !IsContentActor(aActor);
}

We could then add a check to every RecvX and AllocX function. In the RecvX functions, we'd do a check like if (!IsContentActor(this)) { return IPC_FAIL(this, "must be a content actor"); }, and for allocations we'd return nullptr. Most of the functions would have a IsContentActor check except for the ones for the 3 types which you needed to move for socket processes.

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: Bug 1851989 - Add checks in BackgroundParentImpl to see if methods are called by correct actors, r=nika

Comment 4

[Kershaw Chang [:kershaw]]

Comment on attachment 9352249 [details]
Bug 1851989 - Add checks in BackgroundParentImpl to see if methods are called by correct actors, r=nika

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See bug 1758155. We'd like to add some checks to make sure socket process can only create allowed IPC actors.
• User impact if declined: Potential sandbox escape.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Disallowing socket process to open some IPC actors should be no risk.

Comment 5

[Pascal Chevrel:pascalc]

Comment on attachment 9352249 [details]
Bug 1851989 - Add checks in BackgroundParentImpl to see if methods are called by correct actors, r=nika

Approved for ESR 115.3, thanks.

Comment 6

[Pulsebot]

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