Linux: add a seccomp-bpf sandbox for renderers

Linux: add a seccomp-bpf sandbox for renderers

Renderers are now sandboxed under seccomp-bpf.
We also make seccomp-bpf the default sandbox, even when seccomp-legacy
is enabled (which is the case in Debug builds).

BUG=145327
NOTRY=true

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

[jln (very slow on Chromium), 2012-08-28 23:39:41]

Finally!

Now that we have proper sandboxed content/ testing, I feel confident enough to
push the seccomp-bpf renderer sandbox.

This should more or less match what seccomp-legacy was doing, except we'll need
more tightening as soon as we have parameter inspection and helper processes.
(As soon as this land, I'll create a new bug to tighten the existing policies).

Nonetheless, this is a significant improvement and I'm making it the default
since I think it's tight enough to prevent crazy regressions.

Also, since many of our bots and developers still run Lucid, seccomp-legacy will
still get much test coverage.

[Markus (顧孟勤), 2012-08-29 01:58:19]

In the past, when we felt a little scared about potentially invasive chances, we
would first enable them for debug mode only; and if there were no complaints, we
would also enable them for release builds a while later.

This is particularly helpful when dealing with ChromeOS, as provides at least a
small chance of advance warning before we accidentally break assumptions that
they might have made -- and which don't always overlap 100% with the assumptions
made in stand-alone versions of Chrome.

[jln (very slow on Chromium), 2012-08-29 04:30:16]

On 2012/08/29 01:58:19, Markus (顧孟勤) wrote:
> In the past, when we felt a little scared about potentially invasive chances,
we
> would first enable them for debug mode only; and if there were no complaints,
we
> would also enable them for release builds a while later.
> 
> This is particularly helpful when dealing with ChromeOS, as provides at least
a
> small chance of advance warning before we accidentally break assumptions that
> they might have made -- and which don't always overlap 100% with the
assumptions
> made in stand-alone versions of Chrome.

It's a tough balance. We're at the very start of the M23 branch, so we have a
long time before this ever reaches beta. This is relatively less scary, and now
with much better automated test coverage, than our GPU sandbox for instance.

I think I'd rather try to land it as-is and see what happens. I don't feel super
strongly about it though, as long as I can enable it in release mode a week or
two later.

[Chris Evans, 2012-08-29 23:42:07]

On 2012/08/29 04:30:16, Julien Tinnes wrote:
> On 2012/08/29 01:58:19, Markus (顧孟勤) wrote:
> > In the past, when we felt a little scared about potentially invasive
chances,
> we
> > would first enable them for debug mode only; and if there were no
complaints,
> we
> > would also enable them for release builds a while later.
> > 
> > This is particularly helpful when dealing with ChromeOS, as provides at
least
> a
> > small chance of advance warning before we accidentally break assumptions
that
> > they might have made -- and which don't always overlap 100% with the
> assumptions
> > made in stand-alone versions of Chrome.
> 
> It's a tough balance. We're at the very start of the M23 branch, so we have a
> long time before this ever reaches beta. This is relatively less scary, and
now
> with much better automated test coverage, than our GPU sandbox for instance.
> 
> I think I'd rather try to land it as-is and see what happens. I don't feel
super
> strongly about it though, as long as I can enable it in release mode a week or
> two later.

I agree, it's a good time to land to M23, i.e. as early as possible! The moment
we get it in a dev release, we'll get good data on whether our new
content_unittest coverage is any good or not. By looking at crash reports.

Julien, I found my syscall list for an earlier, non-landed CL to do this. Did I
list any syscall not covered by your patch? If so, may warrant investigation.

https://chromiumcodereview.appspot.com/10165018/diff/1/content/common/sandbox...

[Jorge Lucangeli Obes, 2012-08-29 23:49:09]

On 2012/08/29 23:42:07, Chris Evans wrote:
> On 2012/08/29 04:30:16, Julien Tinnes wrote:
> > On 2012/08/29 01:58:19, Markus (顧孟勤) wrote:
> > > In the past, when we felt a little scared about potentially invasive
> chances,
> > we
> > > would first enable them for debug mode only; and if there were no
> complaints,
> > we
> > > would also enable them for release builds a while later.
> > > 
> > > This is particularly helpful when dealing with ChromeOS, as provides at
> least
> > a
> > > small chance of advance warning before we accidentally break assumptions
> that
> > > they might have made -- and which don't always overlap 100% with the
> > assumptions
> > > made in stand-alone versions of Chrome.
> > 
> > It's a tough balance. We're at the very start of the M23 branch, so we have
a
> > long time before this ever reaches beta. This is relatively less scary, and
> now
> > with much better automated test coverage, than our GPU sandbox for instance.
> > 
> > I think I'd rather try to land it as-is and see what happens. I don't feel
> super
> > strongly about it though, as long as I can enable it in release mode a week
or
> > two later.
> 
> I agree, it's a good time to land to M23, i.e. as early as possible! The
moment
> we get it in a dev release, we'll get good data on whether our new
> content_unittest coverage is any good or not. By looking at crash reports.
> 
> Julien, I found my syscall list for an earlier, non-landed CL to do this. Did
I
> list any syscall not covered by your patch? If so, may warrant investigation.
> 

This doesn't seem to blow up on Chrome OS, but I only opened Gmail and a couple
of newspapers.

[jln (very slow on Chromium), 2012-08-29 23:50:58]

On 2012/08/29 23:42:07, Chris Evans wrote:
> On 2012/08/29 04:30:16, Julien Tinnes wrote:
> > On 2012/08/29 01:58:19, Markus (顧孟勤) wrote:
> > > In the past, when we felt a little scared about potentially invasive
> chances,
> > we
> > > would first enable them for debug mode only; and if there were no
> complaints,
> > we
> > > would also enable them for release builds a while later.
> > > 
> > > This is particularly helpful when dealing with ChromeOS, as provides at
> least
> > a
> > > small chance of advance warning before we accidentally break assumptions
> that
> > > they might have made -- and which don't always overlap 100% with the
> > assumptions
> > > made in stand-alone versions of Chrome.
> > 
> > It's a tough balance. We're at the very start of the M23 branch, so we have
a
> > long time before this ever reaches beta. This is relatively less scary, and
> now
> > with much better automated test coverage, than our GPU sandbox for instance.
> > 
> > I think I'd rather try to land it as-is and see what happens. I don't feel
> super
> > strongly about it though, as long as I can enable it in release mode a week
or
> > two later.
> 
> I agree, it's a good time to land to M23, i.e. as early as possible! The
moment
> we get it in a dev release, we'll get good data on whether our new
> content_unittest coverage is any good or not. By looking at crash reports.
> 
> Julien, I found my syscall list for an earlier, non-landed CL to do this. Did
I
> list any syscall not covered by your patch? If so, may warrant investigation.
> 
>
https://chromiumcodereview.appspot.com/10165018/diff/1/content/common/sandbox...

I took a quick look and it looks like we've got everything covered. The one
difference I noted is that I ENOTTY ioctl while you did allow it, but as you
noted in a comment, allowing it inside a renderer was strange anyway.

That's good news as this means that we've got good test coverage (this policy
was generated semi-automatically through tests only!).

Julien

[jln (very slow on Chromium), 2012-08-29 23:51:58]

On 2012/08/29 23:49:09, Jorge Lucangeli Obes wrote:

> This doesn't seem to blow up on Chrome OS, but I only opened Gmail and a
couple
> of newspapers.

Thanks a lot for testing Jorge!

[Jorge Lucangeli Obes, 2012-08-29 23:57:50]

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

https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sand...
content/common/sandbox_seccomp_bpf_linux.cc:1243: return ENOTTY;
We were doing this in the Flash policy because we knew that the ioctl being
called was for a TTY. The man page for ioctl sort-of suggests that ENOTTY would
be OK for any ioctl call, but it might be kinda weird to get ENOTTY for some
ioctl calls, wouldn't it?

[jln (very slow on Chromium), 2012-08-30 00:05:32]

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

https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sand...
content/common/sandbox_seccomp_bpf_linux.cc:1243: return ENOTTY;
On 2012/08/29 23:57:51, Jorge Lucangeli Obes wrote:
> We were doing this in the Flash policy because we knew that the ioctl being
> called was for a TTY. The man page for ioctl sort-of suggests that ENOTTY
would
> be OK for any ioctl call, but it might be kinda weird to get ENOTTY for some
> ioctl calls, wouldn't it?

I was considering EINVAL, but I think ENOTTY is the proper one:

- It seems like a strange name, but it exists for historical reasons.
- ENOTTY is what the kernel returns for "unrecognized ioctl".

There is even a comment in ioctl.c saying the EINVAL is wrong. I checked :)

[Chris Evans, 2012-08-30 00:08:26]

LGTM with just one nitty question.

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

https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sand...
content/common/sandbox_seccomp_bpf_linux.cc:1244: case __NR_fdatasync:
It sort of feels like sync_file_range fits into this little logical group?

I don't know if it adds much surface (e.g. OOB ranges, negative or huge ints,
etc)

[Jorge Lucangeli Obes, 2012-08-30 00:09:24]

On 2012/08/30 00:08:26, Chris Evans wrote:
> LGTM with just one nitty question.
> 
>
https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sand...
> File content/common/sandbox_seccomp_bpf_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sand...
> content/common/sandbox_seccomp_bpf_linux.cc:1244: case __NR_fdatasync:
> It sort of feels like sync_file_range fits into this little logical group?
> 
> I don't know if it adds much surface (e.g. OOB ranges, negative or huge ints,
> etc)

lgtm since ENOTTY seems to be reasonable.

[jln (very slow on Chromium), 2012-08-30 00:29:15]

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

https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sand...
content/common/sandbox_seccomp_bpf_linux.cc:1244: case __NR_fdatasync:
On 2012/08/30 00:08:26, Chris Evans wrote:
> It sort of feels like sync_file_range fits into this little logical group?
> 
> I don't know if it adds much surface (e.g. OOB ranges, negative or huge ints,
> etc)

Yes, it really depends on our strategy. In the same way, I didn't allow the
whole IsAdvancedScheduler (and allowed specific system calls instead) and didn't
put pread64/pwrite64 in the main I/O PSS to be conservative and err on the side
of attack surface reduction, which is why this policy doesn't look as "nice" as
the other ones.

This sandbox could be really robust (in terms of not crashing / stopping
working) if we didn't have to take attack surface reduction into account.

In other words, we have multiple dimensions:

1. What system calls allow the process to do (system call semantics)
2. Attack surface reduction

It's hard to juggle with the two. We could achieve (2) without breaking the
kernel API by relying heavily on brokers, but it comes with a performance
impact.

Sometimes I think about a way to express PSS where I could add "tags" to account
for the second dimension. I don't like that currently we're mixing the two
dimensions in how the sets are defined.

My current attempt at reconciling these aspects are the "IsAllowedXXXX" sets,
where it's still clear what constitutes the whole logical "set", and what's
carved out for attack surface reduction.

For instance if you look at IsAllowedAddressSpaceAccess(), it's clear that
mremap() belongs to the same set as mmap(), even though we exclude it for attack
surface reduction reasons.

I think we should see what happens. If things break often, we should err more on
the side of "strict PSS" and less attack surface reduction, or on the side of
brokering things out.

That's why I've been so adamant that we get good test coverage.

Another thing we could do is to have "two levels" of BPF sandboxes. The first
level for a sandbox that only looks a system calls "semantics", the second level
for attack surface reduction.
The second level could run some basic tests before the sandbox gets enabled, so
that we don't enable it if it'd lead to a crash.

Julien

[cevans, 2012-08-30 00:31:26]

LGTM++

On Wed, Aug 29, 2012 at 5:29 PM, <jln@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10885021/diff/**
>
7001/content/common/sandbox_**seccomp_bpf_linux.cc<https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sandbox_seccomp_bpf_linux.cc>
> File content/common/sandbox_**seccomp_bpf_linux.cc (right):
>
> https://chromiumcodereview.**appspot.com/10885021/diff/**
>
7001/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode1244<https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sandbox_seccomp_bpf_linux.cc#newcode1244>
> content/common/sandbox_**seccomp_bpf_linux.cc:1244: case __NR_fdatasync:
> On 2012/08/30 00:08:26, Chris Evans wrote:
>
>> It sort of feels like sync_file_range fits into this little logical
>>
> group?
>
>  I don't know if it adds much surface (e.g. OOB ranges, negative or
>>
> huge ints,
>
>> etc)
>>
>
> Yes, it really depends on our strategy. In the same way, I didn't allow
> the whole IsAdvancedScheduler (and allowed specific system calls
> instead) and didn't put pread64/pwrite64 in the main I/O PSS to be
> conservative and err on the side of attack surface reduction, which is
> why this policy doesn't look as "nice" as the other ones.
>
> This sandbox could be really robust (in terms of not crashing / stopping
> working) if we didn't have to take attack surface reduction into
> account.
>
> In other words, we have multiple dimensions:
>
> 1. What system calls allow the process to do (system call semantics)
> 2. Attack surface reduction
>
> It's hard to juggle with the two. We could achieve (2) without breaking
> the kernel API by relying heavily on brokers, but it comes with a
> performance impact.
>
> Sometimes I think about a way to express PSS where I could add "tags" to
> account for the second dimension. I don't like that currently we're
> mixing the two dimensions in how the sets are defined.
>
> My current attempt at reconciling these aspects are the "IsAllowedXXXX"
> sets, where it's still clear what constitutes the whole logical "set",
> and what's carved out for attack surface reduction.
>
> For instance if you look at IsAllowedAddressSpaceAccess(), it's clear
> that mremap() belongs to the same set as mmap(), even though we exclude
> it for attack surface reduction reasons.
>
> I think we should see what happens. If things break often, we should err
> more on the side of "strict PSS" and less attack surface reduction, or
> on the side of brokering things out.
>
> That's why I've been so adamant that we get good test coverage.
>
> Another thing we could do is to have "two levels" of BPF sandboxes. The
> first level for a sandbox that only looks a system calls "semantics",
> the second level for attack surface reduction.
> The second level could run some basic tests before the sandbox gets
> enabled, so that we don't enable it if it'd lead to a crash.
>
> Julien
>
>
https://chromiumcodereview.**appspot.com/10885021/<https://chromiumcodereview...
>

[cevans, 2012-08-30 00:31:27]

LGTM++

On Wed, Aug 29, 2012 at 5:29 PM, <jln@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10885021/diff/**
>
7001/content/common/sandbox_**seccomp_bpf_linux.cc<https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sandbox_seccomp_bpf_linux.cc>
> File content/common/sandbox_**seccomp_bpf_linux.cc (right):
>
> https://chromiumcodereview.**appspot.com/10885021/diff/**
>
7001/content/common/sandbox_**seccomp_bpf_linux.cc#**newcode1244<https://chromiumcodereview.appspot.com/10885021/diff/7001/content/common/sandbox_seccomp_bpf_linux.cc#newcode1244>
> content/common/sandbox_**seccomp_bpf_linux.cc:1244: case __NR_fdatasync:
> On 2012/08/30 00:08:26, Chris Evans wrote:
>
>> It sort of feels like sync_file_range fits into this little logical
>>
> group?
>
>  I don't know if it adds much surface (e.g. OOB ranges, negative or
>>
> huge ints,
>
>> etc)
>>
>
> Yes, it really depends on our strategy. In the same way, I didn't allow
> the whole IsAdvancedScheduler (and allowed specific system calls
> instead) and didn't put pread64/pwrite64 in the main I/O PSS to be
> conservative and err on the side of attack surface reduction, which is
> why this policy doesn't look as "nice" as the other ones.
>
> This sandbox could be really robust (in terms of not crashing / stopping
> working) if we didn't have to take attack surface reduction into
> account.
>
> In other words, we have multiple dimensions:
>
> 1. What system calls allow the process to do (system call semantics)
> 2. Attack surface reduction
>
> It's hard to juggle with the two. We could achieve (2) without breaking
> the kernel API by relying heavily on brokers, but it comes with a
> performance impact.
>
> Sometimes I think about a way to express PSS where I could add "tags" to
> account for the second dimension. I don't like that currently we're
> mixing the two dimensions in how the sets are defined.
>
> My current attempt at reconciling these aspects are the "IsAllowedXXXX"
> sets, where it's still clear what constitutes the whole logical "set",
> and what's carved out for attack surface reduction.
>
> For instance if you look at IsAllowedAddressSpaceAccess(), it's clear
> that mremap() belongs to the same set as mmap(), even though we exclude
> it for attack surface reduction reasons.
>
> I think we should see what happens. If things break often, we should err
> more on the side of "strict PSS" and less attack surface reduction, or
> on the side of brokering things out.
>
> That's why I've been so adamant that we get good test coverage.
>
> Another thing we could do is to have "two levels" of BPF sandboxes. The
> first level for a sandbox that only looks a system calls "semantics",
> the second level for attack surface reduction.
> The second level could run some basic tests before the sandbox gets
> enabled, so that we don't enable it if it'd lead to a crash.
>
> Julien
>
>
https://chromiumcodereview.**appspot.com/10885021/<https://chromiumcodereview...
>

[Markus (顧孟勤), 2012-08-30 00:41:41]

Just for the record: LGTM (as discussed off line)

[Markus (顧孟勤), 2012-08-30 00:41:41]

Just for the record: LGTM (as discussed off line)

[commit-bot: I haz the power, 2012-08-30 03:10:13]

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

[commit-bot: I haz the power, 2012-08-30 03:10:27]

Change committed as 154054