| Message ID | 20211007182321.872075-4-mic@digikod.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add trusted_for(2) (was O_MAYEXEC) | expand |
On Thu, Oct 07, 2021 at 08:23:20PM +0200, Mickaël Salaün wrote: > From: Mickaël Salaün <mic@linux.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@zeniv.linux.org.uk> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: Shuah Khan <shuah@kernel.org> > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.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@digikod.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! :)
On 07/10/2021 21:48, Kees Cook wrote: > On Thu, Oct 07, 2021 at 08:23:20PM +0200, Mickaël Salaün wrote: [...] >> 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? Yes, all this is needed to be sure that the tests will be rebuild when a dependency change (either one of the header files or a source file). > > - CFLAGS and LDLIBS will be used by the default rules Yes, but it will only run the build command when a source file change, not a header file. > - khdr is already a pre-dependency when KSFT_KHDR_INSTALL is set Yes, but it is not enough to rebuild the tests (and check the installed files) when a header file change. > - kselftest_harness.h is already a build-dep (see LOCAL_HDRS) Yes, but without an explicit requirement, changing kselftest_harness.h doesn't force a rebuild. > - TEST_GEN_PROGS's .c files are already build-deps It is not enough to trigger test rebuilds. > > 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) > Yep, I re-checked and my Makefile is correct. I didn't find a way to make it lighter while correctly handling dependencies. I'll just move the -I to CFLAGS.
On Fri, Oct 08, 2021 at 12:21:12PM +0200, Mickaël Salaün wrote: > > On 07/10/2021 21:48, Kees Cook wrote: > > On Thu, Oct 07, 2021 at 08:23:20PM +0200, Mickaël Salaün wrote: > > [...] > > >> 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? > > Yes, all this is needed to be sure that the tests will be rebuild when a > dependency change (either one of the header files or a source file). > > > > > - CFLAGS and LDLIBS will be used by the default rules > > Yes, but it will only run the build command when a source file change, > not a header file. > > > - khdr is already a pre-dependency when KSFT_KHDR_INSTALL is set > > Yes, but it is not enough to rebuild the tests (and check the installed > files) when a header file change. > > > - kselftest_harness.h is already a build-dep (see LOCAL_HDRS) > > Yes, but without an explicit requirement, changing kselftest_harness.h > doesn't force a rebuild. > > > - TEST_GEN_PROGS's .c files are already build-deps > > It is not enough to trigger test rebuilds. > > > > > 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) > > > Yep, I re-checked and my Makefile is correct. I didn't find a way to > make it lighter while correctly handling dependencies. > I'll just move the -I to CFLAGS. Okay, thanks for double-checking these. I'll try to fix up kselftests to DTRT here.
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) 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@digikod.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