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

[Ivanov Mikhail]

On 3/27/2024 3:00 PM, Mickaël Salaün wrote:
> 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.
It would probably be better to allow the network stack to perform such
checks before calling LSM hooks. This may lead to following improvements:

1. Fixing extra checks. In the current design, (address)checks are
    performed both in validate_inet_addr() function and
    in network stack methods.

2. The network stack can choose which error cases should not be hidden
    during the LSM access check, and which ones can be.

3. LSM will not be responsible for performing all necessary checks
    for every (necessary) protocol.

This may result in adding new method to socket->ops.
>
> 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);
>   }
>