linux-kernel - Re: [PATCH 2/2] proc: use generic setattr() for /proc/$PID/net

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: <20230713-abitur-alliierte-93460a210b1b@brauner>
Date:   Thu, 13 Jul 2023 13:49:39 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Thomas Weißschuh <linux@...ssschuh.net>
Cc:     Willy Tarreau <w@....eu>, Shuah Khan <shuah@...nel.org>,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, Zhangjin Wu <falcon@...ylab.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 2/2] proc: use generic setattr() for /proc/$PID/net

On Sat, Jun 24, 2023 at 12:30:47PM +0200, Thomas Weißschuh wrote:
> All other files in /proc/$PID/ use proc_setattr().
> 
> Not using it allows the usage of chmod() on /proc/$PID/net, even on
> other processes owned by the same user.
> The same would probably also be true for other attributes to be changed.
> 
> As this technically represents an ABI change it is not marked for
> stable so any unlikely regressions are caught during a full release cycle.
> 
> Fixes: e9720acd728a ("[NET]: Make /proc/net a symlink on /proc/self/net (v3)")
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> ---
>  fs/proc/proc_net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
> index a0c0419872e3..78f9e6b469c0 100644
> --- a/fs/proc/proc_net.c
> +++ b/fs/proc/proc_net.c
> @@ -321,6 +321,7 @@ static int proc_tgid_net_getattr(struct mnt_idmap *idmap,
>  const struct inode_operations proc_net_inode_operations = {
>  	.lookup		= proc_tgid_net_lookup,
>  	.getattr	= proc_tgid_net_getattr,
> +	.setattr        = proc_setattr,
>  };
>  
>  static int proc_tgid_net_readdir(struct file *file, struct dir_context *ctx)

So your concern really is specifically about /proc/$pid/net itself as
that's owned by the user and thus the user itself can chmod it and thus
also restrict access for other processess running with the same uid:

chmod 0000 /proc/1234/net
ls -al /proc/self/net
ls: cannot open directory '/proc/self/net/': Permission denied

Yeah, it's not a huge deal but it's arguably a bug especially since the
original commit from 2006 that introduced proc_setattr() was clear that
it should apply to anything beneath /proc/<pid>/ owned by the user.

So I agree and we should probably try and have the same behavior for
/proc/$pid/net as well. We can see if that breaks something.