Message-ID: <cebcfa4f-885f-45de-b629-165d521a2756@schaufler-ca.com>
Date: Wed, 27 Mar 2024 09:41:24 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Mickaël Salaün <mic@...ikod.net>,
 Paul Moore <paul@...l-moore.com>
Cc: linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
 Alexey Kodanev <alexey.kodanev@...cle.com>,
 Eric Dumazet <edumazet@...gle.com>, Günther Noack
 <gnoack@...gle.com>, Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>,
 Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
 Muhammad Usama Anjum <usama.anjum@...labora.com>,
 "Serge E . Hallyn" <serge@...lyn.com>,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v1 1/2] lsm: Check and handle error priority for
 socket_bind and socket_connect

On 3/27/2024 5:00 AM, 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.
>
> 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@...cle.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Günther Noack <gnoack@...gle.com>
> Cc: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
> Cc: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
> Cc: Muhammad Usama Anjum <usama.anjum@...labora.com>
> Cc: Paul Moore <paul@...l-moore.com>
> Cc: Serge E. Hallyn <serge@...lyn.com>
> Fixes: 20510f2f4e2d ("security: Convert LSM into a static interface")
> Signed-off-by: Mickaël Salaün <mic@...ikod.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;
> +	}

Seems like a clunky way to say:

	if (sock_family != PF_INET && sock_family != PF_INET6)
		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;

The smack_socket_connect() code is among my ugliest. I don't think this
would do any harm, but if you haven't run the Smack test suite you probably
should.

> +
>  	return call_int_hook(socket_connect, sock, address, addrlen);
>  }
>  

Powered by blists - more mailing lists