1758155 - PBackground-managed Actors can be opened for the wrong process

Status: RESOLVED FIXED

(Core :: Networking, task, P2)

Description

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

Since bug 1503834 and bug 1603420, we've been creating the PBackground actor between not just a content and parent process, but also between content & socket or socket & parent processes. For each of these different connections, a different subset of the managed actors and messages on PBackground are actually supported, and the others should never be used.

Unfortunately, there appear to be no checks in the individual Recv and Alloc methods on BackgroundParentImpl confirming that the source process is as expected (https://searchfox.org/mozilla-central/rev/7f1db1d2c556b82114b62f5aa4aa29397ad5bce4/ipc/glue/BackgroundParentImpl.cpp), and producing an error if it is not. This could theoretically allow a compromised content process to open actors which normally are only supposed to be connected between a different process pair to the parent or socket processes.

Methods should be added to help check that the process connection is of each of the relevant types, and checks should probably be added to every method in BackgroundParentImpl to prevent opening an actor in the wrong process when sent an invalid message, in order to reduce the potential untested attack surface.

Alternatively, we could switch away from using PBackground with the socket process, and use a different toplevel actor, giving us static guarantees about which methods are actually available between each process pair.

Comment 1

[:Gijs (he/him)]

Since the stuff from Friday relating to access to webgpu this scares me... more... than it might otherwise have done. Is that reasonable or am I just being overly paranoid? If it's not just me, I guess let's find out how hard it would be to do something here?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Is that reasonable or am I just being overly paranoid?

I think the timing of this bug being filed is not a coincidence ;-)

Comment 3

[Andrew McCreight [:mccr8]]

It has been something I've been thinking about for awhile, but yes, I was inspired to file this by QA asking how to verify the WebGPU issue, and there's no good answer for how to do it. Sorry for being oblique about that.

Unfortunately it does feel like it could be a big project, unless we can somehow reuse Chrome's work. I'm not sure we can afford to do this, but I'm not sure we can afford to not do this.

Comment 4

[Andrew McCreight [:mccr8]]

Oops, I thought I was posting in another bug. Yes, this came out of me mentioning the WebGPU issue to Nika.

Comment 5

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #3)

Unfortunately it does feel like it could be a big project, unless we can somehow reuse Chrome's work. I'm not sure we can afford to do this, but I'm not sure we can afford to not do this.

Can you elaborate a little on this? I'm not terribly familiar with our ipdl stuff - but I would have hoped that we could enforce having certain annotations on the interfaces that describe what process the parent/child is meant to run in, and then enforce those when the actors get instantiated. Does it not work that way? comment #0 mentions having to add checks in every method; is checking on instantiation not sufficient / possible?

Comment 6

[Tom Ritter [:tjr]]

(In reply to :Gijs (he/him) from comment #5)

(In reply to Andrew McCreight [:mccr8] from comment #3)

Unfortunately it does feel like it could be a big project, unless we can somehow reuse Chrome's work. I'm not sure we can afford to do this, but I'm not sure we can afford to not do this.

Can you elaborate a little on this? I'm not terribly familiar with our ipdl stuff - but I would have hoped that we could enforce having certain annotations on the interfaces that describe what process the parent/child is meant to run in, and then enforce those when the actors get instantiated. Does it not work that way? comment #0 mentions having to add checks in every method; is checking on instantiation not sufficient / possible?

mccr8's comment was for a different bug (Bug 1758522).

My general understanding of IPC is that top-level actors (i.e. ones without a manages line) should not be able to be created for a process they should not exist in. i.e. a Parent <-> GPU Actor can't be created by a Content Process between either Content <-> GPU or Content <-> Parent. I haven't seen an example of this being bypassed but I never understood the mechanism that prevented it. (This is what Nika meant by static guarantees I believe.)

However if an actor is a child actor, and its manager actor exists, then the process can request the child actor be created (unless there is an explicit process check that prevents it, but that's prone to omission.) That's the focus of this bug; that PBackground is being used for different types of process connections (e.g. Content <-> Parent, Content <-> Socket). Only some of the methods (and child actors) should be used for each of those types.

While annotations could help here; it really feels like the answer is separating Top-Level Actors by process type.

It might also be necessary to allow child actors to have two managers, so that messages in common for Parent <-> Socket and Content <-> Socket will live once, on a single child actor, managed by separate top-level actors for each type of process.

Comment 7

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

What :tjr said is roughly correct. We do have strong guarantees about what processes toplevel actors etc. can be created in, and that propagates out to everything which uses them. The problem here is specifically that when the socket process was introduced, rather than introducing a new toplevel actor for the socket process' connections, the PBackground protocol was re-used for the new actors. This probably simplified implementation, especially at the time when implementing new toplevel protocols was quite painful, but meant that we didn't get the structural checks preventing this kind of mistake for these new process types.

I think the best approach here would be to split the PBackground actor into different actors. PBackground would stay for {content,parent} -> parent connections, and a new actor would need to be added for content -> socket connections and socket -> parent connections.

If we can't do that, we should probably at least go through all implemented methods on PBackground and add a check for the type of PBackground actor they're acting on to perform the relevant check dynamically.

Comment 8

[Daniel Veditz [:dveditz]]

Calling this "sec-moderate" because we don't know of specific badness, but the pattern is bad and could well escalate to a sec-high or sec-critical if we do find a specific abusable instance.

Comment 9

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

The severity field is not set for this bug.
:jld, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Valentin Gosu [:valentin]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #7)

I think the best approach here would be to split the PBackground actor into different actors. PBackground would stay for {content,parent} -> parent connections, and a new actor would need to be added for content -> socket connections and socket -> parent connections.

I think that is reasonable. Thank you for bringing this up.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - P1: Refactor ParentImpl and ChildImpl for reusing code, r=nika

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - Introduce PSocketBackground and PSocketBackgroundStarter, r=nika

Depends on D167227

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - Move actors to PSocketBackground, r=nika

Depends on D167228

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - Make PMediaTransport and PBackgroundDataBridge toplevel actors, r=nika

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - Remove some PSM actos from PBackground, r=keeler,nika

Depends on D182995

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - Make PWebSocketConnection a toplevel actor, r=#necko

Depends on D182996

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - PBackground clenup, r=nika

Depends on D185031

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b628b437e19
Make PMediaTransport and PBackgroundDataBridge toplevel actors, r=nika,necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/4bc26c75c26a
Remove some PSM actos from PBackground, r=keeler,nika,necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/230add1b5bb5
Make PWebSocketConnection a toplevel actor, r=necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/bec8e6762e2a
PBackground clenup, r=nika

Comment 19

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

Backed out for causing build bustages in NetworkConnectivityService.cpp:

https://hg.mozilla.org/integration/autoland/rev/4624b75883dcdffc153eb7c168709962a46c71b7

Push with failures
Failure log

netwerk/base/NetworkConnectivityService.cpp(50,3): error: use of undeclared identifier 'ClearOnShutdown'

followed by more failure lines.

Comment 20

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - Fix mingwclang build, r=nika

Apparently, MOZ_WEBRTC is not defined on some platforms and it looks like the IPDL parser doesn't like an empty IPDL file.
This patch adds an Dummy IPC message to make the compiler happy.

Comment 21

[Kershaw Chang [:kershaw]]

Attachment: Bug 1758155 - Allow empty top-level IPC protocol, r=nika

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cae68edb7ea6
Make PMediaTransport and PBackgroundDataBridge toplevel actors, r=nika,necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/629d45b0c814
Remove some PSM actos from PBackground, r=keeler,nika,necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/e8c4fe7a2819
Make PWebSocketConnection a toplevel actor, r=necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/247891c73bc5
PBackground clenup, r=nika
https://hg.mozilla.org/integration/autoland/rev/bdcde8c620b4
Allow empty top-level IPC protocol, r=nika
https://hg.mozilla.org/integration/autoland/rev/75efbbfb861e
Fix mingwclang build, r=nika

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/cae68edb7ea6
https://hg.mozilla.org/mozilla-central/rev/629d45b0c814
https://hg.mozilla.org/mozilla-central/rev/e8c4fe7a2819
https://hg.mozilla.org/mozilla-central/rev/247891c73bc5
https://hg.mozilla.org/mozilla-central/rev/bdcde8c620b4
https://hg.mozilla.org/mozilla-central/rev/75efbbfb861e

Comment 24

[Ryan VanderMeulen [:RyanVM]]

FWIW, this will need rebasing for both Beta and ESR.

Comment 25

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

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

• If yes, please nominate the patch for beta approval. Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Comment 26

[Kershaw Chang [:kershaw]]

No need to uplift.

Comment 27

[Tom Ritter [:tjr]]

We'd like to figure out some solution for ESR115 - nika suggested adding manual checks to each method on PBackground if that seems easier and less risky than rebasing.

Comment 28

[Kershaw Chang [:kershaw]]

(In reply to Tom Ritter [:tjr] (Back 9/5) from comment #27)

We'd like to figure out some solution for ESR115 - nika suggested adding manual checks to each method on PBackground if that seems easier and less risky than rebasing.

I filed bug 1851989 for this.
if the patches in this bug cause no regression, we could consider just uplifting them.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

IIUC, ESR won't be getting the changes from this bug in favor of bug 1851989.

Comment 30

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter