Create a class for seccomp-bpf sandboxing in content.

Create a class for seccomp-bpf sandboxing in content.

This adds a SandboxSeccompBpf class to centralize Seccomp BPF sandbox
policies inside of content/

LinuxSandbox is the only user of this class and Linux sandboxing is now fully
unified through LinuxSandbox.

BUG=
NOTRY=true

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

[jln (very slow on Chromium), 2012-08-02 05:20:59]

This is the next step towards a unified LinuxSandbox class.

We create a new class to handle seccomp-bpf policies inside content/
The LinuxSandbox class makes use of this.

InitializeSandbox() only goes through the LinuxSandbox class now.

[Markus (顧孟勤), 2012-08-02 20:39:13]

lgtm

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
content/common/sandbox_linux.cc:173: base::Time::UnixEpoch());
A stat() call makes much more sense than a readdir(). But no idea if you want to
change that right now.

The BPF sandbox has a decent implementation of the code that you would need. So,
we could refactor that, if you want to.

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
content/common/sandbox_linux.h:79: // We must have be pre_initialized_ before
using either of these.
s/be/been/

[jln (very slow on Chromium), 2012-08-02 20:50:40]

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
content/common/sandbox_linux.cc:173: base::Time::UnixEpoch());
On 2012/08/02 20:39:13, Markus (顧孟勤) wrote:
> A stat() call makes much more sense than a readdir(). But no idea if you want
to
> change that right now.
> 
> The BPF sandbox has a decent implementation of the code that you would need.
So,
> we could refactor that, if you want to.

I'll touch this later and use our proc_fd_. Since this could start detecting
threads where we didn't before, I prefer to touch this in another CL.

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/10843042/diff/5009/content/common/sand...
content/common/sandbox_linux.h:79: // We must have be pre_initialized_ before
using either of these.
On 2012/08/02 20:39:13, Markus (顧孟勤) wrote:
> s/be/been/

Done.

[jln (very slow on Chromium), 2012-08-02 20:55:27]

Antoine, would you mind approving this as content/ owner?

[Jorge Lucangeli Obes, 2012-08-02 21:04:12]

LGTM but if you can fix the typo, better.

https://chromiumcodereview.appspot.com/10843042/diff/8002/content/common/sand...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/10843042/diff/8002/content/common/sand...
content/common/sandbox_seccomp_bpf_linux.cc:515: // TODO(jln): pass the savec
proc_fd_ from the LinuxSandbox singleton
saved

[jln (very slow on Chromium), 2012-08-02 21:06:57]

Thanks.

https://chromiumcodereview.appspot.com/10843042/diff/8002/content/common/sand...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/10843042/diff/8002/content/common/sand...
content/common/sandbox_seccomp_bpf_linux.cc:515: // TODO(jln): pass the savec
proc_fd_ from the LinuxSandbox singleton
On 2012/08/02 21:04:12, Jorge Lucangeli Obes wrote:
> saved

Done.

[piman, 2012-08-02 21:40:04]

LGTM+nits

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_init_linux.cc:22: switches::kProcessType);
nit: indent (switches should be at +4)

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

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_linux.cc:161: // but only if it can be attempted.
same as previous CL :)
This comment isn't really helpful here.

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_linux.cc:172:
file_util::CountFilesCreatedAfter(FilePath("/proc/self/task"),
nit: indent (should be +4)

Or you can probably break this as:

int num_threads = file_util::CountFilesCreatedAfter(
    FilePath("/proc/self/task"),
    base::Time::UnixEpoch());

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_linux.cc:209: if (seccomp_bpf_supported()) {
nit: no need for brackets

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

Thanks!

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
File content/common/sandbox_init_linux.cc (right):

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_init_linux.cc:22: switches::kProcessType);
On 2012/08/02 21:40:04, piman wrote:
> nit: indent (switches should be at +4)

Done.

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

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_linux.cc:161: // but only if it can be attempted.
On 2012/08/02 21:40:04, piman wrote:
> same as previous CL :)
> This comment isn't really helpful here.

I tried to make things more clear but if you don't mind I'll keep something.

It can get very confusing because we report this information through the Zygote
IPC at an early time. A goal of the current batch of CLs is to unify the
decision on whether or not to start the sandboxes in one place.

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_linux.cc:172:
file_util::CountFilesCreatedAfter(FilePath("/proc/self/task"),
On 2012/08/02 21:40:04, piman wrote:
> nit: indent (should be +4)
> 
> Or you can probably break this as:
> 
> int num_threads = file_util::CountFilesCreatedAfter(
>     FilePath("/proc/self/task"),
>     base::Time::UnixEpoch());

Done.

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_linux.cc:209: if (seccomp_bpf_supported()) {
On 2012/08/02 21:40:04, piman wrote:
> nit: no need for brackets

Done.

[commit-bot: I haz the power, 2012-08-02 22:23:47]

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

[commit-bot: I haz the power, 2012-08-02 22:23:50]

Failed to apply patch for content/common/sandbox_seccomp_bpf_linux.cc:
While running patch -p1 --forward --force;
patching file content/common/sandbox_seccomp_bpf_linux.cc
Hunk #2 FAILED at 19.
Hunk #3 succeeded at 50 (offset -1 lines).
Hunk #4 succeeded at 420 (offset -1 lines).
Hunk #5 succeeded at 472 (offset -1 lines).
Hunk #6 FAILED at 487.
2 out of 6 hunks FAILED -- saving rejects to file
content/common/sandbox_seccomp_bpf_linux.cc.rej

[piman, 2012-08-02 22:24:40]

lgtm

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

https://chromiumcodereview.appspot.com/10843042/diff/14002/content/common/san...
content/common/sandbox_linux.cc:161: // but only if it can be attempted.
On 2012/08/02 22:20:38, Julien Tinnes wrote:
> On 2012/08/02 21:40:04, piman wrote:
> > same as previous CL :)
> > This comment isn't really helpful here.
> 
> I tried to make things more clear but if you don't mind I'll keep something.
> 
> It can get very confusing because we report this information through the
Zygote
> IPC at an early time. A goal of the current batch of CLs is to unify the
> decision on whether or not to start the sandboxes in one place.

Ok, as you wish. My point is just that users of GetStatus won't necessarily read
the implementation to understand the gotcha.

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

On 2012/08/02 22:24:40, piman wrote:

> Ok, as you wish. My point is just that users of GetStatus won't necessarily
read
> the implementation to understand the gotcha.

Yes, that was a good point, there is now a rather large comment in
sandbox_linux.h about that (from the previous CL you reviewed).
But since it's a bit tricky I like to keep a comment in the implementation as
well.

[commit-bot: I haz the power, 2012-08-02 22:42:04]

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

[commit-bot: I haz the power, 2012-08-02 22:42:06]

Failed to apply patch for content/common/sandbox_seccomp_bpf_linux.cc:
While running patch -p1 --forward --force;
patching file content/common/sandbox_seccomp_bpf_linux.cc
Hunk #2 FAILED at 19.
Hunk #3 succeeded at 50 (offset -1 lines).
Hunk #4 succeeded at 420 (offset -1 lines).
Hunk #5 succeeded at 472 (offset -1 lines).
Hunk #6 FAILED at 487.
2 out of 6 hunks FAILED -- saving rejects to file
content/common/sandbox_seccomp_bpf_linux.cc.rej

[commit-bot: I haz the power, 2012-08-02 23:19:40]

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

[commit-bot: I haz the power, 2012-08-02 23:19:47]

Failed to apply patch for content/common/sandbox_linux.cc:
While running patch -p1 --forward --force;
patching file content/common/sandbox_linux.cc
Hunk #1 FAILED at 8.
Hunk #2 succeeded at 38 (offset 3 lines).
Hunk #3 succeeded at 57 (offset 3 lines).
Hunk #4 succeeded at 76 (offset 3 lines).
Hunk #5 succeeded at 97 (offset 3 lines).
Hunk #6 succeeded at 133 (offset 3 lines).
Hunk #7 succeeded at 143 (offset 3 lines).
Hunk #8 succeeded at 174 (offset 3 lines).
Hunk #9 succeeded at 189 (offset 3 lines).
1 out of 9 hunks FAILED -- saving rejects to file
content/common/sandbox_linux.cc.rej

[commit-bot: I haz the power, 2012-08-02 23:26:31]

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

[commit-bot: I haz the power, 2012-08-02 23:27:27]

Change committed as 149738