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

Message-ID: <d772d47e-5dc0-4295-a302-e17e75ca8dd1@redhat.com>
Date: Tue, 19 Nov 2024 11:51:56 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Stas Sergeev <stsp2@...dex.ru>, 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>, netdev@...r.kernel.org, agx@...xcpu.org,
 jdike@...ux.intel.com
Subject: Re: [PATCH net-next] tun: fix group permission check

On 11/18/24 22:40, Willem de Bruijn wrote:
> Willem de Bruijn wrote:
>> 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.
> 
> I should clarify that I'm not against bringing this file in line with
> normal user/group behavior.
> 
> Just want to give anyone a chance to speak up if they disagree and/or
> recall why the code was originally written as it is.

I think we can't accept a behaviour changing patch this late in the
cycle. If an agreement is reached it should be reposted after the merge
window.

/P