[PATCH v1 1/2] lsm: Check and handle error priority for socket_bind and socket_connect

[Mickaël Salaün]

Because the security_socket_bind and the security_socket_bind hooks are
called before the network stack, it is easy to introduce error code
inconsistencies. Instead of adding new checks to current and future
LSMs, let's fix the related hook instead. The new checks are already
(partially) implemented by SELinux and Landlock, and it should not
change user space behavior but improve error code consistency instead.

The first check is about the minimal sockaddr length according to the
address family. This improves the security of the AF_INET and AF_INET6
sockaddr parsing for current and future LSMs.

The second check is about AF_UNSPEC. This fixes error priority for bind
on PF_INET6 socket when SELinux (and potentially others) is enabled.
Indeed, the IPv6 network stack first checks the sockaddr length (-EINVAL
error) before checking the family (-EAFNOSUPPORT error). See commit
bbf5a1d0e5d0 ("selinux: Fix error priority for bind with AF_UNSPEC on
PF_INET6 socket").

The third check is about consistency between socket family and address
family. Only AF_INET and AF_INET6 are tested (by Landlock tests), so no
other protocols are checked for now.

These new checks should enable to simplify current LSM implementations,
but we may want to first land this patch on all stable branches.

A following patch adds new tests improving AF_UNSPEC test coverage for
Landlock.

Cc: Alexey Kodanev <alexey.kodanev at oracle.com>
Cc: Eric Dumazet <edumazet at google.com>
Cc: Günther Noack <gnoack at google.com>
Cc: Ivanov Mikhail <ivanov.mikhail1 at huawei-partners.com>
Cc: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
Cc: Muhammad Usama Anjum <usama.anjum at collabora.com>
Cc: Paul Moore <paul at paul-moore.com>
Cc: Serge E. Hallyn <serge at hallyn.com>
Fixes: 20510f2f4e2d ("security: Convert LSM into a static interface")
Signed-off-by: Mickaël Salaün <mic at digikod.net>
---
 security/security.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/security/security.c b/security/security.c
index 7e118858b545..64fe07a73b14 100644
--- a/security/security.c
+++ b/security/security.c
@@ -28,7 +28,9 @@
 #include <linux/xattr.h>
 #include <linux/msg.h>
 #include <linux/overflow.h>
+#include <linux/in.h>
 #include <net/flow.h>
+#include <net/ipv6.h>
 
 /* How many LSMs were built into the kernel? */
 #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
@@ -4415,6 +4417,82 @@ int security_socket_socketpair(struct socket *socka, struct socket *sockb)
 }
 EXPORT_SYMBOL(security_socket_socketpair);
 
+static int validate_inet_addr(struct socket *sock, struct sockaddr *address,
+			      int addrlen, bool bind)
+{
+	const int sock_family = sock->sk->sk_family;
+
+	/* Checks for minimal header length to safely read sa_family. */
+	if (addrlen < offsetofend(typeof(*address), sa_family))
+		return -EINVAL;
+
+	/* Only handle inet sockets for now. */
+	switch (sock_family) {
+	case PF_INET:
+	case PF_INET6:
+		break;
+	default:
+		return 0;
+	}
+
+	/* Checks minimal address length for inet sockets. */
+	switch (address->sa_family) {
+	case AF_UNSPEC: {
+		const struct sockaddr_in *sa_in;
+
+		/* Cf. inet_dgram_connect(), __inet_stream_connect() */
+		if (!bind)
+			return 0;
+
+		if (sock_family == PF_INET6) {
+			/* Length check from inet6_bind_sk() */
+			if (addrlen < SIN6_LEN_RFC2133)
+				return -EINVAL;
+
+			/* Family check from __inet6_bind() */
+			goto err_af;
+		}
+
+		/* Length check from inet_bind_sk() */
+		if (addrlen < sizeof(struct sockaddr_in))
+			return -EINVAL;
+
+		sa_in = (struct sockaddr_in *)address;
+		if (sa_in->sin_addr.s_addr != htonl(INADDR_ANY))
+			goto err_af;
+
+		return 0;
+	}
+	case AF_INET:
+		/* Length check from inet_bind_sk() */
+		if (addrlen < sizeof(struct sockaddr_in))
+			return -EINVAL;
+		break;
+	case AF_INET6:
+		/* Length check from inet6_bind_sk() */
+		if (addrlen < SIN6_LEN_RFC2133)
+			return -EINVAL;
+		break;
+	}
+
+	/*
+	 * Checks sa_family consistency to not wrongfully return -EACCES
+	 * instead of -EINVAL.  Valid sa_family changes are only (from AF_INET
+	 * or AF_INET6) to AF_UNSPEC.
+	 */
+	if (address->sa_family != sock_family)
+		return -EINVAL;
+
+	return 0;
+
+err_af:
+	/* SCTP services expect -EINVAL, others -EAFNOSUPPORT. */
+	if (sock->sk->sk_protocol == IPPROTO_SCTP)
+		return -EINVAL;
+
+	return -EAFNOSUPPORT;
+}
+
 /**
  * security_socket_bind() - Check if a socket bind operation is allowed
  * @sock: socket
@@ -4425,11 +4503,23 @@ EXPORT_SYMBOL(security_socket_socketpair);
  * and the socket @sock is bound to the address specified in the @address
  * parameter.
  *
+ * For security reasons and to get consistent error code whatever LSM are
+ * enabled, we first do the same sanity checks against sockaddr as the ones
+ * done by the network stack (executed after hook).  Currently only AF_UNSPEC,
+ * AF_INET, and AF_INET6 are handled.  Please add support for other family
+ * specificities when handled by an LSM.
+ *
  * Return: Returns 0 if permission is granted.
  */
 int security_socket_bind(struct socket *sock,
 			 struct sockaddr *address, int addrlen)
 {
+	int err;
+
+	err = validate_inet_addr(sock, address, addrlen, true);
+	if (err)
+		return err;
+
 	return call_int_hook(socket_bind, sock, address, addrlen);
 }
 
@@ -4447,6 +4537,12 @@ int security_socket_bind(struct socket *sock,
 int security_socket_connect(struct socket *sock,
 			    struct sockaddr *address, int addrlen)
 {
+	int err;
+
+	err = validate_inet_addr(sock, address, addrlen, false);
+	if (err)
+		return err;
+
 	return call_int_hook(socket_connect, sock, address, addrlen);
 }
 
-- 
2.44.0