Date: Sat, 12 Aug 2023 17:37:00 +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 v11.1] selftests/landlock: Add 11 new test suites
 dedicated to network



7/12/2023 10:02 AM, Mickaël Salaün пишет:
> 
> On 06/07/2023 16:55, Mickaël Salaün wrote:
>> From: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>> 
>> This patch is a revamp of the v11 tests [1] with new tests (see the
>> "Changes since v11" description).  I (Mickaël) only added the following
>> todo list and the "Changes since v11" sections in this commit message.
>> I think this patch is good but it would appreciate reviews.
>> You can find the diff of my changes here but it is not really readable:
>> https://git.kernel.org/mic/c/78edf722fba5 (landlock-net-v11 branch)
>> [1] https://lore.kernel.org/all/20230515161339.631577-11-konstantin.meskhidze@huawei.com/
>> TODO:
>> - Rename all "net_service" to "net_port".
>> - Fix the two kernel bugs found with the new tests.
>> - Update this commit message with a small description of all tests.
> 
> [...]
> 
>> +FIXTURE_SETUP(ipv4)
>> +{
>> +	const struct protocol_variant prot = {
>> +		.domain = AF_INET,
>> +		.type = variant->type,
>> +	};
>> +
>> +	disable_caps(_metadata);
>> +
>> +	set_service(&self->srv0, prot, 0);
>> +	set_service(&self->srv1, prot, 1);
>> +
>> +	setup_loopback(_metadata);
>> +};
>> +
>> +FIXTURE_TEARDOWN(ipv4)
>> +{
>> +}
>> +
>> +// Kernel FIXME: tcp_sandbox_with_tcp and tcp_sandbox_with_udp
>> +TEST_F(ipv4, from_unix_to_inet)
>> +{
>> +	int unix_stream_fd, unix_dgram_fd;
>> +
>> +	if (variant->sandbox == TCP_SANDBOX) {
>> +		const struct landlock_ruleset_attr ruleset_attr = {
>> +			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
>> +					      LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> +		};
>> +		const struct landlock_net_service_attr tcp_bind_connect_p0 = {
>> +			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
>> +					  LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> +			.port = self->srv0.port,
>> +		};
>> +		int ruleset_fd;
>> +
>> +		/* Denies connect and bind to check errno value. */
>> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>> +						     sizeof(ruleset_attr), 0);
>> +		ASSERT_LE(0, ruleset_fd);
>> +
>> +		/* Allows connect and bind for srv0.  */
>> +		ASSERT_EQ(0, landlock_add_rule(ruleset_fd,
>> +					       LANDLOCK_RULE_NET_SERVICE,
>> +					       &tcp_bind_connect_p0, 0));
>> +
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		EXPECT_EQ(0, close(ruleset_fd));
>> +	}
>> +
>> +	unix_stream_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
>> +	ASSERT_LE(0, unix_stream_fd);
>> +
>> +	unix_dgram_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
>> +	ASSERT_LE(0, unix_dgram_fd);
>> +
>> +	/* Checks unix stream bind and connect for srv0. */
>> +	EXPECT_EQ(-EINVAL, bind_variant(unix_stream_fd, &self->srv0));
>> +	EXPECT_EQ(-EINVAL, connect_variant(unix_stream_fd, &self->srv0));
>> +
>> +	/* Checks unix stream bind and connect for srv1. */
>> +	EXPECT_EQ(-EINVAL, bind_variant(unix_stream_fd, &self->srv1))
>> +	{
>> +		TH_LOG("Wrong bind error: %s", strerror(errno));
>> +	}
>> +	EXPECT_EQ(-EINVAL, connect_variant(unix_stream_fd, &self->srv1));
>> +
>> +	/* Checks unix datagram bind and connect for srv0. */
>> +	EXPECT_EQ(-EINVAL, bind_variant(unix_dgram_fd, &self->srv0));
>> +	EXPECT_EQ(-EINVAL, connect_variant(unix_dgram_fd, &self->srv0));
>> +
>> +	/* Checks unix datagram bind and connect for srv0. */
>> +	EXPECT_EQ(-EINVAL, bind_variant(unix_dgram_fd, &self->srv1));
>> +	EXPECT_EQ(-EINVAL, connect_variant(unix_dgram_fd, &self->srv1));
>> +}
> 
> We should also add a test to make sure errno is the same with and
> without sandboxing when using port 0 for connect and consistent with
> bind (using an available port). The test fixture and variants should be
> quite similar to the "ipv4" ones, but we can also add AF_INET6 variants,
> which will result in 8 "ip" variants:
> 
> TEST_F(ip, port_zero)
> {
> 	if (variant->sandbox == TCP_SANDBOX) {
> 		/* Denies any connect and bind. */
> 	}
> 	/* Checks errno for port 0. */
> }
As I understand the would be the next test cases:

	1. ip4, sandboxed, bind port 0 -> should return EACCES (denied by 
landlock).
	2. ip4, non-sandboxed, bind port 0 -> should return 0 (should be 
bounded to random port).
	3. ip6, sandboxed, bind port 0 -> should return EACCES (denied by 
landlock).
	4. ip6, non-sandboxed, bind port 0 -> should return 0 (should be 
bounded to random port).
	5. ip4, sandboxed, bind some available port, connect port 0 -> should 
return -EACCES (denied by landlock).
	6. ip4, non-sandboxed, bind some available port, connect port 0 -> 
should return ECONNREFUSED.
	7. ip6, sandboxed, bind some available port, connect port 0 -> should 
return -EACCES (denied by landlock)
	8. ip6, non-sandboxed, some bind available port, connect port 0 -> 
should return ECONNREFUSED.

Correct?

> 
> [...]
> 
>> +FIXTURE(inet)
>> +{
>> +	struct service_fixture srv0, srv1;
>> +};
> 
> The "inet" variants are useless and should be removed. The "inet"
> fixture can then be renamed to "ipv4_tcp".
> 
   So inet should be changed to ipv4_tcp and ipv6_tcp with next variants:

   - ipv4_tcp.no_sandbox_with_ipv4.port_endianness
   - ipv4_tcp.sandbox_with_ipv4.port_endianness
   - ipv6_tcp.no_sandbox_with_ipv6.port_endianness
   - ipv6_tcp.sandbox_with_ipv6.port_endianness
????

    in this case we need double copy of TEST_F(inet, port_endianness) :
	TEST_F(ipv4_tcp, port_endianness)
	TEST_F(ipv6_tcp, port_endianness)
> 
>> +
>> +FIXTURE_VARIANT(inet)
>> +{
>> +	const bool is_sandboxed;
>> +	const struct protocol_variant prot;
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(inet, no_sandbox_with_ipv4) {
>> +	/* clang-format on */
>> +	.is_sandboxed = false,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(inet, sandbox_with_ipv4) {
>> +	/* clang-format on */
>> +	.is_sandboxed = true,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(inet, no_sandbox_with_ipv6) {
>> +	/* clang-format on */
>> +	.is_sandboxed = false,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(inet, sandbox_with_ipv6) {
>> +	/* clang-format on */
>> +	.is_sandboxed = true,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +	},
>> +};
>> +
>> +FIXTURE_SETUP(inet)
>> +{
>> +	const struct protocol_variant ipv4_tcp = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +	};
>> +
>> +	disable_caps(_metadata);
>> +
>> +	ASSERT_EQ(0, set_service(&self->srv0, ipv4_tcp, 0));
>> +	ASSERT_EQ(0, set_service(&self->srv1, ipv4_tcp, 1));
>> +
>> +	setup_loopback(_metadata);
>> +};
>> +
>> +FIXTURE_TEARDOWN(inet)
>> +{
>> +}
>> +
>> +TEST_F(inet, port_endianness)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
>> +				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> +	};
>> +	const struct landlock_net_service_attr bind_host_endian_p0 = {
>> +		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>> +		/* Host port format. */
>> +		.port = self->srv0.port,
>> +	};
>> +	const struct landlock_net_service_attr connect_big_endian_p0 = {
>> +		.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> +		/* Big endian port format. */
>> +		.port = htons(self->srv0.port),
>> +	};
>> +	const struct landlock_net_service_attr bind_connect_host_endian_p1 = {
>> +		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
>> +				  LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> +		/* Host port format. */
>> +		.port = self->srv1.port,
>> +	};
>> +	const unsigned int one = 1;
>> +	const char little_endian = *(const char *)&one;
>> +	int ruleset_fd;
>> +
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
>> +				       &bind_host_endian_p0, 0));
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
>> +				       &connect_big_endian_p0, 0));
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
>> +				       &bind_connect_host_endian_p1, 0));
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +
>> +	/* No restriction for big endinan CPU. */
>> +	test_bind_and_connect(_metadata, &self->srv0, false, little_endian);
>> +
>> +	/* No restriction for any CPU. */
>> +	test_bind_and_connect(_metadata, &self->srv1, false, false);
>> +}
>> +
>> +TEST_HARNESS_MAIN
> .

Powered by blists - more mailing lists