Message-ID: <20250110.chi1Uwa9ahB4@digikod.net>
Date: Fri, 10 Jan 2025 12:25:08 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Eric Paris <eparis@...hat.com>, Paul Moore <paul@...l-moore.com>, 
	Günther Noack <gnoack@...gle.com>, "Serge E . Hallyn" <serge@...lyn.com>
Cc: Ben Scarlato <akhna@...gle.com>, 
	Casey Schaufler <casey@...aufler-ca.com>, Charles Zaffery <czaffery@...lox.com>, 
	Daniel Burgener <dburgener@...ux.microsoft.com>, Francis Laniel <flaniel@...ux.microsoft.com>, 
	James Morris <jmorris@...ei.org>, Jann Horn <jannh@...gle.com>, Jeff Xu <jeffxu@...gle.com>, 
	Jorge Lucangeli Obes <jorgelo@...gle.com>, Kees Cook <kees@...nel.org>, 
	Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Matt Bobrowski <mattbobrowski@...gle.com>, 
	Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, Phil Sutter <phil@....cc>, 
	Praveen K Paladugu <prapal@...ux.microsoft.com>, Robert Salvet <robert.salvet@...lox.com>, 
	Shervin Oloumi <enlightened@...gle.com>, Song Liu <song@...nel.org>, 
	Tahera Fahimi <fahimitahera@...il.com>, Tyler Hicks <code@...icks.com>, audit@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v4 22/30] selftests/landlock: Add
 layout1.umount_sandboxer tests

On Wed, Jan 08, 2025 at 04:43:30PM +0100, Mickaël Salaün wrote:
> Check that a domain is not tied to the executable file that created it.
> For instance, that could happen if a Landlock domain took a reference to
> a struct path.
> 
> Move global path names to common.h and replace copy_binary() with a more
> generic copy_file() helper.
> 
> Cc: Günther Noack <gnoack@...gle.com>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Link: https://lore.kernel.org/r/20250108154338.1129069-23-mic@digikod.net

Pushed in my next tree to simplify next patch series.

> ---
> 
> Changes since v3:
> - New patch to check issue from v2.
> ---
>  tools/testing/selftests/landlock/Makefile     |  2 +-
>  tools/testing/selftests/landlock/common.h     |  3 +
>  tools/testing/selftests/landlock/fs_test.c    | 94 +++++++++++++++++--
>  .../selftests/landlock/sandbox-and-launch.c   | 82 ++++++++++++++++
>  tools/testing/selftests/landlock/wait-pipe.c  | 42 +++++++++
>  5 files changed, 213 insertions(+), 10 deletions(-)
>  create mode 100644 tools/testing/selftests/landlock/sandbox-and-launch.c
>  create mode 100644 tools/testing/selftests/landlock/wait-pipe.c
> 
> diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile
> index 348e2dbdb4e0..b1445c8bee50 100644
> --- a/tools/testing/selftests/landlock/Makefile
> +++ b/tools/testing/selftests/landlock/Makefile
> @@ -10,7 +10,7 @@ src_test := $(wildcard *_test.c)
>  
>  TEST_GEN_PROGS := $(src_test:.c=)
>  
> -TEST_GEN_PROGS_EXTENDED := true
> +TEST_GEN_PROGS_EXTENDED := true sandbox-and-launch wait-pipe
>  
>  # Short targets:
>  $(TEST_GEN_PROGS): LDLIBS += -lcap
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 8391ab574f64..a604ea5d8297 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -28,6 +28,9 @@
>  /* TEST_F_FORK() should not be used for new tests. */
>  #define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)
>  
> +static const char bin_sandbox_and_launch[] = "./sandbox-and-launch";
> +static const char bin_wait_pipe[] = "./wait-pipe";
> +
>  static void _init_caps(struct __test_metadata *const _metadata, bool drop_all)
>  {
>  	cap_t cap_p;
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index a359c0d3107f..8ac9aaf38eaa 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -59,7 +59,7 @@ int open_tree(int dfd, const char *filename, unsigned int flags)
>  #define RENAME_EXCHANGE (1 << 1)
>  #endif
>  
> -#define BINARY_PATH "./true"
> +static const char bin_true[] = "./true";
>  
>  /* Paths (sibling number and depth) */
>  static const char dir_s1d1[] = TMP_DIR "/s1d1";
> @@ -1965,8 +1965,8 @@ TEST_F_FORK(layout1, relative_chroot_chdir)
>  	test_relative_path(_metadata, REL_CHROOT_CHDIR);
>  }
>  
> -static void copy_binary(struct __test_metadata *const _metadata,
> -			const char *const dst_path)
> +static void copy_file(struct __test_metadata *const _metadata,
> +		      const char *const src_path, const char *const dst_path)
>  {
>  	int dst_fd, src_fd;
>  	struct stat statbuf;
> @@ -1976,11 +1976,10 @@ static void copy_binary(struct __test_metadata *const _metadata,
>  	{
>  		TH_LOG("Failed to open \"%s\": %s", dst_path, strerror(errno));
>  	}
> -	src_fd = open(BINARY_PATH, O_RDONLY | O_CLOEXEC);
> +	src_fd = open(src_path, O_RDONLY | O_CLOEXEC);
>  	ASSERT_LE(0, src_fd)
>  	{
> -		TH_LOG("Failed to open \"" BINARY_PATH "\": %s",
> -		       strerror(errno));
> +		TH_LOG("Failed to open \"%s\": %s", src_path, strerror(errno));
>  	}
>  	ASSERT_EQ(0, fstat(src_fd, &statbuf));
>  	ASSERT_EQ(statbuf.st_size,
> @@ -2028,9 +2027,9 @@ TEST_F_FORK(layout1, execute)
>  		create_ruleset(_metadata, rules[0].access, rules);
>  
>  	ASSERT_LE(0, ruleset_fd);
> -	copy_binary(_metadata, file1_s1d1);
> -	copy_binary(_metadata, file1_s1d2);
> -	copy_binary(_metadata, file1_s1d3);
> +	copy_file(_metadata, bin_true, file1_s1d1);
> +	copy_file(_metadata, bin_true, file1_s1d2);
> +	copy_file(_metadata, bin_true, file1_s1d3);
>  
>  	enforce_ruleset(_metadata, ruleset_fd);
>  	ASSERT_EQ(0, close(ruleset_fd));
> @@ -2048,6 +2047,83 @@ TEST_F_FORK(layout1, execute)
>  	test_execute(_metadata, 0, file1_s1d3);
>  }
>  
> +TEST_F_FORK(layout1, umount_sandboxer)
> +{
> +	int pipe_child[2], pipe_parent[2];
> +	char buf_parent;
> +	pid_t child;
> +	int status;
> +
> +	copy_file(_metadata, bin_sandbox_and_launch, file1_s3d3);
> +	ASSERT_EQ(0, pipe2(pipe_child, 0));
> +	ASSERT_EQ(0, pipe2(pipe_parent, 0));
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		char pipe_child_str[12], pipe_parent_str[12];
> +		char *const argv[] = { (char *)file1_s3d3,
> +				       (char *)bin_wait_pipe, pipe_child_str,
> +				       pipe_parent_str, NULL };
> +
> +		/* Passes the pipe FDs to the executed binary and its child. */
> +		EXPECT_EQ(0, close(pipe_child[0]));
> +		EXPECT_EQ(0, close(pipe_parent[1]));
> +		snprintf(pipe_child_str, sizeof(pipe_child_str), "%d",
> +			 pipe_child[1]);
> +		snprintf(pipe_parent_str, sizeof(pipe_parent_str), "%d",
> +			 pipe_parent[0]);
> +
> +		/*
> +		 * We need bin_sandbox_and_launch (copied inside the mount as
> +		 * file1_s3d3) to execute bin_wait_pipe (outside the mount) to
> +		 * make sure the mount point will not be EBUSY because of
> +		 * file1_s3d3 being in use.  This avoids a potential race
> +		 * condition between the following read() and umount() calls.
> +		 */
> +		ASSERT_EQ(0, execve(argv[0], argv, NULL))
> +		{
> +			TH_LOG("Failed to execute \"%s\": %s", argv[0],
> +			       strerror(errno));
> +		};
> +		_exit(1);
> +		return;
> +	}
> +
> +	EXPECT_EQ(0, close(pipe_child[1]));
> +	EXPECT_EQ(0, close(pipe_parent[0]));
> +
> +	/* Waits for the child to sandbox itself. */
> +	EXPECT_EQ(1, read(pipe_child[0], &buf_parent, 1));
> +
> +	/* Tests that the sandboxer is tied to its mount point. */
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +	EXPECT_EQ(-1, umount(dir_s3d2));
> +	EXPECT_EQ(EBUSY, errno);
> +	clear_cap(_metadata, CAP_SYS_ADMIN);
> +
> +	/* Signals the child to launch a grandchild. */
> +	EXPECT_EQ(1, write(pipe_parent[1], ".", 1));
> +
> +	/* Waits for the grandchild. */
> +	EXPECT_EQ(1, read(pipe_child[0], &buf_parent, 1));
> +
> +	/* Tests that the domain's sandboxer is not tied to its mount point. */
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +	EXPECT_EQ(0, umount(dir_s3d2))
> +	{
> +		TH_LOG("Failed to umount \"%s\": %s", dir_s3d2,
> +		       strerror(errno));
> +	};
> +	clear_cap(_metadata, CAP_SYS_ADMIN);
> +
> +	/* Signals the grandchild to terminate. */
> +	EXPECT_EQ(1, write(pipe_parent[1], ".", 1));
> +	ASSERT_EQ(child, waitpid(child, &status, 0));
> +	ASSERT_EQ(1, WIFEXITED(status));
> +	ASSERT_EQ(0, WEXITSTATUS(status));
> +}
> +
>  TEST_F_FORK(layout1, link)
>  {
>  	const struct rule layer1[] = {
> diff --git a/tools/testing/selftests/landlock/sandbox-and-launch.c b/tools/testing/selftests/landlock/sandbox-and-launch.c
> new file mode 100644
> index 000000000000..1ef49f349429
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/sandbox-and-launch.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sandbox itself and execute another program (in a different mount point).
> + *
> + * Used by layout1.umount_sandboxer from fs_test.c
> + *
> + * Copyright © 2024 Microsoft Corporation
> + */
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/prctl.h>
> +#include <unistd.h>
> +
> +#include "wrappers.h"
> +
> +int main(int argc, char *argv[])
> +{
> +	struct landlock_ruleset_attr ruleset_attr = {
> +		.scoped = LANDLOCK_SCOPE_SIGNAL,
> +	};
> +	int pipe_child, pipe_parent, ruleset_fd;
> +	char buf;
> +
> +	/*
> +	 * The first argument must be the file descriptor number of a pipe.
> +	 * The second argument must be the program to execute.
> +	 */
> +	if (argc != 4) {
> +		fprintf(stderr, "Wrong number of arguments (not three)\n");
> +		return 1;
> +	}
> +
> +	pipe_child = atoi(argv[2]);
> +	pipe_parent = atoi(argv[3]);
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	if (ruleset_fd < 0) {
> +		perror("Failed to create ruleset");
> +		return 1;
> +	}
> +
> +	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> +		perror("Failed to call prctl()");
> +		return 1;
> +	}
> +
> +	if (landlock_restrict_self(ruleset_fd, 0)) {
> +		perror("Failed to restrict self");
> +		return 1;
> +	}
> +
> +	if (close(ruleset_fd)) {
> +		perror("Failed to close ruleset");
> +		return 1;
> +	}
> +
> +	/* Signals that we are sandboxed. */
> +	errno = 0;
> +	if (write(pipe_child, ".", 1) != 1) {
> +		perror("Failed to write to the second argument");
> +		return 1;
> +	}
> +
> +	/* Waits for the parent to try to umount. */
> +	if (read(pipe_parent, &buf, 1) != 1) {
> +		perror("Failed to write to the third argument");
> +		return 1;
> +	}
> +
> +	/* Shifts arguments. */
> +	argv[0] = argv[1];
> +	argv[1] = argv[2];
> +	argv[2] = argv[3];
> +	argv[3] = NULL;
> +	execve(argv[0], argv, NULL);
> +	perror("Failed to execute the provided binary");
> +	return 1;
> +}
> diff --git a/tools/testing/selftests/landlock/wait-pipe.c b/tools/testing/selftests/landlock/wait-pipe.c
> new file mode 100644
> index 000000000000..0dbcd260a0fa
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/wait-pipe.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Write in a pipe and wait.
> + *
> + * Used by layout1.umount_sandboxer from fs_test.c
> + *
> + * Copyright © 2024-2025 Microsoft Corporation
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +int main(int argc, char *argv[])
> +{
> +	int pipe_child, pipe_parent;
> +	char buf;
> +
> +	/* The first argument must be the file descriptor number of a pipe. */
> +	if (argc != 3) {
> +		fprintf(stderr, "Wrong number of arguments (not two)\n");
> +		return 1;
> +	}
> +
> +	pipe_child = atoi(argv[1]);
> +	pipe_parent = atoi(argv[2]);
> +
> +	/* Signals that we are waiting. */
> +	if (write(pipe_child, ".", 1) != 1) {
> +		perror("Failed to write to first argument");
> +		return 1;
> +	}
> +
> +	/* Waits for the parent do its test. */
> +	if (read(pipe_parent, &buf, 1) != 1) {
> +		perror("Failed to write to the second argument");
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> -- 
> 2.47.1
> 
> 

Powered by blists - more mailing lists