Message-ID: <220a19f6-f73c-54ef-1c4d-ce498942f106@huawei-partners.com>
Date: Mon, 23 Sep 2024 15:57:47 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack@...gle.com>
CC: <mic@...ikod.net>, <willemdebruijn.kernel@...il.com>,
	<gnoack3000@...il.com>, <linux-security-module@...r.kernel.org>,
	<netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org>,
	<yusongping@...wei.com>, <artem.kuzin@...wei.com>,
	<konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v3 14/19] selftests/landlock: Test socketpair(2)
 restriction

On 9/18/2024 4:47 PM, Günther Noack wrote:
> On Wed, Sep 04, 2024 at 06:48:19PM +0800, Mikhail Ivanov wrote:
>> Add test that checks the restriction on socket creation using
>> socketpair(2).
>>
>> Add `socket_creation` fixture to configure sandboxing in tests in
>> which different socket creation actions are tested.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 101 ++++++++++++++++++
>>   1 file changed, 101 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index 8fc507bf902a..67db0e1c1121 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -738,4 +738,105 @@ TEST_F(packet_protocol, alias_restriction)
>>   	EXPECT_EQ(0, test_socket_variant(&self->prot_tested));
>>   }
>>   
>> +static int test_socketpair(int family, int type, int protocol)
>> +{
>> +	int fds[2];
>> +	int err;
>> +
>> +	err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
>> +	if (err)
>> +		return errno;
>> +	/*
>> +	 * Mixing error codes from close(2) and socketpair(2) should not lead to
>> +	 * any (access type) confusion for this test.
>> +	 */
>> +	if (close(fds[0]) != 0)
>> +		return errno;
>> +	if (close(fds[1]) != 0)
>> +		return errno;
>> +	return 0;
>> +}
>> +
>> +FIXTURE(socket_creation)
>> +{
>> +	bool sandboxed;
>> +	bool allowed;
>> +};
>> +
>> +FIXTURE_VARIANT(socket_creation)
>> +{
>> +	bool sandboxed;
>> +	bool allowed;
>> +};
>> +
>> +FIXTURE_SETUP(socket_creation)
>> +{
>> +	self->sandboxed = variant->sandboxed;
>> +	self->allowed = variant->allowed;
>> +
>> +	setup_loopback(_metadata);
>> +};
>> +
>> +FIXTURE_TEARDOWN(socket_creation)
>> +{
>> +}
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(socket_creation, no_sandbox) {
>> +	/* clang-format on */
>> +	.sandboxed = false,
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(socket_creation, sandbox_allow) {
>> +	/* clang-format on */
>> +	.sandboxed = true,
>> +	.allowed = true,
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(socket_creation, sandbox_deny) {
>> +	/* clang-format on */
>> +	.sandboxed = true,
>> +	.allowed = false,
>> +};
>> +
>> +TEST_F(socket_creation, socketpair)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +	};
>> +	struct landlock_socket_attr unix_socket_create = {
>> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +		.family = AF_UNIX,
>> +		.type = SOCK_STREAM,
>> +	};
>> +	int ruleset_fd;
>> +
>> +	if (self->sandboxed) {
>> +		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,
>> +						       &unix_socket_create, 0));
>> +		}
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		ASSERT_EQ(0, close(ruleset_fd));
>> +	}
>> +
>> +	if (!self->sandboxed || self->allowed) {
>> +		/*
>> +		 * Tries to create sockets when ruleset is not established
>> +		 * or protocol is allowed.
>> +		 */
>> +		EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +	} else {
>> +		/* Tries to create sockets when protocol is restricted. */
>> +		EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +	}
> 
> I am torn on whether socketpair() should be denied at all --
> 
>    * on one hand, the created sockets are connected to each other
>      and the creating process can only talk to itself (or pass one of them on),
>      which seems legitimate and harmless.
> 
>    * on the other hand, it *does* create two sockets, and
>      if they are datagram sockets, it it probably currently possible
>      to disassociate them with connect(AF_UNSPEC). >
> What are your thoughts on that?

Good catch! According to the discussion that you've mentioned [1] (I
believe I found correct one), you've already discussed socketpair(2)
control with Mickaël and came to the conclusion that socketpair(2) and
unnamed pipes do not give access to new resources to the process,
therefore should not be restricted.

[1] 
https://lore.kernel.org/all/e7e24682-5da7-3b09-323e-a4f784f10158@digikod.net/

Therefore, this is more like connect(AF_UNSPEC)-related issue. On
security summit you've mentioned that it will be useful to implement
restriction of connection dissociation for sockets. This feature will
solve the problem of reusage of UNIX sockets that were created with
socketpair(2).

If we want such feature to be implemented, I suggest leaving current
implementation as it is (to prevent vulnerable creation of UNIX dgram
sockets) and enable socketpair(2) in the patchset dedicated to
connect(AF_UNSPEC) restriction. Also it will be useful to create a
dedicated issue on github. WDYT?

(Btw I think that disassociation control can be really useful. If
it were possible to restrict this action for each protocol, we would
have stricter control over the protocols used.)

> 
> Mickaël, I believe we have also discussed similar questions for pipe(2) in the
> past, and you had opinions on that?
> 
> 
> (On a much more technical note; consider replacing self->allowed with
> self->socketpair_error to directly indicate the expected error? It feels that
> this could be more straightforward?)

I've considered this approach and decided that this would
* negatively affect the readability of conditional for adding Landlock
   rule,
* make checking the test_socketpair() error code less explicit.

> 
> —Günther

Powered by blists - more mailing lists