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

Mikhail Ivanov <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx> · Thu, 3 Oct 2024 15:15:25 +0300

On 9/27/2024 5:35 PM, Günther Noack wrote:
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@xxxxxxxxxxxxxxxxxxx>
---
  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?

Ofc, thank you


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@xxxxxxxxxx>







