[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240820.Yohzeik9dei5@digikod.net>
Date: Tue, 20 Aug 2024 18:00:02 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Tahera Fahimi <fahimitahera@...il.com>
Cc: outreachy@...ts.linux.dev, gnoack@...gle.com, paul@...l-moore.com,
jmorris@...ei.org, serge@...lyn.com, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, bjorn3_gh@...tonmail.com, jannh@...gle.com,
netdev@...r.kernel.org
Subject: Re: [PATCH v10 3/6] selftests/Landlock: Abstract UNIX socket
restriction tests
On Mon, Aug 19, 2024 at 10:08:53PM -0600, Tahera Fahimi wrote:
> The patch introduces Landlock ABI version 6 and adds three types of tests
> that examines different scenarios for abstract unix socket:
> 1) unix_socket: base tests of the abstract socket scoping mechanism for a
> landlocked process, same as the ptrace test.
> 2) optional_scoping: generates three processes with different domains and
> tests if a process with a non-scoped domain can connect to other
> processes.
> 3) outside_socket: since the socket's creator credentials are used
> for scoping sockets, this test examines the cases where the socket's
> credentials are different from the process using it.
>
> Signed-off-by: Tahera Fahimi <fahimitahera@...il.com>
> ---
> Changes in versions:
> v10:
> - Code improvements by changing fixture variables to local ones.
> - Rename "unix_sock_special_cases" to "outside_socket"
> v9:
> - Move pathname_address_sockets to a different patch.
> - Extend optional_scoping test scenarios.
> - Removing hardcoded numbers and using "backlog" instead.
> V8:
> - Move tests to scoped_abstract_unix_test.c file.
> - To avoid potential conflicts among Unix socket names in different tests,
> set_unix_address is added to common.h to set different sun_path for Unix sockets.
> - protocol_variant and service_fixture structures are also moved to common.h
> - Adding pathname_address_sockets to cover all types of address formats
> for unix sockets, and moving remove_path() to common.h to reuse in this test.
> V7:
> - Introducing landlock ABI version 6.
> - Adding some edge test cases to optional_scoping test.
> - Using `enum` for different domains in optional_scoping tests.
> - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets.
> - Modifying inline comments.
> V6:
> - Introducing optional_scoping test which ensures a sandboxed process with a
> non-scoped domain can still connect to another abstract unix socket(either
> sandboxed or non-sandboxed).
> - Introducing unix_sock_special_cases test which tests examines scenarios where
> the connecting sockets have different domain than the process using them.
> V4:
> - Introducing unix_socket to evaluate the basic scoping mechanism for abstract
> unix sockets.
> ---
> tools/testing/selftests/landlock/common.h | 38 +
> tools/testing/selftests/landlock/net_test.c | 31 +-
> .../landlock/scoped_abstract_unix_test.c | 931 ++++++++++++++++++
> 3 files changed, 970 insertions(+), 30 deletions(-)
> create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> new file mode 100644
> index 000000000000..65c1ac2895a9
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> @@ -0,0 +1,931 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Landlock tests - Abstract Unix Socket
> + *
> + * Copyright © 2024 Tahera Fahimi <fahimitahera@...il.com>
> + */
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/landlock.h>
> +#include <sched.h>
> +#include <signal.h>
> +#include <stddef.h>
> +#include <sys/prctl.h>
> +#include <sys/socket.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/un.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "common.h"
> +
> +/* Number pending connections queue to be hold. */
> +const short backlog = 10;
> +
> +static void create_fs_domain(struct __test_metadata *const _metadata)
> +{
> + int ruleset_fd;
> + struct landlock_ruleset_attr ruleset_attr = {
> + .handled_access_fs = LANDLOCK_ACCESS_FS_READ_DIR,
> + };
> +
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> + EXPECT_LE(0, ruleset_fd)
> + {
> + TH_LOG("Failed to create a ruleset: %s", strerror(errno));
> + }
> + EXPECT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> + EXPECT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
> + EXPECT_EQ(0, close(ruleset_fd));
> +}
> +
> +static void create_unix_domain(struct __test_metadata *const _metadata)
> +{
> + int ruleset_fd;
> + const struct landlock_ruleset_attr ruleset_attr = {
> + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> + };
> +
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> + EXPECT_LE(0, ruleset_fd)
> + {
> + TH_LOG("Failed to create a ruleset: %s", strerror(errno));
> + }
> + enforce_ruleset(_metadata, ruleset_fd);
> + EXPECT_EQ(0, close(ruleset_fd));
> +}
> +
> +/* clang-format off */
> +FIXTURE(unix_socket) {};
> +/* clang-format on */
> +
> +FIXTURE_VARIANT(unix_socket)
These variant fixture should be renamed to
FIXTURE_VARIANT(scoped_domains) to be usable for all scoped tests (e.g.
abstract unix socket and signal). You can define them in a
scoped_common.h file which will be included by
scoped_abstract_unix_test.c and scoped_signal_test.c
> +{
> + bool domain_both;
> + bool domain_parent;
> + bool domain_child;
> + bool connect_to_parent;
connect_to_parent should not be part of a variant. In this case, we can
create two TEST_F(): a TEST_F(to_parent) like when connect_to_parent is
true, and a TEST_F(to_child) otherwise.
scoped_signal_test.c should have the same TEST_F names with slightly
different implementation but still testing the same semantic.
> +};
> +
> +FIXTURE_SETUP(unix_socket)
The self->stream_address and self->dgram_address initializations were
good. The issue was about socket's file descriptors, but if we have raw
data common to all tests, it makes sense to initialize them here.
> +{
> +}
> +
> +FIXTURE_TEARDOWN(unix_socket)
> +{
> +}
> +
> +/*
> + * No domain
> + *
> + * P1-. P1 -> P2 : allow
> + * \ P2 -> P1 : allow
> + * 'P2
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = false,
> + .domain_child = false,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = false,
> + .domain_child = false,
> + .connect_to_parent = false,
We can see that the picture describing the domains is the same for both
of these variants, which means something is wrong. connect_to_parent
should not be part of the variant definitions.
> +};
> +
> +/*
> + * Child domain
> + *
> + * P1--. P1 -> P2 : allow
> + * \ P2 -> P1 : deny
> + * .'-----.
> + * | P2 |
> + * '------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_with_one_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = false,
> + .domain_child = true,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_with_one_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = false,
> + .domain_child = true,
> + .connect_to_parent = false,
> +};
> +
> +/*
> + * Parent domain
> + * .------.
> + * | P1 --. P1 -> P2 : deny
> + * '------' \ P2 -> P1 : allow
> + * '
> + * P2
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_with_parent_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = true,
> + .domain_child = false,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_with_parent_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = true,
> + .domain_child = false,
> + .connect_to_parent = false,
> +};
> +
> +/*
> + * Parent + child domain (siblings)
> + * .------.
> + * | P1 ---. P1 -> P2 : deny
> + * '------' \ P2 -> P1 : deny
> + * .---'--.
> + * | P2 |
> + * '------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = true,
> + .domain_child = true,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = false,
> + .domain_parent = true,
> + .domain_child = true,
> + .connect_to_parent = false,
> +};
> +
> +/*
> + * Same domain (inherited)
> + * .-------------.
> + * | P1----. | P1 -> P2 : allow
> + * | \ | P2 -> P1 : allow
> + * | ' |
> + * | P2 |
> + * '-------------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = false,
> + .domain_child = false,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = false,
> + .domain_child = false,
> + .connect_to_parent = false,
> +};
> +
> +/*
> + * Inherited + child domain
> + * .-----------------.
> + * | P1----. | P1 -> P2 : allow
> + * | \ | P2 -> P1 : deny
> + * | .-'----. |
> + * | | P2 | |
> + * | '------' |
> + * '-----------------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_nested_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = false,
> + .domain_child = true,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_nested_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = false,
> + .domain_child = true,
> + .connect_to_parent = false,
> +};
> +
> +/*
> + * Inherited + parent domain
> + * .-----------------.
> + * |.------. | P1 -> P2 : deny
> + * || P1 ----. | P2 -> P1 : allow
> + * |'------' \ |
> + * | ' |
> + * | P2 |
> + * '-----------------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, allow_with_nested_and_parent_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = true,
> + .domain_child = false,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_with_nested_and_parent_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = true,
> + .domain_child = false,
> + .connect_to_parent = false,
> +};
> +
> +/*
> + * Inherited + parent and child domain (siblings)
> + * .-----------------.
> + * | .------. | P1 -> P2 : deny
> + * | | P1 . | P2 -> P1 : deny
> + * | '------'\ |
> + * | \ |
> + * | .--'---. |
> + * | | P2 | |
> + * | '------' |
> + * '-----------------'
> + */
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_parent) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = true,
> + .domain_child = true,
> + .connect_to_parent = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_child) {
> + /* clang-format on */
> + .domain_both = true,
> + .domain_parent = true,
> + .domain_child = true,
> + .connect_to_parent = false,
> +};
> +
> +/*
> + * Test unix_stream_connect() and unix_may_send() for parent and child,
> + * when they have scoped domain or no domain.
> + */
> +TEST_F(unix_socket, abstract_unix_socket)
> +{
> + struct service_fixture stream_address, dgram_address;
> + pid_t child;
> + bool can_connect_to_parent, can_connect_to_child;
> + int err, err_dgram, status;
> + int pipe_child[2], pipe_parent[2];
> + char buf_parent;
For all TEST() and TEST_F(), in this patch series and the signal one, we
need to first drop capabilities:
drop_caps(_metadata);
> +
> + memset(&stream_address, 0, sizeof(stream_address));
> + memset(&dgram_address, 0, sizeof(dgram_address));
> + set_unix_address(&stream_address, 0);
> + set_unix_address(&dgram_address, 1);
> + /*
> + * can_connect_to_child is true if a parent process can connect to its
> + * child process. The parent process is not isolated from the child
> + * with a dedicated Landlock domain.
> + */
> + can_connect_to_child = !variant->domain_parent;
> + /*
> + * can_connect_to_parent is true if a child process can connect to its
> + * parent process. This depends on the child process is not isolated from
> + * the parent with a dedicated Landlock domain.
> + */
> + can_connect_to_parent = !variant->domain_child;
> +
> + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> + if (variant->domain_both) {
> + create_unix_domain(_metadata);
> + if (!__test_passed(_metadata))
> + return;
> + }
> +
> + child = fork();
> + ASSERT_LE(0, child);
> + if (child == 0) {
> + char buf_child;
> +
> + ASSERT_EQ(0, close(pipe_parent[1]));
> + ASSERT_EQ(0, close(pipe_child[0]));
> + if (variant->domain_child)
> + create_unix_domain(_metadata);
> +
> + /* Waits for the parent to be in a domain, if any. */
> + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +
> + if (variant->connect_to_parent) {
> + int client, dgram_client;
It looks like you missed some of my previous reviews (e.g. variable
names). Please read back again *all* my reviews/emails for at least the
last two versions.
> +
> + client = socket(AF_UNIX, SOCK_STREAM, 0);
> + dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
> +
> +TEST_F(outside_socket, socket_with_different_domain)
> +{
> + pid_t child;
> + int err, status;
> + int pipe_child[2], pipe_parent[2];
> + char buf_parent;
> + struct service_fixture address, transit_address;
> +
> + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> +
> + memset(&transit_address, 0, sizeof(transit_address));
> + memset(&address, 0, sizeof(address));
> + set_unix_address(&transit_address, 0);
> + set_unix_address(&address, 1);
> +
> + child = fork();
> + ASSERT_LE(0, child);
> + if (child == 0) {
> + char buf_child;
> + int stream_server, client;
> +
> + ASSERT_EQ(0, close(pipe_parent[1]));
> + ASSERT_EQ(0, close(pipe_child[0]));
> +
> + /* client always has domain */
> + create_unix_domain(_metadata);
> +
> + if (variant->domain_server_socket) {
> + int data_socket;
> + int fd_sock = socket(AF_UNIX, variant->type, 0);
> +
> + ASSERT_NE(-1, fd_sock);
> +
> + stream_server = socket(AF_UNIX, SOCK_STREAM, 0);
> +
> + ASSERT_NE(-1, stream_server);
> + ASSERT_EQ(0, bind(stream_server,
> + &transit_address.unix_addr,
> + transit_address.unix_addr_len));
> + ASSERT_EQ(0, listen(stream_server, backlog));
> +
> + ASSERT_EQ(1, write(pipe_child[1], ".", 1));
> +
> + data_socket = accept(stream_server, NULL, NULL);
> +
> + ASSERT_EQ(0, send_fd(data_socket, fd_sock));
> + ASSERT_EQ(0, close(fd_sock));
> + ASSERT_EQ(0, close(stream_server));
> + }
> +
> + client = socket(AF_UNIX, variant->type, 0);
> + ASSERT_NE(-1, client);
> + /* wait for parent signal for connection */
> + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +
> + err = connect(client, &address.unix_addr,
> + address.unix_addr_len);
> + if (!variant->domain_server_socket) {
> + EXPECT_EQ(-1, err);
> + EXPECT_EQ(EPERM, errno);
> + } else {
> + EXPECT_EQ(0, err);
> + }
> + ASSERT_EQ(0, close(client));
> + _exit(_metadata->exit_code);
> + return;
> + }
> + int server_socket;
Variable declarations go at the top of the scope where other variable
are declared.
> +
> + ASSERT_EQ(0, close(pipe_child[1]));
> + ASSERT_EQ(0, close(pipe_parent[0]));
> +
> + if (!variant->domain_server_socket) {
> + server_socket = socket(AF_UNIX, variant->type, 0);
> + } else {
> + int cli = socket(AF_UNIX, SOCK_STREAM, 0);
> +
> + ASSERT_NE(-1, cli);
> + ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
> + ASSERT_EQ(0, connect(cli, &transit_address.unix_addr,
> + transit_address.unix_addr_len));
> +
> + server_socket = recv_fd(cli);
> + ASSERT_LE(0, server_socket);
> + ASSERT_EQ(0, close(cli));
> + }
> +
> + ASSERT_NE(-1, server_socket);
> +
> + if (variant->domain_server)
> + create_unix_domain(_metadata);
> +
> + ASSERT_EQ(0, bind(server_socket, &address.unix_addr,
> + address.unix_addr_len));
> + if (variant->type == SOCK_STREAM)
> + ASSERT_EQ(0, listen(server_socket, backlog));
> + /* signal to child that parent is listening */
> + ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + ASSERT_EQ(0, close(server_socket));
> +
> + if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> + WEXITSTATUS(status) != EXIT_SUCCESS)
> + _metadata->exit_code = KSFT_FAIL;
> +}
> +
> +TEST_HARNESS_MAIN
> --
> 2.34.1
>
>
Powered by blists - more mailing lists