Closed Bug 1580268 Opened 5 years ago Closed 5 years ago

sandbox GPU process on OpenBSD with pledge()

Categories

(Core :: Security: Process Sandboxing, enhancement, P5)

Product:
Core
Component:
Security: Process Sandboxing
Version:
69 Branch
Platform:
All
OpenBSD
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: jcs, Assigned: jcs)

References

Details

Attachments

(2 files, 6 obsolete files)

Bug 1580268: Sandbox GPU process on OpenBSD with pledge() r?gcp
(deleted), text/x-phabricator-request
Details
ff-gpu-pledge7.diff
(deleted), patch
Details | Diff | Splinter Review
Attached patch ff-gpu-pledge.diff (obsolete) (deleted) — — Splinter Review

Add a security.sandbox.pledge.gpu preference that is passed to the GPU process to allow it to call pledge() on OpenBSD for kernel-enforced sandboxing.

Since the user profile preferences are not available in the GPU process, the GPUProcessManager passes them as a command line argument (-GPUpledge).

Attached patch ff-gpu-pledge2.diff (obsolete) (deleted) — — Splinter Review
Attachment #9091829 - Attachment is obsolete: true
Summary: sandbox GPU process on OpenBSD with pledge → sandbox GPU process on OpenBSD with pledge()
Assignee: nobody → jcs
Status: UNCONFIRMED → NEW
Component: General → Security: Process Sandboxing
Ever confirmed: true
Product: Firefox → Core
Hardware: x86_64 → All
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1457092
Comment on attachment 9091832 [details] [diff] [review] ff-gpu-pledge2.diff Technically this also adds support for setting MOZ_DISABLE_PLEDGE in the env to temporarly disable sandboxing for debugging purposes.
Attachment #9091832 - Flags: feedback?(gpascutto)
Comment on attachment 9091832 [details] [diff] [review] ff-gpu-pledge2.diff Review of attachment 9091832 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ipc/GPUProcessManager.cpp @@ +168,5 @@ > + nsAutoCString pledgeString; > + > + if (!PR_GetEnv("MOZ_DISABLE_PLEDGE")) { > + // Preferences are not loaded by the GPU process, so we need to pass the > + // pledge string This is confusing to me? Doesn't the code you patch up above set up the de-serialization of shared preferences in the GPU Process? What do you mean exactly by "Preferences are not loaded by the GPU process"? I think we'd strongly prefer using the current map rather than adding an ad-hock argument, so I'd like the understand why that doesn't work.
Attachment #9091832 - Flags: feedback?(gpascutto)

From my testing, none of the preferences in the profile were available at the time that code runs (fetching it just returns an empty string). I think it was only loading the static preferences from modules/libpref/init/StaticPrefList.h by that time, not any in the user's profile. If I'm mistaken, please let me know how to load them and I can move the sandbox call back to after the SharedPreferenceDeserializer runs.

Priority: -- → P5

Ok I figured it out the problem. The sandboxing needed to be done in GPUParent::Init (which is confusingly called from the GPU child, not the parent, in GPUProcessImpl::Init) after NS_InitMinimalXPCOM() is called. This way the preferences are loaded and I can access the GPU pledge string.

I'll update the patch once I figure out some things in #1580271.

Attached patch ff-gpu-pledge3.diff (obsolete) (deleted) — — Splinter Review

3rd try at pledge patch for GPU process, now able to read pledge string from preferences

Attachment #9091832 - Attachment is obsolete: true
Comment on attachment 9092459 [details] [diff] [review] ff-gpu-pledge3.diff Review of attachment 9092459 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentChild.cpp @@ +4080,3 @@ > > + if (!PR_GetEnv("MOZ_DISABLE_PLEDGE")) { > + if (pledge(promisesString.get(), NULL) == -1) { nullptr
Attachment #9092459 - Flags: feedback+
Attached patch ff-gpu-pledge4.diff (obsolete) (deleted) — — Splinter Review

NULL -> nullptr

Attachment #9092459 - Attachment is obsolete: true
Comment on attachment 9092459 [details] [diff] [review] ff-gpu-pledge3.diff Great stuff ! At that point that looks good to me, unsure if we want to separate the 'MOZ_DISABLE_PLEDGE' addition to a separate commit. gcp, at that point, what's your opinion ? Otherwise i could upload two separate hg patches up for review (not via phabricator though, im not up to this)
Attached patch ff-gpu-pledge5.diff (obsolete) (deleted) — — Splinter Review

A new revision, now rebased on top of #1584839

Attachment #9092679 - Attachment is obsolete: true

gcp, at that point, what's your opinion ? Otherwise i could upload two separate hg patches up for review (not via phabricator though, im not up to this)

Seems good? Patch is minimally invasive anyway. I can deal with Phabricator for you.

Attached patch ff-gpu-pledge6.diff (obsolete) (deleted) — — Splinter Review

New patch against Firefox 70

Attachment #9097219 - Attachment is obsolete: true

technically this depends on bug #1584839 so that pledge config is moved to files first

Depends on: 1584839

jcs, why are you using pledgeFile.Append("pledge.gpu"); instead of OpenBSDFindPledgeFilePath like other process types ? i know this is moot with work from bug #1580271, just curious

Just a mixup trying to juggle all these different revisions.

Attached patch ff-gpu-pledge7.diff (deleted) — — Splinter Review
Attachment #9104536 - Attachment is obsolete: true
Pushed by gpascutto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8e2be8ec03fc Sandbox GPU process on OpenBSD with pledge() r=gcp

https://hg.mozilla.org/mozilla-central/rev/8e2be8ec03fc

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: