netdev - Re: [RFC PATCH v3 06/19] selftests/landlock: Test adding a rule for unhandled access

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: <fd6ef478-4d0b-03f2-78f6-8bfd0fc3a846@huawei-partners.com>
Date: Wed, 11 Sep 2024 11:19:48 +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 06/19] selftests/landlock: Test adding a rule for
 unhandled access

On 9/10/2024 12:22 PM, Günther Noack wrote:
> Hi!
> 
> On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote:
>> Add test that validates behaviour of Landlock after rule with
>> unhandled access is added.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
>> ---
>> Changes since v2:
>> * Replaces EXPECT_EQ with ASSERT_EQ for close().
>> * Refactors commit title and message.
>>
>> Changes since v1:
>> * Refactors commit message.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 33 +++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index 811bdaa95a7a..d2fedfca7193 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access)
>>   	ASSERT_EQ(0, close(ruleset_fd));
>>   }
>>   
>> +TEST_F(protocol, rule_with_unhandled_access)
>> +{
>> +	struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +	};
>> +	struct landlock_socket_attr protocol = {
>> +		.family = self->prot.family,
>> +		.type = self->prot.type,
>> +	};
>> +	int ruleset_fd;
>> +	__u64 access;
>> +
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	for (access = 1; access > 0; access <<= 1) {
>> +		int err;
>> +
>> +		protocol.allowed_access = access;
>> +		err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +					&protocol, 0);
>> +		if (access == ruleset_attr.handled_access_socket) {
>> +			EXPECT_EQ(0, err);
>> +		} else {
>> +			EXPECT_EQ(-1, err);
>> +			EXPECT_EQ(EINVAL, errno);
>> +		}
>> +	}
>> +
>> +	ASSERT_EQ(0, close(ruleset_fd));
>> +}
>> +
> 
> I should probably have noticed this on the first review round; you are not
> actually exercising any scenario here where a rule with unhandled access is
> added.
> 
> To clarify, the notion of an access right being "unhandled" means that the
> access right was not listed at ruleset creation time in the ruleset_attr's
> .handled_access_* field where it would have belonged.  If that is the case,
> adding a ruleset with that access right is going to be denied.
> 
> As an example:
> If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else,
> then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE,
> that call is supposed to fail -- because the "socket creation" access right is
> not handled.

This test was added to exercise adding a rule with future possible
"unhandled" access rights of "socket" type, but since this patch
implements only one, this test is really meaningless. Thank you for
this note!

> 
> IMHO the test would become more reasonable if it was more clearly "handling"
> something entirely unrelated at ruleset creation time, e.g. one of the file
> system access rights.  (And we could do the same for the "net" and "fs" tests as
> well.)
> 
> Your test is a copy of the same test for the "net" rights, which in turn is a
> copy of teh same test for the "fs" rights.  When the "fs" test was written, the
> "fs" access rights were the only ones that could be used at all to create a
> ruleset, but this is not true any more.

Good idea! Can I implement such test in the current patchset?

> 
> —Günther