Re: Possible mistake in commit 3ca459eaba1b ("tun: fix group permission check")

[Ondrej Mosnacek <omosnace@redhat.com>]

On Mon, Jan 27, 2025 at 3:50 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> stsp wrote:
> > 27.01.2025 12:10, Ondrej Mosnacek пишет:
> > > Hello,
> > >
> > > It looks like the commit in $SUBJ may have introduced an unintended
> > > change in behavior. According to the commit message, the intent was to
> > > require just one of {user, group} to match instead of both, which
> > > sounds reasonable, but the commit also changes the behavior for when
> > > neither of tun->owner and tun->group is set. Before the commit the
> > > access was always allowed, while after the commit CAP_NET_ADMIN is
> > > required in this case.
> > >
> > > I'm asking because the tun_tap subtest of selinux-testuite [1] started
> > > to fail after this commit (it assumed CAP_NET_ADMIN was not needed),
> > > so I'm trying to figure out if we need to change the test or if it
> > > needs to be fixed in the kernel.
> > >
> > > Thanks,
> > >
> > > [1] https://github.com/SELinuxProject/selinux-testsuite/
> > >
> > Hi, IMHO having the persistent
> > TAP device inaccessible by anyone
> > but the CAP_NET_ADMIN is rather
> > useless, so the compatibility should
> > be restored on the kernel side.
> > I'd raise the questions about adding
> > the CAP_NET_ADMIN checks into
> > TUNSETOWNER and/or TUNSETPERSIST,
> > but this particular change to TUNSETIFF,
> > at least on my side, was unintentional.
> >
> > Sorry about that. :(
>
> Thanks for the report Ondrej.
>
> Agreed that we need to reinstate this. I suggest this explicit
> extra branch after the more likely cases:
>
>         @@ -585,6 +585,9 @@ static inline bool tun_capable(struct tun_struct *tun)
>                         return 1;
>                 if (gid_valid(tun->group) && in_egroup_p(tun->group))
>                         return 1;
>         +       if (!uid_valid(tun->owner) && !gid_valid(tun->group))
>         +               return 1;
>         +
>                 return 0;
>          }

That could work, but the semantics become a bit weird, actually: When
you set both uid and gid, one of them needs to match. If you unset
uid/gid, you get a stricter condition (gid/uid must match). And if you
then also unset the other one, you suddenly get a less strict
condition than the first two - nothing has to match. Might be
acceptable, but it may confuse people unless well documented.

Also there is another smaller issue in the new code that I forgot to
mention - with LSMs (such as SELinux) the ns_capable() call will
produce an audit record when the capability is denied by an LSM. These
audit records are meant to indicate that the permission was needed but
denied and that the policy was either breached or needs to be
adjusted. Therefore, the ns_capable() call should ideally only happen
after the user/group checks so that only accesses that actually
wouldn't succeed without the capability yield an audit record.

So I would suggest this version:

static inline bool tun_capable(struct tun_struct *tun)
{
    const struct cred *cred = current_cred();
    struct net *net = dev_net(tun->dev);

    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;
    if (!uid_valid(tun->owner) && !gid_valid(tun->group))
        return 1;
    return ns_capable(net->user_ns, CAP_NET_ADMIN);
}

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.