netdev - Re: [PATCH net] tun: revert 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: <67a149227c100_46ecd29438@willemb.c.googlers.com.notmuch>
Date: Mon, 03 Feb 2025 17:54:26 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: stsp <stsp2@...dex.ru>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 netdev@...r.kernel.org
Cc: davem@...emloft.net, 
 kuba@...nel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 jasowang@...hat.com, 
 Willem de Bruijn <willemb@...gle.com>, 
 Ondrej Mosnacek <omosnace@...hat.com>
Subject: Re: [PATCH net] tun: revert fix group permission check

stsp wrote:
> 03.02.2025 22:09, Willem de Bruijn пишет:
> > stsp wrote:
> >> 03.02.2025 18:05, Willem de Bruijn пишет:
> >>> From: Willem de Bruijn <willemb@...gle.com>
> >>>
> >>> This reverts commit 3ca459eaba1bf96a8c7878de84fa8872259a01e3.
> >>>
> >>> The blamed commit caused a regression when neither tun->owner nor
> >>> tun->group is set. This is intended to be allowed, but now requires
> >>> CAP_NET_ADMIN.
> >>>
> >>> Discussion in the referenced thread pointed out that the original
> >>> issue that prompted this patch can be resolved in userspace.
> >> The point of the patch was
> >> not to fix userspace, but this
> >> bug: when you have owner set,
> >> then adding group either changes
> >> nothing at all, or removes all
> >> access. I.e. there is no valid case
> >> for adding group when owner
> >> already set.
> > As long as no existing users are affected, no need to relax this after
> > all these years.
> 
> I only mean the wording.
> My patch initially says what
> exactly does it fix, so the fact
> that the problem can be fixed
> in user-space, was likely obvious
> from the very beginning.
> 
> >> During the discussion it became
> >> obvious that simpler fixes may
> >> exist (like eg either-or semantic),
> >> so why not to revert based on
> >> that?
> > We did not define either-or in detail. Do you mean failing the
> > TUNSETOWNER or TUNSETGROUP ioctl if the other is already set?
> 
> I mean, auto-removing group when
> the owner is being set, for example.
> Its not a functionality change: the
> behaviour is essentially as before,
> except no such case when no one
> can access the device.
> 
> >>> The relaxed access control may now make a device accessible when it
> >>> previously wasn't, while existing users may depend on it to not be.
> >>>
> >>> Since the fix is not critical and introduces security risk, revert,
> >> Well, I don't agree with that justification.
> >> My patch introduced the usability
> >> problem, but not a security risk.
> >> I don't want to be attributed with
> >> the security risk when this wasn't
> >> the case (to the very least, you
> >> still need the perms to open /dev/net/tun),
> >> so could you please remove that part?
> >> I don't think you need to exaggerate
> >> anything: it introduces the usability
> >> regression, which should be enough
> >> for any instant revert.
> > This is not intended to cast blame, of course.
> >
> > That said, I can adjust the wording.
> 
> Would be good.

Will do.
 
> > The access control that we relaxed is when a process is not allowed
> > to access a device until the administrator adds it to the right group.
> 
> No-no, adding doesn't help.
> The process have to die and
> re-login. Besides, not only the
> "process" can't access the device,
> no. Everyone can't. And by the
> mere fact of adding a group...

A device can be created with owner/group constraints before the
intended process (and session) exists.