[PATCH v3] tun: Require CAP_NET_ADMIN to attach to an ownerless device

[PATCH v3] tun: Require CAP_NET_ADMIN to attach to an ownerless device

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4965 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

Twenty years ago I made creating a tuntap device require CAP_NET_ADMIN
in commit ca6bb5d7ab22 ("[NET]: Require CAP_NET_ADMIN to create tuntap
devices.")

The idea was to allow the permissions on /dev/net/tun to be relaxed to
0666 and thus allow owners of tun devices assigned with TUNSETOWNER to
actually open it and get at their devices.

What we missed at the time was that persistent devices with *no* owner
were already essentially world-consumable, and anyone who could open
the device node could also attach to those.

Even then, using persistent devices with no explicitly set owner — on
the (false) presumption that CAP_NET_ADMIN would be required to open
them — wasn't an egregious userspace bug. There are cases where it does
make sense to separate the creation/configuration of the netdevice, from
the process which shovels packets into it.

Since then, TUNSETGROUP was added a year later in commit 8c644623fe7e
("[NET]: Allow group ownership of TUN/TAP devices."). Its commit message
noted that if both uid and gid are set, both have to match (in itself a
bizarre choice) — but still not that with *neither* set, no permission
at all was required.

Then in 2012 commit c8d68e6be1c3 ("tuntap: multiqueue support") allowed
new queues to be attached to a device from the moment of its creation,
with no way for userspace to close the race between creation and a
subsequent TUNSET{OWNER,GROUP}.

In 2024, commit 3ca459eaba1bf ("tun: fix group permission check") tried
to fix things to be saner: only require the user *or* the group to
match, in line with standard POSIX permissions, and require
CAP_NET_ADMIN if neither the user nor group authorised the access
(although it put the CAP_NET_ADMIN check first, and would wrongly have
reported the capability as having been used even when it was not).

That fix was reverted in commit a70c7b3cbc06 ("tun: revert fix group
permission check") due to allegedly causing a regression in an selinux
test suite which *expected* the attachment to succeed when authorised
by none of user, group, nor CAP_NET_ADMIN:
https://lore.kernel.org/netdev/CAFqZXNtkCBT4f+PwyVRmQGoT3p1eVa01fCG_aNtpt6dakXncUg@mail.gmail.com/

We should not break UAPI. But we don't have to consider an selinux test
case which deliberately exercises a security flaw to be UAPI worth
preserving either. To which end, I have fixed it in selinux-testsuite
commit b2f894f0016b ("tests/tun_tap: set an owner on the device for the
relabel tests").

So: fix the lack of CAP_NET_ADMIN check on devices which *haven't* been
assigned an owner or group, while leaving the weird tun ownership model
as it was (user *and* group have to match, if both are set). If someone
still wants to fix the latter (which wasn't actually the reason the
previous attempt was reverted), they can do it under separate cover.

Permit attaching to an existing device only if an owner and/or group was
set and the caller matches all that were set, or if the caller has
CAP_NET_ADMIN. The only behavioural change is that a device with no
owner or group now requires CAP_NET_ADMIN to attach — precisely the
loophole we want to close.

And put the CAP_NET_ADMIN check in the right place this time.

Fixes: ca6bb5d7ab22 ("[NET]: Require CAP_NET_ADMIN to create tuntap devices.")
Reported-by: Andrew Lutomirski <luto@kernel.org>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Cc: stable@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 5a302709a68a..0002e1598e69 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -513,14 +513,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_permitted(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 both uid and gid are set, *both* must match */
+	if ((uid_valid(tun->owner) || gid_valid(tun->group)) &&
+	    (!uid_valid(tun->owner) || uid_eq(cred->euid, tun->owner)) &&
+	    (!gid_valid(tun->group) || in_egroup_p(tun->group)))
+		return true;
+
+	return ns_capable(net->user_ns, CAP_NET_ADMIN);
 }
 
 static void tun_set_real_num_queues(struct tun_struct *tun)
@@ -2872,7 +2876,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_permitted(tun))
 			return -EPERM;
 		err = security_tun_dev_open(tun->security);
 		if (err < 0)
-- 
2.43.0


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]

Re: [PATCH v3] tun: Require CAP_NET_ADMIN to attach to an ownerless device

[Willem de Bruijn]

David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Twenty years ago I made creating a tuntap device require CAP_NET_ADMIN
> in commit ca6bb5d7ab22 ("[NET]: Require CAP_NET_ADMIN to create tuntap
> devices.")
> 
> The idea was to allow the permissions on /dev/net/tun to be relaxed to
> 0666 and thus allow owners of tun devices assigned with TUNSETOWNER to
> actually open it and get at their devices.
> 
> What we missed at the time was that persistent devices with *no* owner
> were already essentially world-consumable, and anyone who could open
> the device node could also attach to those.
> 
> Even then, using persistent devices with no explicitly set owner — on
> the (false) presumption that CAP_NET_ADMIN would be required to open
> them — wasn't an egregious userspace bug. There are cases where it does
> make sense to separate the creation/configuration of the netdevice, from
> the process which shovels packets into it.
> 
> Since then, TUNSETGROUP was added a year later in commit 8c644623fe7e
> ("[NET]: Allow group ownership of TUN/TAP devices."). Its commit message
> noted that if both uid and gid are set, both have to match (in itself a
> bizarre choice) — but still not that with *neither* set, no permission
> at all was required.
> 
> Then in 2012 commit c8d68e6be1c3 ("tuntap: multiqueue support") allowed
> new queues to be attached to a device from the moment of its creation,
> with no way for userspace to close the race between creation and a
> subsequent TUNSET{OWNER,GROUP}.
> 
> In 2024, commit 3ca459eaba1bf ("tun: fix group permission check") tried
> to fix things to be saner: only require the user *or* the group to
> match, in line with standard POSIX permissions, and require
> CAP_NET_ADMIN if neither the user nor group authorised the access
> (although it put the CAP_NET_ADMIN check first, and would wrongly have
> reported the capability as having been used even when it was not).
> 
> That fix was reverted in commit a70c7b3cbc06 ("tun: revert fix group
> permission check") due to allegedly causing a regression in an selinux
> test suite which *expected* the attachment to succeed when authorised
> by none of user, group, nor CAP_NET_ADMIN:
> https://lore.kernel.org/netdev/CAFqZXNtkCBT4f+PwyVRmQGoT3p1eVa01fCG_aNtpt6dakXncUg@mail.gmail.com/
> 
> We should not break UAPI. But we don't have to consider an selinux test
> case which deliberately exercises a security flaw to be UAPI worth
> preserving either. To which end, I have fixed it in selinux-testsuite
> commit b2f894f0016b ("tests/tun_tap: set an owner on the device for the
> relabel tests").
> 
> So: fix the lack of CAP_NET_ADMIN check on devices which *haven't* been
> assigned an owner or group, while leaving the weird tun ownership model
> as it was (user *and* group have to match, if both are set). If someone
> still wants to fix the latter (which wasn't actually the reason the
> previous attempt was reverted), they can do it under separate cover.
> 
> Permit attaching to an existing device only if an owner and/or group was
> set and the caller matches all that were set, or if the caller has
> CAP_NET_ADMIN. The only behavioural change is that a device with no
> owner or group now requires CAP_NET_ADMIN to attach — precisely the
> loophole we want to close.
> 
> And put the CAP_NET_ADMIN check in the right place this time.
> 
> Fixes: ca6bb5d7ab22 ("[NET]: Require CAP_NET_ADMIN to create tuntap devices.")
> Reported-by: Andrew Lutomirski <luto@kernel.org>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@vger.kernel.org

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