[PATCH net-next] tun: fix group permission check

[PATCH net-next] tun: fix group permission check

[Stas Sergeev]

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@yandex.ru>

CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Andrew Lunn <andrew+netdev@lunn.ch>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: netdev@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/net/tun.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9a0f6eb32016..d35b6a48d138 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -574,14 +574,18 @@ static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
 	return ret;
 }
 
-static inline bool tun_not_capable(struct tun_struct *tun)
+static inline bool tun_capable(struct tun_struct *tun)
 {
 	const struct cred *cred = current_cred();
 	struct net *net = dev_net(tun->dev);
 
-	return ((uid_valid(tun->owner) && !uid_eq(cred->euid, tun->owner)) ||
-		  (gid_valid(tun->group) && !in_egroup_p(tun->group))) &&
-		!ns_capable(net->user_ns, CAP_NET_ADMIN);
+	if (ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return 1;
+	if (uid_valid(tun->owner) && uid_eq(cred->euid, tun->owner))
+		return 1;
+	if (gid_valid(tun->group) && in_egroup_p(tun->group))
+		return 1;
+	return 0;
 }
 
 static void tun_set_real_num_queues(struct tun_struct *tun)
@@ -2778,7 +2782,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		    !!(tun->flags & IFF_MULTI_QUEUE))
 			return -EINVAL;
 
-		if (tun_not_capable(tun))
+		if (!tun_capable(tun))
 			return -EPERM;
 		err = security_tun_dev_open(tun->security);
 		if (err < 0)
-- 
2.47.0

Re: [PATCH net-next] tun: fix group permission check

[Willem de Bruijn]

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

Re: [PATCH net-next] tun: fix group permission check

[Willem de Bruijn]

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

Re: [PATCH net-next] tun: fix group permission check

[stsp]

17.11.2024 18:04, Willem de Bruijn пишет:
> 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@yandex.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.

So what would you suggest?
Added Guido Guenther <agx@sigxcpu.org> to CC
for an opinion.
The main problem here is that by
setting user and group properly, you
end up with device inaccessible by
anyone, unless the user belongs to
the tun group. I don't think someone
wants to set up inaccessible devices,
so this property doesn't seem useful.
OTOH if the user does have that group
in his list, then, AFAICT, adding such
group to tun changes nothing: neither
limits nor extends the scope.
If you had group already set and you
set also user, then you limit the scope,
but its the same as just setting user alone.
So I really can't think of any valid usage
scenario of setting both tun user and tun
group.

Re: [PATCH net-next] tun: fix group permission check

[Paolo Abeni]

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

Re: [PATCH net-next] tun: fix group permission check

[stsp]

19.11.2024 13:51, Paolo Abeni пишет:
> 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
Noted, thanks.

Re: [PATCH net-next] tun: fix group permission check

[Willem de Bruijn]

stsp wrote:
> 17.11.2024 18:04, Willem de Bruijn пишет:
> > 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@yandex.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.
> 
> So what would you suggest?
> Added Guido Guenther <agx@sigxcpu.org> to CC
> for an opinion.
> The main problem here is that by
> setting user and group properly, you
> end up with device inaccessible by
> anyone, unless the user belongs to
> the tun group. I don't think someone
> wants to set up inaccessible devices,
> so this property doesn't seem useful.
> OTOH if the user does have that group
> in his list, then, AFAICT, adding such
> group to tun changes nothing: neither
> limits nor extends the scope.
> If you had group already set and you
> set also user, then you limit the scope,
> but its the same as just setting user alone.
> So I really can't think of any valid usage
> scenario of setting both tun user and tun
> group.

Understood. If no one comments before the window reopens, I think it's
fine to just resubmit.

[PATCH net-next] tun: fix group permission check

[Stas Sergeev]

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@yandex.ru>

CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Andrew Lunn <andrew+netdev@lunn.ch>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: netdev@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/net/tun.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9a0f6eb32016..d35b6a48d138 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -574,14 +574,18 @@ static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
 	return ret;
 }
 
-static inline bool tun_not_capable(struct tun_struct *tun)
+static inline bool tun_capable(struct tun_struct *tun)
 {
 	const struct cred *cred = current_cred();
 	struct net *net = dev_net(tun->dev);
 
-	return ((uid_valid(tun->owner) && !uid_eq(cred->euid, tun->owner)) ||
-		  (gid_valid(tun->group) && !in_egroup_p(tun->group))) &&
-		!ns_capable(net->user_ns, CAP_NET_ADMIN);
+	if (ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return 1;
+	if (uid_valid(tun->owner) && uid_eq(cred->euid, tun->owner))
+		return 1;
+	if (gid_valid(tun->group) && in_egroup_p(tun->group))
+		return 1;
+	return 0;
 }
 
 static void tun_set_real_num_queues(struct tun_struct *tun)
@@ -2778,7 +2782,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		    !!(tun->flags & IFF_MULTI_QUEUE))
 			return -EINVAL;
 
-		if (tun_not_capable(tun))
+		if (!tun_capable(tun))
 			return -EPERM;
 		err = security_tun_dev_open(tun->security);
 		if (err < 0)
-- 
2.47.0

Re: [PATCH net-next] tun: fix group permission check

[Willem de Bruijn]

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@yandex.ru>
> 
> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Andrew Lunn <andrew+netdev@lunn.ch>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jakub Kicinski <kuba@kernel.org>
> CC: Paolo Abeni <pabeni@redhat.com>
> CC: netdev@vger.kernel.org
> CC: linux-kernel@vger.kernel.org

Reviewed-by: Willem de Bruijn <willemb@google.com>

A lot more readable this way too.

Re: [PATCH net-next] tun: fix group permission check

[Jason Wang]

On Fri, Dec 6, 2024 at 12:50 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> 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@yandex.ru>
> >
> > CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > CC: Jason Wang <jasowang@redhat.com>
> > CC: Andrew Lunn <andrew+netdev@lunn.ch>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Eric Dumazet <edumazet@google.com>
> > CC: Jakub Kicinski <kuba@kernel.org>
> > CC: Paolo Abeni <pabeni@redhat.com>
> > CC: netdev@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
>
> A lot more readable this way too.
>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH net-next] tun: fix group permission check

[Jakub Kicinski]

On Thu,  5 Dec 2024 10:36:14 +0300 Stas Sergeev wrote:
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> 
> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>

Please avoid empty lines in the future.
I personally put a --- line between SOB and the CCs.
That way git am discards the CCs when patch is applied.

Re: [PATCH net-next] tun: fix group permission check

[Jakub Kicinski]

On Sat, 7 Dec 2024 17:44:00 -0800 Jakub Kicinski wrote:
> On Thu,  5 Dec 2024 10:36:14 +0300 Stas Sergeev wrote:
> > Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> > 
> > CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>  
> 
> Please avoid empty lines in the future.

I mean empty lines between tags, to be clear

Re: [PATCH net-next] tun: fix group permission check

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  5 Dec 2024 10:36:14 +0300 you 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.
> 
> [...]

Here is the summary with links:
  - [net-next] tun: fix group permission check
    https://git.kernel.org/netdev/net-next/c/3ca459eaba1b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] tun: fix group permission check

[stsp]

08.12.2024 04:44, Jakub Kicinski пишет:
> On Thu,  5 Dec 2024 10:36:14 +0300 Stas Sergeev wrote:
>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>>
>> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Please avoid empty lines in the future.

Ok.

> I personally put a --- line between SOB and the CCs.
> That way git am discards the CCs when patch is applied.
>
I simply used the output of
get_maintainer.pl and copy/pasted
it directly to commit msg.
After doing so, git format-patch
would put --- after CCs.
How to do that properly?

Re: [PATCH net-next] tun: fix group permission check

[Jakub Kicinski]

On Sun, 8 Dec 2024 09:53:40 +0300 stsp wrote:
> > I personally put a --- line between SOB and the CCs.
> > That way git am discards the CCs when patch is applied.
> >  
> I simply used the output of
> get_maintainer.pl and copy/pasted
> it directly to commit msg.
> After doing so, git format-patch
> would put --- after CCs.
> How to do that properly?

You can have multiple --- markers, you can insert your marker and let
git format-patch add another.

Re: [PATCH net-next] tun: fix group permission check

[stsp]

10.12.2024 00:44, Jakub Kicinski пишет:
> On Sun, 8 Dec 2024 09:53:40 +0300 stsp wrote:
>>> I personally put a --- line between SOB and the CCs.
>>> That way git am discards the CCs when patch is applied.
>>>   
>> I simply used the output of
>> get_maintainer.pl and copy/pasted
>> it directly to commit msg.
>> After doing so, git format-patch
>> would put --- after CCs.
>> How to do that properly?
> You can have multiple --- markers, you can insert your marker and let
> git format-patch add another.

Thank you.