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

[ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (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;
}