From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-db5eur01on0103.outbound.protection.outlook.com ([104.47.2.103]:50584
	"EHLO EUR01-DB5-obe.outbound.protection.outlook.com"
	rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
	id S1751258AbcGXHKI (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Sun, 24 Jul 2016 03:10:08 -0400
Date: Sat, 23 Jul 2016 23:37:29 -0700
From: Andrew Vagin <avagin@virtuozzo.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Andrey Vagin <avagin@openvz.org>, <linux-kernel@vger.kernel.org>,
	"James Bottomley" <James.Bottomley@HansenPartnership.com>,
	Serge Hallyn <serge.hallyn@canonical.com>,
	<linux-api@vger.kernel.org>,
	<containers@lists.linux-foundation.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>, <criu@openvz.org>,
	<linux-fsdevel@vger.kernel.org>,
	"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Subject: Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace
 for a namespace
Message-ID: <20160724063728.GA17810@outlook.office365.com>
References: <CANaxB-xw_xBUq=0uT14ANv-jfg2NsGaPy=jyDO9=yF03_7toSw@mail.gmail.com>
 <1468548742-32136-1-git-send-email-avagin@openvz.org>
 <1468548742-32136-2-git-send-email-avagin@openvz.org>
 <878twrmxu2.fsf@x220.int.ebiederm.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="koi8-r"
Content-Disposition: inline
In-Reply-To: <878twrmxu2.fsf@x220.int.ebiederm.org>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Sun, Jul 24, 2016 at 12:03:49AM -0500, Eric W. Biederman wrote:
> Andrey Vagin <avagin@openvz.org> writes:
> 
> > Return -EPERM if an owning user namespace is outside of a process
> > current user namespace.
> >
> > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > index a5bc78c..6382e5e 100644
> > --- a/kernel/user_namespace.c
> > +++ b/kernel/user_namespace.c
> > @@ -994,6 +994,30 @@ static int userns_install(struct nsproxy *nsproxy, struct ns_common *ns)
> >  	return commit_creds(cred);
> >  }
> >  
> > +struct ns_common *ns_get_owner(struct ns_common *ns)
> > +{
> > +	const struct cred *cred = current_cred();
> > +	struct user_namespace *user_ns, *p;
> > +
> > +	user_ns = p = ns->user_ns;
> > +	if (user_ns == NULL) { /* ns is init_user_ns */
> > +		/* Unprivileged user should not know that it's init_user_ns. */
> > +		if (capable(CAP_SYS_ADMIN))
> > +			return ERR_PTR(-ENOENT);
> > +		return ERR_PTR(-EPERM);
> > +	}
> 
> This permission check is not what I meant to request.  This does not
> handle nested user namespaces.

Here I handle a case when ns is init_user_ns. init_user_ns doesn't have
a parent, so we need to return an error. We can't return ENOENT in all
cases, because we don't want to expose "that file descriptor is for the
root user namespace" to unprivileged users.
(Trevor suggested to add this check and it looks resonable for me too).
> 
> > +	for (;;) {
> > +		if (p == cred->user_ns)
> > +			break;
> > +		if (p == &init_user_ns)
> > +			return ERR_PTR(-EPERM);
> > +		p = p->parent;
> > +	}
> > +
> 
> The permission check really needs to be down here. And be:
> 
> 	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
>         	return ERR_PTR(-EPERM).
> 
> That cleanly and easily handles more than a depth of a single user
> namespace.
> 
> > +	return &get_user_ns(user_ns)->ns;
> > +}
> > +
> >  const struct proc_ns_operations userns_operations = {
> >  	.name		= "user",
> >  	.type		= CLONE_NEWUSER,
> 
> 
> Eric