Re: [PATCH net] tun: revert fix group permission check

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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@google.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.