[PATCH] cred_to_ucred: use the creator of the right namespace

[PATCH] cred_to_ucred: use the creator of the right namespace

[Serge E. Hallyn]

Hey Eric,

I ported a subset of your nsfd-v5 patchset to current git (took no
tweaking at all) and tested the received values of SCM_CREDENTIALS
ancillary msgs.  You'd asked me if it looked right before and I said
it did, but in fact there is a little bug, fixed by the below patch.

thanks,
-serge

From c99daef4d7927bf002b493039c86e3de70d7b8b1 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Fri, 7 May 2010 17:35:50 -0400
Subject: [PATCH 1/1] cred_to_ucred: use the creator of the right namespace

If cred->creator->user_ns == current->user_ns, then it is the
cred->creator>uid, not the current->user_ns->creator->uid which
we are interested in.

Tested with SCM_CREDENTIALS test program.  Without this patch,
if uid 1001 clones a task with clone(CLONE_NEWUSER), which
then does setresuid(25,25,25) and sends a SCM_CREDENTIALS msg
back to the parent, then the parent gets uid 0 and gid overflowgid.
The reason is that we were returning the uid of the creator of
the *parent*'s userns.

With this patch, the uid, gid, and pid are all correct.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 net/core/sock.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index b5b5929..d3e2077 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -753,9 +753,10 @@ void cred_to_ucred(struct pid *pid, const struct cred *cred,
 			/* Is cred in a child user namespace */
 			tmp = cred_ns;
 			do {
+				struct user_namespace *p = tmp;
 				tmp = tmp->creator->user_ns;
 				if (tmp == current_ns) {
-					ucred->uid = tmp->creator->uid;
+					ucred->uid = p->creator->uid;
 					ucred->gid = overflowgid;
 					return;
 				}
-- 
1.7.0.4

Re: [PATCH] cred_to_ucred: use the creator of the right namespace

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Hey Eric,
> 
> I ported a subset of your nsfd-v5 patchset to current git (took no
> tweaking at all) and tested the received values of SCM_CREDENTIALS
> ancillary msgs.  You'd asked me if it looked right before and I said
> it did, but in fact there is a little bug, fixed by the below patch.
> 
> thanks,
> -serge
> 
> From c99daef4d7927bf002b493039c86e3de70d7b8b1 Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Fri, 7 May 2010 17:35:50 -0400
> Subject: [PATCH 1/1] cred_to_ucred: use the creator of the right namespace
> 
> If cred->creator->user_ns == current->user_ns, then it is the
> cred->creator>uid, not the current->user_ns->creator->uid which
> we are interested in.
> 
> Tested with SCM_CREDENTIALS test program.  Without this patch,
> if uid 1001 clones a task with clone(CLONE_NEWUSER), which
> then does setresuid(25,25,25) and sends a SCM_CREDENTIALS msg
> back to the parent, then the parent gets uid 0 and gid overflowgid.
> The reason is that we were returning the uid of the creator of
> the *parent*'s userns.
> 
> With this patch, the uid, gid, and pid are all correct.

Except, while it is *technically* correct, semantically I'm
not sure that's what we want either.

Assuming again that pid 100 owned by uid 1001 clones a task 200 in a new
user_ns, which does setresuid(25,25,25).  Now 200 sends SCM_CREDENTIALS
to 100.  (Again clearly uid 0 was wrong.)  But sending 1001 isn't quite
right either.  In particular, there is a reason why 100 cloned a new
user namespace: so the uid passed back perhaps should just be
overflowuid?

As a concrete example I'll again use the case where a task owned by uid
0 in init_user_ns creates a child in new user_ns, with uid 25 there, and
now the child calls /sbin/reboot.  reboot talks over abstract unix sock to
init in the parent and sends SCM_CREDENTIALS.  With my patch above, it
would now send uid 0, and so init would reboot the host.  That this is
wrong becomes particularly obvious when we consider sending posix caps
along with uid:  any posix caps which the child has should be N/A to
the init_user_ns.

IIUC you coded this originally for use by NFS - would sending
overflowuid make sense for its use as well?

The existing code is still right in the inverse case - if pid 100
sends SCM_CREDENTIALS to pid 200, it sends uid 0 which I think always
makes sense.

-serge

Re: [PATCH] cred_to_ucred: use the creator of the right namespace

[Eric W. Biederman]

"Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:

> Except, while it is *technically* correct, semantically I'm
> not sure that's what we want either.
>
> Assuming again that pid 100 owned by uid 1001 clones a task 200 in a new
> user_ns, which does setresuid(25,25,25).  Now 200 sends SCM_CREDENTIALS
> to 100.  (Again clearly uid 0 was wrong.)  But sending 1001 isn't quite
> right either.  In particular, there is a reason why 100 cloned a new
> user namespace: so the uid passed back perhaps should just be
> overflowuid?
>
> As a concrete example I'll again use the case where a task owned by uid
> 0 in init_user_ns creates a child in new user_ns, with uid 25 there, and
> now the child calls /sbin/reboot.  reboot talks over abstract unix sock to
> init in the parent and sends SCM_CREDENTIALS.  With my patch above, it
> would now send uid 0, and so init would reboot the host.  That this is
> wrong becomes particularly obvious when we consider sending posix caps
> along with uid:  any posix caps which the child has should be N/A to
> the init_user_ns.

I am conflicted I think there is real justification for showing uids
in a child uid namespace as the creator of the uid namespace.  At the
same time I very much agree that it is safer and less dangerous from
an existing security perspective if we don't map the uids to the
creator of the namespace so I have dropped that feature for now.
Thank you for spotting that.

I have extracted the heart of the code and added two functions
to user_namespace.c as that seems to make the most sense for the
moment.  I will send this all to netdev shortly.

> IIUC you coded this originally for use by NFS - would sending
> overflowuid make sense for its use as well?

The first time I thought of mapping I was indeed thinking of NFS.
Either overflowuid or whatever NFS does for the nobody user that
it uses for root squash.

> The existing code is still right in the inverse case - if pid 100
> sends SCM_CREDENTIALS to pid 200, it sends uid 0 which I think always
> makes sense.

Agreed.

Below is what I am currently working with, the essence of my cred_to_ucred
patch, but in a form the code can be used elsewhere.  I have completely
removed the section for children as we that appears dangerous for the moment.

Eric


uid_t user_ns_map_uid(struct user_namespace *to, const struct cred *cred, uid_t uid)
{
	struct user_namespace *tmp;

	if (likely(to == cred->user->user_ns))
		return uid;


	/* Is cred->user the creator of the target user_ns
	 * or the creator of one of it's parents?
	 */
	for ( tmp = to; tmp != &init_user_ns;
	      tmp = tmp->creator->user_ns ) {
		if (cred->user == tmp->creator) {
			return (uid_t)0;
		}
	}

	/* No useful relationship so no mapping */
	return overflowuid;
}

gid_t user_ns_map_gid(struct user_namespace *to, const struct cred *cred, gid_t gid)
{
	struct user_namespace *tmp;

	if (likely(to == cred->user->user_ns))
		return gid;

	/* Is cred->user the creator of the target user_ns
	 * or the creator of one of it's parents?
	 */
	for ( tmp = to; tmp != &init_user_ns;
	      tmp = tmp->creator->user_ns ) {
		if (cred->user == tmp->creator) {
			return (gid_t)0;
		}
	}

	/* No useful relationship so no mapping */
	return overflowgid;
}