[PATCH] selinux: Fix error priority for bind with AF_UNSPEC on AF_INET6 socket

[Paul Moore]

On Fri, Dec 29, 2023 at 12:19 PM Mickaël Salaün <mic at digikod.net> wrote:
>
> (Removing Alexey Kodanev because the related address is no longer
> valid.)
>
> On Thu, Dec 28, 2023 at 07:19:07PM -0500, Paul Moore wrote:
> > On Thu, Dec 28, 2023 at 6:39 AM Mickaël Salaün <mic at digikod.net> wrote:
> > >
> > > The IPv6 network stack first checks the sockaddr length (-EINVAL error)
> > > before checking the family (-EAFNOSUPPORT error).
> > >
> > > This was discovered thanks to commit a549d055a22e ("selftests/landlock:
> > > Add network tests").
> > >
> > > Cc: Alexey Kodanev <alexey.kodanev at oracle.com>
> > > Cc: Eric Paris <eparis at parisplace.org>
> > > Cc: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
> > > Cc: Paul Moore <paul at paul-moore.com>
> > > Cc: Stephen Smalley <stephen.smalley.work at gmail.com>
> > > Reported-by: Muhammad Usama Anjum <usama.anjum at collabora.com>
> > > Closes: https://lore.kernel.org/r/0584f91c-537c-4188-9e4f-04f192565667@collabora.com
> > > Fixes: 0f8db8cc73df ("selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()")
> > > Signed-off-by: Mickaël Salaün <mic at digikod.net>
> > > ---
> > >  security/selinux/hooks.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index feda711c6b7b..9fc55973d765 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -4667,6 +4667,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
> > >                                 return -EINVAL;
> > >                         addr4 = (struct sockaddr_in *)address;
> > >                         if (family_sa == AF_UNSPEC) {
> > > +                               if (sock->sk->__sk_common.skc_family ==
> > > +                                           AF_INET6 &&
> > > +                                   addrlen < SIN6_LEN_RFC2133)
> > > +                                       return -EINVAL;
> >
> > Please use sock->sk_family to simplify the conditional above, or
> > better yet, use the local variable @family as it is set to the sock's
> > address family near the top of selinux_socket_bind()
>
> Correct, I'll send a v2 with that.
>
> > ... although, as
> > I'm looking at the existing code, is this patch necessary?
> >
> > At the top of the AF_UNSPEC/AF_INET case there is an address length check:
> >
> >   if (addrlen < sizeof(struct sockaddr_in))
> >     return -EINVAL;
>
> This code is correct but not enough in the case of an IPv6 socket.

Okay, I see now.  Let me follow-up in your v2, we may want to fix this
another way.

-- 
paul-moore.com