| 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
| ||
|
Message-ID: <4f949dcc-7757-0f02-758b-2509f6be0c6d@schaufler-ca.com>
Date: Thu, 11 Apr 2019 09:45:17 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
linux-security-module <linux-security-module@...r.kernel.org>
Cc: David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
syzbot+0049bebbf3042dbd2e8f@...kaller.appspotmail.com,
syzbot+915c9f99f3dbc4bd6cd1@...kaller.appspotmail.com,
casey@...aufler-ca.com
Subject: Re: [PATCH] net: socket: Always initialize family field at
move_addr_to_kernel().
On 4/11/2019 4:31 AM, Tetsuo Handa wrote:
> On 2019/04/04 13:49, David Miller wrote:
>> From: Tetsuo Handa <penguin-kernel@...ove.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(-)
Except as noted below this looks fine for Smack.
> ...
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 5c16135..7c19c04 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2805,13 +2805,21 @@ static int smack_socket_socketpair(struct socket *socka,
> *
> * Records the label bound to a port.
> *
> - * Returns 0
> + * Returns 0 on success, and error code otherwise
> */
> static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
> int addrlen)
> {
> - if (sock->sk != NULL && sock->sk->sk_family == PF_INET6)
> + if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
> + /*
> + * Reject if valid length is too short for IPv6 address or
> + * address family is not IPv6.
> + */
> + if (addr_len < SIN6_LEN_RFC2133 ||
+ if (addrlen < SIN6_LEN_RFC2133 ||
> + address->sa_family != AF_INET6)
> + return -EINVAL;
> smk_ipv6_port_label(sock, address);
> + }
> return 0;
> }
> #endif /* SMACK_IPV6_PORT_LABELING */
> @@ -2847,12 +2855,21 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
>
> switch (sock->sk->sk_family) {
> case PF_INET:
> - if (addrlen < sizeof(struct sockaddr_in))
> + /*
> + * Reject if valid length is too short for IPv4 address or
> + * address family is not IPv4.
> + */
> + if (addrlen < sizeof(struct sockaddr_in) ||
> + sap->sa_family != AF_INET)
> return -EINVAL;
> rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
> break;
> case PF_INET6:
> - if (addrlen < sizeof(struct sockaddr_in6))
> + /*
> + * Reject if valid length is too short for IPv6 address or
> + * address family is not IPv6.
> + */
> + if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
> return -EINVAL;
> #ifdef SMACK_IPV6_SECMARK_LABELING
> rsp = smack_ipv6host_label(sip);
> @@ -3682,9 +3699,23 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>
> switch (sock->sk->sk_family) {
> case AF_INET:
> + /*
> + * Reject if valid length is too short for IPv4 address or
> + * address family is not IPv4.
> + */
> + if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
> + sip->sin_family != AF_INET)
> + return -EINVAL;
> rc = smack_netlabel_send(sock->sk, sip);
> break;
> case AF_INET6:
> + /*
> + * Reject if valid length is too short for IPv6 address or
> + * address family is not IPv6.
> + */
> + if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
> + sap->sin6_family != AF_INET6)
> + return -EINVAL;
> #ifdef SMACK_IPV6_SECMARK_LABELING
> rsp = smack_ipv6host_label(sap);
> if (rsp != NULL)
> diff --git a/security/tomoyo/network.c b/security/tomoyo/network.c
> index 9094f4b..3cbd6bd 100644
> --- a/security/tomoyo/network.c
> +++ b/security/tomoyo/network.c
> @@ -505,6 +505,12 @@ static int tomoyo_check_inet_address(const struct sockaddr *addr,
> {
> struct tomoyo_inet_addr_info *i = &address->inet;
>
> + /*
> + * Nothing more to do if valid length is too short to check
> + * address->sa_family.
> + */
> + if (addr_len < offsetofend(struct sockaddr, sa_family))
> + return 0;
> switch (addr->sa_family) {
> case AF_INET6:
> if (addr_len < SIN6_LEN_RFC2133)
> @@ -594,6 +600,12 @@ static int tomoyo_check_unix_address(struct sockaddr *addr,
> {
> struct tomoyo_unix_addr_info *u = &address->unix0;
>
> + /*
> + * Nothing more to do if valid length is too short to check
> + * address->sa_family.
> + */
> + if (addr_len < offsetofend(struct sockaddr, sa_family))
> + return 0;
> if (addr->sa_family != AF_UNIX)
> return 0;
> u->addr = ((struct sockaddr_un *) addr)->sun_path;
Powered by blists - more mailing lists