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] [thread-next>] [day] [month] [year] [list]

Message-ID: <efceaa29-93d0-482a-95d9-28b176c1ffbc@yandex.ru>
Date: Mon, 3 Feb 2025 22:30:23 +0300
From: stsp <stsp2@...dex.ru>
To: 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

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.

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