[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
