netdev - Re: [PATCH v1] landlock: Abstract unix socket restriction tests

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] [day] [month] [year] [list]

Message-ID: <20240708.rootai6weiXe@digikod.net>
Date: Mon, 8 Jul 2024 21:45:42 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Tahera Fahimi <fahimitahera@...il.com>
Cc: Günther Noack <gnoack@...gle.com>, 
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, 
	"Serge E. Hallyn" <serge@...lyn.com>, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Jann Horn <jannh@...gle.com>, outreachy@...ts.linux.dev, netdev@...r.kernel.org
Subject: Re: [PATCH v1] landlock: Abstract unix socket restriction tests

These are good tests!  However, I get errors when running some of them (using
the latest formatted patches):

#  RUN           unix_socket.allow_without_domain_connect_to_parent.abstract_unix_socket ...
# ptrace_test.c:845:abstract_unix_socket:Expected 0 (0) == bind(self->server, (struct sockaddr *)&addr, addrlen) (-1)
# abstract_unix_socket: Test terminated by assertion
#          FAIL  unix_socket.allow_without_domain_connect_to_parent.abstract_unix_socket
not ok 9 unix_socket.allow_without_domain_connect_to_parent.abstract_unix_socket
#  RUN           unix_socket.allow_without_domain_connect_to_child.abstract_unix_socket ...
# ptrace_test.c:793:abstract_unix_socket:Expected 0 (0) == bind(self->server, (struct sockaddr *)&addr, addrlen) (-1)
# ptrace_test.c:826:abstract_unix_socket:Expected 1 (1) == read(pipe_child[0], &buf_parent, 1) (0)
# abstract_unix_socket: Test terminated by assertion
#          FAIL  unix_socket.allow_without_domain_connect_to_child.abstract_unix_socket
not ok 10 unix_socket.allow_without_domain_connect_to_child.abstract_unix_socket


On Thu, Jun 27, 2024 at 05:30:48PM -0600, Tahera Fahimi wrote:
> Tests for scoping abstract unix sockets. The patch has three types of tests:
> i) unix_socket: tests the scoping mechanism for a landlocked process, same as
> ptrace test.
> ii) optional_scoping: generates three processes with different domains and tests if
> a process with a non-scoped domain can connect to other processes.
> iii) unix_sock_special_cases: since the socket's creator credentials are used for
> scoping datagram sockets, this test examine the cases where the socket's credentials
> are different from the process who is using it.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@...il.com>
> ---

> +/* clang-format off */
> +FIXTURE(optional_scoping)
> +{
> +	int parent_server, child_server, client;
> +};
> +/* clang-format on */
> +
> +/* Domain is defined as follows:
> + * 0 --> no domain
> + * 1 --> have domain
> + * 2 --> have domain and is scoped

You should use an enum instead of these hardcoded values.  This is
better to understand/document, to review, and to maintain.

> + **/
> +FIXTURE_VARIANT(optional_scoping)
> +{
> +	int domain_all;
> +	int domain_parent;
> +	int domain_children;
> +	int domain_child;
> +	int domain_grand_child;
> +	int type;
> +};
> +/*
> + * .-----------------.
> + * |         ####### |  P3 -> P2 : allow
> + * |   P1----# P2  # |  P3 -> P1 : deny
> + * |         #  |  # |
> + * |         # P3  # |
> + * |         ####### |
> + * '-----------------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(optional_scoping, deny_scoped) {
> +	.domain_all = 1,
> +	.domain_parent = 0,
> +	.domain_children = 2,
> +	.domain_child = 0,
> +	.domain_grand_child = 0,
> +	.type = SOCK_DGRAM,
> +	/* clang-format on */
> +};
> +/*
> + * .-----------------.
> + * |         .-----. |  P3 -> P2 : allow
> + * |   P1----| P2  | |  P3 -> P1 : allow
> + * |         |     | |
> + * |         | P3  | |
> + * |         '-----' |
> + * '-----------------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(optional_scoping, allow_with_domain) {
> +	.domain_all = 1,
> +	.domain_parent = 0,
> +	.domain_children = 1,
> +	.domain_child = 0,
> +	.domain_grand_child = 0,
> +	.type = SOCK_DGRAM,
> +	/* clang-format on */
> +};

I guess this should failed with the current kernel patch (see my review
of the kernel patch), but something like that should be tested:

FIXTURE_VARIANT_ADD(optional_scoping, allow_with_one_domain) {
	.domain_parent = 0,
	.domain_child = 2,
	.domain_grand_child = 0,
};

grand_child should be able to connect to its parent (child), but not its
grand parent (parent).