| Message ID | 20200417163804.307854-1-richard_c_haines@btinternet.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Ondrej Mosnáček |
| Headers | show |
| Series | [RFC] selinux-testsuite: Add check for key changes on watch_queue | expand |
On Fri, Apr 17, 2020 at 6:38 PM Richard Haines <richard_c_haines@btinternet.com> wrote: > Kernel 5.7 introduced the watch_queue service that allows watching for > key changes. This requires key { view } permission, therefore check if > allowed or not. > > Note that the keyctl_watch_key() function is not yet built into the > keyutils library, therefore a syscall() is used. > > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> OK, so I tried to build kernel RPMs(*) from the linux-next tree and try out this patch, but I ran into a few problems: 1. <linux/watch_queue.h> includes <linux/fcntl.h>, which conflicts with glibc's <sys/fcntl.h>. This will maybe get fixed in glibc/kernel eventually, but I can't apply the patch as-is (maybe we can #define _LINUX_FCNTL_H before including <linux/watch_queue.h> as a temporary workaround?). 2. Even when I got the test program to build, the test failed. I ran out of time to dig further, but it may be due to the issues I spotted later in the test policy (see below). (*) I wanted to try out the new Fedora's kernel build machinery, so I burnt too much time on this... but I now have a semi-automated way to build linux-next kernels as RPMs, which might come in handy in the future :) > --- > defconfig | 5 ++ > policy/Makefile | 2 +- > policy/test_watchkey.te | 34 ++++++++++++ > tests/Makefile | 4 ++ > tests/watchkey/.gitignore | 1 + > tests/watchkey/Makefile | 7 +++ > tests/watchkey/test | 29 ++++++++++ > tests/watchkey/watchkey.c | 113 ++++++++++++++++++++++++++++++++++++++ > 8 files changed, 194 insertions(+), 1 deletion(-) > create mode 100644 policy/test_watchkey.te > create mode 100644 tests/watchkey/.gitignore > create mode 100644 tests/watchkey/Makefile > create mode 100755 tests/watchkey/test > create mode 100644 tests/watchkey/watchkey.c > > diff --git a/defconfig b/defconfig > index 0574f1d..9afbc2f 100644 > --- a/defconfig > +++ b/defconfig > @@ -78,3 +78,8 @@ CONFIG_KEY_DH_OPERATIONS=y > # Test key management socket. > # This is not required for SELinux operation itself. > CONFIG_NET_KEY=m > + > +# watch_queue for key changes. > +# They are not required for SELinux operation itself. > +CONFIG_WATCH_QUEUE=y > +CONFIG_KEY_NOTIFICATIONS=y > diff --git a/policy/Makefile b/policy/Makefile > index 87b2856..b3f5e3d 100644 > --- a/policy/Makefile > +++ b/policy/Makefile > @@ -86,7 +86,7 @@ TARGETS += test_bpf.te test_fdreceive_bpf.te test_binder_bpf.te > endif > > ifeq ($(shell grep -q all_key_perms $(POLDEV)/include/support/all_perms.spt && echo true),true) > -TARGETS += test_keys.te > +TARGETS += test_keys.te test_watchkey.te > endif > > ifeq ($(shell grep -q all_file_perms.*watch $(POLDEV)/include/support/all_perms.spt && echo true),true) > diff --git a/policy/test_watchkey.te b/policy/test_watchkey.te > new file mode 100644 > index 0000000..e1d4c78 > --- /dev/null > +++ b/policy/test_watchkey.te > @@ -0,0 +1,34 @@ > +# > +######### Check watch_queue for key changes policy module ########## > +# > +attribute watchkeydomain; > + > +################# Allow watch_queue key { view } ########################## > +type test_watchkey_t; > +domain_type(test_watchkey_t) > +unconfined_runs_test(test_watchkey_t) > +typeattribute test_watchkey_t testdomain; > +typeattribute test_watchkey_t keydomain; You declare attribute "watchkeydomain" above and later call some interfaces on it, but assign all the types to "keydomain". I assume you meant to assign them to "watchkeydomain"? > + > +allow test_watchkey_t self:capability { ipc_lock }; > +allow test_watchkey_t device_t:chr_file { ioctl open read write }; > +allow test_watchkey_t self:key { view }; > +allow_map(test_watchkey_t, device_t, chr_file) > + > +################# Deny watch_queue key { view } ########################## > +type test_watchkey_no_view_t; > +domain_type(test_watchkey_no_view_t) > +unconfined_runs_test(test_watchkey_no_view_t) > +typeattribute test_watchkey_no_view_t testdomain; > +typeattribute test_watchkey_no_view_t keydomain; > + > +allow test_watchkey_no_view_t self:capability { ipc_lock }; > +allow test_watchkey_no_view_t device_t:chr_file { ioctl open read write }; > +neverallow test_watchkey_no_view_t self:key { view }; > +allow_map(test_watchkey_no_view_t, device_t, chr_file) > + > +# > +########### Allow these domains to be entered from sysadm domain ############ > +# > +miscfiles_domain_entry_test_files(watchkeydomain) > +userdom_sysadm_entry_spec_domtrans_to(watchkeydomain) [...]
On Thu, 2020-05-14 at 10:50 +0200, Ondrej Mosnacek wrote: > On Fri, Apr 17, 2020 at 6:38 PM Richard Haines > <richard_c_haines@btinternet.com> wrote: > > Kernel 5.7 introduced the watch_queue service that allows watching > > for > > key changes. This requires key { view } permission, therefore check > > if > > allowed or not. > > > > Note that the keyctl_watch_key() function is not yet built into the > > keyutils library, therefore a syscall() is used. > > > > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> > > OK, so I tried to build kernel RPMs(*) from the linux-next tree and > try out this patch, but I ran into a few problems: > 1. <linux/watch_queue.h> includes <linux/fcntl.h>, which conflicts > with glibc's <sys/fcntl.h>. This will maybe get fixed in glibc/kernel > eventually, but I can't apply the patch as-is (maybe we can #define > _LINUX_FCNTL_H before including <linux/watch_queue.h> as a temporary > workaround?). > 2. Even when I got the test program to build, the test failed. I ran > out of time to dig further, but it may be due to the issues I spotted > later in the test policy (see below). > > (*) I wanted to try out the new Fedora's kernel build machinery, so I > burnt too much time on this... but I now have a semi-automated way to > build linux-next kernels as RPMs, which might come in handy in the > future :) Thanks for the feedback I'll see about fixing the issues. > > > --- > > defconfig | 5 ++ > > policy/Makefile | 2 +- > > policy/test_watchkey.te | 34 ++++++++++++ > > tests/Makefile | 4 ++ > > tests/watchkey/.gitignore | 1 + > > tests/watchkey/Makefile | 7 +++ > > tests/watchkey/test | 29 ++++++++++ > > tests/watchkey/watchkey.c | 113 > > ++++++++++++++++++++++++++++++++++++++ > > 8 files changed, 194 insertions(+), 1 deletion(-) > > create mode 100644 policy/test_watchkey.te > > create mode 100644 tests/watchkey/.gitignore > > create mode 100644 tests/watchkey/Makefile > > create mode 100755 tests/watchkey/test > > create mode 100644 tests/watchkey/watchkey.c > > > > diff --git a/defconfig b/defconfig > > index 0574f1d..9afbc2f 100644 > > --- a/defconfig > > +++ b/defconfig > > @@ -78,3 +78,8 @@ CONFIG_KEY_DH_OPERATIONS=y > > # Test key management socket. > > # This is not required for SELinux operation itself. > > CONFIG_NET_KEY=m > > + > > +# watch_queue for key changes. > > +# They are not required for SELinux operation itself. > > +CONFIG_WATCH_QUEUE=y > > +CONFIG_KEY_NOTIFICATIONS=y > > diff --git a/policy/Makefile b/policy/Makefile > > index 87b2856..b3f5e3d 100644 > > --- a/policy/Makefile > > +++ b/policy/Makefile > > @@ -86,7 +86,7 @@ TARGETS += test_bpf.te test_fdreceive_bpf.te > > test_binder_bpf.te > > endif > > > > ifeq ($(shell grep -q all_key_perms > > $(POLDEV)/include/support/all_perms.spt && echo true),true) > > -TARGETS += test_keys.te > > +TARGETS += test_keys.te test_watchkey.te > > endif > > > > ifeq ($(shell grep -q all_file_perms.*watch > > $(POLDEV)/include/support/all_perms.spt && echo true),true) > > diff --git a/policy/test_watchkey.te b/policy/test_watchkey.te > > new file mode 100644 > > index 0000000..e1d4c78 > > --- /dev/null > > +++ b/policy/test_watchkey.te > > @@ -0,0 +1,34 @@ > > +# > > +######### Check watch_queue for key changes policy module > > ########## > > +# > > +attribute watchkeydomain; > > + > > +################# Allow watch_queue key { view } > > ########################## > > +type test_watchkey_t; > > +domain_type(test_watchkey_t) > > +unconfined_runs_test(test_watchkey_t) > > +typeattribute test_watchkey_t testdomain; > > +typeattribute test_watchkey_t keydomain; > > You declare attribute "watchkeydomain" above and later call some > interfaces on it, but assign all the types to "keydomain". I assume > you meant to assign them to "watchkeydomain"? > > > + > > +allow test_watchkey_t self:capability { ipc_lock }; > > +allow test_watchkey_t device_t:chr_file { ioctl open read write }; > > +allow test_watchkey_t self:key { view }; > > +allow_map(test_watchkey_t, device_t, chr_file) > > + > > +################# Deny watch_queue key { view } > > ########################## > > +type test_watchkey_no_view_t; > > +domain_type(test_watchkey_no_view_t) > > +unconfined_runs_test(test_watchkey_no_view_t) > > +typeattribute test_watchkey_no_view_t testdomain; > > +typeattribute test_watchkey_no_view_t keydomain; > > + > > +allow test_watchkey_no_view_t self:capability { ipc_lock }; > > +allow test_watchkey_no_view_t device_t:chr_file { ioctl open read > > write }; > > +neverallow test_watchkey_no_view_t self:key { view }; > > +allow_map(test_watchkey_no_view_t, device_t, chr_file) > > + > > +# > > +########### Allow these domains to be entered from sysadm domain > > ############ > > +# > > +miscfiles_domain_entry_test_files(watchkeydomain) > > +userdom_sysadm_entry_spec_domtrans_to(watchkeydomain) > [...] >
diff --git a/defconfig b/defconfig index 0574f1d..9afbc2f 100644 --- a/defconfig +++ b/defconfig @@ -78,3 +78,8 @@ CONFIG_KEY_DH_OPERATIONS=y # Test key management socket. # This is not required for SELinux operation itself. CONFIG_NET_KEY=m + +# watch_queue for key changes. +# They are not required for SELinux operation itself. +CONFIG_WATCH_QUEUE=y +CONFIG_KEY_NOTIFICATIONS=y diff --git a/policy/Makefile b/policy/Makefile index 87b2856..b3f5e3d 100644 --- a/policy/Makefile +++ b/policy/Makefile @@ -86,7 +86,7 @@ TARGETS += test_bpf.te test_fdreceive_bpf.te test_binder_bpf.te endif ifeq ($(shell grep -q all_key_perms $(POLDEV)/include/support/all_perms.spt && echo true),true) -TARGETS += test_keys.te +TARGETS += test_keys.te test_watchkey.te endif ifeq ($(shell grep -q all_file_perms.*watch $(POLDEV)/include/support/all_perms.spt && echo true),true) diff --git a/policy/test_watchkey.te b/policy/test_watchkey.te new file mode 100644 index 0000000..e1d4c78 --- /dev/null +++ b/policy/test_watchkey.te @@ -0,0 +1,34 @@ +# +######### Check watch_queue for key changes policy module ########## +# +attribute watchkeydomain; + +################# Allow watch_queue key { view } ########################## +type test_watchkey_t; +domain_type(test_watchkey_t) +unconfined_runs_test(test_watchkey_t) +typeattribute test_watchkey_t testdomain; +typeattribute test_watchkey_t keydomain; + +allow test_watchkey_t self:capability { ipc_lock }; +allow test_watchkey_t device_t:chr_file { ioctl open read write }; +allow test_watchkey_t self:key { view }; +allow_map(test_watchkey_t, device_t, chr_file) + +################# Deny watch_queue key { view } ########################## +type test_watchkey_no_view_t; +domain_type(test_watchkey_no_view_t) +unconfined_runs_test(test_watchkey_no_view_t) +typeattribute test_watchkey_no_view_t testdomain; +typeattribute test_watchkey_no_view_t keydomain; + +allow test_watchkey_no_view_t self:capability { ipc_lock }; +allow test_watchkey_no_view_t device_t:chr_file { ioctl open read write }; +neverallow test_watchkey_no_view_t self:key { view }; +allow_map(test_watchkey_no_view_t, device_t, chr_file) + +# +########### Allow these domains to be entered from sysadm domain ############ +# +miscfiles_domain_entry_test_files(watchkeydomain) +userdom_sysadm_entry_spec_domtrans_to(watchkeydomain) diff --git a/tests/Makefile b/tests/Makefile index 1cdb1ac..ed09a49 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -78,6 +78,10 @@ SUBDIRS+=module_load endif endif +ifeq ($(shell grep -q all_key_perms $(POLDEV)/include/support/all_perms.spt && test -e $(INCLUDEDIR)/linux/watch_queue.h && grep -qs KEYCTL_WATCH_KEY $(INCLUDEDIR)/linux/keyctl.h && echo true),true) +SUBDIRS += watchkey +endif + ifeq ($(DISTRO),RHEL4) SUBDIRS:=$(filter-out bounds dyntrace dyntrans inet_socket mmap nnp_nosuid overlay unix_socket, $(SUBDIRS)) endif diff --git a/tests/watchkey/.gitignore b/tests/watchkey/.gitignore new file mode 100644 index 0000000..04db718 --- /dev/null +++ b/tests/watchkey/.gitignore @@ -0,0 +1 @@ +watchkey diff --git a/tests/watchkey/Makefile b/tests/watchkey/Makefile new file mode 100644 index 0000000..c2796fb --- /dev/null +++ b/tests/watchkey/Makefile @@ -0,0 +1,7 @@ +TARGETS = watchkey +LDLIBS += -lselinux + +all: $(TARGETS) + +clean: + rm -f $(TARGETS) diff --git a/tests/watchkey/test b/tests/watchkey/test new file mode 100755 index 0000000..f61ff78 --- /dev/null +++ b/tests/watchkey/test @@ -0,0 +1,29 @@ +#!/usr/bin/perl +use Test::More; + +BEGIN { + $basedir = $0; + $basedir =~ s|(.*)/[^/]*|$1|; + + # allow info to be shown during tests + $v = $ARGV[0]; + if ($v) { + if ( $v ne "-v" ) { + plan skip_all => "Invalid option (use -v)"; + } + } + else { + $v = " "; + } + + plan tests => 2; +} + +$result = system "runcon -t test_watchkey_t $basedir/watchkey $v"; +ok( $result eq 0 ); + +# Deny key { view } - EACCES +$result = system "runcon -t test_watchkey_no_view_t $basedir/watchkey $v 2>&1"; +ok( $result >> 8 eq 13 ); + +exit; diff --git a/tests/watchkey/watchkey.c b/tests/watchkey/watchkey.c new file mode 100644 index 0000000..f645a31 --- /dev/null +++ b/tests/watchkey/watchkey.c @@ -0,0 +1,113 @@ +/* Based on kernel source samples/watch_queue/watch_test.c */ + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> +#include <fcntl.h> +#include <errno.h> +#include <stdbool.h> +#include <sys/ioctl.h> +#include <sys/mman.h> +#include <linux/watch_queue.h> +#include <linux/unistd.h> +#include <linux/keyctl.h> +#include <selinux/selinux.h> + +#define BUF_SIZE 4 +#define W_DEV "/dev/watch_queue" + +/* Require syscall() as function not yet in libkeyutils */ +static long keyctl_watch_key(int key, int watch_fd, int watch_id) +{ + return syscall(__NR_keyctl, KEYCTL_WATCH_KEY, key, watch_fd, watch_id); +} + +static void print_usage(char *progname) +{ + fprintf(stderr, + "usage: %s [-v]\n" + "Where:\n\t" + "-v Print information.\n", progname); + exit(-1); +} + +int main(int argc, char **argv) +{ + int opt, fd, result, save_errno; + char *context; + bool verbose = false; + size_t page_size, mmap_size; + struct watch_queue_buffer *buf; + + while ((opt = getopt(argc, argv, "v")) != -1) { + switch (opt) { + case 'v': + verbose = true; + break; + default: + print_usage(argv[0]); + } + } + + if (verbose) { + result = getcon(&context); + if (result < 0) { + fprintf(stderr, "Failed to obtain process context\n"); + exit(-1); + } + + printf("Process context:\n\t%s\n", context); + free(context); + } + + fd = open(W_DEV, O_RDWR); + if (fd < 0) { + fprintf(stderr, "Failed to open: %s Error: %s\n", W_DEV, + strerror(errno)); + exit(-1); + } + if (verbose) + printf("Opened %s\n", W_DEV); + + result = ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE); + if (result < 0) { + fprintf(stderr, "Failed to set watch_queue size: %s\n", + strerror(errno)); + exit(-1); + } + + page_size = sysconf(_SC_PAGESIZE); + if (page_size < 0) { + fprintf(stderr, "Failed sysconf(_SC_PAGESIZE): %s\n", + strerror(errno)); + close(fd); + exit(-1); + } + mmap_size = page_size * BUF_SIZE; + + buf = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + if (buf == MAP_FAILED) { + fprintf(stderr, "Failed mmap(2): %s\n", strerror(errno)); + close(fd); + exit(-1); + } + + save_errno = 0; + /* Requires key { view } */ + result = keyctl_watch_key(KEY_SPEC_PROCESS_KEYRING, fd, + WATCH_TYPE_KEY_NOTIFY); + if (result < 0) { + fprintf(stderr, "Failed keyctl_watch_key(): %s\n", + strerror(errno)); + save_errno = errno; + goto err; + } + if (verbose) + printf("keyctl_watch_key() successful\n"); + +err: + munmap(buf, mmap_size); + close(fd); + return save_errno; +}
Kernel 5.7 introduced the watch_queue service that allows watching for key changes. This requires key { view } permission, therefore check if allowed or not. Note that the keyctl_watch_key() function is not yet built into the keyutils library, therefore a syscall() is used. Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> --- defconfig | 5 ++ policy/Makefile | 2 +- policy/test_watchkey.te | 34 ++++++++++++ tests/Makefile | 4 ++ tests/watchkey/.gitignore | 1 + tests/watchkey/Makefile | 7 +++ tests/watchkey/test | 29 ++++++++++ tests/watchkey/watchkey.c | 113 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 194 insertions(+), 1 deletion(-) create mode 100644 policy/test_watchkey.te create mode 100644 tests/watchkey/.gitignore create mode 100644 tests/watchkey/Makefile create mode 100755 tests/watchkey/test create mode 100644 tests/watchkey/watchkey.c