Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1449151

Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace for a namespace

From ebiederm@xmission.com (Eric W. Biederman)
Newsgroups linux.kernel
Subject Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace for a namespace
Date 2016-07-24 16:50 +0200
Message-ID <rYsT7-DS-9@gated-at.bofh.it> (permalink)
References <rUWZs-6XY-11@gated-at.bofh.it> <rV0Tn-ZV-5@gated-at.bofh.it> <rV0Tn-ZV-15@gated-at.bofh.it> <rYjZv-3Nk-1@gated-at.bofh.it> <rYsT7-DS-11@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw

Andrew Vagin <avagin@virtuozzo.com> writes:

> 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).

Apologies. I was skimming and misread the code.  I mistook that loop for
some useful part of getting the owner.  Looking in more detail...

Your code says:
+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);
+	}
+
+	for (;;) {
+		if (p == cred->user_ns)
+			break;
+		if (p == &init_user_ns)
+			return ERR_PTR(-EPERM);
+		p = p->parent;
+	}
+
+	return &get_user_ns(user_ns)->ns;
+}

And all else being equal it could say:

+struct ns_common *ns_get_owner(struct ns_common *ns)
s+{
+	struct user_namespace *user_ns = ns->user_ns;
+
+	/* Are we allowed to see the user namespace? */
+	if (!ns_capable(user_ns?user_ns:&init_user_ns, CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
+	if (!user_ns)
+		return ERR_PTR(-ENOENT);
+
+	return &get_user_ns(user_ns)->ns;
+}

Which I think is the root of my confusion.  You hand rolled ns_capable,
and I did not recognize it because I was skimming, and just looking to
be certain the permission check was present.

Given that you have to hand roll the pid namespace check that hand
rolling may not be so bad.  But it certainly was confusing the first
time I saw it especially without a comment.

Hmm.

I am not at all certain it makes sense to return -ENOENT.

Without the -ENOENT check the code is much cleaner, and clearer.

I may be blinkered but I don't see the value in letting someone know we
are talking about the initial namespace.  If anything that information
is likely to cause issues with weird corner cases of checkpoint/restart,
as it acts different if you are in a container or not.

When things act different in a container that almost always is a source
of a problem somewhere.

So we could simplify the filter in the code to just this.

+struct ns_common *ns_get_owner(struct ns_common *ns)
+{
+	struct user_namespace *my_user_ns = current_user_ns();
+	struct user_namespace *owner, *p;
+
+	/* See if the owner is in the current user namespace */
+	owner = p = ns->user_ns;
+	for (;;) {
+		if (!p)
+			return ERR_PTR(-EPERM);
+		if (p == my_user_ns)
+			break;
+		p = p->parent;
+	}
+
+	return &get_user_ns(owner)->ns;
+}

And on reflection I do see the point of not using ns_capable as that
requires having privileges in a namespace while all we want here
is to see if someone is in a visible namespace.

So please ignore my comments about ns_capable on the pid namespace
parent.

But please simplify the loop and put an appropriate comment on it like I
have above.  The fewer special cases the easier the code is to get
correct, and the easier it is to read.

Eric

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/5 RFC] Add an interface to discover relationships between namespaces Andrey Vagin <avagin@openvz.org> - 2016-07-14 20:30 +0200
  [PATCH 2/5] kernel: add a helper to get an owning user namespace for a namespace Andrey Vagin <avagin@openvz.org> - 2016-07-14 20:30 +0200
    Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace  for a namespace "W. Trevor King" <wking@tremily.us> - 2016-07-14 21:10 +0200
  Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces Andrey Vagin <avagin@openvz.org> - 2016-07-15 00:10 +0200
    [PATCH 5/5] tools/testing: add a test to check nsfs ioctl-s Andrey Vagin <avagin@openvz.org> - 2016-07-15 04:20 +0200
    [PATCH 4/5] nsfs: add ioctl to get a parent namespace Andrey Vagin <avagin@openvz.org> - 2016-07-15 04:20 +0200
      Re: [PATCH 4/5] nsfs: add ioctl to get a parent namespace ebiederm@xmission.com (Eric W. Biederman) - 2016-07-24 07:30 +0200
    [PATCH 3/5] nsfs: add ioctl to get an owning user namespace for ns file descriptor Andrey Vagin <avagin@openvz.org> - 2016-07-15 04:20 +0200
    [PATCH 2/5] kernel: add a helper to get an owning user namespace for a namespace Andrey Vagin <avagin@openvz.org> - 2016-07-15 04:20 +0200
      Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace for a namespace ebiederm@xmission.com (Eric W. Biederman) - 2016-07-24 07:20 +0200
        Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace for a namespace ebiederm@xmission.com (Eric W. Biederman) - 2016-07-24 16:50 +0200
          Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace  for a namespace "W. Trevor King" <wking@tremily.us> - 2016-07-24 19:10 +0200
      Re: [PATCH 2/5] kernel: add a helper to get an owning user namespace  for a namespace "W. Trevor King" <wking@tremily.us> - 2016-07-24 19:00 +0200
    Re: [PATCH 1/5] namespaces: move user_ns into ns_common ebiederm@xmission.com (Eric W. Biederman) - 2016-07-24 07:20 +0200
    Re: [PATCH 0/5 RFC] Add an interface to discover relationships between namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-07-24 07:30 +0200
  Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2016-07-21 16:50 +0200
    Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces Andrey Vagin <avagin@openvz.org> - 2016-07-22 20:30 +0200
      Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2016-07-25 13:50 +0200
        Re: [PATCH 0/5 RFC] Add an interface to discover relationships between namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-07-25 15:40 +0200
          Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2016-07-25 16:50 +0200
            Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2016-07-25 17:00 +0200
              Re: [PATCH 0/5 RFC] Add an interface to discover relationships between namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-07-25 17:40 +0200
            Re: [PATCH 0/5 RFC] Add an interface to discover relationships between namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-07-25 17:20 +0200
  Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces "W. Trevor King" <wking@tremily.us> - 2016-07-23 23:20 +0200
    Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces James Bottomley <James.Bottomley@HansenPartnership.com> - 2016-07-23 23:40 +0200
      Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces "W. Trevor King" <wking@tremily.us> - 2016-07-24 00:10 +0200
        Re: [PATCH 0/5 RFC] Add an interface to discover relationships between namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-07-24 00:20 +0200
          Re: [PATCH 0/5 RFC] Add an interface to discover relationships  between namespaces "W. Trevor King" <wking@tremily.us> - 2016-07-24 00:40 +0200
            Re: [PATCH 0/5 RFC] Add an interface to discover relationships between namespaces ebiederm@xmission.com (Eric W. Biederman) - 2016-07-24 07:10 +0200

csiph-web