Add NoNewPrivileges setting for linux by mrunalp · Pull Request #290 · opencontainers/runtime-spec

mrunalp · 2016-01-04T23:38:00Z

This is a security setting that could be used to prevent processes in the
container from gaining additional privileges.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

wking · 2016-01-04T23:52:12Z

Missing *bool? At some point I think we should land general specs for the:
Null and missing entries are valid JSON and mean “don't touch this”.
semantics . If we're not comfortable landing that generally, I think this PR needs docs about what runtimes should do if DisableNewPrivileges is null or unset in the JSON.

My suggested generic wording for null/missing is here.

wking · 2016-01-04T23:56:09Z

+1 to including this in the runtime, because setting it in the container process before invoking the user-configured code is both easier and safer (for runtime callers) than expecting bundle authors to handle this themselves.

mrunalp · 2016-01-05T17:56:45Z

@crosbymichael @LK4D4 @vbatts @philips PTAL

crosbymichael · 2016-01-05T18:08:49Z

Should this just be a default in a runtime?

vbatts · 2016-01-05T18:45:24Z

give the line item information on it here (that it's a bool and whether required)

vbatts · 2016-01-05T18:46:07Z

almost seems like it could be a default, but still likely that folks would want to opt-in to

mrunalp · 2016-01-05T18:57:39Z

@crosbymichael @vbatts This tightens up the security but has some downsides if enabled by default. For e.g. ping won't work if your container doesn't already have the net_raw cap. So, I think opt-in might be better for this feature.

rhatdan · 2016-01-05T19:12:35Z

Somewhat equivalent to

docker run --cap-drop=all -u UID -g GID.

I am trying to find if there is any other differences.

So as @mrunalp says if you just ran
docker run -u UID fedora sh

Then tools like ping, chsh, traceroute etc could theoretically work.

mheon · 2016-01-05T19:28:28Z

@rhatdan It does have an affect on how layered Seccomp filters are treated, so that would be one thing that your example doesn't cover.

It would be nice to set this to be enabled by default, or at least note that passing a Seccomp configuration implies that it is enabled, given that it has a beneficial relationship with applications in a container that also use Seccomp.

wking · 2016-01-05T20:37:40Z

On Tue, Jan 05, 2016 at 11:12:38AM -0800, Daniel J Walsh wrote:

Somewhat equivalent to
docker run --cap-drop=all -u UID -g GID.
I am trying to find if there is any other differences.

I don't think it's equivalent. The point of no_new_privs (as outlined
in the kernel docs 1) is to close the execve loopholes that are
generally allowed for priviledge escallation. E.g. after dropping
caps, you can still execute a setuid binary and increase your
privileges. If you want to experiment some, I implemented this option
in ccon last night 2.

Re, opt-in or opt-out, I think “least surprise” points to opt-in.
Otherwise I expect a lot of “why don't my setuid binaries work” bug
reports. And I also prefer opt-in as a general principle 3.

philips · 2016-01-06T10:37:34Z

is there a reason to not call it noNewPrivileges so it matches the kernel version?

philips · 2016-01-06T10:37:51Z

Why not have a generic prctl field?

rhatdan · 2016-01-06T14:07:05Z

Any way we can get it in, I am all for.

I am torn on the OptIn versus OptOut, OptIn will probably be required. The problem with OptIn, is as I have often said, almost no one ever increases the security. Most executers of containers using -u UID probably expect that the process inside of the container can not become a different UID or get any privileges. But they would need to understand, to get their expected behaviour they would need to do

docker run --nonewpriv --cap-drop=all -u UID ...

Note: --cap-=drop=all might not be needed above.

Have --prctl=NO_NEW_PRIVS=1 might allow us to easily add additional prctl in the furture.

philips · 2016-01-06T16:14:58Z

On Wed, Jan 6, 2016 at 7:37 PM Daniel J Walsh notifications@github.com
wrote:

Have --prctl=NO_NEW_PRIVS=1 might allow us to easily add additional prctl
in the furture
The more I think about it the more I really want to avoid adding new terms
to this spec for Linux. Calling a prctl with the name NO_NEW_PRIVS
disableNewPrivelges reduces discoverability and opens the door for
"expanding behavior".

mrunalp · 2016-01-06T16:27:14Z

Downside of using prctl is that not all prctl options make sense for run times. For e.g. There is one for making the parent a reaper but it doesn't make sense for all runtime to support that.

Sent from my iPhone

On Jan 6, 2016, at 8:14 AM, Brandon Philips notifications@github.com wrote:
On Wed, Jan 6, 2016 at 7:37 PM Daniel J Walsh notifications@github.com
wrote:
Have --prctl=NO_NEW_PRIVS=1 might allow us to easily add additional prctl
in the furture
The more I think about it the more I really want to avoid adding new terms
to this spec for Linux. Calling a prctl with the name NO_NEW_PRIVS
disableNewPrivelges reduces discoverability and opens the door for
"expanding behavior".
—
Reply to this email directly or view it on GitHub.

mrunalp · 2016-01-06T16:34:50Z

We could however start with a whitelist of what options are supported.

Sent from my iPhone

On Jan 6, 2016, at 8:27 AM, Mrunal Patel mrunal@me.com wrote:
Downside of using prctl is that not all prctl options make sense for run times. For e.g. There is one for making the parent a reaper but it doesn't make sense for all runtime to support that.
Sent from my iPhone
On Jan 6, 2016, at 8:14 AM, Brandon Philips notifications@github.com wrote:
On Wed, Jan 6, 2016 at 7:37 PM Daniel J Walsh notifications@github.com
wrote:
Have --prctl=NO_NEW_PRIVS=1 might allow us to easily add additional prctl
in the furture
The more I think about it the more I really want to avoid adding new terms
to this spec for Linux. Calling a prctl with the name NO_NEW_PRIVS
disableNewPrivelges reduces discoverability and opens the door for
"expanding behavior".
—
Reply to this email directly or view it on GitHub.

rhatdan · 2016-01-07T14:44:53Z

We have the same problem with --sysctl in that some sysctls are namespaced and some are not. Our pull requests are blocked because a user could shoot himself in the head. We have recently proposed a whitelist approach for runc to allow us to get --sysctl in. But bottom line, is I believe we need sysctl in and I believe we need NO_NEW_PRIV support, however we can get it in.

One potential option from a docker point of view would be

docker run --security-opt privs:NO_NEW_PRIV:1

Which would allow us to add a new security feature without messy-ing up the CLI, too much.

dqminh · 2016-01-07T15:48:49Z

@mrunalp @philips i think prctls + a required list of options works here rather than adding a new fields. Runtime can choose to support additional prctl options.

rhatdan · 2016-01-13T14:55:14Z

Any other comments on this? prctl probably makes sense for runc, then we can argue about docker CLI.

wking · 2016-01-13T17:33:01Z

On Wed, Jan 13, 2016 at 06:55:15AM -0800, Daniel J Walsh wrote:

Any other comments on this? prctl probably makes sense for runc…

Yeah, I'm fine with a generic prctl setting.

crosbymichael · 2016-01-13T18:22:07Z

Negative on the generic prctl setting. That is super horrible api and is the first step going down a dark path of

"syscall": [
[0,2,3,4,1,0]
]

wking · 2016-01-13T18:39:19Z

On Wed, Jan 13, 2016 at 10:22:10AM -0800, Michael Crosby wrote:

Negative on the generic prctl setting…

The problem with adding prctl wrappers (like disableNewPrivileges) one
at a time is that we end up in the same situation that we have now for
cgroups: a thin wrapper around the kernel that takes work to maintain
and adds no value 1. The difference vs. cgroups is that most (all?)
prctl invocations act on the calling thread/process, so it doesn't
decouple as nicely as cgroups (where you can use hooks outside of the
namespaced-process setup dance). Having a generic prctl setting is
the next closest thing we can do to “keep that out of the spec”, and
it allows higher-level tooling to add thin or thick wrappers as they
see fit 2.

And yeah, you could do all of that with a generic syscall wrapper,
but I think everyone agrees that that tips the balance too far (really
easy to write runtimes, but really hard to use them). While for a
generic prctl setting doesn't seem to be much of a usability cost (and
may be more usable 3) over per-prctl wrapper settings. If somebody
has a brilliant idea for a general abstraction that covers
no_new_privs without being a thin wrapper, then that would be a
usability win. But that's not what we have here.

 Subject: removal of cgroups from the OCI Linux spec
Date: Wed, 28 Oct 2015 17:01:59 +0000
Message-ID: <CAD2oYtO1RMCcUp52w-xXemzDTs+J6t4hS5Mm4mX+uBnVONGDfA@mail.gmail.com>

mrunalp · 2016-01-13T19:35:51Z

I agree with @crosbymichael on this. NO_NEW_PRIVS is specific and closes a lot of holes around priviliege escalation and I think it makes sense to have switch for it. Do we have an example for any other prctl settings that make sense to be exposed as a counter argument?

rhatdan · 2016-01-13T19:48:28Z

Some of the other prctl are interesting but they seem to be handled via capability handling and seccomp.

I still think we could handle NO_NEW_PRIVS via SecurityOptions.

wking · 2016-01-13T19:51:19Z

On Wed, Jan 13, 2016 at 11:35:54AM -0800, Mrunal Patel wrote:

I agree with @crosbymichael on this. NO_NEW_PRIVS is specific and
closes a lot of holes around priviliege escalation and I think it
makes sense to have switch for it. Do we have an example for any
other prctl settings that make sense to be exposed as a counter
argument?

I'm currently hard-coding PR_SET_PDEATHSIG to SIGKILL in ccon, but
that could potentially be user configurable. And grepping for PR_SET_
in util-linux turns up setpriv(1) with PR_SET_NO_NEW_PRIVS,
PR_SET_KEEPCAPS, and PR_SET_SECUREBITS.

estesp · 2016-01-13T21:48:31Z

not sure if golint will ever be run, but I think it will want the comment to match the name (capital "D" for DisableNewPrivileges)

vishh · 2016-01-20T18:29:11Z

If the system default false? If yes, then can we drop the * here?

Yes. Removed the pointer.

mrunalp · 2016-01-20T19:29:57Z

Renamed to NoNewPrivileges. PTAL.

This is a security setting that could be used to prevent processes in the container from gaining additional privileges. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

vbatts · 2016-01-20T22:03:54Z

LGTM

vishh · 2016-01-20T22:11:16Z

Should the default be true ?

nope
On Wed, Jan 20, 2016 at 5:11 PM Vish Kannan notifications@github.com
wrote:
In runtime-config-linux.md
#290 (comment):
@@ -503,3 +503,14 @@ Its value is either slave, private, or shared.
"rootfsPropagation": "slave",
+## No new privileges
+
+Setting noNewPrivileges to true prevents the processes in the container from gaining additional privileges.
Should the default be true ?
—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/290/files#r50328023.

Why not?
On Wed, Jan 20, 2016 at 2:14 PM, Vincent Batts notifications@github.com
wrote:
In runtime-config-linux.md
#290 (comment):
@@ -503,3 +503,14 @@ Its value is either slave, private, or shared.
"rootfsPropagation": "slave",
+## No new privileges
+
+Setting noNewPrivileges to true prevents the processes in the container from gaining additional privileges.
nope
… <#1443586824_>
On Wed, Jan 20, 2016 at 5:11 PM Vish Kannan notifications@github.com
wrote: In runtime-config-linux.md <#290 (comment)
https://github.com/opencontainers/specs/pull/290#discussion_r50328023>:
@@ -503,3 +503,14 @@ Its value is either slave, private, or shared. >
json > "rootfsPropagation": "slave", > > + > +## No new privileges >
+Setting noNewPrivileges to true prevents the processes in the
container from gaining additional privileges. Should the default be true ?
— Reply to this email directly or view it on GitHub <
https://github.com/opencontainers/specs/pull/290/files#r50328023>.
—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/290/files#r50328465.

On Wed, Jan 20, 2016 at 02:15:18PM -0800, Vish Kannan wrote: > +Setting `noNewPrivileges` to true prevents the processes in the > container from gaining additional privileges. Why not?
Search for ‘opt-’ in the PR discussion to get previous opt-in vs. opt-out thoughts ;).

Acknowledged.

vishh · 2016-01-20T22:19:08Z

LGTM

Add NoNewPrivileges setting for linux

philips · 2016-01-21T01:14:28Z

Thank you, LGTM

On Wed, Jan 20, 2016 at 11:30 AM Mrunal Patel notifications@github.com
wrote:

Renamed to NoNewPrivileges. PTAL.
—
Reply to this email directly or view it on GitHub
#290 (comment).

Brought to my attention in [1]. [1]: opencontainers/runtime-spec#290

wking reviewed Jan 4, 2016
View reviewed changes

mrunalp force-pushed the no_new_priv branch from 1f022ec to c05575cCompare January 5, 2016 00:03

vbatts reviewed Jan 5, 2016
View reviewed changes

Comment threadruntime-config-linux.md Outdated
vbattsJan 5, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give the line item information on it here (that it's a bool and whether required)

mrunalp force-pushed the no_new_priv branch from c05575c to 7e542a6Compare January 6, 2016 00:30

philips reviewed Jan 6, 2016
View reviewed changes

mrunalp added this to the v0.3.0 milestone Jan 13, 2016

estesp reviewed Jan 13, 2016
View reviewed changes

vishh reviewed Jan 20, 2016
View reviewed changes

mrunalp force-pushed the no_new_priv branch from 7e542a6 to ce3e797Compare January 20, 2016 19:28

mrunalp changed the title ~~Add DisableNewPrivileges setting for linux~~Add NoNewPrivileges setting for linuxJan 20, 2016

Add NoNewPrivileges setting for linux
5f327ba
This is a security setting that could be used to prevent processes in the container from gaining additional privileges. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp force-pushed the no_new_priv branch from ce3e797 to 5f327baCompare January 20, 2016 19:30

vishh reviewed Jan 20, 2016
View reviewed changes

vbatts added a commit that referenced this pull request Jan 20, 2016
Merge pull request#290from mrunalp/no_new_priv
acc1c63
Add NoNewPrivileges setting for linux

vbatts merged commit acc1c63 into opencontainers:masterJan 20, 2016

wking mentioned this pull request Jan 25, 2016
config: Single, unified config file #284
Merged

wking mentioned this pull request Jan 26, 2016
style: Document recent Go-pointer exceptions #317
Merged

mrunalp mentioned this pull request Mar 14, 2016
BLOG: No New Privileges support in docker projectatomic/atomic-site#269
Closed

vbatts mentioned this pull request Apr 8, 2016
Anchor examples #348
Merged

wking added a commit to wking/ccon that referenced this pull request May 13, 2016
ccon: Add process.disableNewPrivileges
948b14f
Brought to my attention in [1]. [1]: opencontainers/runtime-spec#290

cyphar mentioned this pull request Dec 19, 2016
Create no-new-privs support proposal doc. kubernetes/community#180
Closed

wking mentioned this pull request Apr 15, 2017
specs-go: Consistent pointer policy for invalid-value detection? #772
Open

caleblloyd mentioned this pull request Mar 18, 2018
fixuid must be owned by user 'root' and include the setuid bit boxboat/fixuid#9
Closed

Uh oh!

Conversation

mrunalp commented Jan 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Jan 4, 2016 via email

Uh oh!

mrunalp commented Jan 5, 2016

Uh oh!

crosbymichael commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Jan 5, 2016

Uh oh!

mrunalp commented Jan 5, 2016

Uh oh!

rhatdan commented Jan 5, 2016

Uh oh!

mheon commented Jan 5, 2016

Uh oh!

wking commented Jan 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philips commented Jan 6, 2016

Uh oh!

rhatdan commented Jan 6, 2016

Uh oh!

philips commented Jan 6, 2016

Uh oh!

mrunalp commented Jan 6, 2016

Uh oh!

mrunalp commented Jan 6, 2016

Uh oh!

rhatdan commented Jan 7, 2016

Uh oh!

dqminh commented Jan 7, 2016

Uh oh!

rhatdan commented Jan 13, 2016

Uh oh!

wking commented Jan 13, 2016

Uh oh!

crosbymichael commented Jan 13, 2016

Uh oh!

wking commented Jan 13, 2016

Uh oh!

mrunalp commented Jan 13, 2016

Uh oh!

rhatdan commented Jan 13, 2016

Uh oh!

wking commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Jan 20, 2016

Uh oh!

vbatts commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkingJan 20, 2016 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishh commented Jan 20, 2016

Uh oh!

philips commented Jan 21, 2016