Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change seccomp blacklist to a whitelist #18979

Merged
merged 8 commits into from Jan 6, 2016
Merged

Conversation

@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Dec 29, 2015

Running various tests on official images, but opening the discussion. ping @justincormack

@phemmer
Copy link
Contributor

@phemmer phemmer commented Dec 29, 2015

I was wondering why we weren't using a whitelist instead. The whitelist makes more sense to me for one main reason: default action is secure (http://stackoverflow.com/a/504424/486035).
With a blacklist, even if we blacklisted every possible syscall that could be abused, new kernels can come out which introduce new syscalls, and if docker's blacklist is old, it won't protect them.

👍

@jessfraz jessfraz added this to the 1.10 milestone Dec 29, 2015
@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Dec 29, 2015

does this mean you will compile seccomp into your binary now 0:)

@cpuguy83
Copy link
Collaborator

@cpuguy83 cpuguy83 commented Dec 30, 2015

SGTM

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Dec 30, 2015

Using a white list sounds like a better approach to me, so +1

Tentatively moving this to code review

@justincormack
Copy link
Contributor

@justincormack justincormack commented Dec 30, 2015

The clone rule needs fixing, will send patch. Ah sorry this was done I missed the commit.

I am slightly inclined to add back in the comments from the blacklisted syscalls as documentation of the ones not in the whitelist, not sure what anyone thinks.

@justincormack
Copy link
Contributor

@justincormack justincormack commented Dec 30, 2015

Ok I have tested this extensively: tested that individual syscalls are being correctly filtered, used it as my default docker install and built stuff (eg docker/docker, Linux kernel compile), double checked the syscall list for 32 bits, arm, powerpc. So LGTM.

For reference easiest way to check individual syscalls being filtered is:

docker run -it --cap-add ALL justincormack/ljsyscall -e "print(require('syscall').acct('/tmp/t'))"
nil Operation not permitted
docker run -it --cap-add ALL justincormack/ljsyscall -e "print(require('syscall').clone('newuts,newpid'))"
nil Operation not permitted
docker run -it --cap-add ALL --security-opt seccomp:unconfined justincormack/ljsyscall -e "print(require('syscall').acct('/tmp/t'))"
nil No such file or directory
docker run -it --cap-add ALL --security-opt seccomp:unconfined justincormack/ljsyscall -e "print(require('syscall').clone())"
11
0
@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Dec 30, 2015

Thanks I can add those as integration tests :) and I'm working on all new docs with the old comments as well

On Dec 30, 2015, 06:59 -0800, Justin Cormacknotifications@github.com, wrote:

Ok I have tested this extensively: tested that individual syscalls are being correctly filtered, used it as my default docker install and built stuff (eg docker/docker, Linux kernel compile), double checked the syscall list for 32 bits, arm, powerpc. So LGTM.

For reference easiest way to check individual syscalls being filtered is:

docker run -it --cap-add ALL justincormack/ljsyscall -e "print(require('syscall').acct('/tmp/t'))" nil Operation not permitted docker run -it --cap-add ALL justincormack/ljsyscall -e "print(require('syscall').clone('newuts,newpid'))" nil Operation not permitted docker run -it --cap-add ALL --security-opt seccomp:unconfined justincormack/ljsyscall -e "print(require('syscall').acct('/tmp/t'))" nil No such file or directory docker run -it --cap-add ALL --security-opt seccomp:unconfined justincormack/ljsyscall -e "print(require('syscall').clone())" 11 0


Reply to this email directly orview it on GitHub(#18979 (comment)).

@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Dec 30, 2015

opened additional tests here: #19002 and docs will come soon in another PR

@cpuguy83
Copy link
Collaborator

@cpuguy83 cpuguy83 commented Dec 31, 2015

Looks like you have settimeofday in the whitelist, shouldn't that be blocked?

@jessfraz jessfraz force-pushed the jessfraz:make-whitelist branch from 9356732 to f473b64 Dec 31, 2015
@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Dec 31, 2015

Good catch thanks

On Dec 30, 2015, 18:30 -0800, Brian Goffnotifications@github.com, wrote:

Looks like you havesettimeofdayin the whitelist, shouldn't that be blocked?


Reply to this email directly orview it on GitHub(#18979 (comment)).

@cpuguy83
Copy link
Collaborator

@cpuguy83 cpuguy83 commented Jan 4, 2016

LGTM

@jessfraz jessfraz force-pushed the jessfraz:make-whitelist branch from dcb0619 to 7632772 Jan 4, 2016
jessfraz and others added 4 commits Dec 29, 2015
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
…ndle_at

Being able to obtain a file handle is no use as we cannot perform
any operation in it, and it may leak kernel state.

Signed-off-by: Justin Cormack <justin.cormack@unikernel.com>
sysfs and ustat syscalls are marked obsolete.

Signed-off-by: Justin Cormack <justin.cormack@unikernel.com>
This is the newer verion of lseek on many 32 bit platforms

Signed-off-by: Justin Cormack <justin.cormack@unikernel.com>
@jessfraz jessfraz force-pushed the jessfraz:make-whitelist branch from 7632772 to 9236091 Jan 4, 2016
This is used on some 32 bit architectures, eg x86

Signed-off-by: Justin Cormack <justin.cormack@unikernel.com>
@justincormack
Copy link
Contributor

@justincormack justincormack commented Jan 5, 2016

Looking good after more testing. There is one issue that running compatible architectures is currently blocked (eg 32 bit x86 on x86_64). There is an easier fix or a nicer one, will look at it tomorrow, but need not stop merging. Sent fix to @jfrazelle

@calavera calavera closed this Jan 5, 2016
@calavera calavera reopened this Jan 5, 2016
@calavera
Copy link
Contributor

@calavera calavera commented Jan 5, 2016

sorry, wrong button 😄

@calavera
Copy link
Contributor

@calavera calavera commented Jan 5, 2016

That file looks quite long but whatever. LGTM

@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Jan 5, 2016

we could use go generate? I was thinking about that in the beginning

On Tue, Jan 5, 2016 at 9:22 AM, David Calavera notifications@github.com
wrote:

sorry, wrong button [image: 😄]


Reply to this email directly or view it on GitHub
#18979 (comment).

@justincormack
Copy link
Contributor

@justincormack justincormack commented Jan 5, 2016

Unfortunately Linux has too many syscalls, and it matches the libcontainer spec format, well the Go is a bit more readable. It is not ideal, but not sure there is much that can be done.

@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Jan 5, 2016

no i want it to be good :(

On Tue, Jan 5, 2016 at 9:23 AM, David Calavera notifications@github.com
wrote:

That file looks quite long but whatever. LGTM


Reply to this email directly or view it on GitHub
#18979 (comment).

In the default seccomp rule, allow use of 32 bit syscalls on
64 bit architectures, so you can run x86 Linux images on x86_64
without disabling seccomp or using a custom rule.

Signed-off-by: Justin Cormack <justin.cormack@unikernel.com>
This version is sometimes used eg by glibc on x86

Signed-off-by: Justin Cormack <justin.cormack@unikernel.com>
@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Jan 5, 2016

FWIW it will be easier to see the diff once its merged ;)

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jan 5, 2016

will merging make all red Janky's go green? 😇

@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Jan 5, 2016

i think it was becausse llvm repo was screwed up

@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Jan 6, 2016

green \o/

calavera added a commit that referenced this pull request Jan 6, 2016
Change seccomp blacklist to a whitelist
@calavera calavera merged commit 4b1872f into moby:master Jan 6, 2016
6 checks passed
6 checks passed
@GordonTheTurtle
docker/dco-signed All commits signed
Details
@docker-jenkins
documentation success 1 tests run, 1 skipped, 0 failed.
Details
@GordonTheTurtle
experimental Jenkins build Docker-PRs-experimental 13052 has succeeded
Details
@GordonTheTurtle
janky Jenkins build Docker-PRs 21847 has succeeded
Details
@GordonTheTurtle
userns Jenkins build Docker-PRs-userns 4295 has succeeded
Details
@GordonTheTurtle
windows Jenkins build Windows-PRs 19703 has succeeded
Details
@jessfraz jessfraz deleted the jessfraz:make-whitelist branch Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants