Add support for NoNewPrivileges in docker by mrunalp · Pull Request #20727 · moby/moby

mrunalp · 2016-02-26T16:46:22Z

Addresses #20329
This PR adds support for setting the NoNewPrivileges in docker as a security-opt.
Here is how to run docker with this bit set.

docker run -it --rm --security-opt=no-new-privileges fedora bash

Here is how to test it:

# Create a setuid binary that displays the effective uid
[root@dhcp-16-129 dockerfiles]# cat testnnp.c 
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>

int main(int argc, char *argv[])
{
        printf("Effective uid: %d\n", geteuid());
        return 0;
}
[root@dhcp-16-129 dockerfiles]# make testnnp
cc     testnnp.c   -o testnnp

# Add the binary to a docker image
[root@dhcp-16-129 dockerfiles]# cat Dockerfile 
FROM fedora:latest
ADD testnnp /root/testnnp
RUN chmod +s /root/testnnp
ENTRYPOINT /root/testnnp

[root@dhcp-16-129 dockerfiles]# /root/gosrc/src/github.com/docker/docker/bundles/latest/dynbinary/docker build -t testnnp .
Sending build context to Docker daemon 12.29 kB
Step 1 : FROM fedora:latest
 ---> 760a896a323f
Step 2 : ADD testnnp /root/testnnp
 ---> 6c700f277948
Removing intermediate container 0981144fe404
Step 3 : RUN chmod +s /root/testnnp
 ---> Running in c1215bfbe825
 ---> f1f07d05a691
Removing intermediate container c1215bfbe825
Step 4 : ENTRYPOINT /root/testnnp
 ---> Running in 5a4d324d54fa
 ---> 44f767c67e30
Removing intermediate container 5a4d324d54fa
Successfully built 44f767c67e30

# Running without no-new-privileges
root@dhcp-16-129 dockerfiles]# /root/gosrc/src/github.com/docker/docker/bundles/latest/dynbinary/docker run -it --rm --user=1000  testnnp
Effective uid: 0
# Running with no-new-privileges prevents the uid transition while running a setuid binary
[root@dhcp-16-129 dockerfiles]# /root/gosrc/src/github.com/docker/docker/bundles/latest/dynbinary/docker run -it --rm --user=1000 --security-opt=no-new-privileges testnnp
Effective uid: 1000

Signed-off-by: Mrunal Patel [email protected]

mrunalp · 2016-02-26T16:47:00Z

@crosbymichael @LK4D4 PTAL
cc: @rhatdan

jessfraz · 2016-03-03T20:04:25Z

I like it

jessfraz · 2016-03-03T20:06:45Z

Can you add that as a test case for your patch as well if others agree on the design

mrunalp · 2016-03-03T20:10:39Z

@jfrazelle Not sure how to integrate this into the docker test suite. We need to compile C code and add a binary into an image. Is there an example of anything similar?

jessfraz · 2016-03-03T20:14:04Z

YES the seccomp tests :)
https://github.com/docker/docker/tree/master/contrib/syscall-test
https://github.com/docker/docker/blob/master/hack/make/.ensure-syscall-test

On Thu, Mar 3, 2016 at 12:12 PM, Mrunal Patel [email protected]
wrote:

@jfrazelle https://github.com/jfrazelle Not sure how to integrate this
into the docker test suite. We need to compile C code and add a binary into
an image. Is there example of anything similar?

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

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

mrunalp · 2016-03-03T20:16:39Z

@jfrazelle Thanks :) I will look into it.

thaJeztah · 2016-03-03T21:01:53Z

@mrunalp we reviewed this in the maintainers review session and liked this feature. There was a short discussion (sorry, again) if --security-opt should be used or a different named flag, but we thought it was good to keep it like this. Thanks for implementing this!

calavera · 2016-03-03T21:26:27Z

Code LGTM.

Can we add some docs about this new option?

mrunalp · 2016-03-04T00:18:57Z

Added tests.

mrunalp · 2016-03-04T14:42:20Z

All green! :)

jessfraz · 2016-03-04T14:52:24Z

LGTM, can move to docs review thanks!

thaJeztah · 2016-03-04T21:58:09Z

Alright, this probably needs docs in;

https://github.com/docker/docker/blob/master/docs/reference/run.md

Not sure where to put it below
https://github.com/docker/docker/blob/master/docs/security/index.md

As it doesn't fit in apparmor or selinux, so do we need a separate document just for this? Can we expect more options for this?

mrunalp · 2016-03-04T21:59:24Z

@thaJeztah I just added a commit for docs.

mrunalp · 2016-03-04T22:00:23Z

@thaJeztah We can probably add a line for this in the same page. I will add it.

mrunalp · 2016-03-04T22:01:39Z

Actually the security page looks like it has configuration that is set at engine level for all containers while this option is more of a runtime flag.

thaJeztah · 2016-03-04T22:09:31Z

@mrunalp can this option be set on the daemon? i.e. as a default for all containers?

mrunalp · 2016-03-04T22:14:00Z

@thaJeztah We could add that feature later but it could potentially break images so better to opt-in at runtime to start with (as in this PR).

thaJeztah · 2016-03-05T00:34:11Z

@mrunalp oh, didn't mean to imply that, just wondering if it was a daemon level option as well

mrunalp · 2016-03-05T00:40:59Z

No it is not.

Sent from my iPhone

On Mar 4, 2016, at 4:35 PM, Sebastiaan van Stijn [email protected] wrote:

@mrunalp oh, didn't mean to imply that, just wondering if it was a daemon level option as well

—
Reply to this email directly or view it on GitHub.

icecrime · 2016-03-05T02:12:09Z

Can the windowsTP4 be related?

18:02:47 FAIL: dockerCmd_utils_test.go:270: DockerCmdSuite.TestDockerCmdInDirWithTimeout
18:02:47 
18:02:47 dockerCmd_utils_test.go:320:
18:02:47     c.Assert(out, check.Equals, cmd.expectedOut, check.Commentf("Expected output %q for arguments %v, got %q", cmd.expectedOut, cmd.args, out))
18:02:47 ... obtained string = ""
18:02:47 ... expected string = "hello"
18:02:47 ... Expected output "hello" for arguments [run -ti ubuntu echo hello], got ""
18:02:47 
18:02:47 OOPS: 7 passed, 1 FAILED

mrunalp · 2016-03-05T02:22:37Z

No, it can't be.

Sent from my iPhone

On Mar 4, 2016, at 6:13 PM, Arnaud Porterie [email protected] wrote:

Can the windowsTP4 be related?

18:02:47 FAIL: dockerCmd_utils_test.go:270: DockerCmdSuite.TestDockerCmdInDirWithTimeout
18:02:47
18:02:47 dockerCmd_utils_test.go:320:
18:02:47 c.Assert(out, check.Equals, cmd.expectedOut, check.Commentf("Expected output %q for arguments %v, got %q", cmd.expectedOut, cmd.args, out))
18:02:47 ... obtained string = ""
18:02:47 ... expected string = "hello"
18:02:47 ... Expected output "hello" for arguments [run -ti ubuntu echo hello], got ""
18:02:47
18:02:47 OOPS: 7 passed, 1 FAILED
—
Reply to this email directly or view it on GitHub.

thaJeztah · 2016-03-05T19:08:48Z

docs/reference/run.md

Can you move this above your example. Because this applies to the example above :-)

thaJeztah · 2016-03-05T19:11:03Z

Thanks @mrunalp I left some suggestions; also, can you squash your commits?

Signed-off-by: Mrunal Patel <[email protected]> Add tests for no-new-privileges Signed-off-by: Mrunal Patel <[email protected]> Update documentation for no-new-privileges Signed-off-by: Mrunal Patel <[email protected]>

mrunalp · 2016-03-07T17:48:30Z

@thaJeztah Updated and pushed

calavera · 2016-03-07T18:05:14Z

Docs LGTM

thaJeztah · 2016-03-07T18:11:38Z

docs LGTM, thanks @mrunalp!

mrunalp · 2016-03-08T00:19:57Z

The test failures don't look like they are related.

mrunalp · 2016-03-08T19:22:15Z

All green! Merge? :)

Add support for NoNewPrivileges in docker

thaJeztah · 2016-03-08T19:39:48Z

Thanks @mrunalp!

praving5 · 2016-03-26T08:41:50Z

Does this break apparmor as point here.

[--pids-limit](moby/moby#18697) [--security-opt=no-new-privileges](moby/moby#20727)

anshupande · 2016-05-20T18:06:25Z

core@ip-10-74-131-107 sandbox-ap-us-east-1a-control ~ $ docker run -it --rm --security-opt=no-new-privileges fedora bash Unable to find image 'fedora:latest' locally latest: Pulling from library/fedora 6888fc827a3f: Pull complete 9bdb5101e5fc: Pull complete Digest: sha256:1fa98be10c550ffabde65246ed2df16be28dc896d6e370dab56b98460bd27823 Status: Downloaded newer image for fedora:latest Error response from daemon: Invalid --security-opt: "no-new-privileges"

thaJeztah · 2016-05-20T21:07:49Z

@anshupande what version of docker are you running?

anshupande · 2016-05-20T21:32:08Z

@thaJeztah : yes it was 1.9 and not 1.11 and this was addressed #22862

GordonTheTurtle added the status/0-triage label Feb 26, 2016

mrunalp force-pushed the no_new_priv branch from 177cce6 to 668a869 Compare February 26, 2016 16:51

calavera added status/1-design-review and removed status/0-triage labels Feb 26, 2016

thaJeztah added status/2-code-review and removed status/1-design-review labels Mar 3, 2016

mrunalp force-pushed the no_new_priv branch from 668a869 to 10567a3 Compare March 3, 2016 23:47

jessfraz added status/3-docs-review and removed status/2-code-review labels Mar 4, 2016

thaJeztah added impact/changelog impact/cli labels Mar 4, 2016

thaJeztah added this to the 1.11.0 milestone Mar 4, 2016

thaJeztah reviewed Mar 5, 2016
View reviewed changes

Add support for NoNewPrivileges in docker

74bb1ce

Signed-off-by: Mrunal Patel <[email protected]> Add tests for no-new-privileges Signed-off-by: Mrunal Patel <[email protected]> Update documentation for no-new-privileges Signed-off-by: Mrunal Patel <[email protected]>

mrunalp force-pushed the no_new_priv branch from f1b012a to 74bb1ce Compare March 7, 2016 17:47

thaJeztah added status/4-merge and removed status/3-docs-review labels Mar 7, 2016

cpuguy83 added a commit that referenced this pull request Mar 8, 2016

Merge pull request #20727 from mrunalp/no_new_priv

dc702b6

Add support for NoNewPrivileges in docker

cpuguy83 merged commit dc702b6 into moby:master Mar 8, 2016

mrunalp mentioned this pull request Mar 14, 2016

BLOG: No New Privileges support in docker projectatomic/atomic-site#269

Closed

albers mentioned this pull request Mar 29, 2016

bash completion for `docker {run,create} --security-opt no-new-privileges #21610

Merged

geminiyellow added a commit to ilivebox/docker-cheat-sheet that referenced this pull request Apr 25, 2016

docs(zh-cn): Added new security features from docker 1.11

62710b7

[--pids-limit](moby/moby#18697) [--security-opt=no-new-privileges](moby/moby#20727)

tianon mentioned this pull request May 12, 2016

NoNewPrivileges support in docker #20329

Closed

anshupande mentioned this pull request May 20, 2016

Restrict container from acquiring additional privileges #22862

Closed

yosifkit mentioned this pull request May 8, 2019

Update "empty" password handling in "no-hard-coded-passwords" to balk at an empty root password explicitly (CVE-2019-5021) docker-library/official-images#5880

Merged

Conversation

mrunalp commented Feb 26, 2016

Uh oh!

mrunalp commented Feb 26, 2016

Uh oh!

jessfraz commented Mar 3, 2016

Uh oh!

jessfraz commented Mar 3, 2016

Uh oh!

mrunalp commented Mar 3, 2016

Uh oh!

jessfraz commented Mar 3, 2016

Uh oh!

mrunalp commented Mar 3, 2016

Uh oh!

thaJeztah commented Mar 3, 2016

Uh oh!

calavera commented Mar 3, 2016

Uh oh!

mrunalp commented Mar 4, 2016

Uh oh!

mrunalp commented Mar 4, 2016

Uh oh!

jessfraz commented Mar 4, 2016

Uh oh!

thaJeztah commented Mar 4, 2016

Uh oh!

mrunalp commented Mar 4, 2016

Uh oh!

mrunalp commented Mar 4, 2016

Uh oh!

mrunalp commented Mar 4, 2016

Uh oh!

thaJeztah commented Mar 4, 2016

Uh oh!

mrunalp commented Mar 4, 2016

Uh oh!

thaJeztah commented Mar 5, 2016

Uh oh!

mrunalp commented Mar 5, 2016

Uh oh!

icecrime commented Mar 5, 2016

Uh oh!

mrunalp commented Mar 5, 2016

Uh oh!

thaJeztah Mar 5, 2016

Choose a reason for hiding this comment

Uh oh!

mrunalp Mar 7, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 5, 2016

Uh oh!

mrunalp commented Mar 7, 2016

Uh oh!

calavera commented Mar 7, 2016

Uh oh!

thaJeztah commented Mar 7, 2016

Uh oh!

mrunalp commented Mar 8, 2016

Uh oh!

mrunalp commented Mar 8, 2016

Uh oh!

thaJeztah commented Mar 8, 2016

Uh oh!

praving5 commented Mar 26, 2016

Uh oh!

anshupande commented May 20, 2016

Uh oh!

thaJeztah commented May 20, 2016

Uh oh!

anshupande commented May 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!