1316153 - Remove B2G-specific child process privilege levels from IPC

Status: RESOLVED FIXED

(Core :: IPC, defect, P2)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Copying this out of my review comment in bug 1147911 comment #30 just to make sure it doesn't get lost:

::: ipc/chromium/src/base/child_privileges.h
@@ +11,5 @@
> +
> +enum ChildPrivileges {
> +  PRIVILEGES_DEFAULT,
> +  PRIVILEGES_UNPRIVILEGED,
> +  PRIVILEGES_INHERIT,

I'm pretty sure we need only one of these three now.  They're a remnant of early B2G when it used Android's group-based permissions (the last of those went away in bug 976398, after which I think everything was UNPRIVILEGED — and UNPRIVILEGED as implemented needs the parent process to be running as root, so it was always B2G-only).  And B2G is gone now.  Currently it looks like everything winds up being INHERIT, but INHERIT being called INHERIT is probably because there used to be actual nested child processes, so it's maybe not the best name.

Comment 1

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Some notes from reviewing patches in bug 1382099:
* This means that SetCurrentProcessPrivileges, and any other uid/gid related stuff in process_util_linux, can go away.
* Also, kLowRightsSubprocesses in GeckoChildProcessHost.cpp can be constant-propagated.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I have a patch that removes ChildPrivileges entirely.  The only thing it's doing right now is communicating whether a content process may handle file:/// from GeckoChildProcessHost to the Windows sandbox, and that can be done more directly.  I'd been thinking of using it on desktop Linux for sandboxing purposes, but there's a cleaner way to do that.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1316153 - Remove base::ChildPrivileges from IPC.

ChildPrivileges is a leftover from the B2G process model; it's now
mostly unused, except for the Windows sandbox using it to carry whether
a content process has file:/// access.

In general, when sandboxing needs to interact with process launch, the
inputs are some subset of: the GeckoProcessType, the subtype if content,
various prefs and even GPU configuration; and the resulting launch
adjustments are platform-specific.  And on some platforms (e.g., OS X)
it's all done after launch.  So a simple enum used cross-platform isn't
a good fit.

Review commit: https://reviewboard.mozilla.org/r/186580/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186580/

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8915389 [details]
Bug 1316153 - Remove base::ChildPrivileges from IPC.

https://reviewboard.mozilla.org/r/186580/#review191748

I've only skimmed the IPC stuff.
It's possible that we might want to pass down the remote type as the subtype, but a bool is fine for now.
Also, thinking about it, once we get round to proxying the file access we shouldn't even need that.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8915389 [details]
Bug 1316153 - Remove base::ChildPrivileges from IPC.

https://reviewboard.mozilla.org/r/186580/#review193364

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87077c875c63
Remove base::ChildPrivileges from IPC. r=billm,bobowen

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/87077c875c63