Use unrestricted Chromium sandbox for plugin_container for content processes

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla28
x86_64
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Assignee

Description

6 years ago
Bug 922756 Adds Chromium Sandbox to tree.

This bug is to use the Chromium sandbox from the broker process (firefox.exe) to start plugin_container and have plugin_container call the LowerToken() API from the target process.

The scope of this bug is just the basic plumbing without restricting anything.
Assignee

Comment 1

6 years ago
Here's a working proof of concept of using the chromium sandbox with plugin-container.  Everything seems to work fine with the sandboxed plugin-container. It is currently very unrestricted though through the sandbox policy.

The problem with this patch though  is that it is changing plugin-container to use /MT runtime. This makes the file sizes larger. This is because the sandbox libraries must be linked to plugin-container and some other dll that we use from firefox.exe.  Currently that is browsercomps.dll.  Both browsercomps.dll and plugin-container have conflicting runtimes.  It was easier to change plugin-container so that's what this patch did.

I talked to bsmedberg and he'd prefer to instead use a different new dll instead of browsercomps.dll and probably just not use xpcom at all. Then use this dll from within ipc/glue.

We can't put things in xul.dll directly by the way because of the conflicting ipc base code.
Assignee

Comment 2

6 years ago
Posted patch Patch 1 - Build config. rev1 (obsolete) — Splinter Review
Attachment #819054 - Attachment is obsolete: true
Attachment #819329 - Flags: review?(benjamin)
Assignee

Updated

6 years ago
Attachment #819329 - Attachment description: Patch v1 - Build config. rev1 → Patch 1 - Build config. rev1

Updated

6 years ago
Attachment #819329 - Flags: review?(benjamin) → review+

Comment 5

6 years ago
I don't want to be the reviewer for the rest of this. aklotz maybe?
Assignee

Updated

6 years ago
Attachment #819330 - Flags: review?(benjamin) → review?(aklotz)
Assignee

Updated

6 years ago
Attachment #819332 - Flags: review?(benjamin) → review?(aklotz)
Assignee

Comment 6

6 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> I don't want to be the reviewer for the rest of this. aklotz maybe?

No problem, thanks for the build-config review, and thanks for the discussions around implementation.
Sorry for the delay, I'll start reviewing this tomorrow morning.
Assignee

Comment 8

6 years ago
Missed the packaging part of this in the build config patch
Attachment #821259 - Flags: review?(benjamin)
Assignee

Updated

6 years ago
Attachment #819332 - Attachment description: Patch 3 of 3 - Target process (One who gets sandboxed) → Patch 3 - Target process (One who gets sandboxed)
Comment on attachment 819330 [details] [diff] [review]
Patch 2 - Sandbox broker code (one who starts the sandbox process)

Review of attachment 819330 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Can you please add vim and emacs mode lines?

@@ +8,5 @@
> +
> +namespace mozilla
> +{
> +
> +SandboxBroker::SandboxBroker() : mBrokerService(nullptr)

nit: Initialization list on new line

@@ +25,5 @@
> +    mBrokerService = sandbox::SandboxFactory::GetBrokerServices();
> +    if (!mBrokerService) {
> +      return false;
> +    }
> +    if (result = mBrokerService->Init()) {

Can you please revise this statement so that it is a little bit more explicit about Init's return value, such as comparing it against SBOX_ALL_OK? I found this line a bit confusing because we usually deal with Init functions that return a boolean or are wrapped with NS_SUCCEEDED. Anything that makes it a bit easier for the reader to see what's going on without needing to look up the definition of the enum would be nice.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Can you please add vim and emacs mode lines?
Attachment #819330 - Flags: review?(aklotz) → review+
Attachment #819332 - Flags: review?(aklotz) → review+
Assignee

Updated

6 years ago
Summary: Use Chromium sandbox for plugin_container for content processes (step 1: unrestricted) → Use unrestricted Chromium sandbox for plugin_container for content processes
Assignee

Comment 10

6 years ago
Implemented review nits, carrying forward r+.
Attachment #819330 - Attachment is obsolete: true
Attachment #822752 - Flags: review+
Assignee

Comment 12

6 years ago
Comment on attachment 821259 [details] [diff] [review]
Patch 4 of 4 - Packaging

Review of attachment 821259 [details] [diff] [review]:
-----------------------------------------------------------------

+r? khuey
In case you can get to this first, it's the last thing waiting for review so this can land and it's pretty small.
Attachment #821259 - Flags: review?(khuey)
Assignee

Comment 13

6 years ago
Rebased patch, moved FORCE_STATIC_LIB = True to moz.build file. Carrying forward r+ on patch 1.
Attachment #819329 - Attachment is obsolete: true
Attachment #824071 - Flags: review+

Updated

6 years ago
Attachment #821259 - Flags: review?(khuey)
Attachment #821259 - Flags: review?(benjamin)
Attachment #821259 - Flags: review+
Assignee

Updated

6 years ago
Depends on: 934445
Assignee

Updated

6 years ago
Depends on: 935980
Assignee

Comment 17

6 years ago
Not intentional, bug 935980 was filed to fix it. Thanks. This is not built by default on Windows anywhere so non-critical short term.
You need to log in before you can comment on or make changes to this bug.