Implement GetSandboxType() on all platforms and implement for all process types.

Implement GetSandboxType() on all platforms and implement for all process types. This fixes a small issue where bootstrap sandbox would never be enabled on OS X. (Note: bootstrap sandbox is disabled anyway until crbug.com/501128 can be fixed.)

Move App Container SID generation to embedder so it can be different for each type of child process. Move App Container policy code to sandbox_win.cc

BUG=499523, 382931
Committed: https://crrev.com/182da09cea04f762ebd006083878a1a6b143b4dd
Cr-Commit-Position: refs/heads/master@{#335973}

[Will Harris, 2015-06-16 18:18:46]

wfh@chromium.org changed reviewers:
+ jschuh@chromium.org, nasko@chromium.org, sky@chromium.org

[Will Harris, 2015-06-16 18:19:06]

wfh@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Will Harris, 2015-06-16 18:20:33]

PTAL

First part of two CLs to enable App Container on PPAPI process. The next CL is
1177923002

jschuh -> sandbox stuff and general design comments
sky -> chrome/
nasko -> content/
mseaborn -> nacl

[Will Harris, 2015-06-16 23:31:53]

https://codereview.chromium.org/1189823003/ should fix the trybot issues.

[Will Harris, 2015-06-17 16:30:14]

the OS X bots seem happy now, so PTAL. Thanks!

[nasko, 2015-06-19 12:18:20]

Mostly nits, though I have one question about browser tests in content/.

https://codereview.chromium.org/1185333003/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1185333003/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:579: void
MaybeAddAppContainerPolicy(sandbox::TargetPolicy* policy,
nit: I'm not a fan of the "Maybe" prefix. While it is true that AppContainers
are not applicable on OS versions, I think it is fine to call it
AddAppContainerPolicy, which in cases it will apply an "empty" policy.

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:647: virtual base::string16
GetAppContainerSidForSandboxType(
I don't see an implementation of this interface for content_shell. Is this
intentional? It might lead to browser tests running the renderers without an
AppContainer, which is not going to be matching real browser build.

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.h:66: #endif
nit: empty line after the #endif, before the comment.

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.h:67: // Gets the
SandboxType to enforce on the process. Return SANDBOX_TYPE_INVALID
nit: Returns

[jschuh, 2015-06-19 14:05:08]

https://codereview.chromium.org/1185333003/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1185333003/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:579: void
MaybeAddAppContainerPolicy(sandbox::TargetPolicy* policy,
On 2015/06/19 12:18:19, nasko (paris) wrote:
> nit: I'm not a fan of the "Maybe" prefix. While it is true that AppContainers
> are not applicable on OS versions, I think it is fine to call it
> AddAppContainerPolicy, which in cases it will apply an "empty" policy.

Agreed on the "maybe" and I had typed a comment to the same effect before I saw
this one. It's normal for the sandbox code to let you add a policy without it
applying due to the OS version (e.g. SetIntegrityLevel and
SetProcessMitigations)

[Will Harris, 2015-06-23 16:21:12]

question on the preferred way of implementing GetAppContainerSidForSandboxType
on non Chrome embedders.

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:647: virtual base::string16
GetAppContainerSidForSandboxType(
On 2015/06/19 12:18:19, nasko (paris) wrote:
> I don't see an implementation of this interface for content_shell. Is this
> intentional? It might lead to browser tests running the renderers without an
> AppContainer, which is not going to be matching real browser build.

There's quite a few subclasses of ContentBrowserClient:

ShellContentBrowserClient
TestContentBrowserClient
ViewsContentBrowserClient

Perhaps implement a single SID version inside ContentBrowserClient and then make
ChromeContentBrowserClient override and be aware of GOOGLE_CHROME_BUILD. Not
entirely sure what is best here to ensure full test coverage.

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.h:66: #endif
On 2015/06/19 12:18:19, nasko (paris) wrote:
> nit: empty line after the #endif, before the comment.

Done, but looks weird now, I couldn't find the standard on what I was supposed
to do here, almost looks like there is too much whitespace now.

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.h:67: // Gets the
SandboxType to enforce on the process. Return SANDBOX_TYPE_INVALID
On 2015/06/19 12:18:20, nasko (paris) wrote:
> nit: Returns

Done.

[nasko, 2015-06-24 08:04:08]

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:647: virtual base::string16
GetAppContainerSidForSandboxType(
On 2015/06/23 16:21:11, Will Harris wrote:
> On 2015/06/19 12:18:19, nasko (paris) wrote:
> > I don't see an implementation of this interface for content_shell. Is this
> > intentional? It might lead to browser tests running the renderers without an
> > AppContainer, which is not going to be matching real browser build.
> 
> There's quite a few subclasses of ContentBrowserClient:
> 
> ShellContentBrowserClient
> TestContentBrowserClient
> ViewsContentBrowserClient
> 
> Perhaps implement a single SID version inside ContentBrowserClient and then
make
> ChromeContentBrowserClient override and be aware of GOOGLE_CHROME_BUILD. Not
> entirely sure what is best here to ensure full test coverage.

Indeed, that sounds like the right approach to me. This is why we have basic
implementations in the ContentBrowserClient, such that we have some sane default
and each other client can provide a different implementation.

I'd suggest returning some value from ContentBrowserClient that will ensure we
get AppContainer support, but not worrying too much that it matches the actual
production setup.

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.h:67: // Gets the
SandboxType to enforce on the process. Return SANDBOX_TYPE_INVALID
On 2015/06/23 16:21:11, Will Harris wrote:
> On 2015/06/19 12:18:20, nasko (paris) wrote:
> > nit: Returns
> 
> Done.

I don't see the changes in this file from the previous patchset to the current
one. Did you accidentally forget to include them in the upload?

[Will Harris, 2015-06-24 11:37:09]

PTAL. Thanks.

https://codereview.chromium.org/1185333003/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1185333003/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:579: void
MaybeAddAppContainerPolicy(sandbox::TargetPolicy* policy,
On 2015/06/19 14:05:08, jschuh (very slow) wrote:
> On 2015/06/19 12:18:19, nasko (paris) wrote:
> > nit: I'm not a fan of the "Maybe" prefix. While it is true that
AppContainers
> > are not applicable on OS versions, I think it is fine to call it
> > AddAppContainerPolicy, which in cases it will apply an "empty" policy.
> 
> Agreed on the "maybe" and I had typed a comment to the same effect before I
saw
> this one. It's normal for the sandbox code to let you add a policy without it
> applying due to the OS version (e.g. SetIntegrityLevel and
> SetProcessMitigations)

Done.

https://codereview.chromium.org/1185333003/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:579: void
MaybeAddAppContainerPolicy(sandbox::TargetPolicy* policy,
On 2015/06/19 12:18:19, nasko (paris) wrote:
> nit: I'm not a fan of the "Maybe" prefix. While it is true that AppContainers
> are not applicable on OS versions, I think it is fine to call it
> AddAppContainerPolicy, which in cases it will apply an "empty" policy.

Done.

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:647: virtual base::string16
GetAppContainerSidForSandboxType(
On 2015/06/24 08:04:08, nasko (paris) wrote:
> On 2015/06/23 16:21:11, Will Harris wrote:
> > On 2015/06/19 12:18:19, nasko (paris) wrote:
> > > I don't see an implementation of this interface for content_shell. Is this
> > > intentional? It might lead to browser tests running the renderers without
an
> > > AppContainer, which is not going to be matching real browser build.
> > 
> > There's quite a few subclasses of ContentBrowserClient:
> > 
> > ShellContentBrowserClient
> > TestContentBrowserClient
> > ViewsContentBrowserClient
> > 
> > Perhaps implement a single SID version inside ContentBrowserClient and then
> make
> > ChromeContentBrowserClient override and be aware of GOOGLE_CHROME_BUILD. Not
> > entirely sure what is best here to ensure full test coverage.
> 
> Indeed, that sounds like the right approach to me. This is why we have basic
> implementations in the ContentBrowserClient, such that we have some sane
default
> and each other client can provide a different implementation.
> 
> I'd suggest returning some value from ContentBrowserClient that will ensure we
> get AppContainer support, but not worrying too much that it matches the actual
> production setup.

Done.

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1185333003/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.h:67: // Gets the
SandboxType to enforce on the process. Return SANDBOX_TYPE_INVALID
Sorry, I was going to upload them with the same PS as the restructured
GetAppContainerSid code. Now uploaded!

[nasko, 2015-06-24 13:11:38]

content/ LGTM with a few nits.

https://codereview.chromium.org/1185333003/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1185333003/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2374: return sid + L"129201925";
nit: Since the PPAPI process isn't using the SID quite yet, let's have this
return empty string and put the real one in a CL with the PPAPI work.

https://codereview.chromium.org/1185333003/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2385: return base::string16();
NOTREACHED before the return? Could also be CHECK, as we don't expect to want to
allocate a SID but fail to do so and get to the end of the method, right?

https://codereview.chromium.org/1185333003/diff/80001/content/common/sandbox_...
File content/common/sandbox_win.h (right):

https://codereview.chromium.org/1185333003/diff/80001/content/common/sandbox_...
content/common/sandbox_win.h:33: // Add App container policy for |sid| on
supported OS.
nit: standardize on "App container" capitalization and be consistent.

https://codereview.chromium.org/1185333003/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/1185333003/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.cc:343: // sandbox type.
Let's put a Note: here to point out that content level tests will run all
process types in the same app container.

https://codereview.chromium.org/1185333003/diff/80001/content/public/common/s...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1185333003/diff/80001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:67: // Returns the
SandboxType to enforce on the process. Returns
nit: s/process. Returns/process, otherwise /

[Will Harris, 2015-06-24 15:17:20]

Thanks. All done. PTAL other OWNERS:

mseaborn -> components/nacl
jschuh -> content/common/sandbox_win.cc
sky -> chrome/common/chrome_content_client.cc

https://codereview.chromium.org/1185333003/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1185333003/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2374: return sid + L"129201925";
On 2015/06/24 13:11:38, nasko (paris) wrote:
> nit: Since the PPAPI process isn't using the SID quite yet, let's have this
> return empty string and put the real one in a CL with the PPAPI work.

Done.

https://codereview.chromium.org/1185333003/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2385: return base::string16();
On 2015/06/24 13:11:38, nasko (paris) wrote:
> NOTREACHED before the return? Could also be CHECK, as we don't expect to want
to
> allocate a SID but fail to do so and get to the end of the method, right?

Done.

https://codereview.chromium.org/1185333003/diff/80001/content/common/sandbox_...
File content/common/sandbox_win.h (right):

https://codereview.chromium.org/1185333003/diff/80001/content/common/sandbox_...
content/common/sandbox_win.h:33: // Add App container policy for |sid| on
supported OS.
On 2015/06/24 13:11:38, nasko (paris) wrote:
> nit: standardize on "App container" capitalization and be consistent.

Done.

https://codereview.chromium.org/1185333003/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/1185333003/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.cc:343: // sandbox type.
On 2015/06/24 13:11:38, nasko (paris) wrote:
> Let's put a Note: here to point out that content level tests will run all
> process types in the same app container.

Done.

https://codereview.chromium.org/1185333003/diff/80001/content/public/common/s...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1185333003/diff/80001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:67: // Returns the
SandboxType to enforce on the process. Returns
On 2015/06/24 13:11:38, nasko (paris) wrote:
> nit: s/process. Returns/process, otherwise /

Done.

[Mark Seaborn, 2015-06-24 15:48:19]

On 2015/06/24 15:17:20, Will Harris wrote:
> Thanks. All done. PTAL other OWNERS:
> 
> mseaborn -> components/nacl
> jschuh -> content/common/sandbox_win.cc
> sky -> chrome/common/chrome_content_client.cc

LGTM for components/nacl/.

[sky, 2015-06-24 15:57:33]

chrome/common/chrome_content_client.cc LGTM

[Will Harris, 2015-06-24 18:41:50]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2015-06-24 18:41:50]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
mseaborn@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1185333003/#ps160001
(title: "keep rebasing keep rebasing...")

[commit-bot: I haz the power, 2015-06-24 18:44:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185333003/160001

[commit-bot: I haz the power, 2015-06-24 19:23:13]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-24 19:24:04]

Patchset 9 (id:??) landed as
https://crrev.com/182da09cea04f762ebd006083878a1a6b143b4dd
Cr-Commit-Position: refs/heads/master@{#335973}