netdev - Re: [PATCH] ipv6: enable net.ipv6.route sysctls in network namespace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87pmsqyuqy.fsf@disp2133>
Date:   Thu, 30 Sep 2021 09:59:33 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Alexander Kuznetsov <wwfq@...dex-team.ru>
Cc:     netdev@...r.kernel.org, zeil@...dex-team.ru,
        Jakub Kicinski <kuba@...nel.org>,
        Florian Westphal <fw@...len.de>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH] ipv6: enable net.ipv6.route sysctls in network namespace

Alexander Kuznetsov <wwfq@...dex-team.ru> writes:

> We want to increase route cache size in network namespace
> created with user namespace. Currently ipv6 route settings
> are disabled for non-initial network namespaces.
> Since routes are per network namespace it is safe
> to enable these sysctls.

The case where this matters is when the network namespaces is created by
by the root user in a user namespace.  AKA this is something we allow
any user to do.

These were disabled because out of an abundance of caution rather than
any particular policy when the kernel started allowing non-root users
to create network namespaces.

That said it would really help if your commit message listed the
sysctls you are enabling and listed why it was safe to enable them.

That is it would really help if you performed the review that says
the sysctls are safe for ordinary users to use and that they won't
enable DOS attacks or the like.

If you just care about the route cache size you can only enable
that sysctl.

These are the 10 sysctls you are enabling.  All you talk about
in your commit message is route cache size which I believe is
the 3rd entry in the table net->ipv6.sysctl.ip6_rt_max_size.
It certainly is not all of them.

		table[0].data = &net->ipv6.sysctl.flush_delay;
		table[1].data = &net->ipv6.ip6_dst_ops.gc_thresh;
		table[2].data = &net->ipv6.sysctl.ip6_rt_max_size;
		table[3].data = &net->ipv6.sysctl.ip6_rt_gc_min_interval;
		table[4].data = &net->ipv6.sysctl.ip6_rt_gc_timeout;
		table[5].data = &net->ipv6.sysctl.ip6_rt_gc_interval;
		table[6].data = &net->ipv6.sysctl.ip6_rt_gc_elasticity;
		table[7].data = &net->ipv6.sysctl.ip6_rt_mtu_expires;
		table[8].data = &net->ipv6.sysctl.ip6_rt_min_advmss;
		table[9].data = &net->ipv6.sysctl.ip6_rt_gc_min_interval;
		table[10].data = &net->ipv6.sysctl.skip_notify_on_dev_down;

I took a quick look and we don't enable any of these for ipv4 either.

I suspect it is probably reasonable to enable these sysctls for
all users of the system to use, but can we please show the reason
for each sysctl why it is safe?

Thank you.

Eric

> Signed-off-by: Alexander Kuznetsov <wwfq@...dex-team.ru>
> Acked-by: Dmitry Yakunin <zeil@...dex-team.ru>
> ---
>  net/ipv6/route.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b6ddf23..de85e3b 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -6415,10 +6415,6 @@ struct ctl_table * __net_init ipv6_route_sysctl_init(struct net *net)
>  		table[8].data = &net->ipv6.sysctl.ip6_rt_min_advmss;
>  		table[9].data = &net->ipv6.sysctl.ip6_rt_gc_min_interval;
>  		table[10].data = &net->ipv6.sysctl.skip_notify_on_dev_down;
> -
> -		/* Don't export sysctls to unprivileged users */
> -		if (net->user_ns != &init_user_ns)
> -			table[0].procname = NULL;
>  	}
>  
>  	return table;