| Message ID | 20231228113917.62089-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: Fix error priority for bind with AF_UNSPEC on AF_INET6 socket | expand |
On Thu, Dec 28, 2023 at 6:39 AM Mickaël Salaün <mic@digikod.net> wrote: > > The IPv6 network stack first checks the sockaddr length (-EINVAL error) > before checking the family (-EAFNOSUPPORT error). > > This was discovered thanks to commit a549d055a22e ("selftests/landlock: > Add network tests"). > > Cc: Alexey Kodanev <alexey.kodanev@oracle.com> > Cc: Eric Paris <eparis@parisplace.org> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > Closes: https://lore.kernel.org/r/0584f91c-537c-4188-9e4f-04f192565667@collabora.com > Fixes: 0f8db8cc73df ("selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()") > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > security/selinux/hooks.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index feda711c6b7b..9fc55973d765 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4667,6 +4667,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in > return -EINVAL; > addr4 = (struct sockaddr_in *)address; > if (family_sa == AF_UNSPEC) { > + if (sock->sk->__sk_common.skc_family == > + AF_INET6 && > + addrlen < SIN6_LEN_RFC2133) > + return -EINVAL; Please use sock->sk_family to simplify the conditional above, or better yet, use the local variable @family as it is set to the sock's address family near the top of selinux_socket_bind() ... although, as I'm looking at the existing code, is this patch necessary? At the top of the AF_UNSPEC/AF_INET case there is an address length check: if (addrlen < sizeof(struct sockaddr_in)) return -EINVAL; ... which I believe should be performing the required sockaddr length check (and it is checking for IPv4 address lengths not IPv6 as in the patch). I see that we have a similar check for AF_INET6, so we should be covered there as well. I'm probably still in a bit of a holiday fog, can you help me see what I'm missing here? > /* see __inet_bind(), we only want to allow > * AF_UNSPEC if the address is INADDR_ANY > */ > -- > 2.43.0
(Removing Alexey Kodanev because the related address is no longer valid.) On Thu, Dec 28, 2023 at 07:19:07PM -0500, Paul Moore wrote: > On Thu, Dec 28, 2023 at 6:39 AM Mickaël Salaün <mic@digikod.net> wrote: > > > > The IPv6 network stack first checks the sockaddr length (-EINVAL error) > > before checking the family (-EAFNOSUPPORT error). > > > > This was discovered thanks to commit a549d055a22e ("selftests/landlock: > > Add network tests"). > > > > Cc: Alexey Kodanev <alexey.kodanev@oracle.com> > > Cc: Eric Paris <eparis@parisplace.org> > > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > > Cc: Paul Moore <paul@paul-moore.com> > > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > Closes: https://lore.kernel.org/r/0584f91c-537c-4188-9e4f-04f192565667@collabora.com > > Fixes: 0f8db8cc73df ("selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()") > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > --- > > security/selinux/hooks.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index feda711c6b7b..9fc55973d765 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -4667,6 +4667,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in > > return -EINVAL; > > addr4 = (struct sockaddr_in *)address; > > if (family_sa == AF_UNSPEC) { > > + if (sock->sk->__sk_common.skc_family == > > + AF_INET6 && > > + addrlen < SIN6_LEN_RFC2133) > > + return -EINVAL; > > Please use sock->sk_family to simplify the conditional above, or > better yet, use the local variable @family as it is set to the sock's > address family near the top of selinux_socket_bind() Correct, I'll send a v2 with that. > ... although, as > I'm looking at the existing code, is this patch necessary? > > At the top of the AF_UNSPEC/AF_INET case there is an address length check: > > if (addrlen < sizeof(struct sockaddr_in)) > return -EINVAL; This code is correct but not enough in the case of an IPv6 socket. > > ... which I believe should be performing the required sockaddr length > check (and it is checking for IPv4 address lengths not IPv6 as in the > patch). I see that we have a similar check for AF_INET6, so we should > be covered there as well. The existing similar check (addrlen < SIN6_LEN_RFC2133) is when the af_family is AF_INET6, but this patch adds a check for AF_UNSPEC on an PF_INET6 socket. The IPv6 network stack first checks that the addrlen is valid for an IPv6 address even if the requested af_family is AF_UNSPEC, hence this patch. > > I'm probably still in a bit of a holiday fog, can you help me see what > I'm missing here? The tricky part is that AF_UNSPEC can be checked against the PF_INET or the PF_INET6 socket implementations, and the return error code may not be the same according to addrlen, especially when sizeof(struct sockaddr_in) < addrlen < SIN6_LEN_RFC2133 The (new) Landlock network tests check this kind of corner case to make sure the same error codes are return with and without a Landlock sandbox. Muhammad reported that some of these tests failed on KernelCI and I found that, when SELinux is enabled (which is the case with the defconfig), SElinux gets the request after Landlock and returns a wrong error code (before the network stack can do anything). See tools/testing/selftests/landlock/net_test.c +728 which checks with and without a Landlock sandbox. I tested this patch with SELinux and Landlock enabled, and all the Landlock tests pass. I'm working on a more global approach to cover all LSMs, with more checks and Landlock tests, but this will be more complex and then will take more time to review.
On Fri, Dec 29, 2023 at 12:19 PM Mickaël Salaün <mic@digikod.net> wrote: > > (Removing Alexey Kodanev because the related address is no longer > valid.) > > On Thu, Dec 28, 2023 at 07:19:07PM -0500, Paul Moore wrote: > > On Thu, Dec 28, 2023 at 6:39 AM Mickaël Salaün <mic@digikod.net> wrote: > > > > > > The IPv6 network stack first checks the sockaddr length (-EINVAL error) > > > before checking the family (-EAFNOSUPPORT error). > > > > > > This was discovered thanks to commit a549d055a22e ("selftests/landlock: > > > Add network tests"). > > > > > > Cc: Alexey Kodanev <alexey.kodanev@oracle.com> > > > Cc: Eric Paris <eparis@parisplace.org> > > > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > > > Cc: Paul Moore <paul@paul-moore.com> > > > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > > > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > > Closes: https://lore.kernel.org/r/0584f91c-537c-4188-9e4f-04f192565667@collabora.com > > > Fixes: 0f8db8cc73df ("selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()") > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > --- > > > security/selinux/hooks.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index feda711c6b7b..9fc55973d765 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -4667,6 +4667,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in > > > return -EINVAL; > > > addr4 = (struct sockaddr_in *)address; > > > if (family_sa == AF_UNSPEC) { > > > + if (sock->sk->__sk_common.skc_family == > > > + AF_INET6 && > > > + addrlen < SIN6_LEN_RFC2133) > > > + return -EINVAL; > > > > Please use sock->sk_family to simplify the conditional above, or > > better yet, use the local variable @family as it is set to the sock's > > address family near the top of selinux_socket_bind() > > Correct, I'll send a v2 with that. > > > ... although, as > > I'm looking at the existing code, is this patch necessary? > > > > At the top of the AF_UNSPEC/AF_INET case there is an address length check: > > > > if (addrlen < sizeof(struct sockaddr_in)) > > return -EINVAL; > > This code is correct but not enough in the case of an IPv6 socket. Okay, I see now. Let me follow-up in your v2, we may want to fix this another way.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index feda711c6b7b..9fc55973d765 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4667,6 +4667,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in return -EINVAL; addr4 = (struct sockaddr_in *)address; if (family_sa == AF_UNSPEC) { + if (sock->sk->__sk_common.skc_family == + AF_INET6 && + addrlen < SIN6_LEN_RFC2133) + return -EINVAL; /* see __inet_bind(), we only want to allow * AF_UNSPEC if the address is INADDR_ANY */
The IPv6 network stack first checks the sockaddr length (-EINVAL error) before checking the family (-EAFNOSUPPORT error). This was discovered thanks to commit a549d055a22e ("selftests/landlock: Add network tests"). Cc: Alexey Kodanev <alexey.kodanev@oracle.com> Cc: Eric Paris <eparis@parisplace.org> Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> Closes: https://lore.kernel.org/r/0584f91c-537c-4188-9e4f-04f192565667@collabora.com Fixes: 0f8db8cc73df ("selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()") Signed-off-by: Mickaël Salaün <mic@digikod.net> --- security/selinux/hooks.c | 4 ++++ 1 file changed, 4 insertions(+)