On 4/11/2019 4:31 AM, Tetsuo Handa 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(-)
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;
