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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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>