Re: [RFC PATCH v3 15/19] selftests/landlock: Test SCTP peeloff restriction

["Günther Noack" <gnoack@google.com>]

On Wed, Sep 04, 2024 at 06:48:20PM +0800, Mikhail Ivanov wrote:
> It is possible to branch off an SCTP UDP association into a separate
> user space UDP socket. Add test validating that such scenario is not
> restricted by Landlock.
> 
> Move setup_loopback() helper from net_test to common.h to use it to
> enable connection in this test.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  tools/testing/selftests/landlock/common.h     |  12 +++
>  tools/testing/selftests/landlock/net_test.c   |  11 --
>  .../testing/selftests/landlock/socket_test.c  | 102 +++++++++++++++++-
>  3 files changed, 113 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 28df49fa22d5..07d959a8ac7b 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -16,6 +16,7 @@
>  #include <sys/types.h>
>  #include <sys/wait.h>
>  #include <unistd.h>
> +#include <sched.h>
>  
>  #include "../kselftest_harness.h"
>  
> @@ -227,3 +228,14 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
>  		TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
>  	}
>  }
> +
> +static void setup_loopback(struct __test_metadata *const _metadata)
> +{
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +	ASSERT_EQ(0, unshare(CLONE_NEWNET));
> +	clear_cap(_metadata, CAP_SYS_ADMIN);
> +
> +	set_ambient_cap(_metadata, CAP_NET_ADMIN);
> +	ASSERT_EQ(0, system("ip link set dev lo up"));
> +	clear_ambient_cap(_metadata, CAP_NET_ADMIN);
> +}
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index f21cfbbc3638..0b8386657c72 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -103,17 +103,6 @@ static int set_service(struct service_fixture *const srv,
>  	return 1;
>  }
>  
> -static void setup_loopback(struct __test_metadata *const _metadata)
> -{
> -	set_cap(_metadata, CAP_SYS_ADMIN);
> -	ASSERT_EQ(0, unshare(CLONE_NEWNET));
> -	clear_cap(_metadata, CAP_SYS_ADMIN);
> -
> -	set_ambient_cap(_metadata, CAP_NET_ADMIN);
> -	ASSERT_EQ(0, system("ip link set dev lo up"));
> -	clear_ambient_cap(_metadata, CAP_NET_ADMIN);
> -}
> -
>  static bool is_restricted(const struct protocol_variant *const prot,
>  			  const enum sandbox_type sandbox)
>  {
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 67db0e1c1121..2ab27196fa3d 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -11,8 +11,11 @@
>  #include <linux/pfkeyv2.h>
>  #include <linux/kcm.h>
>  #include <linux/can.h>
> -#include <linux/in.h>
> +#include <sys/socket.h>
> +#include <stdint.h>
> +#include <linux/sctp.h>
>  #include <sys/prctl.h>
> +#include <arpa/inet.h>
>  
>  #include "common.h"
>  
> @@ -839,4 +842,101 @@ TEST_F(socket_creation, socketpair)
>  	}
>  }
>  
> +static const char loopback_ipv4[] = "127.0.0.1";
> +static const int backlog = 10;
> +static const int loopback_port = 1024;
> +
> +TEST_F(socket_creation, sctp_peeloff)
> +{
> +	int status, ret;
> +	pid_t child;
> +	struct sockaddr_in addr;
> +	int server_fd;
> +
> +	server_fd =
> +		socket(AF_INET, SOCK_SEQPACKET | SOCK_CLOEXEC, IPPROTO_SCTP);
> +	ASSERT_LE(0, server_fd);
> +
> +	addr.sin_family = AF_INET;
> +	addr.sin_port = htons(loopback_port);
> +	addr.sin_addr.s_addr = inet_addr(loopback_ipv4);
> +
> +	ASSERT_EQ(0, bind(server_fd, &addr, sizeof(addr)));
> +	ASSERT_EQ(0, listen(server_fd, backlog));
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		int client_fd;
> +		sctp_peeloff_flags_arg_t peeloff;
> +		socklen_t peeloff_size = sizeof(peeloff);
> +		const struct landlock_ruleset_attr ruleset_attr = {
> +			.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> +		};
> +		struct landlock_socket_attr sctp_socket_create = {
> +			.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> +			.family = AF_INET,
> +			.type = SOCK_SEQPACKET,
> +		};
> +
> +		/* Closes listening socket for the child. */
> +		ASSERT_EQ(0, close(server_fd));
> +
> +		client_fd = socket(AF_INET, SOCK_SEQPACKET | SOCK_CLOEXEC,
> +				   IPPROTO_SCTP);
> +		ASSERT_LE(0, client_fd);
> +
> +		/*
> +		 * Establishes connection between sockets and
> +		 * gets SCTP association id.
> +		 */
> +		ret = setsockopt(client_fd, IPPROTO_SCTP, SCTP_SOCKOPT_CONNECTX,
> +				 &addr, sizeof(addr));
> +		ASSERT_LE(0, ret);
> +
> +		if (self->sandboxed) {
> +			/* Denies creation of SCTP sockets. */
> +			int ruleset_fd = landlock_create_ruleset(
> +				&ruleset_attr, sizeof(ruleset_attr), 0);
> +			ASSERT_LE(0, ruleset_fd);
> +
> +			if (self->allowed) {
> +				ASSERT_EQ(0, landlock_add_rule(
> +						     ruleset_fd,
> +						     LANDLOCK_RULE_SOCKET,
> +						     &sctp_socket_create, 0));
> +			}
> +			enforce_ruleset(_metadata, ruleset_fd);
> +			ASSERT_EQ(0, close(ruleset_fd));
> +		}
> +		/*
> +		 * Branches off current SCTP association into a separate socket
> +		 * and returns it to user space.
> +		 */
> +		peeloff.p_arg.associd = ret;
> +		ret = getsockopt(client_fd, IPPROTO_SCTP, SCTP_SOCKOPT_PEELOFF,
> +				 &peeloff, &peeloff_size);
> +
> +		/*
> +		 * Creation of SCTP socket by branching off existing SCTP association
> +		 * should not be restricted by Landlock.
> +		 */
> +		EXPECT_LE(0, ret);
> +
> +		/* Closes peeloff socket if such was created. */
> +		if (!ret) {
> +			ASSERT_EQ(0, close(peeloff.p_arg.sd));
> +		}

Nit: Should this check for (ret >= 0) instead?

I imagine that getsockopt returns -1 on error, normally,
and that would make it past the EXPECT_LE (even if it logs a failure).


> +		ASSERT_EQ(0, close(client_fd));
> +		_exit(_metadata->exit_code);
> +		return;
> +	}
> +
> +	ASSERT_EQ(child, waitpid(child, &status, 0));
> +	ASSERT_EQ(1, WIFEXITED(status));
> +	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
> +
> +	ASSERT_EQ(0, close(server_fd));
> +}
> +
>  TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 

Reviewed-by: Günther Noack <gnoack@google.com>