user namespaces: fix some uid/privilege leaks

From: "Serge E. Hallyn" <[email protected]>

A few modules are using cap_raised(current_cap(), cap) to authorize actions.  This means that tasks which are privileged in non-initial user namespaces will be deemed privileged.  The privilege should only be granted if the task is in the initial user namespace.

Switching the calls to capable() would change the behavior - it would cause the LSM capable hooks to be called, and set PF_SUPERPRIV if the capability was used.  So instead, put in an explicit check and refuse privilege if the caller is not in init_user_ns.

Vasiliy had suggested introducing a new helper for this.  I'm open to suggestions, but for four callers and for a discouraged idiom, I'd rather not pollute the capable* function namespace with a bad name. (even has_capability goes through the LSM hooks)

Signed-off-by Serge E. Hallyn <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Andrew Morgan <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

---
 drivers/block/drbd/drbd_nl.c           |    5 +++++ drivers/md/dm-log-userspace-transfer.c |    3 +++ drivers/staging/pohmelfs/config.c      |    3 +++ drivers/video/uvesafb.c                |    3 +++

 4 files changed, 14 insertions(+), 0 deletions(-)

drivers/block/drbd/drbd_nl.c +0 -0

index 0feab26..9a87a14 100644

drivers/block/drbd/drbd_nl.c +4 -0

+               retcode = ERR_PERM;
+               goto fail;
+       }
+
        if (!cap_raised(current_cap(), CAP_SYS_ADMIN)) {
                retcode = ERR_PERM;
                goto fail;

drivers/md/dm-log-userspace-transfer.c +3 -0

index 1f23e04..140ca81 100644
@@ -134,6 +134,9 @@ static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
  {
        struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1);
 
+       if (current_user_ns() != &init_user_ns)
+               return;
+
        if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
                return;
 

drivers/staging/pohmelfs/config.c +3 -0

index b6c42cb..cd259d0 100644
@@ -525,6 +525,9 @@ static void pohmelfs_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *n
  {
        int err;
 
+       if (current_user_ns() != &init_user_ns)
+               return;
+
        if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
                return;
 

drivers/video/uvesafb.c +3 -1

index 7f8472c..71dab8e 100644
@@ -73,6 +73,9 @@ static void uvesafb_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *ns
        struct uvesafb_task *utask;
        struct uvesafb_ktask *task;
 
+       if (current_user_ns() != &init_user_ns)
+               return;
+
        if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
                return;
 
--

1.7.5.4

#define NS_IS_NON_DEFAULT (current_user_ns() != &init_user_ns)

Quoted 2 lines

#define NS_IS_NON_DEFAULT (current_user_ns() != &init_user_ns)

How about:

	#define IN_ROOT_USER_NS (current_user_ns() == &init_user_ns)

And then:

	if (!IN_ROOT_USER_NS() || !cap_raised(current_cap(), CAP_SYS_ADMIN)) {

I think it reads better.

A few modules are using cap_raised(current_cap(), cap) to authorize actions.  This means that tasks which are privileged in non-initial user namespaces will be deemed privileged.  The privilege should only be granted if the task is in the initial user namespace.

Switching the calls to capable() would change the behavior - it would cause the LSM capable hooks to be called, and set PF_SUPERPRIV if the capability was used.  So instead, put in an explicit check and refuse privilege if the caller is not in init_user_ns.

Changelog:
Oct 23: Use a nice macro to make the code shorter and easier to read, per advice from Andrew Morgan and David Howells.

Signed-off-by Serge E. Hallyn <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Andrew Morgan <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc David Howells <[email protected]>

---
 drivers/block/drbd/drbd_nl.c           |    2 +-
 drivers/md/dm-log-userspace-transfer.c |    2 +-
 drivers/staging/pohmelfs/config.c      |    2 +-
 drivers/video/uvesafb.c                |    2 +-
 include/linux/cred.h                   |    2 ++

 5 files changed, 6 insertions(+), 4 deletions(-)

drivers/block/drbd/drbd_nl.c +1 -1

index 0feab26..f799b15 100644
@@ -2297,7 +2297,7 @@ static void drbd_connector_callback(struct cn_msg *req, struct netlink_skb_parms
                return;
        }
 
-       if (!cap_raised(current_cap(), CAP_SYS_ADMIN)) {
+       if (!IN_ROOT_USER_NS() || !cap_raised(current_cap(), CAP_SYS_ADMIN)) {
                retcode = ERR_PERM;
                goto fail;
        }

drivers/md/dm-log-userspace-transfer.c +1 -1

index 1f23e04..126a79b 100644
@@ -134,7 +134,7 @@ static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
  {
        struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1);
 
-       if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
+       if (!IN_ROOT_USER_NS() || !cap_raised(current_cap(), CAP_SYS_ADMIN))
                return;
 
        spin_lock(&receiving_list_lock);

drivers/staging/pohmelfs/config.c +1 -1

index b6c42cb..327c047 100644
@@ -525,7 +525,7 @@ static void pohmelfs_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *n
  {
        int err;
 
-       if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
+       if (!IN_ROOT_USER_NS() || !cap_raised(current_cap(), CAP_SYS_ADMIN))
                return;
 
        switch (msg->flags) {

drivers/video/uvesafb.c +1 -1

index 7f8472c..94e0e9d 100644
@@ -73,7 +73,7 @@ static void uvesafb_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *ns
        struct uvesafb_task *utask;
        struct uvesafb_ktask *task;
 
-       if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
+       if (!IN_ROOT_USER_NS() || !cap_raised(current_cap(), CAP_SYS_ADMIN))
                return;
 
        if (msg->seq >= UVESAFB_TASKS_MAX)

include/linux/cred.h +2 -1

index 4030896..2f75da7 100644
@@ -359,9 +359,11 @@ static inline void put_cred(const struct cred *_cred)
 
  #ifdef CONFIG_USER_NS
  #define current_user_ns() (current_cred_xxx(user_ns))
+#define IN_ROOT_USER_NS() (current_user_ns() == &init_user_ns)
  #else
  extern struct user_namespace init_user_ns;
  #define current_user_ns() (&init_user_ns)
+#define IN_ROOT_USER_NS() (1)
  #endif
 
 
--

1.7.5.4

Serge,

It seems as if this whole thing is really idiomatic. How about?

#define IN_ROOT_USER_NS_CAPABLE(cap)  \
   ((current_user_ns() == &init_user_ns) && cap_raised(current_cap(), cap))

Quoted 6 lines · Andrew G. Morgan

Serge,

It seems as if this whole thing is really idiomatic. How about?

#define IN_ROOT_USER_NS_CAPABLE(cap)  \
   ((current_user_ns() == &init_user_ns) && cap_raised(current_cap(), cap))

My objection to this was that it seems to encourage others to use it :)  I'm not sure we want that.  Also, IN_ROOT_USER_NS seems more generally useful.

But if I'm the only one who feels this way I'll go ahead and do it...

Quoted 5 lines · Serge E. Hallyn

Quoted 4 lines · Andrew G. Morgan

Serge,

It seems as if this whole thing is really idiomatic. How about?

Quoted 5 lines

Quoted 2 lines

#define IN_ROOT_USER_NS_CAPABLE(cap)  \
   ((current_user_ns() == &init_user_ns) && cap_raised(current_cap(), cap))

My objection to this was that it seems to encourage others to use it :)  I'm not sure we want that.  Also, IN_ROOT_USER_NS seems more generally useful.

What is driving the choice of when its appropriate? How can a

developer determine this? If you make it hard, presumably folk won't do it by default, but will that create a burdon on others to go round patching things like this up?

Quoted 1 line

But if I'm the only one who feels this way I'll go ahead and do it...

I'm more of a optimize for a human to read the source code (ie. debug a problem) kind of person. If IN_ROOT_USER_NS is useful, you could always define IN_ROOT_USER_NS_CAPABLE in terms of IN_ROOT_USER_NS &&

... and provide both.

I guess I'm unclear, however, when you want developers to use one or the other variant of the basic capable() functionality. Since I'm not clear, I'm suspecting this is a fragile situation.

Quoted 7 lines · Andrew G. Morgan

Quoted 5 lines · Serge E. Hallyn

Quoted 4 lines · Andrew G. Morgan

Serge,

It seems as if this whole thing is really idiomatic. How about?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

#define IN_ROOT_USER_NS_CAPABLE(cap)  \
   ((current_user_ns() == &init_user_ns) && cap_raised(current_cap(), cap))

My objection to this was that it seems to encourage others to use it :) I'm not sure we want that. Also, IN_ROOT_USER_NS seems more generally useful.

What is driving the choice of when its appropriate? How can a

I'd like to say it's never appropriate. The reason is that it bypasses the whole security_ops->capable() sequence, so for instance SELinux is kept in the dark.

Quoted 9 lines

developer determine this? If you make it hard, presumably folk won't do it by default, but will that create a burdon on others to go round patching things like this up?

Quoted 1 line

But if I'm the only one who feels this way I'll go ahead and do it...

I'm more of a optimize for a human to read the source code (ie. debug a problem) kind of person. If IN_ROOT_USER_NS is useful, you could always define IN_ROOT_USER_NS_CAPABLE in terms of IN_ROOT_USER_NS &&

My other objection is that, in contrast to IN_ROOT_USER_NS(), which is very clear, IN_ROOT_USER_NS_CAPABLE() is not as helpful. I'm sure a better name is out there somewhere, though.

Quoted 5 lines

... and provide both.

I guess I'm unclear, however, when you want developers to use one or the other variant of the basic capable() functionality. Since I'm not clear, I'm suspecting this is a fragile situation.

I think only security code (LSMs) should be using cap_raised directly. Everything else should go through the capable()/has_capability() family of functions. Which, incidentally, have been (or are about to be) made less of a mess and thus less fragile by Eric Paris' patchset starting at
http://www.spinics.net/linux/fedora/linux-security-module/msg11896.html

Quoted 40 lines · Serge E. Hallyn

Quoted 7 lines · Andrew G. Morgan

Quoted 5 lines · Serge E. Hallyn

Quoted 4 lines · Andrew G. Morgan

Serge,

It seems as if this whole thing is really idiomatic. How about?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

#define IN_ROOT_USER_NS_CAPABLE(cap)  \
   ((current_user_ns() == &init_user_ns) && cap_raised(current_cap(), cap))

My objection to this was that it seems to encourage others to use it :) I'm not sure we want that. Also, IN_ROOT_USER_NS seems more generally useful.

What is driving the choice of when its appropriate? How can a

I'd like to say it's never appropriate. The reason is that it bypasses the whole security_ops->capable() sequence, so for instance SELinux is kept in the dark.

Quoted 9 lines

developer determine this? If you make it hard, presumably folk won't do it by default, but will that create a burdon on others to go round patching things like this up?

Quoted 1 line

But if I'm the only one who feels this way I'll go ahead and do it...

I'm more of a optimize for a human to read the source code (ie. debug a problem) kind of person. If IN_ROOT_USER_NS is useful, you could always define IN_ROOT_USER_NS_CAPABLE in terms of IN_ROOT_USER_NS &&

My other objection is that, in contrast to IN_ROOT_USER_NS(), which is very clear, IN_ROOT_USER_NS_CAPABLE() is not as helpful. I'm sure a better name is out there somewhere, though.

Quoted 5 lines

... and provide both.

I guess I'm unclear, however, when you want developers to use one or the other variant of the basic capable() functionality. Since I'm not clear, I'm suspecting this is a fragile situation.

Quoted 5 lines

I think only security code (LSMs) should be using cap_raised directly. Everything else should go through the capable()/has_capability() family of functions. Which, incidentally, have been (or are about to be) made less of a mess and thus less fragile by Eric Paris' patchset starting at
http://www.spinics.net/linux/fedora/linux-security-module/msg11896.html

(sorry out all last week)

I was going to ask why we have these user at all. Is there a reason they are bypassing the LSM and not setting PF_PRIV? Is the best solution to just bring them back into the capable fold?

Quoted 33 lines · Serge Hallyn

From: "Serge E. Hallyn" <[email protected]>

A few modules are using cap_raised(current_cap(), cap) to authorize actions.  This means that tasks which are privileged in non-initial user namespaces will be deemed privileged.  The privilege should only be granted if the task is in the initial user namespace.

Switching the calls to capable() would change the behavior - it would cause the LSM capable hooks to be called, and set PF_SUPERPRIV if the capability was used.  So instead, put in an explicit check and refuse privilege if the caller is not in init_user_ns.

Vasiliy had suggested introducing a new helper for this.  I'm open to suggestions, but for four callers and for a discouraged idiom, I'd rather not pollute the capable* function namespace with a bad name. (even has_capability goes through the LSM hooks)

Signed-off-by Serge E. Hallyn <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Andrew Morgan <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

---
 drivers/block/drbd/drbd_nl.c           |    5 +++++ drivers/md/dm-log-userspace-transfer.c |    3 +++ drivers/staging/pohmelfs/config.c      |    3 +++ drivers/video/uvesafb.c                |    3 +++

 4 files changed, 14 insertions(+), 0 deletions(-)

drivers/block/drbd/drbd_nl.c +0 -0

index 0feab26..9a87a14 100644

drivers/block/drbd/drbd_nl.c static void drbd_connector_callback(struct cn_msg *req, struct netlink_skb_parms · line 2300

Quoted 4 lines

drivers/block/drbd/drbd_nl.c +1 -0

@@ -2297,6 +2297,11 @@ static void drbd_connector_callback(struct cn_msg *req, struct netlink_skb_parms
                return;
        }
 
+       if (current_user_ns() != &init_user_ns) {

I'd feel much happier with this if it did use some inline or macro here.

#define NS_IS_NON_DEFAULT (current_user_ns() != &init_user_ns)

if (NS_IS_NON_DEFAULT || !cap_raised(current_cap(), CAP_SYS_ADMIN)) {
        retcode = ERR_PERM;
        goto fail;
}

Is it important to support the situation that NS support is not configured?

To this new task, any resource belonging to the initial user namespace will appear to belong to user and group 'nobody', which are UID and GID -1. Permission to open such files will be granted according to world access permissions. UID comparisons and group membership checks will return false, and privilege will be denied.

When a task belonging to (for example) userid 500 in the initial user namespace

Userid mapping for the VFS is not yet implemented, though prototypes exist.

... Therefore, attempts to exercise privilege to resources in, for instance, a particular network namespace, can be properly validated by checking whether the caller has the needed privilege (i.e. CAP_NET_ADMIN) targeted to the user namespace which owns the network namespace.

As an example, if a new task is cloned with a private user namespace but no

private network namespace, then the task's network namespace is owned by the parent user namespace. The new task has no

privilege to

the
parent user namespace, so it will not be able to create or configure

network devices

. If,

instead, the task were cloned with both private
user and network namespaces, then the private network namespace is owned by the private user namespace, and so root in the new user namespace will have privilege targeted to

the network namespace. It will be able
to create and configure network devices.

Serge Hallyn <[email protected]> wrote:

From: "Serge E. Hallyn" <[email protected]>

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

Quoted 1 line · Serge Hallyn

From: "Serge E. Hallyn" <[email protected]>

Hi Serge.

Just some trivial style notes.

Quoted 3 lines

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

[]

net/core/scm.c line 47

Quoted 5 lines

net/core/scm.c +2 -1

 
-static __inline__ int scm_check_creds(struct ucred *creds)
+static __inline__ bool uidequiv(const struct cred *src, struct ucred *tgt,
+			       struct user_namespace *ns)

Perhaps inline is better than __inline__ and do these functions really need to be marked inline at all?

net/core/scm.c line 52

Quoted 5 lines

net/core/scm.c +5 -0

+{
+	if (src->user_ns != ns)
+		goto check_capable;
+	if (src->uid == tgt->uid || src->euid == tgt->uid ||
+	    src->suid == tgt->uid)

Perhaps this is less prone to typo errors and are a bit more readable as:

	if (tgt->uid == src->uid ||
	    tgt->uid == src->euid ||
	    tgt->uid == src->suid)

From: "Serge E. Hallyn" <[email protected]>

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

In the past, either your uids had to match, or you had to have CAP_SETXID. In a namespaced world, you must either (both be in the same user namespace and have your uids match), or you must have CAP_SETXID targeted at the other user namespace. The latter can happen for instance if uid 500 created a new user namespace and now interacts with uid 0 in it.

Quoted 6 lines · Serge Hallyn

From: "Serge E. Hallyn" <[email protected]>

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

Quoted 6 lines

In the past, either your uids had to match, or you had to have CAP_SETXID. In a namespaced world, you must either (both be in the same user namespace and have your uids match), or you must have CAP_SETXID targeted at the other user namespace. The latter can happen for instance if uid 500 created a new user namespace and now interacts with uid 0 in it.

Serge this approach is wrong.

Because we pass the cred and the pid through the socket socket itself is just a conduit and should be ignored in this context.

The only interesting test should be are you allowed to impersonate other users in your current userk namespace.

So it should be possible to simplify the entire patch to just:
static __inline__ int scm_check_creds(struct ucred *creds) {

net/core/scm.c +6 -5

 	const struct cred *cred = current_cred();
+	struct user_namespace *ns = cred->user_ns;
 
-	if ((creds->pid == task_tgid_vnr(current) || capable(CAP_SYS_ADMIN)) &&
-	    ((creds->uid == cred->uid   || creds->uid == cred->euid ||
-	      creds->uid == cred->suid) || capable(CAP_SETUID)) &&
-	    ((creds->gid == cred->gid   || creds->gid == cred->egid ||
-	      creds->gid == cred->sgid) || capable(CAP_SETGID))) {
+	if ((creds->pid == task_tgid_vnr(current) || ns_capable(ns, CAP_SYS_ADMIN)) &&
+	    ((creds->uid == cred->uid   || creds->uid == cred->euid ||
+	      creds->uid == cred->suid) || ns_capable(ns, CAP_SETUID)) &&
+	    ((creds->gid == cred->gid   || creds->gid == cred->egid ||
+	      creds->gid == cred->sgid) || ns_capable(ns, CAP_SETGID))) {
  	       return 0;
  	}
  	return -EPERM;
  }

Quoted 8 lines · Eric W. Biederman

Quoted 6 lines · Serge Hallyn

From: "Serge E. Hallyn" <[email protected]>

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

Quoted 8 lines

Quoted 6 lines

In the past, either your uids had to match, or you had to have CAP_SETXID. In a namespaced world, you must either (both be in the same user namespace and have your uids match), or you must have CAP_SETXID targeted at the other user namespace. The latter can happen for instance if uid 500 created a new user namespace and now interacts with uid 0 in it.

Serge this approach is wrong.

Thanks for looking, Eric.

Quoted 2 lines

Because we pass the cred and the pid through the socket socket itself is just a conduit and should be ignored in this context.

Ok, that makes sense, but

Quoted 2 lines

The only interesting test should be are you allowed to impersonate other users in your current userk namespace.

Why in your current user namespace? Shouldn't it be in the target user ns? I understand it could be wrong to tie the user ns owning the socket to the target userns (though I still kind of like it), but just because I have CAP_SETUID in my own user_ns doesn't mean I should be able to pose as another uid in your user_ns.

(Now I also see that cred_to_ucred() translates to the current user_ns, so that should have been a hint to me before about your intent, but I'm not convinced I agree with your intent).

And you do the same with the pid. Why is that a valid assumption?

Quoted 28 lines · Serge E. Hallyn

Quoted 8 lines · Eric W. Biederman

Quoted 6 lines · Serge Hallyn

From: "Serge E. Hallyn" <[email protected]>

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

Quoted 8 lines

Quoted 6 lines

In the past, either your uids had to match, or you had to have CAP_SETXID. In a namespaced world, you must either (both be in the same user namespace and have your uids match), or you must have CAP_SETXID targeted at the other user namespace. The latter can happen for instance if uid 500 created a new user namespace and now interacts with uid 0 in it.

Serge this approach is wrong.

Thanks for looking, Eric.

Quoted 2 lines

Because we pass the cred and the pid through the socket socket itself is just a conduit and should be ignored in this context.

Ok, that makes sense, but

Quoted 2 lines

The only interesting test should be are you allowed to impersonate other users in your current userk namespace.

Quoted 6 lines

Why in your current user namespace? Shouldn't it be in the target user ns? I understand it could be wrong to tie the user ns owning the socket to the target userns (though I still kind of like it), but just because I have CAP_SETUID in my own user_ns doesn't mean I should be able to pose as another uid in your user_ns.

First and foremost it is important that you be able if you have the capability to impersonate other users in your current user namespace. That is what the capability actually controls.

None of this allows you to impersonate any user in any other user namespace. The translation between users prevents that.

Quoted 5 lines

(Now I also see that cred_to_ucred() translates to the current user_ns, so that should have been a hint to me before about your intent, but I'm not convinced I agree with your intent).

And you do the same with the pid. Why is that a valid assumption?

Yes. Basically all the code is allow you to impersonate people you would have been able to impersonate before. If your target is in another namespace you can not fool them.

With pids the logic should be a lot clearer. Pretend to be a pid you can see in your current pid namespace. Lookup and convert to struct pid aka the namespace agnostic object. On output return the pid value that

the target process will know you as.

Ultimately I think we need a ns_capable for the current user namespace instead of a global one. But I don't see any rush to introduce ns_capable here.

Eric

Quoted 54 lines · Eric W. Biederman

Quoted 28 lines · Serge E. Hallyn

Quoted 8 lines · Eric W. Biederman

Quoted 6 lines · Serge Hallyn

From: "Serge E. Hallyn" <[email protected]>

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

Quoted 8 lines

Quoted 6 lines

In the past, either your uids had to match, or you had to have CAP_SETXID. In a namespaced world, you must either (both be in the same user namespace and have your uids match), or you must have CAP_SETXID targeted at the other user namespace. The latter can happen for instance if uid 500 created a new user namespace and now interacts with uid 0 in it.

Serge this approach is wrong.

Thanks for looking, Eric.

Quoted 2 lines

Because we pass the cred and the pid through the socket socket itself is just a conduit and should be ignored in this context.

Ok, that makes sense, but

Quoted 2 lines

The only interesting test should be are you allowed to impersonate other users in your current userk namespace.

Quoted 6 lines

Why in your current user namespace? Shouldn't it be in the target user ns? I understand it could be wrong to tie the user ns owning the socket to the target userns (though I still kind of like it), but just because I have CAP_SETUID in my own user_ns doesn't mean I should be able to pose as another uid in your user_ns.

First and foremost it is important that you be able if you have the capability to impersonate other users in your current user namespace. That is what the capability actually controls.

None of this allows you to impersonate any user in any other user namespace. The translation between users prevents that.

Quoted 5 lines

(Now I also see that cred_to_ucred() translates to the current user_ns, so that should have been a hint to me before about your intent, but I'm not convinced I agree with your intent).

And you do the same with the pid. Why is that a valid assumption?

Yes. Basically all the code is allow you to impersonate people you would have been able to impersonate before. If your target is in another namespace you can not fool them.

Quoted 3 lines

With pids the logic should be a lot clearer. Pretend to be a pid you can see in your current pid namespace. Lookup and convert to struct pid aka the namespace agnostic object. On output return the pid value that

No. That conversion is happending before the user-specified pid is set.

Never mind, it all gets a little convoluted, but I see how it works, and - when the time comes - how to do it right for userns. :) Sorry about that.

Quoted 28 lines · Serge E. Hallyn

Quoted 8 lines · Eric W. Biederman

Quoted 6 lines · Serge Hallyn

From: "Serge E. Hallyn" <[email protected]>

Currently uids are compared without regard for the user namespace. Fix that to prevent tasks in a different user namespace from wrongly matching on SCM_CREDENTIALS.

Quoted 8 lines

Quoted 6 lines

In the past, either your uids had to match, or you had to have CAP_SETXID. In a namespaced world, you must either (both be in the same user namespace and have your uids match), or you must have CAP_SETXID targeted at the other user namespace. The latter can happen for instance if uid 500 created a new user namespace and now interacts with uid 0 in it.

Serge this approach is wrong.

Thanks for looking, Eric.

Quoted 2 lines

Because we pass the cred and the pid through the socket socket itself is just a conduit and should be ignored in this context.

Ok, that makes sense, but

Quoted 2 lines

The only interesting test should be are you allowed to impersonate other users in your current userk namespace.

Quoted 6 lines

Why in your current user namespace? Shouldn't it be in the target user ns? I understand it could be wrong to tie the user ns owning the socket to the target userns (though I still kind of like it), but just because I have CAP_SETUID in my own user_ns doesn't mean I should be able to pose as another uid in your user_ns.

First and foremost it is important that you be able if you have the capability to impersonate other users in your current user namespace. That is what the capability actually controls.

None of this allows you to impersonate any user in any other user namespace. The translation between users prevents that.

Quoted 5 lines

(Now I also see that cred_to_ucred() translates to the current user_ns, so that should have been a hint to me before about your intent, but I'm not convinced I agree with your intent).

And you do the same with the pid. Why is that a valid assumption?

Yes. Basically all the code is allow you to impersonate people you would have been able to impersonate before. If your target is in another namespace you can not fool them.

With pids the logic should be a lot clearer. Pretend to be a pid you can see in your current pid namespace. Lookup and convert to struct pid aka the namespace agnostic object. On output return the pid value that the target process will know you as.

Ultimately I think we need a ns_capable for the current user namespace instead of a global one. But I don't see any rush to introduce ns_capable here.