Phase 1: Initial seccomp support #17989

Merged
merged 8 commits into from Dec 5, 2015

Projects

None yet
@jessfraz
Contributor

This is a phase 1 of the security profiles described in #17142 (comment).

It allows for passing a seccomp profile in the form of:

{
     "defaultAction": "SCMP_ACT_ALLOW",
     "syscalls": [
         {
             "name": "getcwd",
             "action": "SCMP_ACT_ERRNO"
         }
     ]
 }

based off the runc runtime spec https://github.com/opencontainers/specs/blob/master/runtime-config-linux.md#seccomp

Example:

$ docker run --rm -it --security-ops seccomp:/path/to/container-profile.json jess/i-am-malicious

@LK4D4 @crosbymichael let me know if we are not ready to import the spec quite yet for this, but I didn't want to repeat the structs unless you deemed necessary :)

TODO: obviously this needs moarrr tests and docs, coming super soon ;)

I feel like there was something else I wanted to note, but I forgot... so... coming soon as well....

@jessfraz jessfraz referenced this pull request Nov 15, 2015
Closed

Seccomp initial support #17359

@jessfraz jessfraz added this to the 1.10 milestone Nov 15, 2015
@crosbymichael
Member

There are no UI changes in this PR right?

@jessfraz
Contributor

No just --security-opt seccomp:path but I need to write docs and tests

@diogomonica diogomonica and 1 other commented on an outdated diff Nov 18, 2015
daemon/execdriver/native/seccomp.go
+}
+
+func setupSeccomp(config *specs.Seccomp) (*configs.Seccomp, error) {
+ if config == nil {
+ return nil, nil
+ }
+
+ // No default action specified, no syscalls listed, assume seccomp disabled
+ if config.DefaultAction == "" && len(config.Syscalls) == 0 {
+ return nil, nil
+ }
+
+ newConfig := new(configs.Seccomp)
+ newConfig.Syscalls = []*configs.Syscall{}
+
+ if len(config.Architectures) > 0 {
@diogomonica
diogomonica Nov 18, 2015 Contributor

Shouldn't this fail if len(config.Architectures) == 0?

Maybe a nice error for the user instead of this failing silently?

@jessfraz
jessfraz Nov 18, 2015 Contributor

added comment

@diogomonica diogomonica and 1 other commented on an outdated diff Nov 18, 2015
daemon/execdriver/native/seccomp.go
+ newConfig := new(configs.Seccomp)
+ newConfig.Syscalls = []*configs.Syscall{}
+
+ if len(config.Architectures) > 0 {
+ newConfig.Architectures = []string{}
+ for _, arch := range config.Architectures {
+ newArch, err := seccomp.ConvertStringToArch(string(arch))
+ if err != nil {
+ return nil, err
+ }
+ newConfig.Architectures = append(newConfig.Architectures, newArch)
+ }
+ }
+
+ // Convert default action from string representation
+ newDefaultAction, err := seccomp.ConvertStringToAction(string(config.DefaultAction))
@diogomonica
diogomonica Nov 18, 2015 Contributor

Why are we doing string(config.DefaultAction))? It seems that it would fail on line 40 if config.DefaultAction == "" if not.

@jessfraz
jessfraz Nov 18, 2015 Contributor

action is an int (seems like)?

@diogomonica diogomonica and 1 other commented on an outdated diff Nov 18, 2015
daemon/execdriver/native/seccomp.go
+ newConfig.Architectures = []string{}
+ for _, arch := range config.Architectures {
+ newArch, err := seccomp.ConvertStringToArch(string(arch))
+ if err != nil {
+ return nil, err
+ }
+ newConfig.Architectures = append(newConfig.Architectures, newArch)
+ }
+ }
+
+ // Convert default action from string representation
+ newDefaultAction, err := seccomp.ConvertStringToAction(string(config.DefaultAction))
+ if err != nil {
+ return nil, err
+ }
+ newConfig.DefaultAction = newDefaultAction
@diogomonica
diogomonica Nov 18, 2015 Contributor

What about having newConfig.DefaultAction, err := seccomp... on line 59?

@jessfraz
jessfraz Nov 18, 2015 Contributor

fixed

@mheon
Contributor
mheon commented Nov 18, 2015

Overall looks solid, though it would be nice to have the ability to specify a default ruleset when the daemon starts that will be applied to all containers started by the daemon (there are some syscalls you want to block almost unconditionally). This would require more UI work (security-opt would need to support the ability to disable Seccomp filtering, and possibly add/remove individual rules). I've been working on frontend code for this on and off, so I have some code demonstrating this - I'll see about getting it ported over to this backend to demonstrate.

@jessfraz
Contributor

@mheon you should look at #17142 (comment)

@sroze sroze and 1 other commented on an outdated diff Nov 19, 2015
contrib/builder/deb/generate.sh
@@ -58,6 +58,8 @@ for version in "${versions[@]}"; do
libdevmapper-dev # for "libdevmapper.h"
libltdl-dev # for pkcs11 "ltdl.h"
libsqlite3-dev # for "sqlite3.h"
+ libseccomp2 \ # for "libseccomp.so.2"
@sroze
sroze Nov 19, 2015

I think that you should remove the backslashes to fix the generated Dockerfiles

@jessfraz
jessfraz Nov 19, 2015 Contributor

Yes will do

@jessfraz
jessfraz Nov 19, 2015 Contributor

Tis what happens when you make PRs at 4am :P

@jessfraz
Contributor

I need docker/libnetwork#770 here as well..

@jessfraz
Contributor

woohoo updated and rebased!

@estesp
Member
estesp commented Nov 30, 2015

Just FYI--hopefully this will get in somewhat soon so we can have broader platform support--however, that would require not just grabbing libseccomp from distro repos as I think a new version of the base library is going to come out as well (with fixes for Linux kernel 4.3 as well, apparently): https://groups.google.com/forum/#!topic/libseccomp/JXRvNsdGGPw

I don't think this needs to hold up this PR, but without the full architecture support, we may have to turn off libseccomp build tags for non-supported architectures for now?

@cpuguy83
Contributor

Code LGTM, just a couple of minor nits.

@jessfraz
Contributor

@estesp should I remove the seccomp build flag from certain archs

@estesp
Member
estesp commented Nov 30, 2015

@jfrazelle hmm..yeah, so I guess it's added in hack/make.sh, but not sure we want to clutter that with a arch/os case.. given ARM is already there, I guess this only affects POWER/z for now, although Windows (daemon) officially shouldn't be building the seccomp tag either.

@jessfraz
Contributor

gotcha will find pretty way of fixing :)

@rhatdan
Contributor
rhatdan commented Dec 1, 2015

I agree with @mheon (No surprise there) If this is only opt in, then most users will never opt in. Having a default black list like systemd-nspawn and libvirt/qemu should also be possible. This will give all container greater security and not just for those crazy enough to try out --security-opt type flags.

@jessfraz
Contributor
jessfraz commented Dec 1, 2015

Can we save that for a phase 2, I think it would be cool but don't want to break anyone right out of the gates

@estesp
Member
estesp commented Dec 1, 2015

Not to mention I think ideas discussed at DockerCon EU around security profiles (a future item still under discussion) would provide the more "user friendly" approach to using underlying security techniques without forcing users to understand seccomp, apparmor, SELinux, capabilities lists, etc. This Phase 1 makes seccomp accessible to power users, and hopefully profiles will make it usable with good defaults for general users.

@jessfraz
Contributor
jessfraz commented Dec 1, 2015

@estesp +1 <3

@rhatdan
Contributor
rhatdan commented Dec 1, 2015

My point being is if the only way to turn these on are by choice, no one (Or a very small percentage of users will).

Everyone is using SELinux by default. Everyone is using Dropped Capabilities by default, Everone is using read/only mount points by default.

How can we get seccomp for everyone by default?

@jessfraz
Contributor
jessfraz commented Dec 1, 2015

@rhatdan i thinnk this can be discussed on #17142 (comment)

@jessfraz
Contributor
jessfraz commented Dec 1, 2015

@estesp I added the seccomp buildtag to the dockerfile so it DNE in the gccgo or power dockerfiles and will build without support :)

@jessfraz
Contributor
jessfraz commented Dec 1, 2015

idk why the docs test failed :/

EDIT: my b fixed

@jessfraz
Contributor
jessfraz commented Dec 2, 2015

so 1 problem, looks like nothing provides seccomp.h on centos or fedora :/ ping @vbatts @rhatdan

EDIT: my bad

@mheon
Contributor
mheon commented Dec 2, 2015

@jfrazelle Package is libseccomp-devel on Fedora, should be the same on Cent

@jessfraz
Contributor
jessfraz commented Dec 2, 2015

wow sorry just saw that

@jessfraz
Contributor
jessfraz commented Dec 2, 2015

greeeennn

@jessfraz jessfraz referenced this pull request Dec 2, 2015
Closed

remove dockerinit #18355

@tianon tianon and 1 other commented on an outdated diff Dec 2, 2015
contrib/builder/deb/generate.sh
@@ -58,6 +58,8 @@ for version in "${versions[@]}"; do
libdevmapper-dev # for "libdevmapper.h"
libltdl-dev # for pkcs11 "ltdl.h"
libsqlite3-dev # for "sqlite3.h"
+ libseccomp2 # for "libseccomp.so.2"
@tianon
tianon Dec 2, 2015 Member

Doesn't libseccomp-dev pull this in properly already? Did we need it to be explicit for some other reason?

@jessfraz
jessfraz Dec 2, 2015 Contributor

i kind of liked it for the provides comment but i can remove

@jessfraz
jessfraz Dec 2, 2015 Contributor

i guess it provides it too haha, ill remove

@tianon
tianon via email Dec 2, 2015 Member
@jessfraz
jessfraz Dec 2, 2015 Contributor

I updated :)

@calavera
Contributor
calavera commented Dec 2, 2015

LGTM ๐ŸŽ‰ ๐Ÿ‘พ

It needs a rebase though :godmode:

@jessfraz
Contributor
jessfraz commented Dec 2, 2015

rebased, also pinging @tianon to checkout last commit and tell me how much he disapproves for the problem w dockerinit needing libseccomp.a for debs & rpms

otherwise if you hate it we can work on updating the libseccomp dev packages and update the script after or remove dockerinit and update the script after

@jessfraz
Contributor
jessfraz commented Dec 2, 2015

ping @estesp for code review ๐Ÿ˜‡

@tianon tianon commented on an outdated diff Dec 2, 2015
contrib/builder/deb/debian-jessie/Dockerfile
@@ -4,11 +4,15 @@
FROM debian:jessie
-RUN apt-get update && apt-get install -y apparmor bash-completion btrfs-tools build-essential curl ca-certificates debhelper dh-apparmor dh-systemd git libapparmor-dev libdevmapper-dev libltdl-dev libsqlite3-dev libsystemd-journal-dev --no-install-recommends && rm -rf /var/lib/apt/lists/*
+RUN apt-get update && apt-get install -y apparmor bash-completion btrfs-tools build-essential curl ca-certificates debhelper dh-apparmor dh-systemd git libapparmor-dev libdevmapper-dev libltdl-dev libsqlite3-dev libseccomp-dev libsystemd-journal-dev --no-install-recommends && rm -rf /var/lib/apt/lists/*
+
+ENV SECCOMP_VERSION v2.2.3
+RUN buildDeps=' automake libtool ' && set -x && apt-get update && apt-get install -y $buildDeps --no-install-recommends && rm -rf /var/lib/apt/lists/* && export SECCOMP_PATH=$(mktemp -d) && git clone https://github.com/seccomp/libseccomp.git "$SECCOMP_PATH" && ( cd "$SECCOMP_PATH" && git checkout "$SECCOMP_VERSION" && ./autogen.sh && ./configure --prefix=/usr && make && install -c src/.libs/libseccomp.a /usr/lib/libseccomp.a && chmod 644 /usr/lib/libseccomp.a && ranlib /usr/lib/libseccomp.a && ldconfig -n /usr/lib ) && rm -rf "$SECCOMP_PATH"
@tianon
tianon Dec 2, 2015 Member

Hahahahaha, this is adorable

@tianon tianon and 1 other commented on an outdated diff Dec 2, 2015
contrib/builder/deb/generate.sh
@@ -92,6 +104,40 @@ for version in "${versions[@]}"; do
echo >> "$version/Dockerfile"
+ # debian jessie & ubuntu trusty/vivid do not have a libseccomp.a for compiling static dockerinit
+ # ONLY install libseccomp.a from source, this can be removed once dockerinit is removed
+ case "$suite" in
+ jessie|trusty|vivid)
+ awk '$1 == "ENV" && $2 == "SECCOMP_VERSION" { print; exit }' ../../../Dockerfile >> "$version/Dockerfile"
+ cat <<-EOF >> "$version/Dockerfile"
@tianon
tianon Dec 2, 2015 Member

I don't see any actual environment variables from the enclosing environment used here; couldn't we switch this to use <<-'EOF' and drop a few \ on all those $?

@tianon
tianon Dec 2, 2015 Member

Might also be worth dropping a # TODO remove this manual seccomp compilation once dockerinit is gone or no longer needs to be statically compiled or something like that

@jessfraz
jessfraz Dec 2, 2015 Contributor

smart, added

@tianon tianon and 1 other commented on an outdated diff Dec 2, 2015
contrib/builder/deb/generate.sh
@@ -92,6 +104,40 @@ for version in "${versions[@]}"; do
echo >> "$version/Dockerfile"
+ # debian jessie & ubuntu trusty/vivid do not have a libseccomp.a for compiling static dockerinit
+ # ONLY install libseccomp.a from source, this can be removed once dockerinit is removed
+ case "$suite" in
+ jessie|trusty|vivid)
+ awk '$1 == "ENV" && $2 == "SECCOMP_VERSION" { print; exit }' ../../../Dockerfile >> "$version/Dockerfile"
+ cat <<-EOF >> "$version/Dockerfile"
+ RUN buildDeps=' \
+ automake \
+ libtool \
+ ' \
+ && set -x \
+ && apt-get update && apt-get install -y \$buildDeps --no-install-recommends \
+ && rm -rf /var/lib/apt/lists/* \
+ && export SECCOMP_PATH=\$(mktemp -d) \
+ && git clone https://github.com/seccomp/libseccomp.git "\$SECCOMP_PATH" \
@tianon
tianon Dec 2, 2015 Member

Since we don't care about verbose warnings from git in this build output, couldn't we change this to something like git clone -b "$SECCOMP_VERSION" --depth 1 ... so that it goes slightly faster?

@jessfraz
jessfraz Dec 2, 2015 Contributor

updated

@tianon tianon and 1 other commented on an outdated diff Dec 2, 2015
contrib/builder/rpm/generate.sh
+ && set -x \
+ && ${installer} install -y \$buildDeps \
+ && export SECCOMP_PATH=\$(mktemp -d) \
+ && git clone https://github.com/seccomp/libseccomp.git "\$SECCOMP_PATH" \
+ && ( \
+ cd "\$SECCOMP_PATH" \
+ && git checkout "\$SECCOMP_VERSION" \
+ && ./autogen.sh \
+ && ./configure --prefix=/usr \
+ && make \
+ && install -c src/.libs/libseccomp.a /usr/lib/libseccomp.a \
+ && chmod 644 /usr/lib/libseccomp.a \
+ && ranlib /usr/lib/libseccomp.a \
+ && ldconfig -n /usr/lib \
+ ) \
+ && rm -rf "\$SECCOMP_PATH"
@tianon
tianon Dec 2, 2015 Member

Did you make a variable for buildDeps so they could have an apt-get purge --auto-remove $buildDeps down here, too?

@tianon
tianon Dec 2, 2015 Member

Doh, wrong section... facepalm

@jessfraz
jessfraz Dec 2, 2015 Contributor

fixed!

@estesp
Member
estesp commented Dec 2, 2015

Looks like @tianon took care of the bash/Dockerfile content review pretty completely ๐Ÿ˜‡ ๐Ÿ‘

I had already reviewed and tested much of this a few weeks ago, but just refreshed my memory going through all the code portion changes--I don't have any outstanding issues with the code, so I'm definitely code and tests LGTM

@tianon
Member
tianon commented Dec 2, 2015

LGTM ๐Ÿ‘

@thaJeztah
Member

looks like this can be moved to docs review? or are more eyes needed at the code?

@jessfraz
Contributor
jessfraz commented Dec 2, 2015

docs review \o/

@NathanMcCauley NathanMcCauley and 1 other commented on an outdated diff Dec 3, 2015
docs/security/seccomp.md
@@ -0,0 +1,66 @@
+<!-- [metadata]>
++++
+title = "Seccomp security profiles for Docker"
+description = "Enabling seccomp in Docker"
+keywords = ["seccomp, security, docker, documentation"]
@NathanMcCauley
NathanMcCauley Dec 3, 2015 Contributor

extra space

@jessfraz
jessfraz Dec 4, 2015 Contributor

fixed

@NathanMcCauley
Contributor

Docs LGTM

@jessfraz
Contributor
jessfraz commented Dec 3, 2015
@moxiegirl moxiegirl and 1 other commented on an outdated diff Dec 4, 2015
docs/security/seccomp.md
@@ -0,0 +1,66 @@
+<!-- [metadata]>
++++
+title = "Seccomp security profiles for Docker"
+description = "Enabling seccomp in Docker"
+keywords = ["seccomp, security, docker, documentation"]
+[menu.main]
+draft = true
@moxiegirl
moxiegirl Dec 4, 2015 Contributor

@jfrazelle Is this meant to be a draft? Draft's don't go out into the live docs.

When would you like it to go live?

@jessfraz
jessfraz Dec 4, 2015 Contributor

oh no i just copy pasted I have no idea what im doing sorry

On Thu, Dec 3, 2015 at 4:09 PM, moxiegirl notifications@github.com wrote:

In docs/security/seccomp.md
#17989 (comment):

@@ -0,0 +1,66 @@
+<!-- [metadata]>
++++
+title = "Seccomp security profiles for Docker"
+description = "Enabling seccomp in Docker"
+keywords = ["seccomp, security, docker, documentation"]
+[menu.main]
+draft = true

@jfrazelle https://github.com/jfrazelle Is this meant to be a draft?
Draft's don't go out into the live docs.

When would you like it to go live?

โ€”
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/17989/files#r46632327.

@jessfraz
jessfraz Dec 4, 2015 Contributor

unrelated but this is a draft and probably shouldnt be https://raw.githubusercontent.com/docker/docker/master/docs/security/apparmor.md

@jessfraz
jessfraz Dec 4, 2015 Contributor

fixed

jessfraz added some commits Nov 15, 2015
@jessfraz jessfraz dockerfile update for seccomp
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
d616a09
@jessfraz jessfraz update debs/rpms for seccomp
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
255004e
@jessfraz jessfraz inital seccomp support
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
6707f4b
@jessfraz jessfraz update hack/vendor.sh scripts and run vendor
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
ed5853d
@jessfraz jessfraz update bash completion for seccomp
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
ae76f7e
@jessfraz jessfraz update packagers.md and kernel config check
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
cde9e8b
@jessfraz jessfraz hacky workaround for dockerinit static binary needing libseccomp.a foโ€ฆ
โ€ฆr debs and rpms

Signed-off-by: Jessica Frazelle <acidburn@docker.com>
ec6d339
@jessfraz jessfraz add docs
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
831af89
@moxiegirl
Contributor

@jfrazelle Ok, since it is going in --- I'll do the review now and turn it around this morning.

@moxiegirl moxiegirl commented on the diff Dec 4, 2015
docs/security/seccomp.md
@@ -0,0 +1,64 @@
+<!-- [metadata]>
++++
+title = "Seccomp security profiles for Docker"
+description = "Enabling seccomp in Docker"
+keywords = ["seccomp, security, docker, documentation"]
++++
+<![end-metadata]-->
+
+Seccomp security profiles for Docker
+------------------------------------
+
+The seccomp() system call operates on the Secure Computing (seccomp)
+state of the calling process.
+
+This operation is available only if the kernel is configured
@moxiegirl
moxiegirl Dec 4, 2015 Contributor

Is this a reference to the Docker Host kernel? It isn't clear.

@jessfraz
jessfraz Dec 4, 2015 Contributor

yes it is i can update

On Fri, Dec 4, 2015 at 11:04 AM, moxiegirl notifications@github.com wrote:

In docs/security/seccomp.md
#17989 (comment):

@@ -0,0 +1,64 @@
+
+
+Seccomp security profiles for Docker
+------------------------------------
+
+The seccomp() system call operates on the Secure Computing (seccomp)
+state of the calling process.
+
+This operation is available only if the kernel is configured

Is this a reference to the Docker Host kernel? It isn't clear.

โ€”
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/17989/files#r46718340.

@moxiegirl
moxiegirl Dec 4, 2015 Contributor

Hi Jess, don't worry, I'm updating it. I just wanted to check. In looking at the history are you attaching the flag --securty-opt to run or the daemon start? I need to update the command.

@thaJeztah
Member

@moxiegirl perhaps we can carry the documentation for this PR in a follow up, would that work for you?

@moxiegirl
Contributor

@jfrazelle @thaJeztah That works for me. Since the Apparmour material also goes in, I have to update that and some structural files.

@thaJeztah
Member

Sounds like a plan;

LGTM (and update docs in a follow up)

@jessfraz
Contributor
jessfraz commented Dec 5, 2015

Thanks!!!!!

@jessfraz jessfraz merged commit 87a614e into docker:master Dec 5, 2015

6 checks passed

docker/dco-signed All commits signed
Details
documentation success 2 tests run, 0 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 11819 has succeeded
Details
janky Jenkins build Docker-PRs 20586 has succeeded
Details
userns Jenkins build Docker-PRs-userns 3051 has succeeded
Details
windows Jenkins build Windows-PRs 18234 has succeeded
Details
@jessfraz jessfraz deleted the jessfraz:initial-seccomp-support branch Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment