netdev - Re: [PATCH] selinux: Fix error priority for bind with AF_UNSPEC on AF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHC9VhQcBEmF_pL9pk0O84aWyrZxSX9y1i0i=c+a3dQQawgEMA@mail.gmail.com>
Date: Fri, 29 Dec 2023 16:41:30 -0500
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Eric Paris <eparis@...isplace.org>, Stephen Smalley <stephen.smalley.work@...il.com>, 
	Günther Noack <gnoack@...gle.com>, 
	Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, 
	Muhammad Usama Anjum <usama.anjum@...labora.com>, linux-security-module@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH] selinux: Fix error priority for bind with AF_UNSPEC on
 AF_INET6 socket

On Fri, Dec 29, 2023 at 12:19 PM Mickaël Salaün <mic@...ikod.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@...ikod.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@...cle.com>
> > > Cc: Eric Paris <eparis@...isplace.org>
> > > Cc: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
> > > Cc: Paul Moore <paul@...l-moore.com>
> > > Cc: Stephen Smalley <stephen.smalley.work@...il.com>
> > > Reported-by: Muhammad Usama Anjum <usama.anjum@...labora.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@...ikod.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