Add support for NoNewPrivileges in docker

[mrunalp]

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 mrunalp@gmail.com

[mrunalp]

@crosbymichael @LK4D4 PTAL
cc: @rhatdan

[jessfraz]

I like it

[jessfraz]

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

[mrunalp]

@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]

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 notifications@github.com
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]

@jfrazelle Thanks :) I will look into it.

[thaJeztah]

@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]

Code LGTM.

Can we add some docs about this new option?

[mrunalp]

Added tests.

[mrunalp]

All green! :)

[jessfraz]

LGTM, can move to docs review thanks!

[thaJeztah]

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]

@thaJeztah I just added a commit for docs.

[mrunalp]

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

[mrunalp]

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]

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

[mrunalp]

@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]

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

[mrunalp]

No it is not.

Sent from my iPhone

On Mar 4, 2016, at 4:35 PM, Sebastiaan van Stijn notifications@github.com 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]

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]

No, it can't be.

Sent from my iPhone

On Mar 4, 2016, at 6:13 PM, Arnaud Porterie notifications@github.com 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]

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

[mrunalp]

Fixed.

[thaJeztah]

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

[mrunalp]

@thaJeztah Updated and pushed

[calavera]

Docs LGTM

[thaJeztah]

docs LGTM, thanks @mrunalp!

[mrunalp]

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

[mrunalp]

All green! Merge? :)

[thaJeztah]

Thanks @mrunalp!

[praving5]

Does this break apparmor as point here.

[anshupande]

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]

@anshupande what version of docker are you running?

[anshupande]

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