netdev - Re: [RFC PATCH v3 13/19] selftests/landlock: Test packet protocol alias

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZurWqFq_dGWOsgUU@google.com>
Date: Wed, 18 Sep 2024 15:33:28 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.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 13/19] selftests/landlock: Test packet protocol alias

On Wed, Sep 04, 2024 at 06:48:18PM +0800, Mikhail Ivanov wrote:
> (AF_INET, SOCK_PACKET) is an alias for (AF_PACKET, SOCK_PACKET)
> (Cf. __sock_create). Landlock shouldn't restrict one pair if the other
> was allowed. Add `packet_protocol` fixture and test to
> validate these scenarios.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
> ---
>  .../testing/selftests/landlock/socket_test.c  | 75 ++++++++++++++++++-
>  1 file changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 23698b8c2f4d..8fc507bf902a 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -7,7 +7,7 @@
>  
>  #define _GNU_SOURCE
>  
> -#include "landlock.h"
> +#include <linux/landlock.h>
>  #include <linux/pfkeyv2.h>
>  #include <linux/kcm.h>
>  #include <linux/can.h>
> @@ -665,4 +665,77 @@ TEST(kernel_socket)
>  	EXPECT_EQ(0, test_socket(AF_SMC, SOCK_STREAM, 0));
>  }
>  
> +FIXTURE(packet_protocol)
> +{
> +	struct protocol_variant prot_allowed, prot_tested;
> +};
> +
> +FIXTURE_VARIANT(packet_protocol)
> +{
> +	bool packet;
> +};
> +
> +FIXTURE_SETUP(packet_protocol)
> +{
> +	self->prot_allowed.type = self->prot_tested.type = SOCK_PACKET;
> +
> +	self->prot_allowed.family = variant->packet ? AF_PACKET : AF_INET;
> +	self->prot_tested.family = variant->packet ? AF_INET : AF_PACKET;

Nit: You might as well write these resulting prot_allowed and prot_tested struct
values out in the two fixture variants.  It's one layer of indirection less and
clarity trumps deduplication in tests, IMHO.  Fine either way though.


> +
> +	/* Packet protocol requires NET_RAW to be set (Cf. packet_create). */
> +	set_cap(_metadata, CAP_NET_RAW);
> +};
> +
> +FIXTURE_TEARDOWN(packet_protocol)
> +{
> +	clear_cap(_metadata, CAP_NET_RAW);
> +}
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(packet_protocol, packet_allows_inet) {
> +	/* clang-format on */
> +	.packet = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(packet_protocol, inet_allows_packet) {
> +	/* clang-format on */
> +	.packet = false,
> +};
> +
> +TEST_F(packet_protocol, alias_restriction)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> +	};
> +	struct landlock_socket_attr packet_socket_create = {
> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> +		.family = self->prot_allowed.family,
> +		.type = self->prot_allowed.type,
> +	};
> +	int ruleset_fd;
> +
> +	/*
> +	 * Checks that packet socket is created sucessfuly without

Typo nit: "successfully"

Please also check in other locations, I might well have missed some ;-)

> +	 * landlock restrictions.
> +	 */
> +	ASSERT_EQ(0, test_socket_variant(&self->prot_tested));
> +
> +	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_SOCKET,
> +				       &packet_socket_create, 0));
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/*
> +	 * (AF_INET, SOCK_PACKET) is an alias for the (AF_PACKET, SOCK_PACKET)
> +	 * (Cf. __sock_create). Checks that Landlock does not restrict one pair
> +	 * if the other was allowed.
> +	 */
> +	EXPECT_EQ(0, test_socket_variant(&self->prot_tested));

Why not check both AF_INET and AF_PACKET in both fixtures?
Since they are synonymous, they should both work, no matter which
of the two variants was used in the rule.

It would be slightly more comprehensive and make the fixture smaller.
WDYT?

> +}
> +
>  TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 

—Günther