Linux sandbox: restrict clone() in renderer processes.

Linux sandbox: restrict clone() in renderer processes.

In renderers, we restrict the use of clone() to the creation of threads.

BUG=129097
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180939

[jln (very slow on Chromium), 2013-02-06 01:40:43]

Should be fairly safe, but slightly worried about 32 bits and fringe plugins.

[Chris Evans, 2013-02-06 03:27:59]

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:116: _exit(1);
Is exit_group() better?

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1259: CLONE_PARENT_SETTID |
CLONE_CHILD_CLEARTID,
So you permit one exact bitmask of flags.
Isn't the termination signal part of that argument? SIGCHLD?

I'm confused as to why it works.

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1267: return AllowCloneForThreads();
Can we do it for Flash too? Flash is most likely to be compromised of all
process types. Let me know if you want to put some time in testing.

[jln (very slow on Chromium), 2013-02-06 03:32:31]

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:116: _exit(1);
On 2013/02/06 03:27:59, Chris Evans wrote:
> Is exit_group() better?

I can't recall I did exit and not exit group. In the compiler's unit test
exit_group is usually used.

It' similar to the one done in the crash signal handler and I would rather not
touch this as part of this CL.

There is an upcoming refactor of all the reporting code anyways.

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1259: CLONE_PARENT_SETTID |
CLONE_CHILD_CLEARTID,
On 2013/02/06 03:27:59, Chris Evans wrote:
> So you permit one exact bitmask of flags.
> Isn't the termination signal part of that argument? SIGCHLD?
> 
> I'm confused as to why it works.

This is the mask used by pthread. For various reasons, you can't really use
anything else than pthread if something in your process does use it.

However this is quite highly architecture dependent, I'll limit this to 64 bits
x86 in the first iteration.

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1267: return AllowCloneForThreads();
On 2013/02/06 03:27:59, Chris Evans wrote:
> Can we do it for Flash too? Flash is most likely to be compromised of all
> process types. Let me know if you want to put some time in testing.

Let's do them separately ? I'm happy to either first.

[Chris Evans, 2013-02-06 03:36:11]

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:110: reinterpret_cast<volatile
char*>(clone_flags & 0xFFFFFF);
BTW, on 32-bit systems that's a pretty big mask. Might be a mapped area?

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1259: CLONE_PARENT_SETTID |
CLONE_CHILD_CLEARTID,
Sounds good

On 2013/02/06 03:32:31, Julien Tinnes wrote:
> On 2013/02/06 03:27:59, Chris Evans wrote:
> > So you permit one exact bitmask of flags.
> > Isn't the termination signal part of that argument? SIGCHLD?
> > 
> > I'm confused as to why it works.
> 
> This is the mask used by pthread. For various reasons, you can't really use
> anything else than pthread if something in your process does use it.
> 
> However this is quite highly architecture dependent, I'll limit this to 64
bits
> x86 in the first iteration.

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1267: return AllowCloneForThreads();
On 2013/02/06 03:32:31, Julien Tinnes wrote:
> On 2013/02/06 03:27:59, Chris Evans wrote:
> > Can we do it for Flash too? Flash is most likely to be compromised of all
> > process types. Let me know if you want to put some time in testing.
> 
> Let's do them separately ? I'm happy to either first.

Ok separately is good

[Chris Evans, 2013-02-06 03:37:25]

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/12209029/diff/2001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:116: _exit(1);
Ok maybe add a comment about considering exit_group() in the future?

On 2013/02/06 03:32:31, Julien Tinnes wrote:
> On 2013/02/06 03:27:59, Chris Evans wrote:
> > Is exit_group() better?
> 
> I can't recall I did exit and not exit group. In the compiler's unit test
> exit_group is usually used.
> 
> It' similar to the one done in the crash signal handler and I would rather not
> touch this as part of this CL.
> 
> There is an upcoming refactor of all the reporting code anyways.

[Chris Evans, 2013-02-06 03:44:54]

LGTM with nits

https://codereview.chromium.org/12209029/diff/8001/content/common/sandbox_sec...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/12209029/diff/8001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:112: // Hit the NULL page if this
fails.
Nit: "fails to fault"?

https://codereview.chromium.org/12209029/diff/8001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:1268: #if defined(__x86_64__)
Nit: add TODO(jln) for other platforms?

[jln (very slow on Chromium), 2013-02-06 04:00:51]

https://codereview.chromium.org/12209029/diff/8001/content/common/sandbox_sec...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/12209029/diff/8001/content/common/sandbox_sec...
content/common/sandbox_seccomp_bpf_linux.cc:112: // Hit the NULL page if this
fails.
On 2013/02/06 03:44:54, Chris Evans wrote:
> Nit: "fails to fault"?

Done.

[commit-bot: I haz the power, 2013-02-06 08:45:37]

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

[commit-bot: I haz the power, 2013-02-06 08:56:05]

Change committed as 180939