Re: [PATCH] user_ns: Use nsown_capable instead of capable in net_ctl_permissions

[ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)]

To expand a bit on Serge's reply.

Huang Qiang <h.huangqiang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> writes:

> From: Zhao Hongjiang <zhaohongjiang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>
> HI:
> When I use an unprivileged user exec the following command:
> # nsexec -cUn /bin/bash
> to create a container with new user_ns and net_ns.
>
> Then I exec "echo 4096 4096 4096 > /proc/sys/net/ipv4/tcp_mem",
> the result is Permission Denied which we hope it should be allowed.
>
> It is because of capable(CAP_NET_ADMIN).
>
> Even my unprivileged user have the CAP_NET_ADMIN in the new user_ns and the
> tcp_mem is belong to the new net_ns, the capable(CAP_NET_ADMIN) checking is
> that this must in the init_user_ns, so the result is the network administrator
> can't have the same access as root.
>
> Use nsown_capable(...) the problem is solved.
>
> PS: I changed lxc almostly like what serge done, then use an unprivileged user
> to start a container, several Permission Denied occur(such as mount), all this
> is caused by capabale(...), when i use nsown_capable(...) the container is
> running like everything is ok.

> Is this capabale() methed is obsolete? If so, i'll send a new patch to solve
> all this problems.

No capable is not really obsolete.

Your patch is a bit scary, and this is definitely an area we
need to do some work in.

There are a couple of pieces to this.  If you raise tcp_mem you can
allow yourself to take up unlimited amounts of kernel memory.  We
should not allow that for an unprivilged user, and unprivilged users
are allowed to create a user namespaces and then network namespaces.

The replacement should be ns_capable not nsown_capable.  We don't
want to allow any process that happens to have CAP_NET_ADMIN in their
user namespace to have root privileges over any syctl file they can
get a file descriptor to.

cap_capable exists so that we can take our time and audit these things.
Potentially we could change all cap_capable to
"ns_capable(&init_user_ns, ...)" but that doesn't buy us much in the short
term.

So while I think your patch is in the right ballpark, I think a correct
version of allowing an unprivileged user to raise tcp_mem is something
we need to do a bit more carefully.

Eric


> Signed-off-by: Zhao Hongjiang<zhaohongjiang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Huang Qiang <h.huangqiang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  net/sysctl_net.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/sysctl_net.c b/net/sysctl_net.c
> index c3e65ae..ee31777 100644
> --- a/net/sysctl_net.c
> +++ b/net/sysctl_net.c
> @@ -47,7 +47,7 @@ static int net_ctl_permissions(struct ctl_table_root *root,
>  			       struct ctl_table *table)
>  {
>  	/* Allow network administrator to have same access as root. */
> -	if (capable(CAP_NET_ADMIN)) {
> +	if (nsown_capable(CAP_NET_ADMIN)) {
>  		int mode = (table->mode >> 6) & 7;
>  		return (mode << 6) | (mode << 3) | mode;
>  	}