cred_to_ucred: use the creator of the right namespace

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/[email protected]>
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.

---
 net/core/sock.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

index b5b5929..d3e2077 100644
@@ -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

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

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.

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.

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;
}