netdev - Re: [PATCH net-next] tun: fix group permission check

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: <673ca7102dba5_2a097e2948f@willemb.c.googlers.com.notmuch>
Date: Tue, 19 Nov 2024 09:56:16 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: stsp <stsp2@...dex.ru>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 linux-kernel@...r.kernel.org
Cc: Jason Wang <jasowang@...hat.com>, 
 Andrew Lunn <andrew+netdev@...n.ch>, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 netdev@...r.kernel.org, 
 agx@...xcpu.org, 
 jdike@...ux.intel.com, 
 Guido Guenther <agx@...xcpu.org>
Subject: Re: [PATCH net-next] tun: fix group permission check

stsp wrote:
> 17.11.2024 18:04, Willem de Bruijn пишет:
> > Stas Sergeev wrote:
> >> Currently tun checks the group permission even if the user have matched.
> >> Besides going against the usual permission semantic, this has a
> >> very interesting implication: if the tun group is not among the
> >> supplementary groups of the tun user, then effectively no one can
> >> access the tun device. CAP_SYS_ADMIN still can, but its the same as
> >> not setting the tun ownership.
> >>
> >> This patch relaxes the group checking so that either the user match
> >> or the group match is enough. This avoids the situation when no one
> >> can access the device even though the ownership is properly set.
> >>
> >> Also I simplified the logic by removing the redundant inversions:
> >> tun_not_capable() --> !tun_capable()
> >>
> >> Signed-off-by: Stas Sergeev <stsp2@...dex.ru>
> > This behavior goes back through many patches to commit 8c644623fe7e:
> >
> >      [NET]: Allow group ownership of TUN/TAP devices.
> >
> >      Introduce a new syscall TUNSETGROUP for group ownership setting of tap
> >      devices. The user now is allowed to send packages if either his euid or
> >      his egid matches the one specified via tunctl (via -u or -g
> >      respecitvely). If both, gid and uid, are set via tunctl, both have to
> >      match.
> >
> > The choice evidently was on purpose. Even if indeed non-standard.
> 
> So what would you suggest?
> Added Guido Guenther <agx@...xcpu.org> to CC
> for an opinion.
> The main problem here is that by
> setting user and group properly, you
> end up with device inaccessible by
> anyone, unless the user belongs to
> the tun group. I don't think someone
> wants to set up inaccessible devices,
> so this property doesn't seem useful.
> OTOH if the user does have that group
> in his list, then, AFAICT, adding such
> group to tun changes nothing: neither
> limits nor extends the scope.
> If you had group already set and you
> set also user, then you limit the scope,
> but its the same as just setting user alone.
> So I really can't think of any valid usage
> scenario of setting both tun user and tun
> group.

Understood. If no one comments before the window reopens, I think it's
fine to just resubmit.