userns: honour no_new_privs for cap_bset during user ns creation/switch

From: Maciej ?enczykowski <[email protected]>

This allows locking down user namespaces tighter,
and it could even be considered a security fix.

Signed-off-by Maciej ?enczykowski <[email protected]>

---
 kernel/user_namespace.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

kernel/user_namespace.c +2 -0

index 246d4d4ce5c7..2354f7ade78a 100644
@@ -50,11 +50,12 @@ static void set_cred_user_ns(struct cred *cred, struct user_namespace *user_ns)
 	 * anything as the capabilities are bound to the new user namespace.
 	 */
 	cred->securebits = SECUREBITS_DEFAULT;
+	cred->cap_bset = task_no_new_privs(current) ? current_cred()->cap_bset
+						    : CAP_FULL_SET;

No. This makes no logical sense.

A task that enters a user namespace loses all capabilities to everything outside of the user namespace. Capabilities inside a user namespace are only valid for objects created inside that user namespace.

So limiting capabilities inside a user namespace when the capability bounding set is already fully honored by not giving the processes any of those capabilities makes no logical sense.

If the concern is kernel attack surface versus logical permissions we can look at ways to reduce the attack surface but that needs to be fully discussed in the change log.

Quoted 6 lines · Eric W. Biederman

No. This makes no logical sense.

A task that enters a user namespace loses all capabilities to everything outside of the user namespace. Capabilities inside a user namespace are only valid for objects created inside that user namespace.

Quoted 7 lines

So limiting capabilities inside a user namespace when the capability bounding set is already fully honored by not giving the processes any of those capabilities makes no logical sense.

If the concern is kernel attack surface versus logical permissions we can look at ways to reduce the attack surface but that needs to be fully discussed in the change log.

Here's an example of using user namespaces to read a file you shouldn't be able to.

lpk19:~# uname -r
4.15.0-smp-d1ce8ceb8ba8

(we start as true global root)

lpk19:~# id
uid=0(root) gid=0(root) groups=0(root)

(cleanup after previous run)

lpk19:~# cd /; chattr -i /immu; rm -f /immu/log; rmdir /immu

(now we create an append only logfile owned by target user:group)

lpk19:~# cd /; mkdir /immu; touch /immu/log; chown produser:prod
/immu/log; chmod a-rwx,u+w /immu/log; chattr +a /immu/log

(let's show what things look like)

lpk19:~# chattr +i /immu; ls -ld / /immu /immu/log; lsattr -d / /immu /immu/log
drwxr-xr-x 22 root root 4096 Dec 21 16:33 /
drwxr-xr-x 2 root root 4096 Dec 21 16:23 /immu
--w------- 1 produser prod 0 Dec 21 16:23 /immu/log
-----------I--e---- /
----i---------e---- /immu
-----a--------e---- /immu/log

(the immutable bit prevents us from changing permissions on the file)

lpk19:/# chmod a+rwx /immu/log
chmod: changing permissions of '/immu/log': Operation not permitted

(the append only bit prevents us from simply overwriting the file)

lpk19:/# echo log1 > /immu/log
-bash: /immu/log: Operation not permitted

(but we can append to it)

lpk19:/# echo log1 >> /immu/log

(we're global root with CAP_DAC_OVERRIDE, so we can *still* read it)

lpk19:/# cat /immu/log
log1

(let's transition to target user)

lpk19:/# su - produser

produser@lpk19:~$ id
uid=2080(produser) gid=620(prod) groups=620(prod)

(we can't overwrite it)

produser@lpk19:~$ echo log2 > /immu/log
-su: /immu/log: Operation not permitted

(but we can log to it: as intended)

produser@lpk19:~$ echo log2 >> /immu/log

(we can't change its permissions, cause it's in an immutable directory)

produser@lpk19:~$ chmod u+r /immu/log
chmod: changing permissions of '/immu/log': Operation not permitted

(we can't dump the file, cause we don't have CAP_DAC_OVERRIDE)

produser@lpk19:~$ cat /immu/log
cat: /immu/log: Permission denied

(or can we?)

produser@lpk19:~$ unshare -U -r cat /immu/log
log1
log2

----

Now, of course, the above patch doesn't actually fix this on it's own, since 'su' doesn't (yet?) know to restrict bset or to set no_new_privs.

But: it allows the sandbox equivalent of su to drop CAP_DAC_OVERRIDE from it's inh/eff/perm/ambient/bset, and set no_new_privs. Now the unshare won't gain CAP_DAC_OVERRIDE and won't be able to cat the non-readable append-only log file.

IMHO the point of having a capability bounding set and/or no_new_privs is to never be able to regain capabilities.
Note also that 'no_new_privs' isn't cleared across a unshare(CLONE_NEWUSER) [presumably also applies to setns()].

We can of course argue the implementation details (for example instead of using the existing no_new_privs flag, add a new keep_bset_across_userns_transitions securebits flag)... but *something* has to be done.

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

Given little things like that I can completely see no_new_privs meaning you can't create a user namespace. That seems consistent with the meaning and philosophy of no_new_privs. So simple it is hard to get wrong.

We could do more clever things like plug this whole in user namespaces, and that would not hurt my feelings.

However unless that is our only
choice to avoid badly breaking userspace I would have to have to depend on user namespaces being perfect for no_new_privs to be a proper jail.

As a general rule user namespaces are where we tackle the subtle scary things that should work, and no_new_privs is where we implement a simple hard to get wrong jail. Most of the time the effect is the same to an outside observer (bounded permissions), but there is a real difference in difficulty of implementation.

Quoted 5 lines

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

Append-only files are useful - particularly for logging. It could also simply be a non-readable file on a R/O filesystem.

Quoted 4 lines

Given little things like that I can completely see no_new_privs meaning you can't create a user namespace. That seems consistent with the meaning and philosophy of no_new_privs. So simple it is hard to get wrong.

Yes, I could totally buy the argument that no_new_privs should prevent creating a user ns.

However, there's also setns() and that's a fair bit harder to reason about. Entirely deny it? But that actually seems potentially useful... Allow it but cap it? That's what this does...

Quoted 2 lines

We could do more clever things like plug this whole in user namespaces, and that would not hurt my feelings.

Sure, this particular one wouldn't be all that easy I think... and how many such holes are there?
I found this particular one *after* your first reply in this thread.

Quoted 3 lines

However unless that is our only
choice to avoid badly breaking userspace I would have to have to depend on user namespaces being perfect for no_new_privs to be a proper jail.

This stuff is ridiculously complex to get right from userspace. :-(

Quoted 5 lines

As a general rule user namespaces are where we tackle the subtle scary things that should work, and no_new_privs is where we implement a simple hard to get wrong jail. Most of the time the effect is the same to an outside observer (bounded permissions), but there is a real difference in difficulty of implementation.

So, where to now...

Would you accept patches that:

- make no_new_priv block user ns creation?

- make no_new_priv block user ns transition?

Or perhaps we can assume that lack of create privs is sufficient, and if there's a pre-existing user ns for you to enter, then that's acceptable...
Although this implies you probably always want to combine no_new_privs with a leaf user ns, or no_new_privs isn't all that useful for root in root ns...
This added complexity, probably means it should be blocked...

- inherits bset across user ns creation/transition based on X? [this is the one we care about, because there are simply too many bugs in the kernel wrt. certain caps]

X could be:
- a new flag similar to no_new_priv
- a new securebit flag (w/lockbit) [provided securebits survive a userns transition, haven't checked]
- or perhaps a new capability
- something else?

How do we make forward progress?

Quoted 46 lines · Maciej ?enczykowski

Quoted 5 lines

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

Append-only files are useful - particularly for logging. It could also simply be a non-readable file on a R/O filesystem.

Quoted 4 lines

Given little things like that I can completely see no_new_privs meaning you can't create a user namespace. That seems consistent with the meaning and philosophy of no_new_privs. So simple it is hard to get wrong.

Yes, I could totally buy the argument that no_new_privs should prevent creating a user ns.

However, there's also setns() and that's a fair bit harder to reason about. Entirely deny it? But that actually seems potentially useful... Allow it but cap it? That's what this does...

Quoted 2 lines

We could do more clever things like plug this whole in user namespaces, and that would not hurt my feelings.

Sure, this particular one wouldn't be all that easy I think... and how many such holes are there?
I found this particular one *after* your first reply in this thread.

Quoted 3 lines

However unless that is our only
choice to avoid badly breaking userspace I would have to have to depend on user namespaces being perfect for no_new_privs to be a proper jail.

This stuff is ridiculously complex to get right from userspace. :-(

Quoted 5 lines

As a general rule user namespaces are where we tackle the subtle scary things that should work, and no_new_privs is where we implement a simple hard to get wrong jail. Most of the time the effect is the same to an outside observer (bounded permissions), but there is a real difference in difficulty of implementation.

So, where to now...

Would you accept patches that:

- make no_new_priv block user ns creation?

Quoted 1 line

- make no_new_priv block user ns transition?

Yes.

The approach will need to be rethought if there is anything deliberately combining user namespaces and no_new_privs. As regressions are a no-no. So we need wide spread testing, to avoid that.

But as much as possible I want no_new_privs to be simple and doing it's job.

I will also take and encourage patches that close this minor privilege escalation from the user namespace side. As ideally creating a user namespace should be as safe as no_new_privs.

Quoted 7 lines

Or perhaps we can assume that lack of create privs is sufficient, and if there's a pre-existing user ns for you to enter, then that's acceptable...
Although this implies you probably always want to combine no_new_privs with a leaf user ns, or no_new_privs isn't all that useful for root in root ns...
This added complexity, probably means it should be blocked...

Yes.

Quoted 3 lines

- inherits bset across user ns creation/transition based on X? [this is the one we care about, because there are simply too many bugs in the kernel wrt. certain caps]

That was my suspicion, and attack surface reduction is a different discussion. Would no_new_privs preventing a userns transition be enough for the cases you care about?

Otherwise this is a different conversation because it is not about semantics but about making the code safer to use. In general if code is simply not safe to user in a user namespace I would prefer to tighten the permission checks, and just not allow that code.

Mostly what I have seen in previous conversations is simply concerns about code that is not used or needed, being a problem.

Quoted 7 lines

X could be:
- a new flag similar to no_new_priv
- a new securebit flag (w/lockbit) [provided securebits survive a userns transition, haven't checked]
- or perhaps a new capability
- something else?

Quoted 1 line

How do we make forward progress?

We start by causing no_new_privs to block userns creation and entering.

Quoted 46 lines · Maciej Żenczykowski

Quoted 5 lines

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

Append-only files are useful - particularly for logging. It could also simply be a non-readable file on a R/O filesystem.

Quoted 4 lines

Given little things like that I can completely see no_new_privs meaning you can't create a user namespace. That seems consistent with the meaning and philosophy of no_new_privs. So simple it is hard to get wrong.

Yes, I could totally buy the argument that no_new_privs should prevent creating a user ns.

However, there's also setns() and that's a fair bit harder to reason about. Entirely deny it? But that actually seems potentially useful... Allow it but cap it? That's what this does...

Quoted 2 lines

We could do more clever things like plug this whole in user namespaces, and that would not hurt my feelings.

Sure, this particular one wouldn't be all that easy I think... and how many such holes are there?
I found this particular one *after* your first reply in this thread.

Quoted 3 lines

However unless that is our only
choice to avoid badly breaking userspace I would have to have to depend on user namespaces being perfect for no_new_privs to be a proper jail.

This stuff is ridiculously complex to get right from userspace. :-(

Quoted 5 lines

As a general rule user namespaces are where we tackle the subtle scary things that should work, and no_new_privs is where we implement a simple hard to get wrong jail. Most of the time the effect is the same to an outside observer (bounded permissions), but there is a real difference in difficulty of implementation.

So, where to now...

Would you accept patches that:

- make no_new_priv block user ns creation?

Quoted 1 line

- make no_new_priv block user ns transition?

Yes.

The approach will need to be rethought if there is anything deliberately combining user namespaces and no_new_privs. As regressions are a no-no. So we need wide spread testing, to avoid that.

But as much as possible I want no_new_privs to be simple and doing it's job.

I will also take and encourage patches that close this minor privilege escalation from the user namespace side. As ideally creating a user namespace should be as safe as no_new_privs.

Quoted 7 lines

Or perhaps we can assume that lack of create privs is sufficient, and if there's a pre-existing user ns for you to enter, then that's acceptable...
Although this implies you probably always want to combine no_new_privs with a leaf user ns, or no_new_privs isn't all that useful for root in root ns...
This added complexity, probably means it should be blocked...

Yes.

Quoted 3 lines

- inherits bset across user ns creation/transition based on X? [this is the one we care about, because there are simply too many bugs in the kernel wrt. certain caps]

That was my suspicion, and attack surface reduction is a different discussion. Would no_new_privs preventing a userns transition be enough for the cases you care about?

Otherwise this is a different conversation because it is not about semantics but about making the code safer to use. In general if code is simply not safe to user in a user namespace I would prefer to tighten the permission checks, and just not allow that code.

Mostly what I have seen in previous conversations is simply concerns about code that is not used or needed, being a problem.

Quoted 7 lines

X could be:
- a new flag similar to no_new_priv
- a new securebit flag (w/lockbit) [provided securebits survive a userns transition, haven't checked]
- or perhaps a new capability
- something else?

Quoted 1 line

How do we make forward progress?

We start by causing no_new_privs to block userns creation and entering.

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

Given little things like that I can completely see no_new_privs meaning you can't create a user namespace. That seems consistent with the meaning and philosophy of no_new_privs. So simple it is hard to get wrong.

We could do more clever things like plug this whole in user namespaces, and that would not hurt my feelings.

However unless that is our only
choice to avoid badly breaking userspace I would have to have to depend on user namespaces being perfect for no_new_privs to be a proper jail.

As a general rule user namespaces are where we tackle the subtle scary things that should work, and no_new_privs is where we implement a simple hard to get wrong jail. Most of the time the effect is the same to an outside observer (bounded permissions), but there is a real difference in difficulty of implementation.

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

Quoted 5 lines · Eric W. Biederman

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

This problem reminds me of the whole "unmapped group" problem. If you have access to a file through an unmapped group you can still access a file -- which to me is wrong. I understand the need for checking unmapped groups in order to fix the "chmod 707" problem, but I think that unmapped groups should only *block* access and never *grant* it.

I was working on a patch for that issue a while ago but it touched more VFS than I was comfortable with. Eric, is that a fix you would be interested in?

Quoted 5 lines · Eric W. Biederman

Good point about CAP_DAC_OVERRIDE on files you own.

I think there is an argument that you are playing dangerous games with the permission system there, as it isn't effectively a file you own if you can't read it, and you can't change it's permissions.

This problem reminds me of the whole "unmapped group" problem. If you have access to a file through an unmapped group you can still access a file -- which to me is wrong. I understand the need for checking unmapped groups in order to fix the "chmod 707" problem, but I think that unmapped groups should only *block* access and never *grant* it.

I was working on a patch for that issue a while ago but it touched more VFS than I was comfortable with. Eric, is that a fix you would be interested in?

No. This makes no logical sense.

A task that enters a user namespace loses all capabilities to everything outside of the user namespace. Capabilities inside a user namespace are only valid for objects created inside that user namespace.

So limiting capabilities inside a user namespace when the capability bounding set is already fully honored by not giving the processes any of those capabilities makes no logical sense.

If the concern is kernel attack surface versus logical permissions we can look at ways to reduce the attack surface but that needs to be fully discussed in the change log.

From: Maciej Żenczykowski <[email protected]>

This allows locking down user namespaces tighter,
and it could even be considered a security fix.

Signed-off-by Maciej Żenczykowski <[email protected]>

---
 kernel/user_namespace.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

kernel/user_namespace.c +2 -0

index 246d4d4ce5c7..2354f7ade78a 100644
@@ -50,11 +50,12 @@ static void set_cred_user_ns(struct cred *cred, struct user_namespace *user_ns)
 	 * anything as the capabilities are bound to the new user namespace.
 	 */
 	cred->securebits = SECUREBITS_DEFAULT;
+	cred->cap_bset = task_no_new_privs(current) ? current_cred()->cap_bset
+						    : CAP_FULL_SET;