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

[stsp <stsp2@yandex.ru>]

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.

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