Introduce support for syscall filtering in containers #237 #384

mheon · 2015-02-17T16:15:37Z

Obsoletes #263

This introduces Seccomp system call filtering, which restricts system calls, preventing them from being made inside containers.

Compared to the previous pull request, several changes have been made:

The previous version of the patches only allowed blacklists. This version can be configured to blacklist or whitelist system calls.
Support for conditionally-blocked syscalls has been added. This allows a system call to be blocked only if its arguments meet certain conditions

The vendored bindings for libseccomp are less than optimal right now - they're located in the main libseccomp repository, so checking them out also checks out fifty-odd C files. If there are suggestions for a better way of doing things, I'd be open to changing this.

mrunalp · 2015-02-20T00:40:58Z

@mheon The api branch was just merged into master. Would you mind rebasing? Thanks!

mheon · 2015-02-20T20:06:27Z

@mrunalp Rebased, though it looks like I might have broken the test suite along the way - looking into it.

mrunalp · 2015-03-04T19:53:11Z

@mheon Could you rebase again? Github is complaining about conflicts. I will test this out today.

mheon · 2015-03-04T21:05:49Z

@mrunalp Rebased, unit tests passing.

Worth noting that there's a bug in libseccomp upstream at the moment which breaks conditional syscall blocking. A fix is incoming, but for the moment there are no tests on blocking a syscall based on multiple conditions

mrunalp · 2015-03-04T22:31:03Z

@mheon Thanks!

mrunalp · 2015-03-04T23:53:47Z

The seccomp tests aren't actually getting run in jenkins because of the seccomp tag.

mrunalp · 2015-03-04T23:59:17Z

sample_configs/seccomp.json

+    "user": "daemon",
+    "seccomp_config": {
+	"enable": true,
+	"whitelist": false,


whitelist_toggle?

mheon · 2015-03-12T15:24:27Z

I've added the seccomp tag to the Dockerfile so the tests should be running automatically now. There were previously issues with running Seccomp tests on the CI server, but these seem to be resolved.

It should even be possible to remove the seccomp tag completely at this point, but it could still be desirable to build libcontainer without libseccomp support.

mheon · 2015-03-16T19:56:08Z

Rebased on latest master. Some changes in integration tests, which broke on the new version of busybox.

mrunalp · 2015-03-16T20:52:20Z

@mheon Thanks! I will try to get to this by end of today.

mrunalp · 2015-03-19T23:55:28Z

@crosbymichael @LK4D4 ping

mrunalp · 2015-03-19T23:56:51Z

I took a quick look and overall looks good.

LK4D4 · 2015-03-31T22:47:50Z

@mheon Can you rebase pls?

tiborvass · 2015-03-31T22:50:39Z

@gabrtv here you go :)

mheon · 2015-03-31T22:54:56Z

@LK4D4 Rebased

LK4D4 · 2015-04-01T15:31:57Z

@mheon I have question. Is this makes sense to release goseccomp as separate package under your name?

LK4D4 · 2015-04-01T15:33:23Z

security/seccomp/types.go

+	Conditions []SyscallCondition `json:"conditions,omitempty"`
+}
+
+type SeccompConfig struct {


I prefer call this type Config because seccomp part always obvious from package name.

Here probably docstrings too. At least for WhitelistToggle

mheon · 2015-04-01T15:36:20Z

@LK4D4 From an upstream perspective, it makes things easier for the Libseccomp maintainers. However, given it does create some issues with vendoring the bindings, I'd be willing to maintain a copy of the bindings independent of the main repository.

LK4D4 · 2015-04-01T15:36:45Z

security/seccomp/seccomp.go

+)
+
+var (
+	actAllow libseccomp.ScmpAction = libseccomp.ActAllow


libseccomp.ScmpAction is redundant, type will be inferred from right side.

pcmoore · 2015-04-02T00:33:19Z

Can someone please explain why including the libseccomp docker bindings with the rest of libseccomp is problematic? I'm willing to make reasonable adjustments, but no one has explained the problem.

mheon · 2015-04-02T00:42:54Z

@pcmoore All the Go dependencies for Libcontainer are included in this repository. The way of consistently acquiring or updating this externally-available dependencies is by a vendoring script, which clones their Git repositories. If the Libseccomp Go bindings are in the Libseccomp repository, this would grab the entire repository, including all the C source code and test suite. Ideally we'd just want the Go bindings. Right now, my solution is to clone the entire repo, and then remove every file that's not the Go bindings and license, which is kind of gross.

mrunalp · 2015-04-02T03:59:39Z

@pcmoore @mheon Based on the libct backend dependencies (C + go) that we will end up pulling into libcontainer, I think it should be fine to pull all of libseccomp.
@LK4D4 @crosbymichael WDYT?

LK4D4 · 2015-04-02T04:12:03Z

@mrunalp I don't care.
Just don't like ugly imports and I'm not fan of libct bindings in repo too. I used to pure golang packages. Also I'm not sure about contributor/maintainer relationships in such projects, it isn't unusual that author of library is not willing to maintain all possible bindings in addition to library or giving write access to repo for all bindings authors.
Now you can see how much I don't care :)

This PR introduces the ability to filter system calls on a per-container basis on Linux, using libseccomp to support multiple architectures. This adds another layer of security between containers and the kernel. System calls which are unnecessary in a container or problematic from a security perspective can be restricted to prevent their use. Most of the truly problematic syscalls are already restricted by dropping capabilities; this adds an additional, finer-grained layer of protection. This PR adds a vendored library dependency (Go bindings for libseccomp) and a build dependency on libseccomp >= v2.1. The actual changes to libcontainer are fairly minimal, most of the delta is in the libseccomp bindings. Docker-DCO-1.1-Signed-off-by: Dan Walsh <[email protected]> (github: rhatdan) Docker-DCO-1.1-Signed-off-by: Matt Heon <[email protected]> (github: mheon)

Also update library itself to latest master. Signed-off-by: Matthew Heon <[email protected]>

Signed-off-by: Matthew Heon <[email protected]>

mheon · 2015-04-02T18:21:53Z

Requested changes have been made.

Most notably, we pull in the entire Libseccomp repository now, meaning the diffstat just exploded.

Signed-off-by: Matthew Heon <[email protected]>

pcmoore · 2015-04-03T10:14:00Z

Okay, so all is good now with respect to the repo?

rhatdan · 2015-04-06T13:10:01Z

Certainly would be good to get this into docker-1.7.

diogomonica · 2015-04-12T05:29:25Z

@rhatdan agreed. I think the big question is what a default profile will look like.

rhatdan · 2015-04-13T12:41:30Z

@diogomonica I think Matt (And I) have come up with a pretty good default blocking profile. Mostly stolen from other container tools and through suggestions from kernel developers. If we could at least start getting some of this merged, then we could release an experimental patch to Rawhide, to see if anyone sees issues with the default list of blocked syscalls.

mrunalp · 2015-04-13T16:54:12Z

@rhatdan There were some licensing concerns around merging this. (LGPL).

mheon · 2015-04-13T16:59:48Z

@mrunalp We're looking into it. I wouldn't be opposed to relicensing, but the bindings are going in the upstream repository as well, so we want to examine options there. Updates on that (hopefully) soon!

On defaults: I think the most sane policy for a default blocklist is a blacklist, as I think we should allow syscalls introduced by new kernel versions by default, and in general whitelisting is more cumbersome than blacklisting for syscalls (an absolutely minimal whitelist to get from Docker to "return 0" is around 25 syscalls, and that goes up sharply in number if you want to do basic I/O, file operations, networking, etc).

hqhq · 2015-04-14T02:34:15Z

Can we take a look at this PR #529, it made it in another way.

pcmoore · 2015-04-14T11:33:02Z

Interesting, it handles the seccomp BPF filter generation completely in Golang. I'm obviously a little biased so I won't comment on which approach is better, but I will mention that just a quick glance at the code in PR #529 revealed a few issues/concerns:

It doesn't appear that x32 is handled correctly, resulting in a gap in protection on some systems
The syscall tables are wrong for ARM, e.g. OABI_SYSCALL_BASE (?)
The code doesn't abstract away architecture specifics, e.g. socketcall()
The generated BPF filter code looks like it might be twice as large as necessary resulting in unnecessary per-syscall performance overhead
The generated BPF filter code has a trap/signal return action, yet I don't see a signal handler defined (?) ... possible this may be misunderstanding how Golang handles signals

I suspect if I look closer there are more issues as well, not to mention the long term maintenence issues (who will keep the syscall tables updated?).

rhatdan · 2015-04-14T17:18:41Z

To me it would seem prudent to merge in libseccomp support and then allow the golang implementation to grow in support til it matches libseccomp. At that point, I would suggest we look at this again. But I really do not want this effort to get derailed.

diogomonica · 2015-04-14T17:20:39Z

@rhatdan agreed. This said, BPF filter generation completely in Golang is totally the right way to go.

crosbymichael · 2015-04-14T17:37:34Z

@mheon what about the license? Even if it's in upstream LGPL does not make sense for Go because of the static compiles.

mheon · 2015-04-14T17:40:24Z

@crosbymichael I'd be fine with relicensing the bindings, though I'd have to go through @pcmoore to verify that he's alright with it as well.

Big issue from my perspective is that the actual libseccomp library, itself LGPL, is going to have to be statically-linked into Docker.

crosbymichael · 2015-05-20T22:57:47Z

@mheon we can dynamically link it into docker but i still don't think it makes sense for Go because every go pkg is static. what do you think?

rhatdan · 2015-05-21T13:05:01Z

Why is this even a problem. Don't we already staticly link in libc.so which is LGPL?

LK4D4 · 2015-05-21T15:19:14Z

@rhatdan I think no, we're not linking libc, that's why we have our own user library.

crosbymichael · 2015-06-09T17:15:46Z

#613 was merged with a pure go implementation. Thanks for your help and input on this feature.

rhatdan · 2015-06-09T19:36:14Z

We would like to question this. We have strong reservations about the go lang bindings.

mheon · 2015-06-09T23:00:29Z

To add to this: I think the comments from @pcmoore on #529 provide a good summary of our reasoning for an alternative implementation (ease of maintenance, feature completeness, performance).

This PR hadn't seen much love in a long time (I'd moved on to other projects and not found time to rebase/solve the licensing issues). However, the way forward is fairly clear (relicense the bindings to a non-copyleft license... I was thinking BSD?), and I think I could have an updated version free of licensing hurdles by the end of tomorrow. I'd like this at least to be considered as an alternative before locking in to supporting a Golang implementation for the foreseeable future.

rhatdan mentioned this pull request Feb 17, 2015

Introduce support for syscall filtering in containers #263

Closed

mheon force-pushed the seccomp branch from 09f2735 to 07b402f Compare February 20, 2015 19:56

mheon force-pushed the seccomp branch 2 times, most recently from 848420b to ee60d35 Compare March 4, 2015 21:00

mrunalp reviewed Mar 4, 2015
View reviewed changes

sample_configs/seccomp.json

"user": "daemon",

"seccomp_config": {

"enable": true,

"whitelist": false,

Copy link

Contributor

mrunalp Mar 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitelist_toggle?

mheon force-pushed the seccomp branch from ee60d35 to d6af5cb Compare March 12, 2015 15:21

mheon force-pushed the seccomp branch from d6af5cb to 34c8e16 Compare March 16, 2015 19:55

mheon force-pushed the seccomp branch from 34c8e16 to 5573954 Compare March 19, 2015 12:07

mheon force-pushed the seccomp branch from 5573954 to 4258342 Compare March 31, 2015 22:54

LK4D4 reviewed Apr 1, 2015
View reviewed changes

mheon added 3 commits April 2, 2015 11:44

Vendor entire Libseccomp library, not just Go bindings.

8d2525b

Also update library itself to latest master. Signed-off-by: Matthew Heon <[email protected]>

Small updates throughout based on feedback

118bb9e

Signed-off-by: Matthew Heon <[email protected]>

mheon force-pushed the seccomp branch from 4258342 to 118bb9e Compare April 2, 2015 18:19

Fix unit test failures

c1de343

Signed-off-by: Matthew Heon <[email protected]>

crosbymichael closed this Jun 9, 2015

mheon mentioned this pull request Jun 15, 2015

Move Seccomp support to use Libseccomp as backend #633

Closed

Introduce support for syscall filtering in containers #237 #384

Introduce support for syscall filtering in containers #237 #384

Conversation

mheon commented Feb 17, 2015

mrunalp commented Feb 20, 2015

mheon commented Feb 20, 2015

mrunalp commented Mar 4, 2015

mheon commented Mar 4, 2015

mrunalp commented Mar 4, 2015

mrunalp commented Mar 4, 2015

mrunalp Mar 4, 2015

Choose a reason for hiding this comment

mheon commented Mar 12, 2015

mheon commented Mar 16, 2015

mrunalp commented Mar 16, 2015

mrunalp commented Mar 19, 2015

mrunalp commented Mar 19, 2015

LK4D4 commented Mar 31, 2015

tiborvass commented Mar 31, 2015

mheon commented Mar 31, 2015

LK4D4 commented Apr 1, 2015

LK4D4 Apr 1, 2015

Choose a reason for hiding this comment

LK4D4 Apr 1, 2015

Choose a reason for hiding this comment

mheon commented Apr 1, 2015

LK4D4 Apr 1, 2015

Choose a reason for hiding this comment

pcmoore commented Apr 2, 2015

mheon commented Apr 2, 2015

mrunalp commented Apr 2, 2015

LK4D4 commented Apr 2, 2015

mheon commented Apr 2, 2015

pcmoore commented Apr 3, 2015

rhatdan commented Apr 6, 2015

diogomonica commented Apr 12, 2015

rhatdan commented Apr 13, 2015

mrunalp commented Apr 13, 2015

mheon commented Apr 13, 2015

hqhq commented Apr 14, 2015

pcmoore commented Apr 14, 2015

rhatdan commented Apr 14, 2015

diogomonica commented Apr 14, 2015

crosbymichael commented Apr 14, 2015

mheon commented Apr 14, 2015

crosbymichael commented May 20, 2015

rhatdan commented May 21, 2015

LK4D4 commented May 21, 2015

crosbymichael commented Jun 9, 2015

rhatdan commented Jun 9, 2015

mheon commented Jun 9, 2015