[PATCH v1 1/2] lsm: Check and handle error priority for socket_bind and socket_connect
Casey Schaufler
casey at schaufler-ca.com
Wed Mar 27 16:41:24 UTC 2024
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 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;
> + }
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);
> }
>
More information about the Linux-security-module-archive
mailing list