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

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

[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/

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

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

[stsp]

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. :(

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

[Willem de Bruijn]

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;
         }

The intent clearly has always been to allow access if owner and group
are not explicitly set.

It's easy to see when group support was added in commit 8c644623fe7e
("[NET]: Allow group ownership of TUN/TAP devices."), and the even
simpler check before that:

                /* Check permissions */
-               if (tun->owner != -1 &&
-                   current->euid != tun->owner && !capable(CAP_NET_ADMIN))
+               if (((tun->owner != -1 &&
+                     current->euid != tun->owner) ||
+                    (tun->group != -1 &&
+                     current->egid != tun->group)) &&
+                    !capable(CAP_NET_ADMIN))
                        return -EPERM;

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

[stsp]

27.01.2025 17:50, Willem de Bruijn пишет:
> 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;
>           }
>
> The intent clearly has always been to allow access if owner and group
> are not explicitly set.
Perfectly fine with me.
I'd raise the question about
the security implications, but
definitely not within this
regression subject.

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

[Ondrej Mosnacek]

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.

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

[stsp]

28.01.2025 17:20, Ondrej Mosnacek пишет:
> 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.
Maybe this means that
unsetting with -1 is something
that shouldn't be done and/or
allowed?
In this case you only stricten.
Modulo the inability to set both
user/group at the same time,
so you still get "less strict" when
setting group after user already
set...

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

[stsp]

28.01.2025 17:45, stsp пишет:
> 28.01.2025 17:20, Ondrej Mosnacek пишет:
>> 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.
> Maybe this means that
> unsetting with -1 is something
> that shouldn't be done and/or
> allowed?
> In this case you only stricten.
> Modulo the inability to set both
> user/group at the same time,
> so you still get "less strict" when
> setting group after user already
> set...

It may actually be possible to
add the ioctl to set both at once.
In this case you also reset both
(with the same ioctl or add another
one for resetting both), which
makes the problem fully solved.

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

[Willem de Bruijn]

Ondrej Mosnacek wrote:
> 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).

I don't follow this point.

Judging from the history, the intent was that

- if user is set, then it must match.
- if group is set, then it must match.

And I think the group constraint was added with the idea that no one
would try to use both constraints at the same time.

The referenced patch intended to (only) relax the condition when both
are set after all.

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

I find that ownership is optional and must be set explicitly through
TUNSETOWNER and TUNSETGROUP quite surprising too.

But this is only reverting to long established behavior.

> 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);
> }

Improvement makes sense, thanks.

One more point, based on the problem description in the referenced
patch:

    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.

The intent was to skip the group check if the user matches. Not
necessarily the reverse.

To minimize the impact of the patch, perhaps it can still always deny
if tun->owner is set and does not match. That keeps the group check
iff the owner is not explicitly set as well.
 
> -- 
> Ondrej Mosnacek
> Senior Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
> 

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

[stsp]

28.01.2025 18:04, Willem de Bruijn пишет:
> Ondrej Mosnacek wrote:
>> 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).
> I don't follow this point.
>
> Judging from the history, the intent was that
>
> - if user is set, then it must match.
> - if group is set, then it must match.
>
> And I think the group constraint was added with the idea that no one
> would try to use both constraints at the same time.
>
> The referenced patch intended to (only) relax the condition when both
> are set after all.
>
>> 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.
> I find that ownership is optional and must be set explicitly through
> TUNSETOWNER and TUNSETGROUP quite surprising too.
>
> But this is only reverting to long established behavior.
>
>> 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);
>> }
> Improvement makes sense, thanks.
>
> One more point, based on the problem description in the referenced
> patch:
>
>      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.
>
> The intent was to skip the group check if the user matches. Not
> necessarily the reverse.
>
> To minimize the impact of the patch, perhaps it can still always deny
> if tun->owner is set and does not match. That keeps the group check
> iff the owner is not explicitly set as well.

Doesn't this mean, if the user
is set then group is completely
ignored?
By doing that you indeed avoid
the problem of "completely
inaccessible tap". However, that
breaks my setup, as I really
intended to provide tap to the
owner and the unrelated group.
This is because, eg when setting
a CI job, you can add the needed
user to the needed group, but
you also need to re-login, which
is not always possible. :(

Also completely ignoring group
when the user is set, is somewhat
questionable. At the very least,
perhaps then you need to explicitly
clear the group when the user
is set, to avoid the confusion.
Having "either user or group"
sounds like a sensible semantic,
but its a different semantic.

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

[Willem de Bruijn]

stsp wrote:
> 28.01.2025 18:04, Willem de Bruijn пишет:
> > Ondrej Mosnacek wrote:
> >> 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).
> > I don't follow this point.
> >
> > Judging from the history, the intent was that
> >
> > - if user is set, then it must match.
> > - if group is set, then it must match.
> >
> > And I think the group constraint was added with the idea that no one
> > would try to use both constraints at the same time.
> >
> > The referenced patch intended to (only) relax the condition when both
> > are set after all.
> >
> >> 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.
> > I find that ownership is optional and must be set explicitly through
> > TUNSETOWNER and TUNSETGROUP quite surprising too.
> >
> > But this is only reverting to long established behavior.
> >
> >> 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);
> >> }
> > Improvement makes sense, thanks.
> >
> > One more point, based on the problem description in the referenced
> > patch:
> >
> >      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.
> >
> > The intent was to skip the group check if the user matches. Not
> > necessarily the reverse.
> >
> > To minimize the impact of the patch, perhaps it can still always deny
> > if tun->owner is set and does not match. That keeps the group check
> > iff the owner is not explicitly set as well.
> 
> Doesn't this mean, if the user
> is set then group is completely
> ignored?

Yes

> By doing that you indeed avoid
> the problem of "completely
> inaccessible tap". However, that
> breaks my setup, as I really
> intended to provide tap to the
> owner and the unrelated group.
> This is because, eg when setting
> a CI job, you can add the needed
> user to the needed group, but
> you also need to re-login, which
> is not always possible. :(

Could you leave tun->owner unset?

> Also completely ignoring group
> when the user is set, is somewhat
> questionable. At the very least,
> perhaps then you need to explicitly
> clear the group when the user
> is set, to avoid the confusion.
> Having "either user or group"
> sounds like a sensible semantic,
> but its a different semantic.

True. I think that would have satisfied the intent of adding the
group check at the time, and would have avoided this situation.

But we indeed cannot retroactively restrict allowed behavior.
As that will break users.

Conversely, it might be that an existing user out there depends on
the prior behavior that only a process that matches both user and
group can use the device. Which might be reason for reverting the
patch entirely.

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

[stsp]

29.01.2025 01:59, Willem de Bruijn пишет:
> stsp wrote:
>> By doing that you indeed avoid
>> the problem of "completely
>> inaccessible tap". However, that
>> breaks my setup, as I really
>> intended to provide tap to the
>> owner and the unrelated group.
>> This is because, eg when setting
>> a CI job, you can add the needed
>> user to the needed group, but
>> you also need to re-login, which
>> is not always possible. :(
> Could you leave tun->owner unset?

That's exactly the problem: when
the user is not in the needed group,
then you need to unset _both_.
Unsetting only owner is not enough.
Adding the user to the group is not
enough because then you need to
re-login (bad for CI jobs).

I actually tried to address the
supplementary groups problem:
https://lore.kernel.org/lkml/20241108204102.1752206-1-stsp2@yandex.ru/T/
but nothing came out, so I have
to walk around multiple projects,
talking them into a new semantics
and representing the problems
like this one. If people instead
concentrate on solving the inability
to change the supplementary group
list, nothing like this would ever
happen. :)

>> Also completely ignoring group
>> when the user is set, is somewhat
>> questionable. At the very least,
>> perhaps then you need to explicitly
>> clear the group when the user
>> is set, to avoid the confusion.
>> Having "either user or group"
>> sounds like a sensible semantic,
>> but its a different semantic.
> True. I think that would have satisfied the intent of adding the
> group check at the time, and would have avoided this situation.
>
> But we indeed cannot retroactively restrict allowed behavior.
> As that will break users.
>
> Conversely, it might be that an existing user out there depends on
> the prior behavior that only a process that matches both user and
> group can use the device. Which might be reason for reverting the
> patch entirely.
But this is not an option too, let
me remind the previous situation:
1. If the user is in the group, then
    the group doesn't have any effect
    at all.
2. if the user is not in the group -
    no one can access the device.

"either-or" semantic is a direct fix
to that, as it represents case 1 and
fixes case 2. My semantic covers the
real-world situation of inability to
change the group list, but it needs
further tweaking and discussing.
Applying "either-or" may be feasible,
but the complete revert looks like
returning to a quite broken state.

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

[Willem de Bruijn]

stsp wrote:
> 29.01.2025 01:59, Willem de Bruijn пишет:
> > stsp wrote:
> >> By doing that you indeed avoid
> >> the problem of "completely
> >> inaccessible tap". However, that
> >> breaks my setup, as I really
> >> intended to provide tap to the
> >> owner and the unrelated group.
> >> This is because, eg when setting
> >> a CI job, you can add the needed
> >> user to the needed group, but
> >> you also need to re-login, which
> >> is not always possible. :(
> > Could you leave tun->owner unset?
> 
> That's exactly the problem: when
> the user is not in the needed group,
> then you need to unset _both_.
> Unsetting only owner is not enough.
> Adding the user to the group is not
> enough because then you need to
> re-login (bad for CI jobs).

At some point we can question whether the issue is with the setup,
rather than the kernel mechanism.

Why does your setup have an initial user that lacks the group
permissions of the later processes, and a tun instance that has both
owner and group constraints set?

Can this be fixed in userspace, rather than allow this odd case in the
kernel. Is it baked deeply into common containerization tools, say?
 
> I actually tried to address the
> supplementary groups problem:
> https://lore.kernel.org/lkml/20241108204102.1752206-1-stsp2@yandex.ru/T/
> but nothing came out, so I have
> to walk around multiple projects,
> talking them into a new semantics
> and representing the problems
> like this one. If people instead
> concentrate on solving the inability
> to change the supplementary group
> list, nothing like this would ever
> happen. :)
> 
> >> Also completely ignoring group
> >> when the user is set, is somewhat
> >> questionable. At the very least,
> >> perhaps then you need to explicitly
> >> clear the group when the user
> >> is set, to avoid the confusion.
> >> Having "either user or group"
> >> sounds like a sensible semantic,
> >> but its a different semantic.
> > True. I think that would have satisfied the intent of adding the
> > group check at the time, and would have avoided this situation.
> >
> > But we indeed cannot retroactively restrict allowed behavior.
> > As that will break users.
> >
> > Conversely, it might be that an existing user out there depends on
> > the prior behavior that only a process that matches both user and
> > group can use the device. Which might be reason for reverting the
> > patch entirely.
> But this is not an option too, let
> me remind the previous situation:
> 1. If the user is in the group, then
>     the group doesn't have any effect
>     at all.
> 2. if the user is not in the group -
>     no one can access the device.
> 
> "either-or" semantic is a direct fix
> to that, as it represents case 1 and
> fixes case 2. My semantic covers the
> real-world situation of inability to
> change the group list, but it needs
> further tweaking and discussing.
> Applying "either-or" may be feasible,
> but the complete revert looks like
> returning to a quite broken state.
> 

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

[stsp]

29.01.2025 17:12, Willem de Bruijn пишет:
> stsp wrote:
>> 29.01.2025 01:59, Willem de Bruijn пишет:
>>> stsp wrote:
>>>> By doing that you indeed avoid
>>>> the problem of "completely
>>>> inaccessible tap". However, that
>>>> breaks my setup, as I really
>>>> intended to provide tap to the
>>>> owner and the unrelated group.
>>>> This is because, eg when setting
>>>> a CI job, you can add the needed
>>>> user to the needed group, but
>>>> you also need to re-login, which
>>>> is not always possible. :(
>>> Could you leave tun->owner unset?
>> That's exactly the problem: when
>> the user is not in the needed group,
>> then you need to unset _both_.
>> Unsetting only owner is not enough.
>> Adding the user to the group is not
>> enough because then you need to
>> re-login (bad for CI jobs).
> At some point we can question whether the issue is with the setup,
> rather than the kernel mechanism.
>
> Why does your setup have an initial user that lacks the group
> permissions of the later processes, and a tun instance that has both
> owner and group constraints set?
>
> Can this be fixed in userspace, rather than allow this odd case in the
> kernel. Is it baked deeply into common containerization tools, say?

No-no, its not a real or unfixible
problem. At the end, I can just
drop both group and user ownership
of the TAP, and simply not to care.
My aforementioned attempt to
allow changing suppl groups, was
not directed to this particular case -
inability to change suppl groups
create much bigger problems in
other areas, but my TAP problem
is really very small.
Which is why, eg if you decide to
use "either-or" semantic - fine with
me. I just think that completely
reverting the patch is a sub-optimal
choice, as the previous situation
was even more broken than now.

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

[Willem de Bruijn]

stsp wrote:
> 29.01.2025 17:12, Willem de Bruijn пишет:
> > stsp wrote:
> >> 29.01.2025 01:59, Willem de Bruijn пишет:
> >>> stsp wrote:
> >>>> By doing that you indeed avoid
> >>>> the problem of "completely
> >>>> inaccessible tap". However, that
> >>>> breaks my setup, as I really
> >>>> intended to provide tap to the
> >>>> owner and the unrelated group.
> >>>> This is because, eg when setting
> >>>> a CI job, you can add the needed
> >>>> user to the needed group, but
> >>>> you also need to re-login, which
> >>>> is not always possible. :(
> >>> Could you leave tun->owner unset?
> >> That's exactly the problem: when
> >> the user is not in the needed group,
> >> then you need to unset _both_.
> >> Unsetting only owner is not enough.
> >> Adding the user to the group is not
> >> enough because then you need to
> >> re-login (bad for CI jobs).
> > At some point we can question whether the issue is with the setup,
> > rather than the kernel mechanism.
> >
> > Why does your setup have an initial user that lacks the group
> > permissions of the later processes, and a tun instance that has both
> > owner and group constraints set?
> >
> > Can this be fixed in userspace, rather than allow this odd case in the
> > kernel. Is it baked deeply into common containerization tools, say?
> 
> No-no, its not a real or unfixible
> problem. At the end, I can just
> drop both group and user ownership
> of the TAP, and simply not to care.

In that case the safest course of action is to revert the patch.

It relaxes some access control restrictions that other users may have
come to depend on.

Say, someone expects that no process can use the device until it
adds the user to one of the groups.

It's farfetched, but in cases of access control, err on the side of
caution. Especially retroactively.

> My aforementioned attempt to
> allow changing suppl groups, was
> not directed to this particular case -
> inability to change suppl groups
> create much bigger problems in
> other areas, but my TAP problem
> is really very small.
> Which is why, eg if you decide to
> use "either-or" semantic - fine with
> me. I just think that completely
> reverting the patch is a sub-optimal
> choice, as the previous situation
> was even more broken than now.
> 

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

[Paul Moore]

On Thu, Jan 30, 2025 at 11:48 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> stsp wrote:
> > 29.01.2025 17:12, Willem de Bruijn пишет:
> > > stsp wrote:
> > >> 29.01.2025 01:59, Willem de Bruijn пишет:
> > >>> stsp wrote:
> > >>>> By doing that you indeed avoid
> > >>>> the problem of "completely
> > >>>> inaccessible tap". However, that
> > >>>> breaks my setup, as I really
> > >>>> intended to provide tap to the
> > >>>> owner and the unrelated group.
> > >>>> This is because, eg when setting
> > >>>> a CI job, you can add the needed
> > >>>> user to the needed group, but
> > >>>> you also need to re-login, which
> > >>>> is not always possible. :(
> > >>> Could you leave tun->owner unset?
> > >> That's exactly the problem: when
> > >> the user is not in the needed group,
> > >> then you need to unset _both_.
> > >> Unsetting only owner is not enough.
> > >> Adding the user to the group is not
> > >> enough because then you need to
> > >> re-login (bad for CI jobs).
> > > At some point we can question whether the issue is with the setup,
> > > rather than the kernel mechanism.
> > >
> > > Why does your setup have an initial user that lacks the group
> > > permissions of the later processes, and a tun instance that has both
> > > owner and group constraints set?
> > >
> > > Can this be fixed in userspace, rather than allow this odd case in the
> > > kernel. Is it baked deeply into common containerization tools, say?
> >
> > No-no, its not a real or unfixible
> > problem. At the end, I can just
> > drop both group and user ownership
> > of the TAP, and simply not to care.
>
> In that case the safest course of action is to revert the patch.
>
> It relaxes some access control restrictions that other users may have
> come to depend on.
>
> Say, someone expects that no process can use the device until it
> adds the user to one of the groups.
>
> It's farfetched, but in cases of access control, err on the side of
> caution. Especially retroactively.

If a revert is the best path forward for v6.14, do you think it would
be possible to get this fixed this week, or do you expect it to take
longer?

--
paul-moore.com

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

[Ondrej Mosnacek]

On Tue, Feb 4, 2025 at 1:30 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jan 30, 2025 at 11:48 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> > stsp wrote:
> > > 29.01.2025 17:12, Willem de Bruijn пишет:
> > > > stsp wrote:
> > > >> 29.01.2025 01:59, Willem de Bruijn пишет:
> > > >>> stsp wrote:
> > > >>>> By doing that you indeed avoid
> > > >>>> the problem of "completely
> > > >>>> inaccessible tap". However, that
> > > >>>> breaks my setup, as I really
> > > >>>> intended to provide tap to the
> > > >>>> owner and the unrelated group.
> > > >>>> This is because, eg when setting
> > > >>>> a CI job, you can add the needed
> > > >>>> user to the needed group, but
> > > >>>> you also need to re-login, which
> > > >>>> is not always possible. :(
> > > >>> Could you leave tun->owner unset?
> > > >> That's exactly the problem: when
> > > >> the user is not in the needed group,
> > > >> then you need to unset _both_.
> > > >> Unsetting only owner is not enough.
> > > >> Adding the user to the group is not
> > > >> enough because then you need to
> > > >> re-login (bad for CI jobs).
> > > > At some point we can question whether the issue is with the setup,
> > > > rather than the kernel mechanism.
> > > >
> > > > Why does your setup have an initial user that lacks the group
> > > > permissions of the later processes, and a tun instance that has both
> > > > owner and group constraints set?
> > > >
> > > > Can this be fixed in userspace, rather than allow this odd case in the
> > > > kernel. Is it baked deeply into common containerization tools, say?
> > >
> > > No-no, its not a real or unfixible
> > > problem. At the end, I can just
> > > drop both group and user ownership
> > > of the TAP, and simply not to care.
> >
> > In that case the safest course of action is to revert the patch.
> >
> > It relaxes some access control restrictions that other users may have
> > come to depend on.
> >
> > Say, someone expects that no process can use the device until it
> > adds the user to one of the groups.
> >
> > It's farfetched, but in cases of access control, err on the side of
> > caution. Especially retroactively.
>
> If a revert is the best path forward for v6.14, do you think it would
> be possible to get this fixed this week, or do you expect it to take
> longer?

Willem has already posted patches on netdev [1][2] (thanks!), so I
expect it will be fixed soon.

[1] https://lore.kernel.org/netdev/20250204161015.739430-1-willemdebruijn.kernel@gmail.com/
[2] https://lore.kernel.org/netdev/20250203150615.96810-1-willemdebruijn.kernel@gmail.com/

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

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

[Paul Moore]

On Tue, Feb 4, 2025 at 11:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Feb 4, 2025 at 1:30 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > If a revert is the best path forward for v6.14, do you think it would
> > be possible to get this fixed this week, or do you expect it to take
> > longer?
>
> Willem has already posted patches on netdev [1][2] (thanks!), so I
> expect it will be fixed soon.

Great, thanks everyone!

-- 
paul-moore.com

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

[Willem de Bruijn]

Paul Moore wrote:
> On Tue, Feb 4, 2025 at 11:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Tue, Feb 4, 2025 at 1:30 AM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > If a revert is the best path forward for v6.14, do you think it would
> > > be possible to get this fixed this week, or do you expect it to take
> > > longer?
> >
> > Willem has already posted patches on netdev [1][2] (thanks!), so I
> > expect it will be fixed soon.

You just beated me as I wrote a response :)

> Great, thanks everyone!

The revert has now just been merged to net.

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

[Willem de Bruijn]

Ondrej Mosnacek wrote:
> 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);
> }

The conversation got a bit in the weeds. Which is helpful to
understand how exactly this is being used.

But in short, this patch LGTM.