diff mbox series

[RFC,v3,13/19] selftests/landlock: Test packet protocol alias

Message ID 20240904104824.1844082-14-ivanov.mikhail1@huawei-partners.com (mailing list archive)
State RFC
Headers show
Series Support socket access-control | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch, async

Commit Message

Mikhail Ivanov Sept. 4, 2024, 10:48 a.m. UTC 
(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@huawei-partners.com>
---
 .../testing/selftests/landlock/socket_test.c  | 75 ++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

Comments

Günther Noack Sept. 18, 2024, 1:33 p.m. UTC | #1 
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@huawei-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
Mikhail Ivanov Sept. 18, 2024, 2:01 p.m. UTC | #2 
On 9/18/2024 4:33 PM, Günther Noack wrote:
> 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@huawei-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.

Agreed, thanks!

> 
> 
>> +
>> +	/* 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 ;-)

Of course, sorry for that)

> 
>> +	 * 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?

Agreed, prot_tested should be removed.

> 
>> +}
>> +
>>   TEST_HARNESS_MAIN
>> -- 
>> 2.34.1
>>
> 
> —Günther
diff mbox series

Patch

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;
+
+	/* 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
+	 * 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));
+}
+
 TEST_HARNESS_MAIN