Message-ID: <202110071227.669B5A91C@keescook>
Date:   Thu, 7 Oct 2021 12:48:21 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Mickaël Salaün <mic@...ikod.net>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Aleksa Sarai <cyphar@...har.com>,
        Andy Lutomirski <luto@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Christian Heimes <christian@...hon.org>,
        Deven Bowers <deven.desai@...ux.microsoft.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Eric Biggers <ebiggers@...nel.org>,
        Eric Chiang <ericchiang@...gle.com>,
        Florian Weimer <fweimer@...hat.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        James Morris <jmorris@...ei.org>, Jan Kara <jack@...e.cz>,
        Jann Horn <jannh@...gle.com>, Jonathan Corbet <corbet@....net>,
        Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>,
        "Madhavan T . Venkataraman" <madvenka@...ux.microsoft.com>,
        Matthew Garrett <mjg59@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        Miklos Szeredi <mszeredi@...hat.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Paul Moore <paul@...l-moore.com>,
        Philippe Trébuchet 
        <philippe.trebuchet@....gouv.fr>,
        Scott Shell <scottsh@...rosoft.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Shuah Khan <shuah@...nel.org>,
        Steve Dower <steve.dower@...hon.org>,
        Steve Grubb <sgrubb@...hat.com>,
        Thibaut Sautereau <thibaut.sautereau@....gouv.fr>,
        Vincent Strubel <vincent.strubel@....gouv.fr>,
        kernel-hardening@...ts.openwall.com, linux-api@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-integrity@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Mickaël Salaün <mic@...ux.microsoft.com>
Subject: Re: [PATCH v13 3/3] selftest/interpreter: Add tests for
 trusted_for(2) policies

On Thu, Oct 07, 2021 at 08:23:20PM +0200, Mickaël Salaün wrote:
> From: Mickaël Salaün <mic@...ux.microsoft.com>
> 
> Test that checks performed by trusted_for(2) on file descriptors are
> consistent with noexec mount points and file execute permissions,
> according to the policy configured with the fs.trust_policy sysctl.
> 
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Shuah Khan <shuah@...nel.org>
> Signed-off-by: Mickaël Salaün <mic@...ux.microsoft.com>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@....gouv.fr>
> Link: https://lore.kernel.org/r/20211007182321.872075-4-mic@digikod.net
> ---
> 
> Changes since v12:
> * Fix Makefile's license.
> 
> Changes since v10:
> * Update selftest Makefile.
> 
> Changes since v9:
> * Rename the syscall and the sysctl.
> * Update tests for enum trusted_for_usage
> 
> Changes since v8:
> * Update with the dedicated syscall introspect_access(2) and the renamed
>   fs.introspection_policy sysctl.
> * Remove check symlink which can't be use as is anymore.
> * Use socketpair(2) to test UNIX socket.
> 
> Changes since v7:
> * Update tests with faccessat2/AT_INTERPRETED, including new ones to
>   check that setting R_OK or W_OK returns EINVAL.
> * Add tests for memfd, pipefs and nsfs.
> * Rename and move back tests to a standalone directory.
> 
> Changes since v6:
> * Add full combination tests for all file types, including block
>   devices, character devices, fifos, sockets and symlinks.
> * Properly save and restore initial sysctl value for all tests.
> 
> Changes since v5:
> * Refactor with FIXTURE_VARIANT, which make the tests much more easy to
>   read and maintain.
> * Save and restore initial sysctl value (suggested by Kees Cook).
> * Test with a sysctl value of 0.
> * Check errno in sysctl_access_write test.
> * Update tests for the CAP_SYS_ADMIN switch.
> * Update tests to check -EISDIR (replacing -EACCES).
> * Replace FIXTURE_DATA() with FIXTURE() (spotted by Kees Cook).
> * Use global const strings.
> 
> Changes since v3:
> * Replace RESOLVE_MAYEXEC with O_MAYEXEC.
> * Add tests to check that O_MAYEXEC is ignored by open(2) and openat(2).
> 
> Changes since v2:
> * Move tests from exec/ to openat2/ .
> * Replace O_MAYEXEC with RESOLVE_MAYEXEC from openat2(2).
> * Cleanup tests.
> 
> Changes since v1:
> * Move tests from yama/ to exec/ .
> * Fix _GNU_SOURCE in kselftest_harness.h .
> * Add a new test sysctl_access_write to check if CAP_MAC_ADMIN is taken
>   into account.
> * Test directory execution which is always forbidden since commit
>   73601ea5b7b1 ("fs/open.c: allow opening only regular files during
>   execve()"), and also check that even the root user can not bypass file
>   execution checks.
> * Make sure delete_workspace() always as enough right to succeed.
> * Cosmetic cleanup.
> ---
>  tools/testing/selftests/Makefile              |   1 +
>  .../testing/selftests/interpreter/.gitignore  |   2 +
>  tools/testing/selftests/interpreter/Makefile  |  21 +
>  tools/testing/selftests/interpreter/config    |   1 +
>  .../selftests/interpreter/trust_policy_test.c | 362 ++++++++++++++++++
>  5 files changed, 387 insertions(+)
>  create mode 100644 tools/testing/selftests/interpreter/.gitignore
>  create mode 100644 tools/testing/selftests/interpreter/Makefile
>  create mode 100644 tools/testing/selftests/interpreter/config
>  create mode 100644 tools/testing/selftests/interpreter/trust_policy_test.c
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index c852eb40c4f7..3a032a545f74 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -20,6 +20,7 @@ TARGETS += ftrace
>  TARGETS += futex
>  TARGETS += gpio
>  TARGETS += intel_pstate
> +TARGETS += interpreter
>  TARGETS += ipc
>  TARGETS += ir
>  TARGETS += kcmp
> diff --git a/tools/testing/selftests/interpreter/.gitignore b/tools/testing/selftests/interpreter/.gitignore
> new file mode 100644
> index 000000000000..82a4846cbc4b
> --- /dev/null
> +++ b/tools/testing/selftests/interpreter/.gitignore
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +/*_test
> diff --git a/tools/testing/selftests/interpreter/Makefile b/tools/testing/selftests/interpreter/Makefile
> new file mode 100644
> index 000000000000..1f71a161d40b
> --- /dev/null
> +++ b/tools/testing/selftests/interpreter/Makefile
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +CFLAGS += -Wall -O2
> +LDLIBS += -lcap
> +
> +src_test := $(wildcard *_test.c)
> +TEST_GEN_PROGS := $(src_test:.c=)
> +
> +KSFT_KHDR_INSTALL := 1
> +include ../lib.mk
> +
> +khdr_dir = $(top_srcdir)/usr/include
> +
> +$(khdr_dir)/asm-generic/unistd.h: khdr
> +	@:
> +
> +$(khdr_dir)/linux/trusted-for.h: khdr
> +	@:
> +
> +$(OUTPUT)/%_test: %_test.c $(khdr_dir)/asm-generic/unistd.h $(khdr_dir)/linux/trusted-for.h ../kselftest_harness.h
> +	$(LINK.c) $< $(LDLIBS) -o $@ -I$(khdr_dir)

Is all this really needed?

- CFLAGS and LDLIBS will be used by the default rules
- khdr is already a pre-dependency when KSFT_KHDR_INSTALL is set
- kselftest_harness.h is already a build-dep (see LOCAL_HDRS)
- TEST_GEN_PROGS's .c files are already build-deps

kselftest does, oddly, lack a common -I when KSFT_KHDR_INSTALL is set
(which likely should get fixed, though separately from here).

I think you just want:


src_test := $(wildcard *_test.c)
TEST_GEN_PROGS := $(src_test:.c=)

KSFT_KHDR_INSTALL := 1
include ../lib.mk

CFLAGS += -Wall -O2 -I$(BUILD)/usr/include
LDLIBS += -lcap

$(OUTPUT)/%_test: $(BUILD)/usr/include/linux/trusted-for.h


(untested)

> diff --git a/tools/testing/selftests/interpreter/config b/tools/testing/selftests/interpreter/config
> new file mode 100644
> index 000000000000..dd53c266bf52
> --- /dev/null
> +++ b/tools/testing/selftests/interpreter/config
> @@ -0,0 +1 @@
> +CONFIG_SYSCTL=y
> diff --git a/tools/testing/selftests/interpreter/trust_policy_test.c b/tools/testing/selftests/interpreter/trust_policy_test.c
> new file mode 100644
> index 000000000000..4818c5524ec0
> --- /dev/null
> +++ b/tools/testing/selftests/interpreter/trust_policy_test.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test trusted_for(2) with fs.trust_policy sysctl
> + *
> + * Copyright © 2018-2020 ANSSI
> + *
> + * Author: Mickaël Salaün <mic@...ikod.net>
> + */
> +
> +#define _GNU_SOURCE
> +#include <asm-generic/unistd.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/trusted-for.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/capability.h>
> +#include <sys/mman.h>
> +#include <sys/mount.h>
> +#include <sys/socket.h>
> +#include <sys/stat.h>
> +#include <sys/syscall.h>
> +#include <sys/sysmacros.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "../kselftest_harness.h"
> +
> +#ifndef trusted_for
> +static int trusted_for(const int fd, const enum trusted_for_usage usage,
> +		const __u32 flags)
> +{
> +	errno = 0;
> +	return syscall(__NR_trusted_for, fd, usage, flags);
> +}
> +#endif
> +
> +static const char sysctl_path[] = "/proc/sys/fs/trust_policy";
> +
> +static const char workdir_path[] = "./test-mount";
> +static const char reg_file_path[] = "./test-mount/regular_file";
> +static const char dir_path[] = "./test-mount/directory";
> +static const char block_dev_path[] = "./test-mount/block_device";
> +static const char char_dev_path[] = "./test-mount/character_device";
> +static const char fifo_path[] = "./test-mount/fifo";
> +
> +static void ignore_dac(struct __test_metadata *_metadata, int override)
> +{
> +	cap_t caps;
> +	const cap_value_t cap_val[2] = {
> +		CAP_DAC_OVERRIDE,
> +		CAP_DAC_READ_SEARCH,
> +	};
> +
> +	caps = cap_get_proc();
> +	ASSERT_NE(NULL, caps);
> +	ASSERT_EQ(0, cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_val,
> +				override ? CAP_SET : CAP_CLEAR));
> +	ASSERT_EQ(0, cap_set_proc(caps));
> +	EXPECT_EQ(0, cap_free(caps));
> +}
> +
> +static void ignore_sys_admin(struct __test_metadata *_metadata, int override)
> +{
> +	cap_t caps;
> +	const cap_value_t cap_val[1] = {
> +		CAP_SYS_ADMIN,
> +	};
> +
> +	caps = cap_get_proc();
> +	ASSERT_NE(NULL, caps);
> +	ASSERT_EQ(0, cap_set_flag(caps, CAP_EFFECTIVE, 1, cap_val,
> +				override ? CAP_SET : CAP_CLEAR));
> +	ASSERT_EQ(0, cap_set_proc(caps));
> +	EXPECT_EQ(0, cap_free(caps));
> +}
> +
> +static void test_omx(struct __test_metadata *_metadata,
> +		const char *const path, const int err_access)
> +{
> +	int flags = O_RDONLY | O_CLOEXEC;
> +	int fd, access_ret, access_errno;
> +
> +	/* Do not block on pipes. */
> +	if (path == fifo_path)
> +		flags |= O_NONBLOCK;
> +
> +	fd = open(path, flags);
> +	ASSERT_LE(0, fd) {
> +		TH_LOG("Failed to open %s: %s", path, strerror(errno));
> +	}
> +	access_ret = trusted_for(fd, TRUSTED_FOR_EXECUTION, 0);
> +	access_errno = errno;
> +	if (err_access) {
> +		ASSERT_EQ(err_access, access_errno) {
> +			TH_LOG("Wrong error for trusted_for(2) with %s: %s",
> +					path, strerror(access_errno));
> +		}
> +		ASSERT_EQ(-1, access_ret);
> +	} else {
> +		ASSERT_EQ(0, access_ret) {
> +			TH_LOG("Access denied for %s: %s", path, strerror(access_errno));
> +		}
> +	}
> +
> +	/* Tests unsupported trusted usage. */
> +	access_ret = trusted_for(fd, 0, 0);
> +	ASSERT_EQ(-1, access_ret);
> +	ASSERT_EQ(EINVAL, errno);
> +
> +	access_ret = trusted_for(fd, 2, 0);
> +	ASSERT_EQ(-1, access_ret);
> +	ASSERT_EQ(EINVAL, errno);
> +
> +	EXPECT_EQ(0, close(fd));
> +}
> +
> +static void test_policy_fd(struct __test_metadata *_metadata, const int fd,
> +		const bool has_policy)
> +{
> +	const int ret = trusted_for(fd, TRUSTED_FOR_EXECUTION, 0);
> +
> +	if (has_policy) {
> +		ASSERT_EQ(-1, ret);
> +		ASSERT_EQ(EACCES, errno) {
> +			TH_LOG("Wrong error for trusted_for(2) with FD: %s", strerror(errno));
> +		}
> +	} else {
> +		ASSERT_EQ(0, ret) {
> +			TH_LOG("Access denied for FD: %s", strerror(errno));
> +		}
> +	}
> +}
> +
> +FIXTURE(access) {
> +	char initial_sysctl_value;
> +	int memfd, pipefd;
> +	int pipe_fds[2], socket_fds[2];
> +};
> +
> +static void test_file_types(struct __test_metadata *_metadata, FIXTURE_DATA(access) *self,
> +		const int err_code, const bool has_policy)
> +{
> +	/* Tests are performed on a tmpfs mount point. */
> +	test_omx(_metadata, reg_file_path, err_code);
> +	test_omx(_metadata, dir_path, has_policy ? EACCES : 0);
> +	test_omx(_metadata, block_dev_path, has_policy ? EACCES : 0);
> +	test_omx(_metadata, char_dev_path, has_policy ? EACCES : 0);
> +	test_omx(_metadata, fifo_path, has_policy ? EACCES : 0);
> +
> +	/* Checks that exec is denied for any socket FD. */
> +	test_policy_fd(_metadata, self->socket_fds[0], has_policy);
> +
> +	/* Checks that exec is denied for any memfd. */
> +	test_policy_fd(_metadata, self->memfd, has_policy);
> +
> +	/* Checks that exec is denied for any pipefs FD. */
> +	test_policy_fd(_metadata, self->pipefd, has_policy);
> +}
> +
> +static void test_files(struct __test_metadata *_metadata, FIXTURE_DATA(access) *self,
> +		const int err_code, const bool has_policy)
> +{
> +	/* Tests as root. */
> +	ignore_dac(_metadata, 1);
> +	test_file_types(_metadata, self, err_code, has_policy);
> +
> +	/* Tests without bypass. */
> +	ignore_dac(_metadata, 0);
> +	test_file_types(_metadata, self, err_code, has_policy);
> +}
> +
> +static void sysctl_write_char(struct __test_metadata *_metadata, const char value)
> +{
> +	int fd;
> +
> +	fd = open(sysctl_path, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, fd);
> +	ASSERT_EQ(1, write(fd, &value, 1));
> +	EXPECT_EQ(0, close(fd));
> +}
> +
> +static char sysctl_read_char(struct __test_metadata *_metadata)
> +{
> +	int fd;
> +	char sysctl_value;
> +
> +	fd = open(sysctl_path, O_RDONLY | O_CLOEXEC);
> +	ASSERT_LE(0, fd);
> +	ASSERT_EQ(1, read(fd, &sysctl_value, 1));
> +	EXPECT_EQ(0, close(fd));
> +	return sysctl_value;
> +}
> +
> +FIXTURE_VARIANT(access) {
> +	const bool mount_exec;
> +	const bool file_exec;
> +	const int sysctl_err_code[3];
> +};
> +
> +FIXTURE_VARIANT_ADD(access, mount_exec_file_exec) {
> +	.mount_exec = true,
> +	.file_exec = true,
> +	.sysctl_err_code = {0, 0, 0},
> +};
> +
> +FIXTURE_VARIANT_ADD(access, mount_exec_file_noexec)
> +{
> +	.mount_exec = true,
> +	.file_exec = false,
> +	.sysctl_err_code = {0, EACCES, EACCES},
> +};
> +
> +FIXTURE_VARIANT_ADD(access, mount_noexec_file_exec)
> +{
> +	.mount_exec = false,
> +	.file_exec = true,
> +	.sysctl_err_code = {EACCES, 0, EACCES},
> +};
> +
> +FIXTURE_VARIANT_ADD(access, mount_noexec_file_noexec)
> +{
> +	.mount_exec = false,
> +	.file_exec = false,
> +	.sysctl_err_code = {EACCES, EACCES, EACCES},
> +};
> +
> +FIXTURE_SETUP(access)
> +{
> +	int procfd_path_size;
> +	static const char path_template[] = "/proc/self/fd/%d";
> +	char procfd_path[sizeof(path_template) + 10];
> +
> +	/*
> +	 * Cleans previous workspace if any error previously happened (don't
> +	 * check errors).
> +	 */
> +	umount(workdir_path);
> +	rmdir(workdir_path);
> +
> +	/* Creates a clean mount point. */
> +	ASSERT_EQ(0, mkdir(workdir_path, 00700));
> +	ASSERT_EQ(0, mount("test", workdir_path, "tmpfs", MS_MGC_VAL |
> +				(variant->mount_exec ? 0 : MS_NOEXEC),
> +				"mode=0700,size=4k"));
> +
> +	/* Creates a regular file. */
> +	ASSERT_EQ(0, mknod(reg_file_path, S_IFREG | (variant->file_exec ? 0500 : 0400), 0));
> +	/* Creates a directory. */
> +	ASSERT_EQ(0, mkdir(dir_path, variant->file_exec ? 0500 : 0400));
> +	/* Creates a character device: /dev/null. */
> +	ASSERT_EQ(0, mknod(char_dev_path, S_IFCHR | 0400, makedev(1, 3)));
> +	/* Creates a block device: /dev/loop0 */
> +	ASSERT_EQ(0, mknod(block_dev_path, S_IFBLK | 0400, makedev(7, 0)));
> +	/* Creates a fifo. */
> +	ASSERT_EQ(0, mknod(fifo_path, S_IFIFO | 0400, 0));
> +
> +	/* Creates a regular file without user mount point. */
> +	self->memfd = memfd_create("test-interpreted", MFD_CLOEXEC);
> +	ASSERT_LE(0, self->memfd);
> +	/* Sets mode, which must be ignored by the exec check. */
> +	ASSERT_EQ(0, fchmod(self->memfd, variant->file_exec ? 0500 : 0400));
> +
> +	/* Creates a pipefs file descriptor. */
> +	ASSERT_EQ(0, pipe(self->pipe_fds));
> +	procfd_path_size = snprintf(procfd_path, sizeof(procfd_path),
> +			path_template, self->pipe_fds[0]);
> +	ASSERT_LT(procfd_path_size, sizeof(procfd_path));
> +	self->pipefd = open(procfd_path, O_RDONLY | O_CLOEXEC);
> +	ASSERT_LE(0, self->pipefd);
> +	ASSERT_EQ(0, fchmod(self->pipefd, variant->file_exec ? 0500 : 0400));
> +
> +	/* Creates a socket file descriptor. */
> +	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0, self->socket_fds));
> +
> +	/* Saves initial sysctl value. */
> +	self->initial_sysctl_value = sysctl_read_char(_metadata);
> +
> +	/* Prepares for sysctl writes. */
> +	ignore_sys_admin(_metadata, 1);
> +}
> +
> +FIXTURE_TEARDOWN(access)
> +{
> +	EXPECT_EQ(0, close(self->memfd));
> +	EXPECT_EQ(0, close(self->pipefd));
> +	EXPECT_EQ(0, close(self->pipe_fds[0]));
> +	EXPECT_EQ(0, close(self->pipe_fds[1]));
> +	EXPECT_EQ(0, close(self->socket_fds[0]));
> +	EXPECT_EQ(0, close(self->socket_fds[1]));
> +
> +	/* Restores initial sysctl value. */
> +	sysctl_write_char(_metadata, self->initial_sysctl_value);
> +
> +	/* There is no need to unlink the test files. */
> +	ASSERT_EQ(0, umount(workdir_path));
> +	ASSERT_EQ(0, rmdir(workdir_path));
> +}
> +
> +TEST_F(access, sysctl_0)
> +{
> +	/* Do not enforce anything. */
> +	sysctl_write_char(_metadata, '0');
> +	test_files(_metadata, self, 0, false);
> +}
> +
> +TEST_F(access, sysctl_1)
> +{
> +	/* Enforces mount exec check. */
> +	sysctl_write_char(_metadata, '1');
> +	test_files(_metadata, self, variant->sysctl_err_code[0], true);
> +}
> +
> +TEST_F(access, sysctl_2)
> +{
> +	/* Enforces file exec check. */
> +	sysctl_write_char(_metadata, '2');
> +	test_files(_metadata, self, variant->sysctl_err_code[1], true);
> +}
> +
> +TEST_F(access, sysctl_3)
> +{
> +	/* Enforces mount and file exec check. */
> +	sysctl_write_char(_metadata, '3');
> +	test_files(_metadata, self, variant->sysctl_err_code[2], true);
> +}
> +
> +FIXTURE(cleanup) {
> +	char initial_sysctl_value;
> +};
> +
> +FIXTURE_SETUP(cleanup)
> +{
> +	/* Saves initial sysctl value. */
> +	self->initial_sysctl_value = sysctl_read_char(_metadata);
> +}
> +
> +FIXTURE_TEARDOWN(cleanup)
> +{
> +	/* Restores initial sysctl value. */
> +	ignore_sys_admin(_metadata, 1);
> +	sysctl_write_char(_metadata, self->initial_sysctl_value);
> +}
> +
> +TEST_F(cleanup, sysctl_access_write)
> +{
> +	int fd;
> +	ssize_t ret;
> +
> +	ignore_sys_admin(_metadata, 1);
> +	sysctl_write_char(_metadata, '0');
> +
> +	ignore_sys_admin(_metadata, 0);
> +	fd = open(sysctl_path, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, fd);
> +	ret = write(fd, "0", 1);
> +	ASSERT_EQ(-1, ret);
> +	ASSERT_EQ(EPERM, errno);
> +	EXPECT_EQ(0, close(fd));
> +}
> +
> +TEST_HARNESS_MAIN
> -- 
> 2.32.0
> 

Yay tests! :)

-- 
Kees Cook

Powered by blists - more mailing lists