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

Mickaël Salaün mic at digikod.net
Fri Dec 29 17:18:58 UTC 2023


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

> 
> ... which I believe should be performing the required sockaddr length
> check (and it is checking for IPv4 address lengths not IPv6 as in the
> patch).  I see that we have a similar check for AF_INET6, so we should
> be covered there as well.

The existing similar check (addrlen < SIN6_LEN_RFC2133) is when the
af_family is AF_INET6, but this patch adds a check for AF_UNSPEC on an
PF_INET6 socket. The IPv6 network stack first checks that the addrlen is
valid for an IPv6 address even if the requested af_family is AF_UNSPEC,
hence this patch.

> 
> I'm probably still in a bit of a holiday fog, can you help me see what
> I'm missing here?

The tricky part is that AF_UNSPEC can be checked against the PF_INET or
the PF_INET6 socket implementations, and the return error code may not
be the same according to addrlen, especially when
sizeof(struct sockaddr_in) < addrlen < SIN6_LEN_RFC2133

The (new) Landlock network tests check this kind of corner case to make
sure the same error codes are return with and without a Landlock
sandbox. Muhammad reported that some of these tests failed on KernelCI
and I found that, when SELinux is enabled (which is the case with the
defconfig), SElinux gets the request after Landlock and returns a wrong
error code (before the network stack can do anything).
See tools/testing/selftests/landlock/net_test.c +728
which checks with and without a Landlock sandbox.

I tested this patch with SELinux and Landlock enabled, and all the
Landlock tests pass.

I'm working on a more global approach to cover all LSMs, with more
checks and Landlock tests, but this will be more complex and then will
take more time to review.



More information about the Linux-security-module-archive mailing list