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: <ZuAP8iSv_sjmlYIp@google.com>
Date: Tue, 10 Sep 2024 11:22:58 +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 06/19] selftests/landlock: Test adding a rule for
 unhandled access

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.

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.

—Günther