Create a LinuxSandbox class

Create a LinuxSandbox class.

The LinuxSandbox class aims to become the central place for Linux
sandboxing inside content/.

For now, this refactors mostly code from the Zygote.

BUG=
NOTRY=true


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149692

[jln (very slow on Chromium), 2012-08-01 05:47:18]

This is one of the many steps to centralize our Linux sandboxing.

The LinuxSandbox class is a singleton. It will eventually be the way through
which three of our sandboxes get started. It already embeds a SetuiSandboxClient
object.

This first step mostly refactors code from the Zygote (dealing with the setuid
sandbox and seccomp-legacy) into one class. With it, our Linux client sandbox
code is now mostly in content/common/sandbox_init_linux and
content/common/sandbox_linux.

The next step will extend the usage of the class to start seccomp-legacy and
seccomp-bpf sandboxes.

[Markus (顧孟勤), 2012-08-01 22:09:19]

lgtm

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
content/common/sandbox_linux.cc:22: inline bool IsSeccompLegacyDesired() {
Do we ever explicitly use "inline" in our code. Sounds like a micro-optimization
that should be handled by the compiler.

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
content/common/sandbox_linux.cc:59: delete(setuid_sandbox_);
"delete" is an operator and not a function in C++, unless I am really confused.
Shouldn't that imply that we never put parenthesis around its arguments?

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
content/common/sandbox_linux.h:53: void PreinitializeSandboxFinish(const
std::string& process_type);
In general, I find it really hard to read without extra blank lines. But I know
you are reluctant to add those. Your call.

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
content/common/sandbox_linux.h:55: // PreinitlizeSandbox(). This is a bitmask
anduses the constants defined in
spelling: s/Preinitlize/Preinitialize/ s/anduses/and uses/

[jln (very slow on Chromium), 2012-08-01 22:21:53]

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
content/common/sandbox_linux.cc:22: inline bool IsSeccompLegacyDesired() {
On 2012/08/01 22:09:19, Markus (顧孟勤) wrote:
> Do we ever explicitly use "inline" in our code. Sounds like a
micro-optimization
> that should be handled by the compiler.

Done.

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
content/common/sandbox_linux.cc:59: delete(setuid_sandbox_);
On 2012/08/01 22:09:19, Markus (顧孟勤) wrote:
> "delete" is an operator and not a function in C++, unless I am really
confused.
> Shouldn't that imply that we never put parenthesis around its arguments?

Done.

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/18/content/common/sandbo...
content/common/sandbox_linux.h:55: // PreinitlizeSandbox(). This is a bitmask
anduses the constants defined in
On 2012/08/01 22:09:19, Markus (顧孟勤) wrote:
> spelling: s/Preinitlize/Preinitialize/ s/anduses/and uses/

Done.

[jln (very slow on Chromium), 2012-08-01 22:26:23]

jam@: do you mind approving as owner of content ?

For things that I don't have ownership of, this is mostly a refactor.

However: this creates a new sandbox_linux.cc file and I changed the comment in
the OWNERS file. Please check if this is ok with you.

There are probably a couple more files coming for the Linux sandbox. Let me know
if you'd like me to create a sub-directory instead (either in this CL, or in
another one).

[Jorge Lucangeli Obes, 2012-08-01 22:37:50]

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.cc:58: if (!instance->setuid_sandbox_) {
Do we use braces around one-line if bodies?

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.cc:150: if
(ShouldEnableSeccompLegacy(process_type)) {
Do we ask if we should enable even if the legacy seccomp sandbox was #ifdef'd
out?

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.h:66: // There is no StartSuidSandbox() the
SetuidSandboxClient instance should be
comma or period after "StartSuidSandbox()"? Inconsistent use of Suid and Setuid.

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.h:70: // Check the policy and eventually the
seccomp-legacy sandbox if the policy.
I don't understand this comment.

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.h:72: // Check the policy and eventually the
seccomp-legacy sandbox if the policy.
Ditto.

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/zygote/zyg...
File content/zygote/zygote_main_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/zygote/zyg...
content/zygote/zygote_main_linux.cc:485: if
(setuid_sandbox->IsInNewNETNamespace() && !has_started_new_init) {
PID namespace?

[jln (very slow on Chromium), 2012-08-01 22:48:11]

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.cc:58: if (!instance->setuid_sandbox_) {
On 2012/08/01 22:37:50, Jorge Lucangeli Obes wrote:
> Do we use braces around one-line if bodies?

The style guide has always been vague on this, and it depends on individual
preferences. I have a tendency to uses braces around anything that remotely
looks like a macro. Been burned before :)

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.cc:150: if
(ShouldEnableSeccompLegacy(process_type)) {
On 2012/08/01 22:37:50, Jorge Lucangeli Obes wrote:
> Do we ask if we should enable even if the legacy seccomp sandbox was #ifdef'd
> out?

Yes, I think that's the right way. In general my rule of thumb is to have #ifdef
cover as *little* an area as possible.

In practice, in "generated binaries", this will not change anything this
"IsSeccompLegacyDesired()" will always be false, all of this will get
optimized-out.

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.h:66: // There is no StartSuidSandbox() the
SetuidSandboxClient instance should be
On 2012/08/01 22:37:50, Jorge Lucangeli Obes wrote:
> comma or period after "StartSuidSandbox()"? Inconsistent use of Suid and
Setuid.

Done.

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.h:70: // Check the policy and eventually the
seccomp-legacy sandbox if the policy.
On 2012/08/01 22:37:50, Jorge Lucangeli Obes wrote:
> I don't understand this comment.

Ooch, thanks.

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/common/san...
content/common/sandbox_linux.h:72: // Check the policy and eventually the
seccomp-legacy sandbox if the policy.
On 2012/08/01 22:37:50, Jorge Lucangeli Obes wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/zygote/zyg...
File content/zygote/zygote_main_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/14003/content/zygote/zyg...
content/zygote/zygote_main_linux.cc:485: if
(setuid_sandbox->IsInNewNETNamespace() && !has_started_new_init) {
On 2012/08/01 22:37:50, Jorge Lucangeli Obes wrote:
> PID namespace?

Good catch, thanks!

[Jorge Lucangeli Obes, 2012-08-01 22:53:57]

LGTM but please fix the second comment.

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/OWNERS
File content/common/OWNERS (right):

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/OWNERS...
content/common/OWNERS:1: # For sandbox_*_linux.*
Aren't there sandbox_linux.* files that wouldn't match above?

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/sandbo...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/sandbo...
content/common/sandbox_linux.h:72: // Check the policy and eventually start the
seccomp-legacy sandbox.
seccomp-bpf

[jln (very slow on Chromium), 2012-08-01 22:57:36]

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/OWNERS
File content/common/OWNERS (right):

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/OWNERS...
content/common/OWNERS:1: # For sandbox_*_linux.*
On 2012/08/01 22:53:57, Jorge Lucangeli Obes wrote:
> Aren't there sandbox_linux.* files that wouldn't match above?

Done.

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/sandbo...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/19/content/common/sandbo...
content/common/sandbox_linux.h:72: // Check the policy and eventually start the
seccomp-legacy sandbox.
On 2012/08/01 22:53:57, Jorge Lucangeli Obes wrote:
> seccomp-bpf
Not my day. Thanks.

[jln (very slow on Chromium), 2012-08-02 03:33:41]

Antoine: jam@ seems to be OOO according to his calendar.

Do you mind approving this? Specifically:

""
For things that I don't have ownership of, this is mostly a refactor.

However: this creates a new sandbox_linux.cc file and I changed the comment in
the OWNERS file. Please check if this is ok with you.

There are probably a couple more files coming for the Linux sandbox. Let me know
if you'd like me to create a sub-directory instead (either in this CL, or in
another one).
""

Thanks!

[piman, 2012-08-02 17:31:20]

mostly nits

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:51: delete setuid_sandbox_;
That calls for scoped_ptr

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:57: instance->setuid_sandbox_ =
sandbox::SetuidSandboxClient::Create();
Arguably, that part could go to the constructor. Either way.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:73: // Now is a good time to figure out if we
can support seccomp sandboxing at
The comment seems indented wrong.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:117: // Simplify the life of the caller if they
know their process type.
I don't think that comment helps anything, you can just remove it.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:135: // but only if it can be attempted.
It sounds like the comment would be more useful on the enum than in here (that
nobody will read).

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.h:12: #define SECCOMP_BPF_SANDBOX
High-level question: how much of the SECCOMP_SANDBOX and SECCOMP_BPF_SANDBOX is
needed to compile at all?
i.e. would it be possible to not have these defines at all in interfaces, only
implementations, if at all?
e.g. if they don't compile on non-x86, just return failure from the various
initialization functions. That way it makes the calling code much more readable,
and restricts the per-platform ugliness to a few files.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.h:53: void PreinitializeSandbox();
Maybe you can name those 2 PreinitializeSandboxBegin and PreinitializeSandboxEnd
to make it more obvious.
Among others, style insists on not overloading.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.h:68: sandbox::SetuidSandboxClient*
setuid_sandbox();
setuid_sandbox_client() ?

You can make that method inline, and even const.

[jln (very slow on Chromium), 2012-08-02 18:25:22]

Thanks, PTAL!

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:51: delete setuid_sandbox_;
On 2012/08/02 17:31:20, piman wrote:
> That calls for scoped_ptr

Done.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:73: // Now is a good time to figure out if we
can support seccomp sandboxing at
On 2012/08/02 17:31:20, piman wrote:
> The comment seems indented wrong.

Done.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:117: // Simplify the life of the caller if they
know their process type.
On 2012/08/02 17:31:20, piman wrote:
> I don't think that comment helps anything, you can just remove it.

Done.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.cc:135: // but only if it can be attempted.
On 2012/08/02 17:31:20, piman wrote:
> It sounds like the comment would be more useful on the enum than in here (that
> nobody will read).

Done.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.h:12: #define SECCOMP_BPF_SANDBOX
On 2012/08/02 17:31:20, piman wrote:
> High-level question: how much of the SECCOMP_SANDBOX and SECCOMP_BPF_SANDBOX
is
> needed to compile at all?
> i.e. would it be possible to not have these defines at all in interfaces, only
> implementations, if at all?
> e.g. if they don't compile on non-x86, just return failure from the various
> initialization functions. That way it makes the calling code much more
readable,
> and restricts the per-platform ugliness to a few files.

Yes, I think this gets address in the next CL
(https://chromiumcodereview.appspot.com/10843042/). This is all very hairy and
we are plagued by #ifdef. Once the next CL lands, thing will be much better.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.h:53: void PreinitializeSandbox();
On 2012/08/02 17:31:20, piman wrote:
> Maybe you can name those 2 PreinitializeSandboxBegin and
PreinitializeSandboxEnd
> to make it more obvious.
> Among others, style insists on not overloading.

AFAIK, style states pverloading is fine if we have a different number of
argument. However your suggestion makes things more clear. Done.

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.h:68: sandbox::SetuidSandboxClient*
setuid_sandbox();
On 2012/08/02 17:31:20, piman wrote:
> setuid_sandbox_client() ?
> 
> You can make that method inline, and even const.

Done.

[piman, 2012-08-02 18:29:55]

lgtm

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10826093/diff/12009/content/common/san...
content/common/sandbox_linux.h:53: void PreinitializeSandbox();
On 2012/08/02 18:25:22, Julien Tinnes wrote:
> On 2012/08/02 17:31:20, piman wrote:
> > Maybe you can name those 2 PreinitializeSandboxBegin and
> PreinitializeSandboxEnd
> > to make it more obvious.
> > Among others, style insists on not overloading.
> 
> AFAIK, style states pverloading is fine if we have a different number of
> argument. However your suggestion makes things more clear. Done.

Only for constructors (where you don't have a choice of a name).
In any case, thanks for the fix.

[commit-bot: I haz the power, 2012-08-02 18:52:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/10826093/15004

[commit-bot: I haz the power, 2012-08-02 20:37:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/10826093/4060

[commit-bot: I haz the power, 2012-08-02 20:38:06]

Change committed as 149692