Date:	Thu, 13 Dec 2012 18:33:27 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Andy Lutomirski <luto@...capital.net>
Cc:	"Serge E. Hallyn" <serge@...lyn.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	containers@...ts.linux-foundation.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-security-module@...r.kernel.org
Subject: Re: [RFC][PATCH] Fix cap_capable to only allow owners in the parent user namespace to have caps.


Andy thank you for your review.

Andy Lutomirski <luto@...capital.net> writes:
> This is confusing enough that I can't immediately tell whether it's
> correct.  I think it's close but out of order.

Yeah.  That is the trick. Figuring out how to write that code so it is
correct and obvious.

I have added a comment at the top and removed the extra variable I was
adding.

The order except for verifying a user_ns->parent is valid by checking
for targ_ns == &init_user_ns doesn't make a difference.  Because
cred->user_ns can only be one of targ_ns or targ_ns->parent.

> Should this be transitive?

Yes.
>  I.e. suppose uid 1 owns a child of
> init_user_ns and uid 2 (mapped in the first ns as the identity) owns
> an inner ns.  Does uid 2 in the root ns have all caps inside?  I'd say
> no, but I don't have a great argument for that. 

I also say no.  It is more code and it doesn't fit my nice small
definition.  You have to be the owner and you have to be in the parent
of the target user namespace.  Being able to remember the rules in
your head is important.

> But uid 1 presumably
> does have caps because it could enter the parent with setns, then
> change uid, then enter the child.

Yes. uid 1 does have caps.

> How about (severely whitespace damaged):

You know that makes the termination condition a bit clearer.  But it
looses the nice place to put a comment when we loop again.  This loop
is just subtle enough that I want to preserve my comments.

I think I must have put -EPERM towards the end for the same reason to
make it clear that is the termination condition.

In practice I think it is important to have the cap_raised case first,
as that is the common case, and if we can be clear and still test that
case first it means the code will be faster.  With my reordering it is
obvious that nothing strange happens in the initial user namespace with
the owner test after the exit when we are the initial user namespace.

> int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
>                 int cap, int audit)
> {
>         struct user_namespace *here = targ_ns;
>
>         /* Walk up the namespace hierarchy until we find our own namespace. */
>         for (;;) {
>                 /* The owner of an ancestor namespace has all caps, if
> that owner is in the parent ns. */
>                 if (cred->user_ns == here->parent &&
> uid_eq(targ_ns->owner, cred->euid))
>                         return 0;

This would have needed a check that (here != &init_user_ns).  Because
the init_user_ns does not have a parent.

>                 /* Do we have the necessary capabilities? */
>                 if (here == cred->user_ns)
>                         return cap_raised(cred->cap_effective, cap) ?
> 0 : -EPERM;
>
>                 /* Have we tried all of the parent namespaces? */
>                 if (here == &init_user_ns)
>                         return -EPERM;
>                 else
>                         here = targ_ns->parent;
>         }
> }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists