Re: [PATCH] net: socket: Always initialize family field at move_addr_to_kernel().

[Paul Moore <paul@paul-moore.com>]

On Thu, Apr 11, 2019 at 7:32 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2019/04/04 13:49, David Miller wrote:
> > From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> > Date: Wed, 3 Apr 2019 06:07:40 +0900
> >
> >> On 2019/04/03 5:23, David Miller wrote:
> >>> Please fix RDS and other protocols to examine the length properly
> >>> instead.
> >>
> >> Do you prefer adding branches only for allow reading the family of socket address?
> >
> > If the length is zero, there is no point in reading the family.
> >
>
> (Adding LSM people.)
>
> syzbot is reporting that RDS is not checking valid length of address given from userspace.
> It turned out that there are several users who access "struct sockaddr"->family without
> checking valid length (which will be reported by KMSAN).
>
> Unfortunately, since tipc_bind() in net/tipc/socket.c accepts length == 0 as a valid input,
> we can't reject length < offsetofend(struct sockaddr, sa_family) with -EINVAL at
> move_addr_to_kernel() which are called from bind()/connect() system calls. I proposed
> always setting "struct sockaddr"->family at move_addr_to_kernel() but David does not
> like such trick.
>
> Therefore, LSM modules which checks address and/or port have to check valid length
> before accessing "struct sockaddr"->family in order to determine IPv4 or IPv6 or UNIX.
>
> Below is all-in-one change (for x86_64 allmodconfig). Well, I've just realized that
> move_addr_to_kernel() is also called by sendmsg() system call and for now added changes
> for only security/ directory. Is this change appropriate for you? (I wish we could
> simplify this by automatically initializing "struct sockaddr_storage" with 0 at
> move_addr_to_kernel()...)
> ---
>  drivers/isdn/mISDN/socket.c |  4 ++--
>  net/bluetooth/sco.c         |  4 ++--
>  net/core/filter.c           |  2 ++
>  net/ipv6/udp.c              |  2 ++
>  net/llc/af_llc.c            |  3 +--
>  net/netlink/af_netlink.c    |  3 ++-
>  net/rds/af_rds.c            |  3 +++
>  net/rds/bind.c              |  2 ++
>  net/rxrpc/af_rxrpc.c        |  3 ++-
>  net/sctp/socket.c           |  3 ++-
>  security/apparmor/lsm.c     |  6 ++++++
>  security/selinux/hooks.c    | 12 ++++++++++++
>  security/smack/smack_lsm.c  | 39 +++++++++++++++++++++++++++++++++++----
>  security/tomoyo/network.c   | 12 ++++++++++++
>  14 files changed, 85 insertions(+), 13 deletions(-)

Some minor nits/comments below, but I think this is still okay as-is
from a SELinux perspective.

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d5fdcb0..710d386 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4503,6 +4503,12 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>         err = sock_has_perm(sk, SOCKET__BIND);
>         if (err)
>                 goto out;
> +       /*
> +        * Nothing more to do if valid length is too short to check
> +        * address->sa_family.
> +        */
> +       if (addrlen < offsetofend(struct sockaddr, sa_family))
> +               goto out;

SELinux already checks the address length further down in
selinux_socket_bind() for the address families it care about, although
it does read sa_family before the address length check so no
unfortunately it's of no help.

I imagine you could move this new length check inside the
PF_INET/PF_INET6 if-then code block to minimize the impact to other
address families, but I suppose there is some value in keeping it
where it is too.

>         /* If PF_INET or PF_INET6, check name_bind permission for the port. */
>         family = sk->sk_family;
> @@ -4634,6 +4640,12 @@ static int selinux_socket_connect_helper(struct socket *sock,
>         err = sock_has_perm(sk, SOCKET__CONNECT);
>         if (err)
>                 return err;
> +       /*
> +        * Nothing more to do if valid length is too short to check
> +        * address->sa_family.
> +        */
> +       if (addrlen < offsetofend(struct sockaddr, sa_family))
> +               return 0;

Similar comments as above, including moving the check inside the if-then block.

>         /*
>          * If a TCP, DCCP or SCTP socket, check name_connect permission

-- 
paul moore
www.paul-moore.com