netdev - Re: [PATCH v5 12/15] seltests/landlock: rules overlapping test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4806f5ed-41c0-f9f2-d7a1-2173c8494399@digikod.net>
Date:   Mon, 16 May 2022 19:41:58 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc:     willemdebruijn.kernel@...il.com,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, yusongping@...wei.com,
        anton.sirazetdinov@...wei.com
Subject: Re: [PATCH v5 12/15] seltests/landlock: rules overlapping test

Please fix these kind of subjects (selftests). I'd also like the subject 
description to (quickly) describe what is done (with a verb), to start 
with a capital (like a title), and to contain "network", something like 
this:
selftests/landlock: Add test for overlapping network rules

This is a good test though.


On 16/05/2022 17:20, Konstantin Meskhidze wrote:
> This patch adds overlapping rules for one port.
> First rule adds just bind() access right for a port.
> The second one adds both bind() and connect()
> access rights for the same port.
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
> ---
> 
> Changes since v3:
> * Add ruleset_overlap test.
> 
> Changes since v4:
> * Refactoring code with self->port, self->addr4 variables.
> 
> ---
>   tools/testing/selftests/landlock/net_test.c | 51 +++++++++++++++++++++
>   1 file changed, 51 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index bf8e49466d1d..1d8c9dfdbd48 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -677,4 +677,55 @@ TEST_F_FORK(socket_test, connect_afunspec_with_restictions) {
>   	ASSERT_EQ(1, WIFEXITED(status));
>   	ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
>   }
> +
> +TEST_F_FORK(socket_test, ruleset_overlap) {

Please run clang-format-14 on all files (and all commits).

> +
> +	int sockfd;
> +
> +	struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
> +				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
> +	};
> +	struct landlock_net_service_attr net_service_1 = {
> +		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
> +
> +		.port = self->port[0],
> +	};
> +
> +		struct landlock_net_service_attr net_service_2 = {
> +		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
> +				  LANDLOCK_ACCESS_NET_CONNECT_TCP,
> +
> +		.port = self->port[0],
> +	};
> +
> +	const int ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +					sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	/* Allows bind operations to the port[0] socket */

Please ends this kind of comments with a final dot (all files/commits).

> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
> +				       &net_service_1, 0));
> +	/* Allows connect and bind operations to the port[0] socket */
> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
> +				       &net_service_2, 0));
> +
> +	/* Enforces the ruleset. */
> +	enforce_ruleset(_metadata, ruleset_fd);
> +
> +	/* Creates a server socket */
> +	sockfd = create_socket(_metadata, false, false);
> +	ASSERT_LE(0, sockfd);
> +
> +	/* Binds the socket to address with port[0] */
> +	ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], sizeof(self->addr4[0])));
> +
> +	/* Makes connection to socket with port[0] */
> +	ASSERT_EQ(0, connect(sockfd, (struct sockaddr *)&self->addr4[0],

Can you please get rid of this (struct sockaddr *) type casting please 
(without compiler warning)?

> +						   sizeof(self->addr4[0])));

Here, you can enforce a new ruleset with net_service_1 and check that 
bind() is still allowed but not connect().

> +
> +	/* Closes socket */
> +	ASSERT_EQ(0, close(sockfd));
> +}
> +
>   TEST_HARNESS_MAIN
> --
> 2.25.1
>