925571 - Use unrestricted Chromium sandbox for plugin_container for content processes

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Brian R. Bondy [:bbondy]]

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.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Working WIP - mods still needed though

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.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 - Build config. rev1

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Sandbox broker code (one who starts the sandbox process)

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch 3 - Target process (One who gets sandboxed)

Comment 5

[Benjamin Smedberg]

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

Comment 6

[Brian R. Bondy [:bbondy]]

(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.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Sorry for the delay, I'll start reviewing this tomorrow morning.

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch 4 of 4 - Packaging

Missed the packaging part of this in the build config patch

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

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?

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Sandbox broker code (one who starts the sandbox process). rev2

Implemented review nits, carrying forward r+.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Sandbox broker code (one who starts the sandbox process). rev2'

Comment 12

[Brian R. Bondy [:bbondy]]

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.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 - Build config. rev2

Rebased patch, moved FORCE_STATIC_LIB = True to moz.build file. Carrying forward r+ on patch 1.

Comment 14

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34cdb11de791
https://hg.mozilla.org/integration/mozilla-inbound/rev/7915aa34a9d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f997b62e1290
https://hg.mozilla.org/integration/mozilla-inbound/rev/401ddfc06cab

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34cdb11de791
https://hg.mozilla.org/mozilla-central/rev/7915aa34a9d8
https://hg.mozilla.org/mozilla-central/rev/f997b62e1290
https://hg.mozilla.org/mozilla-central/rev/401ddfc06cab

Comment 16

[Mike Hommey [:glandium]]

Is it expected that https://mxr.mozilla.org/mozilla-central/source/toolkit/library/Makefile.in#43 is in a ifdef ACCESSIBILITY block?

Comment 17

[Brian R. Bondy [:bbondy]]

Not intentional, bug 935980 was filed to fix it. Thanks. This is not built by default on Windows anywhere so non-critical short term.