Closed Bug 925571 Opened 7 years ago Closed 7 years ago

Use unrestricted Chromium sandbox for plugin_container for content processes

Categories

(Core :: Security, defect)

x86_64
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
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.
Attached patch Patch 1 - Build config. rev1 (obsolete) — Splinter Review
Attachment #819054 - Attachment is obsolete: true
Attachment #819329 - Flags: review?(benjamin)
Attachment #819329 - Attachment description: Patch v1 - Build config. rev1 → Patch 1 - Build config. rev1
Attachment #819329 - Flags: review?(benjamin) → review+
I don't want to be the reviewer for the rest of this. aklotz maybe?
Attachment #819330 - Flags: review?(benjamin) → review?(aklotz)
Attachment #819332 - Flags: review?(benjamin) → review?(aklotz)
(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.
Missed the packaging part of this in the build config patch
Attachment #821259 - Flags: review?(benjamin)
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+
Summary: Use Chromium sandbox for plugin_container for content processes (step 1: unrestricted) → Use unrestricted Chromium sandbox for plugin_container for content processes
Implemented review nits, carrying forward r+.
Attachment #819330 - Attachment is obsolete: true
Attachment #822752 - Flags: review+
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)
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+
Attachment #821259 - Flags: review?(khuey)
Attachment #821259 - Flags: review?(benjamin)
Attachment #821259 - Flags: review+
Depends on: 934445
Depends on: 935980
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.