Message-ID: <CAHC9VhR42y0BaUPB_BgW+8oadDc36xPJRzEqh9Mwqa1RaMMZXQ@mail.gmail.com>
Date: Fri, 29 Mar 2024 17:34:07 -0400
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: linux-security-module@...r.kernel.org, netdev@...r.kernel.org, 
	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>
Subject: Re: [PATCH v1 1/2] lsm: Check and handle error priority for
 socket_bind and socket_connect

On Wed, Mar 27, 2024 at 8:00 AM Mickaël Salaün <mic@...ikod.net> 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.

[Dropping Alexey Kodanev due to email problems]

This isn't something I would want to see backported to the various
stable trees, this is a consolidation and cleanup for future work, not
really a bugfix.  If an individual LSM is currently missing an address
sanity check that should be resolved with a targeted patch that can be
safely backported without affecting other LSMs.

Now, all that doesn't mean I don't think this is a good idea.
Assuming we can't get the network stack to validate addresses before
calling into these LSM hooks, I think this is an improvement over the
current approach.  I would like to see the patchset include individual
patches which do the desired adjustments to the Smack, TOMOYO,
AppArmor, Landlock, and SELinux code now that the sanity checks have
migrated to the LSM layer.  I expect that to be fairly
straightforward, but given all the corner cases I want to make sure
all the individual LSMs are okay with the changes.

> 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(+)

-- 
paul-moore.com

Powered by blists - more mailing lists