On Thu, Apr 11, 2019 at 7:32 AM Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp> wrote: > On 2019/04/04 13:49, David Miller wrote: > > From: Tetsuo Handa <penguin-ker...@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 <p...@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