| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Aug 2023 17:13:28 +0300
From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <artem.kuzin@...wei.com>, <gnoack3000@...il.com>,
<willemdebruijn.kernel@...il.com>, <yusongping@...wei.com>,
<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
<netfilter-devel@...r.kernel.org>
Subject: Re: [PATCH] landlock: Fix and test network AF inconsistencies
8/17/2023 4:00 PM, Mickaël Salaün пишет:
> Check af_family consistency while handling AF_UNSPEC specifically.
>
> This patch should be squashed into the "Network support for Landlock"
> v11 patch series.
Thank you so much.
Can I find this patch in
https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux ???
>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> ---
> security/landlock/net.c | 29 ++++-
> tools/testing/selftests/landlock/net_test.c | 124 +++++++++++++-------
> 2 files changed, 108 insertions(+), 45 deletions(-)
>
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index f8d2be53ac0d..ea5373f774f9 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -80,11 +80,11 @@ static int check_socket_access(struct socket *const sock,
> if (WARN_ON_ONCE(domain->num_layers < 1))
> return -EACCES;
>
> - /* Checks if it's a TCP socket. */
> + /* Checks if it's a (potential) TCP socket. */
> if (sock->type != SOCK_STREAM)
> return 0;
>
> - /* Checks for minimal header length. */
> + /* Checks for minimal header length to safely read sa_family. */
> if (addrlen < offsetofend(struct sockaddr, sa_family))
> return -EINVAL;
>
> @@ -106,7 +106,6 @@ static int check_socket_access(struct socket *const sock,
> return 0;
> }
>
> - /* Specific AF_UNSPEC handling. */
> if (address->sa_family == AF_UNSPEC) {
> /*
> * Connecting to an address with AF_UNSPEC dissolves the TCP
> @@ -114,6 +113,10 @@ static int check_socket_access(struct socket *const sock,
> * connection while retaining the socket object (i.e., the file
> * descriptor). As for dropping privileges, closing
> * connections is always allowed.
> + *
> + * For a TCP access control system, this request is legitimate.
> + * Let the network stack handle potential inconsistencies and
> + * return -EINVAL if needed.
> */
> if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP)
> return 0;
> @@ -124,14 +127,34 @@ static int check_socket_access(struct socket *const sock,
> * INADDR_ANY (cf. __inet_bind). Checking the address is
> * required to not wrongfully return -EACCES instead of
> * -EAFNOSUPPORT.
> + *
> + * We could return 0 and let the network stack handle these
> + * checks, but it is safer to return a proper error and test
> + * consistency thanks to kselftest.
> */
> if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
> + /* addrlen has already been checked for AF_UNSPEC. */
> const struct sockaddr_in *const sockaddr =
> (struct sockaddr_in *)address;
>
> + if (sock->sk->__sk_common.skc_family != AF_INET)
> + return -EINVAL;
> +
> if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
> return -EAFNOSUPPORT;
> }
> + } else {
> + /*
> + * 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.
> + *
> + * We could return 0 and let the network stack handle this
> + * check, but it is safer to return a proper error and test
> + * consistency thanks to kselftest.
> + */
> + if (address->sa_family != sock->sk->__sk_common.skc_family)
> + return -EINVAL;
> }
>
> id.key.data = (__force uintptr_t)port;
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index 12dc127ea7d1..504a26c63fd9 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -233,7 +233,7 @@ static int connect_variant(const int sock_fd,
>
> FIXTURE(protocol)
> {
> - struct service_fixture srv0, srv1, srv2, unspec_any, unspec_srv0;
> + struct service_fixture srv0, srv1, srv2, unspec_any0, unspec_srv0;
> };
>
> FIXTURE_VARIANT(protocol)
> @@ -257,8 +257,8 @@ FIXTURE_SETUP(protocol)
>
> ASSERT_EQ(0, set_service(&self->unspec_srv0, prot_unspec, 0));
>
> - ASSERT_EQ(0, set_service(&self->unspec_any, prot_unspec, 0));
> - self->unspec_any.ipv4_addr.sin_addr.s_addr = htonl(INADDR_ANY);
> + ASSERT_EQ(0, set_service(&self->unspec_any0, prot_unspec, 0));
> + self->unspec_any0.ipv4_addr.sin_addr.s_addr = htonl(INADDR_ANY);
>
> setup_loopback(_metadata);
> };
> @@ -615,20 +615,18 @@ TEST_F(protocol, connect)
> // Kernel FIXME: tcp_sandbox_with_ipv6_tcp and tcp_sandbox_with_unix_stream
> TEST_F(protocol, bind_unspec)
> {
> + const struct landlock_ruleset_attr ruleset_attr = {
> + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
> + };
> + const struct landlock_net_service_attr tcp_bind = {
> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
> + .port = self->srv0.port,
> + };
> int bind_fd, ret;
>
> if (variant->sandbox == TCP_SANDBOX) {
> - const struct landlock_ruleset_attr ruleset_attr = {
> - .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
> - };
> - const struct landlock_net_service_attr tcp_bind = {
> - .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
> - .port = self->srv0.port,
> - };
> - int ruleset_fd;
> -
> - ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> - sizeof(ruleset_attr), 0);
> + const int ruleset_fd = landlock_create_ruleset(
> + &ruleset_attr, sizeof(ruleset_attr), 0);
> ASSERT_LE(0, ruleset_fd);
>
> /* Allows bind. */
> @@ -642,8 +640,8 @@ TEST_F(protocol, bind_unspec)
> bind_fd = socket_variant(&self->srv0);
> ASSERT_LE(0, bind_fd);
>
> - /* Binds on AF_UNSPEC/INADDR_ANY. */
> - ret = bind_variant(bind_fd, &self->unspec_any);
> + /* Allowed bind on AF_UNSPEC/INADDR_ANY. */
> + ret = bind_variant(bind_fd, &self->unspec_any0);
> if (variant->prot.domain == AF_INET) {
> EXPECT_EQ(0, ret)
> {
> @@ -655,6 +653,33 @@ TEST_F(protocol, bind_unspec)
> }
> EXPECT_EQ(0, close(bind_fd));
>
> + if (variant->sandbox == TCP_SANDBOX) {
> + const int ruleset_fd = landlock_create_ruleset(
> + &ruleset_attr, sizeof(ruleset_attr), 0);
> + ASSERT_LE(0, ruleset_fd);
> +
> + /* Denies bind. */
> + enforce_ruleset(_metadata, ruleset_fd);
> + EXPECT_EQ(0, close(ruleset_fd));
> + }
> +
> + bind_fd = socket_variant(&self->srv0);
> + ASSERT_LE(0, bind_fd);
> +
> + /* Denied bind on AF_UNSPEC/INADDR_ANY. */
> + ret = bind_variant(bind_fd, &self->unspec_any0);
> + if (variant->prot.domain == AF_INET) {
> + if (is_restricted(&variant->prot, variant->sandbox)) {
> + EXPECT_EQ(-EACCES, ret);
> + } else {
> + EXPECT_EQ(0, ret);
> + }
> + } else {
> + EXPECT_EQ(-EINVAL, ret);
> + }
> + EXPECT_EQ(0, close(bind_fd));
> +
> + /* Checks bind with AF_UNSPEC and the loopback address. */
> bind_fd = socket_variant(&self->srv0);
> ASSERT_LE(0, bind_fd);
> ret = bind_variant(bind_fd, &self->unspec_srv0);
> @@ -671,34 +696,16 @@ TEST_F(protocol, bind_unspec)
>
> TEST_F(protocol, connect_unspec)
> {
> + const struct landlock_ruleset_attr ruleset_attr = {
> + .handled_access_net = LANDLOCK_ACCESS_NET_CONNECT_TCP,
> + };
> + const struct landlock_net_service_attr tcp_connect = {
> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
> + .port = self->srv0.port,
> + };
> int bind_fd, client_fd, status;
> pid_t child;
>
> - if (variant->sandbox == TCP_SANDBOX) {
> - const struct landlock_ruleset_attr ruleset_attr = {
> - .handled_access_net = LANDLOCK_ACCESS_NET_CONNECT_TCP,
> - };
> - const struct landlock_net_service_attr tcp_connect = {
> - .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
> - .port = self->srv0.port,
> - };
> - int ruleset_fd;
> -
> - ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> - sizeof(ruleset_attr), 0);
> - ASSERT_LE(0, ruleset_fd);
> -
> - /* Allows connect. */
> - ASSERT_EQ(0, landlock_add_rule(ruleset_fd,
> - LANDLOCK_RULE_NET_SERVICE,
> - &tcp_connect, 0));
> - enforce_ruleset(_metadata, ruleset_fd);
> - EXPECT_EQ(0, close(ruleset_fd));
> - }
> -
> - /* Generic connection tests. */
> - test_bind_and_connect(_metadata, &self->srv0, false, false);
> -
> /* Specific connection tests. */
> bind_fd = socket_variant(&self->srv0);
> ASSERT_LE(0, bind_fd);
> @@ -726,8 +733,22 @@ TEST_F(protocol, connect_unspec)
> EXPECT_EQ(0, ret);
> }
>
> + if (variant->sandbox == TCP_SANDBOX) {
> + const int ruleset_fd = landlock_create_ruleset(
> + &ruleset_attr, sizeof(ruleset_attr), 0);
> + ASSERT_LE(0, ruleset_fd);
> +
> + /* Allows connect. */
> + ASSERT_EQ(0,
> + landlock_add_rule(ruleset_fd,
> + LANDLOCK_RULE_NET_SERVICE,
> + &tcp_connect, 0));
> + enforce_ruleset(_metadata, ruleset_fd);
> + EXPECT_EQ(0, close(ruleset_fd));
> + }
> +
> /* Disconnects already connected socket, or set peer. */
> - ret = connect_variant(connect_fd, &self->unspec_any);
> + ret = connect_variant(connect_fd, &self->unspec_any0);
> if (self->srv0.protocol.domain == AF_UNIX &&
> self->srv0.protocol.type == SOCK_STREAM) {
> EXPECT_EQ(-EINVAL, ret);
> @@ -744,6 +765,25 @@ TEST_F(protocol, connect_unspec)
> EXPECT_EQ(0, ret);
> }
>
> + if (variant->sandbox == TCP_SANDBOX) {
> + const int ruleset_fd = landlock_create_ruleset(
> + &ruleset_attr, sizeof(ruleset_attr), 0);
> + ASSERT_LE(0, ruleset_fd);
> +
> + /* Denies connect. */
> + enforce_ruleset(_metadata, ruleset_fd);
> + EXPECT_EQ(0, close(ruleset_fd));
> + }
> +
> + ret = connect_variant(connect_fd, &self->unspec_any0);
> + if (self->srv0.protocol.domain == AF_UNIX &&
> + self->srv0.protocol.type == SOCK_STREAM) {
> + EXPECT_EQ(-EINVAL, ret);
> + } else {
> + /* Always allowed to disconnect. */
> + EXPECT_EQ(0, ret);
> + }
> +
> EXPECT_EQ(0, close(connect_fd));
> _exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
> return;
Powered by blists - more mailing lists