netdev - Re: [PATCH] net: always initialize sysctl ownership

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240316003958.65385-1-kuniyu@amazon.com>
Date: Fri, 15 Mar 2024 17:39:58 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <linux@...ssschuh.net>
CC: <davem@...emloft.net>, <dmitry.torokhov@...il.com>,
	<ebiederm@...ssion.com>, <edumazet@...gle.com>, <j.granados@...sung.com>,
	<kuba@...nel.org>, <linux-kernel@...r.kernel.org>, <mcgrof@...nel.org>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <kuniyu@...zon.com>
Subject: Re: [PATCH] net: always initialize sysctl ownership

From: Thomas Weißschuh <linux@...ssschuh.net>
Date: Fri, 15 Mar 2024 17:20:31 +0100
> The sysctl core does not initialize these fields when the set_ownership
> callback is present.
> So always do it in the callback.

Could you add few more words here to explain what the problem is
like commit 5ec27ec735ba ("fs/proc/proc_sysctl.c: fix the default
values of i_uid/i_gid on /proc/sys inodes.").

BTW, it seems that we can change the value even if the uid/gid is
invalid unlike the issue mentioned in 5ec27ec735ba.

So, the problem is just the invalid uid/gid exposed to userspace
or am I missing something ?

---8<---
[root@...alhost ~]# unshare --user --keep-caps
[nobody@...alhost ~]$ id
uid=65534(nobody) gid=65534(nobody) groups=65534(nobody)
[nobody@...alhost ~]$ unshare --net --keep-caps
[nobody@...alhost ~]$ stat /proc/sys/net/core/somaxconn
  File: /proc/sys/net/core/somaxconn
  Size: 0         	Blocks: 0          IO Block: 1024   regular empty file
Device: 13h/19d	Inode: 3726        Links: 1
Access: (0644/-rw-r--r--)  Uid: (65534/  nobody)   Gid: (65534/  nobody)
Access: 2024-03-16 00:28:51.937789000 +0000
Modify: 2024-03-16 00:28:51.937789000 +0000
Change: 2024-03-16 00:28:51.937789000 +0000
 Birth: -
[nobody@...alhost ~]$ cat /proc/sys/net/core/somaxconn
4096
[nobody@...alhost ~]$ echo 2048 > /proc/sys/net/core/somaxconn
[nobody@...alhost ~]$ cat /proc/sys/net/core/somaxconn
2048
---8<---


> 
> Fixes: e79c6a4fc923 ("net: make net namespace sysctls belong to container's owner")
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> ---
>  net/sysctl_net.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sysctl_net.c b/net/sysctl_net.c
> index 051ed5f6fc93..03e320ddacc9 100644
> --- a/net/sysctl_net.c
> +++ b/net/sysctl_net.c
> @@ -62,12 +62,10 @@ static void net_ctl_set_ownership(struct ctl_table_header *head,
>  	kgid_t ns_root_gid;
>  
>  	ns_root_uid = make_kuid(net->user_ns, 0);
> -	if (uid_valid(ns_root_uid))
> -		*uid = ns_root_uid;
> +	*uid = uid_valid(ns_root_uid) ? ns_root_uid : GLOBAL_ROOT_UID;
>  
>  	ns_root_gid = make_kgid(net->user_ns, 0);
> -	if (gid_valid(ns_root_gid))
> -		*gid = ns_root_gid;
> +	*gid = gid_valid(ns_root_gid) ? ns_root_gid : GLOBAL_ROOT_GID;
>  }

How about setting the default in proc_sys_make_inode() instead ?
because the default value configured by new_inode() is not
appropriate for procfs, IIUC.

Thanks!

>  
>  static struct ctl_table_root net_sysctl_root = {
> 
> ---
> base-commit: e5eb28f6d1afebed4bb7d740a797d0390bd3a357
> change-id: 20240315-sysctl-net-ownership-bc4e17eaeea6
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@...ssschuh.net>