Message-ID: <20240819.jooTheeng2Ah@digikod.net>
Date: Mon, 19 Aug 2024 17:38:41 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Tahera Fahimi <fahimitahera@...il.com>
Cc: outreachy@...ts.linux.dev, gnoack@...gle.com, paul@...l-moore.com, 
	jmorris@...ei.org, serge@...lyn.com, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org, bjorn3_gh@...tonmail.com, jannh@...gle.com, 
	netdev@...r.kernel.org
Subject: Re: [PATCH v9 2/5] selftests/Landlock: Abstract unix socket
 restriction tests

On Fri, Aug 16, 2024 at 05:08:26PM -0600, Tahera Fahimi wrote:
> On Fri, Aug 16, 2024 at 11:23:05PM +0200, Mickaël Salaün wrote:
> > Please make sure all subject's prefixes have "landlock", not "Landlock"
> > for consistency with current commits.
> > 
> > On Wed, Aug 14, 2024 at 12:22:20AM -0600, Tahera Fahimi wrote:
> > > The patch introduces Landlock ABI version 6 and has three types of tests
> > > that examines different scenarios for abstract unix socket connection:
> > > 1) unix_socket: base tests of the abstract socket scoping mechanism for a
> > >    landlocked process, same as the ptrace test.
> > > 2) optional_scoping: generates three processes with different domains and
> > >    tests if a process with a non-scoped domain can connect to other
> > >    processes.
> > > 3) unix_sock_special_cases: since the socket's creator credentials are used
> > >    for scoping sockets, this test examines the cases where the socket's
> > >    credentials are different from the process using it.
> > > 
> > > Signed-off-by: Tahera Fahimi <fahimitahera@...il.com>
> > > ---
> > > Changes in versions:
> > > v9:
> > > - Move pathname_address_sockets to a different patch.
> > > - Extend optional_scoping test scenarios.
> > > - Removing hardcoded numbers and using "backlog" instead.
> > 
> > You may have missed some parts of my previous review (e.g. local
> > variables).
> Hi Mickaël,
> Thanks for the feedback. I actually did not apply the local variable for
> the reason below.
> > > V8:
> > > - Move tests to scoped_abstract_unix_test.c file.
> > > - To avoid potential conflicts among Unix socket names in different tests,
> > >   set_unix_address is added to common.h to set different sun_path for Unix sockets.
> > > - protocol_variant and service_fixture structures are also moved to common.h
> > > - Adding pathname_address_sockets to cover all types of address formats
> > >   for unix sockets, and moving remove_path() to common.h to reuse in this test.
> > > V7:
> > > - Introducing landlock ABI version 6.
> > > - Adding some edge test cases to optional_scoping test.
> > > - Using `enum` for different domains in optional_scoping tests.
> > > - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets.
> > > - Modifying inline comments.
> > > V6:
> > > - Introducing optional_scoping test which ensures a sandboxed process with a
> > >   non-scoped domain can still connect to another abstract unix socket(either
> > >   sandboxed or non-sandboxed).
> > > - Introducing unix_sock_special_cases test which tests examines scenarios where
> > >   the connecting sockets have different domain than the process using them.
> > > V4:
> > > - Introducing unix_socket to evaluate the basic scoping mechanism for abstract
> > >   unix sockets.
> > > ---
> > >  tools/testing/selftests/landlock/base_test.c  |   2 +-
> > >  tools/testing/selftests/landlock/common.h     |  38 +
> > >  tools/testing/selftests/landlock/net_test.c   |  31 +-
> > >  .../landlock/scoped_abstract_unix_test.c      | 942 ++++++++++++++++++
> > >  4 files changed, 982 insertions(+), 31 deletions(-)
> > >  create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> > > 
> > 
> > > +static void create_unix_domain(struct __test_metadata *const _metadata)
> > > +{
> > > +	int ruleset_fd;
> > > +	const struct landlock_ruleset_attr ruleset_attr = {
> > > +		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> > > +	};
> > > +
> > > +	ruleset_fd =
> > > +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> > > +	EXPECT_LE(0, ruleset_fd)
> > > +	{
> > > +		TH_LOG("Failed to create a ruleset: %s", strerror(errno));
> > > +	}
> > > +	enforce_ruleset(_metadata, ruleset_fd);
> > > +	EXPECT_EQ(0, close(ruleset_fd));
> > > +}
> > > +
> > > +/* clang-format off */
> > 
> > It should not be required to add this clang-format comment here nor in
> > most places, except variant declarations (that might change if we remove
> > variables though).
> > 
> > > +FIXTURE(unix_socket)
> > > +{
> > > +	struct service_fixture stream_address, dgram_address;
> > > +	int server, client, dgram_server, dgram_client;
> > 
> > These variables don't need to be in the fixture but they should be local
> > instead (and scoped to the if/else condition where they are used).
> > 
> > > +};
> > > +
> > > +/* clang-format on */
> > > +FIXTURE_VARIANT(unix_socket)
> > > +{
> > > +	bool domain_both;
> > > +	bool domain_parent;
> > > +	bool domain_child;
> > > +	bool connect_to_parent;
> > > +};
> > 
> > > +/* 
> > > + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and child,
> > > + * when they have scoped domain or no domain.
> > > + */
> > > +TEST_F(unix_socket, abstract_unix_socket)
> > > +{
> > > +	int status;
> > > +	pid_t child;
> > > +	bool can_connect_to_parent, can_connect_to_child;
> > > +	int err, err_dgram;
> > > +	int pipe_child[2], pipe_parent[2];
> > > +	char buf_parent;
> > > +
> > > +	/*
> > > +	 * can_connect_to_child is true if a parent process can connect to its
> > > +	 * child process. The parent process is not isolated from the child
> > > +	 * with a dedicated Landlock domain.
> > > +	 */
> > > +	can_connect_to_child = !variant->domain_parent;
> > > +	/*
> > > +	 * can_connect_to_parent is true if a child process can connect to its
> > > +	 * parent process. This depends on the child process is not isolated from
> > > +	 * the parent with a dedicated Landlock domain.
> > > +	 */
> > > +	can_connect_to_parent = !variant->domain_child;
> > > +
> > > +	ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> > > +	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> > > +	if (variant->domain_both) {
> > > +		create_unix_domain(_metadata);
> > > +		if (!__test_passed(_metadata))
> > > +			return;
> > > +	}
> > > +
> > > +	child = fork();
> > > +	ASSERT_LE(0, child);
> > > +	if (child == 0) {
> > > +		char buf_child;
> > > +
> > > +		ASSERT_EQ(0, close(pipe_parent[1]));
> > > +		ASSERT_EQ(0, close(pipe_child[0]));
> > > +		if (variant->domain_child)
> > > +			create_unix_domain(_metadata);
> > > +
> > > +		/* Waits for the parent to be in a domain, if any. */
> > > +		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> > > +
> > > +		if (variant->connect_to_parent) {
> > > +			self->client = socket(AF_UNIX, SOCK_STREAM, 0);
> > 
> > int stream_client;
> > 
> > stream_client = socket(AF_UNIX, SOCK_STREAM, 0);
> > 
> > ditto for dgram_client
> > 
> > > +			self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
> > > +
> > > +			ASSERT_NE(-1, self->client);
> > > +			ASSERT_NE(-1, self->dgram_client);
> > > +			ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> > > +
> > > +			err = connect(self->client,
> > > +				      &self->stream_address.unix_addr,
> > > +				      (self->stream_address).unix_addr_len);
> > > +			err_dgram =
> > > +				connect(self->dgram_client,
> > > +					&self->dgram_address.unix_addr,
> > > +					(self->dgram_address).unix_addr_len);
> > > +
> > > +			if (can_connect_to_parent) {
> > > +				EXPECT_EQ(0, err);
> > > +				EXPECT_EQ(0, err_dgram);
> > > +			} else {
> > > +				EXPECT_EQ(-1, err);
> > > +				EXPECT_EQ(-1, err_dgram);
> > > +				EXPECT_EQ(EPERM, errno);
> > > +			}
> > 
> > EXPECT_EQ(0, close(stream_client));
> > EXPECT_EQ(0, close(dgram_client));
> > 
> > > +		} else {
> > > +			self->server = socket(AF_UNIX, SOCK_STREAM, 0);
> > 
> > int stream_server;
> > 
> > server = socket(AF_UNIX, SOCK_STREAM, 0);
> > 
> > ditto for dgram_server
> > 
> > > +			self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
> > > +			ASSERT_NE(-1, self->server);
> > > +			ASSERT_NE(-1, self->dgram_server);
> > > +
> > > +			ASSERT_EQ(0,
> > > +				  bind(self->server,
> > > +				       &self->stream_address.unix_addr,
> > > +				       (self->stream_address).unix_addr_len));
> > > +			ASSERT_EQ(0, bind(self->dgram_server,
> > > +					  &self->dgram_address.unix_addr,
> > > +					  (self->dgram_address).unix_addr_len));
> > > +			ASSERT_EQ(0, listen(self->server, backlog));
> > > +
> > > +			/* signal to parent that child is listening */
> > > +			ASSERT_EQ(1, write(pipe_child[1], ".", 1));
> > > +			/* wait to connect */
> > > +			ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> > 
> > ditto
> > 
> > > +		}
> > > +		_exit(_metadata->exit_code);
> > > +		return;
> > > +	}
> > > +
> > > +	ASSERT_EQ(0, close(pipe_child[1]));
> > > +	ASSERT_EQ(0, close(pipe_parent[0]));
> > > +
> > > +	if (variant->domain_parent)
> > > +		create_unix_domain(_metadata);
> > > +
> > > +	/* Signals that the parent is in a domain, if any. */
> > > +	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> > > +
> > > +	if (!variant->connect_to_parent) {
> > > +		self->client = socket(AF_UNIX, SOCK_STREAM, 0);
> > > +		self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
> > 
> > ditto
> > 
> > > +
> > > +		ASSERT_NE(-1, self->client);
> > > +		ASSERT_NE(-1, self->dgram_client);
> > > +
> > > +		/* Waits for the child to listen */
> > > +		ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
> > > +		err = connect(self->client, &self->stream_address.unix_addr,
> > > +			      (self->stream_address).unix_addr_len);
> > > +		err_dgram = connect(self->dgram_client,
> > > +				    &self->dgram_address.unix_addr,
> > > +				    (self->dgram_address).unix_addr_len);
> > > +
> > > +		if (can_connect_to_child) {
> > > +			EXPECT_EQ(0, err);
> > > +			EXPECT_EQ(0, err_dgram);
> > > +		} else {
> > > +			EXPECT_EQ(-1, err);
> > > +			EXPECT_EQ(-1, err_dgram);
> > > +			EXPECT_EQ(EPERM, errno);
> > > +		}
> > > +		ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> > 
> > ditto
> > 
> > > +	} else {
> > > +		self->server = socket(AF_UNIX, SOCK_STREAM, 0);
> > > +		self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
> > 
> > ditto
> > 
> > > +		ASSERT_NE(-1, self->server);
> > > +		ASSERT_NE(-1, self->dgram_server);
> > > +		ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr,
> > > +				  (self->stream_address).unix_addr_len));
> > > +		ASSERT_EQ(0, bind(self->dgram_server,
> > > +				  &self->dgram_address.unix_addr,
> > > +				  (self->dgram_address).unix_addr_len));
> > > +		ASSERT_EQ(0, listen(self->server, backlog));
> > > +
> > > +		/* signal to child that parent is listening */
> > > +		ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> > 
> > ditto
> I think if I define "server" and "dgram_server" as local variables, then
> in here, we should ensure that the clients connections are finished and
> then close the server sockets. The client can write on the pipe after the
> connection test is finished and then servers can close the sockets, but
> the current version is much easier. Simply when the test is finished,
> the FIXTURE_TEARDOWN closes all the sockets. What do you think about this?

Right, a close() call here would not work, but calling
close(stream_server) and close(dgram_server) at the end of this TEST_F()
will be OK and cleaner than in the teardown.  The scope of these
variable should then be TEST_F() too.

> > > +	}
> > > +
> > > +	ASSERT_EQ(child, waitpid(child, &status, 0));
> > > +
> > > +	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> > > +	    WEXITSTATUS(status) != EXIT_SUCCESS)
> > > +		_metadata->exit_code = KSFT_FAIL;
> > > +}
> > > +

Powered by blists - more mailing lists