On Tue, Sep 10, 2019 at 08:26:57AM -0400, Stephen Smalley wrote: > On 9/10/19 1:59 AM, Dominick Grift wrote: > > On Wed, Jul 10, 2019 at 09:39:17AM -0400, Aaron Goidel wrote: > > > Added a suite to test permissions for setting inotify and fanotify watches > > > on filesystem objects. Tests watch, watch_with_perm, and watch_reads permissions. > > > > Ive also "tested" the fsnotify patch. And my tests indicate that this might cause issues: > > > > I added the access vectors to my policy, but on older systems (debian 10) cron fails to start > > cron needs the "entrypoint" permission on /etc/crontab but it looks like appending the "watch" access vectors to common-file disrupted the ordering > > The result is that now i have to allow cron to "watch_read" /etc/crontab even though the neither kernel nor selinux user space are aware of the fsnotify access vectors > > It seems the cron selinux code got confused and now thinks watch_read is entrypoint (its using selinux code to determine whether it can manually transition to cronjob domains on crontabs) > > > > I am hoping this issue with resolve itself on systems with kernels and user spaces that suppose fsnotify. > > However unless i am overlooking something this is still likely to disrupt compatibility > > So, IIUC, the issue is that: > a) older cron was directly using the fixed FILE__ENTRYPOINT definition from > libselinux in a security_compute_av() call rather than dynamically looking > up entrypoint permission, > b) the value of the file entrypoint permission in policy has changed because > you updated your policy and chose to place the watch* permissions in the > common file definition to match the kernel. > > Note that you could have instead sprinkled copies of the watch* permissions > across all of the individual file/dir/*_file classes and placed them at the > end of the classes if you wanted to avoid this compatibility issue. The > policy and kernel definitions no longer need to be identical; the kernel > will dynamically map between them for its own permission checks. > > Also, this should have already been broken for you when map permission was > added, which would have shown up as execute_no_trans being checked instead > of entrypoint. This was already filed as a bug on cron in debian here: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=924716 > > So, wrt compatibility, IIUC: > - the kernel change does not break compatibility for old policy + old > userspace, > - new policy can choose to add the new permissions in a backward-compatible > manner if desired, although refpolicy has already chosen not to do this for > map permission, > - the offending code in cron produces build warnings that were introduced in > 2014 in libselinux to update their code to use the dynamic class/perm > mapping support, which would resolve the issue. this does not work (cron[8934]: ((null)) ENTRYPOINT FAILED): https://defensec.nl/gitweb/dssp2.git/commitdiff/914b1d5366922f92435cba780519c5d2bb9f4f7d [kcinimod@brutus dssp2 (master=)]$ seinfo policy.31 -x --common common_file Commons: 1 common common_file { append lock mounton unlink read create quotaon execute setattr map relabelto rename link ioctl getattr relabelfrom } [kcinimod@brutus dssp2 (master=)]$ seinfo policy.31 -xcfile Classes: 1 class file inherits common_file { execmod watch write watch_reads execute_no_trans watch_mount watch_with_perm open watch_sb audit_access entrypoint } > > > > > > > > > Signed-off-by: Aaron Goidel <acgoide@xxxxxxxxxxxxx> > > > --- > > > policy/Makefile | 4 ++ > > > policy/test_notify.te | 74 ++++++++++++++++++++++++ > > > tests/Makefile | 4 ++ > > > tests/notify/Makefile | 5 ++ > > > tests/notify/test | 101 +++++++++++++++++++++++++++++++++ > > > tests/notify/test_fanotify.c | 105 +++++++++++++++++++++++++++++++++++ > > > tests/notify/test_inotify.c | 43 ++++++++++++++ > > > 7 files changed, 336 insertions(+) > > > create mode 100644 policy/test_notify.te > > > create mode 100644 tests/notify/Makefile > > > create mode 100755 tests/notify/test > > > create mode 100644 tests/notify/test_fanotify.c > > > create mode 100644 tests/notify/test_inotify.c > > > > > > diff --git a/policy/Makefile b/policy/Makefile > > > index 305b572..65f88c5 100644 > > > --- a/policy/Makefile > > > +++ b/policy/Makefile > > > @@ -71,6 +71,10 @@ ifeq ($(shell grep -q corenet_sctp_bind_all_nodes $(POLDEV)/include/kernel/coren > > > TARGETS += test_sctp.te > > > endif > > > +ifeq ($(shell grep -q all_file_perms.*watch $(POLDEV)/include/support/all_perms.spt && echo true),true) > > > +TARGETS+=test_notify.te > > > +endif > > > + > > > ifeq (x$(DISTRO),$(filter x$(DISTRO),xRHEL4 xRHEL5 xRHEL6)) > > > TARGETS:=$(filter-out test_overlayfs.te test_mqueue.te, $(TARGETS)) > > > endif > > > diff --git a/policy/test_notify.te b/policy/test_notify.te > > > new file mode 100644 > > > index 0000000..8ba6f1a > > > --- /dev/null > > > +++ b/policy/test_notify.te > > > @@ -0,0 +1,74 @@ > > > +#################################################### > > > +# Policy for testing inoftify and fanotify watches # > > > +#################################################### > > > + > > > +attribute test_notify_domain; > > > + > > > +# Type for the file on which we want to set a watch > > > +type test_notify_file_t; > > > +files_type(test_notify_file_t); > > > + > > > +# Domain for the process which CAN set a non-permission watch > > > +type test_watch_t; > > > +domain_type(test_watch_t); > > > +unconfined_runs_test(test_watch_t); > > > + > > > +typeattribute test_watch_t test_notify_domain; > > > +typeattribute test_watch_t testdomain; > > > + > > > +allow test_notify_domain self:capability sys_admin; > > > + > > > +allow test_watch_t test_notify_file_t:file { read write open watch }; > > > + > > > +# Domain for the process which CAN set a NON-access watch on a file > > > +type test_perm_watch_t; > > > +domain_type(test_perm_watch_t); > > > +unconfined_runs_test(test_perm_watch_t); > > > + > > > +typeattribute test_perm_watch_t test_notify_domain; > > > +typeattribute test_perm_watch_t testdomain; > > > + > > > +allow test_perm_watch_t test_notify_file_t:file { read write open watch watch_with_perm }; > > > + > > > +# Domain which CAN set a NON-perm watch which MAY read accesses > > > +type test_read_watch_t; > > > +domain_type(test_read_watch_t); > > > +unconfined_runs_test(test_read_watch_t); > > > + > > > +typeattribute test_read_watch_t test_notify_domain; > > > +typeattribute test_read_watch_t testdomain; > > > + > > > +allow test_read_watch_t test_notify_file_t:file { read write open watch watch_reads }; > > > + > > > +# Domain which CAN set any watch which CAN read accesses > > > +type test_perm_read_watch_t; > > > +domain_type(test_perm_read_watch_t); > > > +unconfined_runs_test(test_perm_read_watch_t); > > > + > > > +typeattribute test_perm_read_watch_t test_notify_domain; > > > +typeattribute test_perm_read_watch_t testdomain; > > > + > > > +allow test_perm_read_watch_t test_notify_file_t:file { read write open watch watch_with_perm watch_reads }; > > > + > > > +# Domain which CANNOT set any watches > > > +type test_no_watch_t; > > > +domain_type(test_no_watch_t); > > > +unconfined_runs_test(test_no_watch_t); > > > + > > > +typeattribute test_no_watch_t test_notify_domain; > > > +typeattribute test_no_watch_t testdomain; > > > + > > > +allow test_no_watch_t test_notify_file_t:file { read write open }; > > > + > > > +# Domain which has no write access but can watch > > > +type test_rdonly_t; > > > +domain_type(test_rdonly_t); > > > +unconfined_runs_test(test_rdonly_t); > > > + > > > +typeattribute test_rdonly_t test_notify_domain; > > > +typeattribute test_rdonly_t testdomain; > > > + > > > +allow test_rdonly_t test_notify_file_t:file { read open watch }; > > > + > > > +miscfiles_domain_entry_test_files(test_notify_domain); > > > +userdom_sysadm_entry_spec_domtrans_to(test_notify_domain); > > > diff --git a/tests/Makefile b/tests/Makefile > > > index 63aa325..b99c96e 100644 > > > --- a/tests/Makefile > > > +++ b/tests/Makefile > > > @@ -50,6 +50,10 @@ ifeq ($(shell grep "^SELINUX_INFINIBAND_PKEY_TEST=" infiniband_pkey/ibpkey_test. > > > SUBDIRS += infiniband_pkey > > > endif > > > +ifeq ($(shell grep -q all_file_perms.*watch $(POLDEV)/include/support/all_perms.spt && echo true),true) > > > +SUBDIRS+=notify > > > +endif > > > + > > > ifeq ($(DISTRO),RHEL4) > > > SUBDIRS:=$(filter-out bounds dyntrace dyntrans inet_socket mmap nnp_nosuid overlay unix_socket, $(SUBDIRS)) > > > endif > > > diff --git a/tests/notify/Makefile b/tests/notify/Makefile > > > new file mode 100644 > > > index 0000000..78c4b3b > > > --- /dev/null > > > +++ b/tests/notify/Makefile > > > @@ -0,0 +1,5 @@ > > > +TARGETS=test_inotify test_fanotify > > > + > > > +all: $(TARGETS) > > > +clean: > > > + rm -f $(TARGETS) > > > diff --git a/tests/notify/test b/tests/notify/test > > > new file mode 100755 > > > index 0000000..21f03de > > > --- /dev/null > > > +++ b/tests/notify/test > > > @@ -0,0 +1,101 @@ > > > +#!/usr/bin/perl > > > + > > > +use Test; > > > +BEGIN { plan tests => 14 } # number of tests to run > > > + > > > +# help the test script locate itself > > > +$basedir = $0; > > > +$basedir =~ s|(.*)/[^/]*|$1|; > > > + > > > +# Get rid of a testfile from last run if it's there (just in case) > > > +system("rm -f $basedir/watch_me"); > > > + > > > +# Create a new test file > > > +system("touch $basedir/watch_me"); > > > +system("chcon -t test_notify_file_t $basedir/watch_me"); > > > + > > > +## TESTS > > > + > > > +## TEST BASIC WATCH PERMISSION > > > +# Should be able to set inotify watch > > > +$exit_val = > > > + system("runcon -t test_watch_t $basedir/test_inotify $basedir/watch_me 2>&1"); > > > +ok( $exit_val, 0 ); > > > + > > > +# Should be able to set non-permissions based fanotify watch > > > +$exit_val = system( > > > + "runcon -t test_watch_t $basedir/test_fanotify $basedir/watch_me 2>&1"); > > > +ok( $exit_val, 0 ); > > > + > > > +# Should NOT be able to set permission based fanotify watch > > > +$exit_val = system( > > > + "runcon -t test_watch_t $basedir/test_fanotify -p $basedir/watch_me 2>&1"); > > > +ok($exit_val); # this should fail > > > + > > > +# Should NOT be able to set read based fanotify watch > > > +$exit_val = system( > > > + "runcon -t test_watch_t $basedir/test_fanotify -r $basedir/watch_me 2>&1"); > > > +ok($exit_val); # this should fail > > > + > > > +# Should NOT be able to set read based inotify watch > > > +$exit_val = system( > > > + "runcon -t test_watch_t $basedir/test_inotify -r $basedir/watch_me 2>&1"); > > > +ok($exit_val); # this should fail > > > + > > > +## TEST PERM WATCH > > > +# Should be able to set permission based fanotify watch > > > +$exit_val = system( > > > +"runcon -t test_perm_watch_t $basedir/test_fanotify -p $basedir/watch_me 2>&1" > > > +); > > > +ok( $exit_val, 0 ); > > > + > > > +# Should NOT be able to set watch of accesses > > > +$exit_val = system( > > > +"runcon -t test_perm_watch_t $basedir/test_fanotify -r $basedir/watch_me 2>&1" > > > +); > > > +ok($exit_val); # this should fail > > > + > > > +## TEST READ NO PERM WATCH PERMSISSIONS > > > +# Should NOT be able to set read and perm watch > > > +$exit_val = system( > > > +"runcon -t test_read_watch_t $basedir/test_fanotify -p -r $basedir/watch_me 2>&1" > > > +); > > > +ok($exit_val); # should fail > > > + > > > +# Should be able to set read inotify watch > > > +$exit_val = system( > > > +"runcon -t test_read_watch_t $basedir/test_inotify -r $basedir/watch_me 2>&1" > > > +); > > > +ok( $exit_val, 0 ); > > > + > > > +## TEST READ WITH PERM WATCH PERMSISSIONS > > > +# Should be able to set read and perm watch > > > +$exit_val = system( > > > +"runcon -t test_perm_read_watch_t $basedir/test_fanotify -p -r $basedir/watch_me 2>&1" > > > +); > > > +ok( $exit_val, 0 ); > > > + > > > +## TEST NO WATCH PERMSISSIONS > > > +# Should NOT be able to set inotify watch > > > +$exit_val = system( > > > + "runcon -t test_no_watch_t $basedir/test_inotify $basedir/watch_me 2>&1"); > > > +ok($exit_val); # this should fail > > > + > > > +# Should NOT be able to set any fanotify watch > > > +$exit_val = system( > > > + "runcon -t test_no_watch_t $basedir/test_fanotify $basedir/watch_me 2>&1"); > > > +ok($exit_val); # this should fail > > > + > > > +## TEST READ ONLY > > > +# Should NOT be able to get read-write descriptor > > > +$exit_val = system( > > > + "runcon -t test_rdonly_t $basedir/test_fanotify -l $basedir/watch_me 2>&1"); > > > +ok($exit_val); # this should fail > > > + > > > +# Should be able to get read-write descriptor > > > +$exit_val = system( > > > + "runcon -t test_watch_t $basedir/test_fanotify -l $basedir/watch_me 2>&1"); > > > +ok( $exit_val, 0 ); > > > + > > > +# Clean up test file > > > +system("rm -f $basedir/watch_me"); > > > diff --git a/tests/notify/test_fanotify.c b/tests/notify/test_fanotify.c > > > new file mode 100644 > > > index 0000000..fff773f > > > --- /dev/null > > > +++ b/tests/notify/test_fanotify.c > > > @@ -0,0 +1,105 @@ > > > +#define _GNU_SOURCE 1 > > > + > > > +#include <stdio.h> > > > +#include <stdlib.h> > > > + > > > +#include <getopt.h> > > > + > > > +#include <fcntl.h> > > > +#include <poll.h> > > > +#include <sys/fanotify.h> > > > +#include <unistd.h> > > > + > > > +void printUsage() > > > +{ > > > + fprintf(stderr, "Usage: test_fanotify [-p] [-r] [-l] file_name\n"); > > > + exit(1); > > > +} > > > + > > > +int main(int argc, char *argv[]) > > > +{ > > > + if (argc < 2) { > > > + printUsage(); > > > + } > > > + > > > + int fd, ret, arg; > > > + int mask = FAN_OPEN; // default mask > > > + int listening = 0; > > > + > > > + // the -p flag will test for watch_with_perm > > > + // the mask used at mark will contain FAN_OPEN_PERM > > > + // > > > + // the -r flag will test for watching accesses to files for reads > > > + // the mask will contain FAN_ACCESS > > > + while ((arg = getopt(argc, argv, "prl")) != -1) { > > > + switch (arg) { > > > + case 'p': > > > + mask |= FAN_OPEN_PERM; > > > + break; > > > + case 'r': > > > + mask |= FAN_ACCESS; > > > + break; > > > + case 'l': > > > + listening = 1; > > > + break; > > > + default: > > > + printUsage(); > > > + } > > > + } > > > + > > > + // get file descriptor for new fanotify event queue > > > + fd = fanotify_init(FAN_CLASS_CONTENT, O_RDWR); > > > + if (fd < 0) { > > > + perror("fanotify_init:bad file descriptor"); > > > + exit(1); > > > + } > > > + > > > + // mark a filesystem object and add mark to event queue > > > + // get notifications on file opens, accesses, and closes > > > + // use current working directory as base dir > > > + ret = fanotify_mark(fd, FAN_MARK_ADD, mask, AT_FDCWD, argv[optind]); > > > + > > > + if (ret < 0) { > > > + perror("test_fanotify:watch denied"); > > > + exit(1); > > > + } > > > + > > > + // logic to actually listen for an event if the -l flag is passed > > > + // this is used to test if an app with read-only access can get a read/write > > > + // handle to the watched file > > > + if (listening) { > > > + if (fork() == 0) { // fork a child process to cause an event on the file > > > + FILE *f; > > > + > > > + f = fopen(argv[optind], "r"); // open file for reading > > > + fgetc(f); // read char from file > > > + > > > + fclose(f); > > > + } else { // logic to watch for events and try to access file read/write > > > + struct pollfd fds; > > > + fds.fd = fd; > > > + fds.events = POLLIN; > > > + > > > + while (listening) { > > > + int polled = poll(&fds, 1, 1); > > > + if (polled > 0) { > > > + if (fds.revents & POLLIN) { > > > + struct fanotify_event_metadata buff[200]; > > > + > > > + size_t len = read(fd, (void *)&buff, sizeof(buff)); > > > + if (len == -1) { > > > + perror("test_fanotify:can't open file"); > > > + exit(1); > > > + } else { > > > + listening = 0; > > > + break; > > > + } > > > + } > > > + } else if (polled == -1) { > > > + listening = 0; > > > + } > > > + } > > > + } > > > + } > > > + exit(0); > > > +} > > > diff --git a/tests/notify/test_inotify.c b/tests/notify/test_inotify.c > > > new file mode 100644 > > > index 0000000..17c3565 > > > --- /dev/null > > > +++ b/tests/notify/test_inotify.c > > > @@ -0,0 +1,43 @@ > > > +#include <stdio.h> > > > +#include <stdlib.h> > > > +#include <sys/inotify.h> > > > +#include <getopt.h> > > > + > > > +int main(int argc, char *argv[]) > > > +{ > > > + if (argc < 2) { > > > + fprintf(stderr, "Usage: test_inotify [-r] file_name\n"); > > > + exit(1); > > > + } > > > + > > > + int fd, wd, arg; > > > + int mask = IN_MODIFY; > > > + > > > + while ((arg = getopt(argc, argv, "pr")) != -1) { > > > + switch (arg) { > > > + case 'r': > > > + mask |= IN_ACCESS; > > > + break; > > > + default: > > > + fprintf(stderr, "Usage: test_inotify [-r] file_name\n"); > > > + exit(1); > > > + } > > > + } > > > + > > > + // get new file descriptor for inotify access > > > + fd = inotify_init(); > > > + if (fd < 0) { > > > + perror("inotify_init:bad file descriptor"); > > > + exit(1); > > > + } > > > + > > > + // set watch on file and get watch descriptor for accessing events on it > > > + wd = inotify_add_watch(fd, argv[optind], mask); > > > + > > > + if (wd < 0) { > > > + perror("test_inotify:watch denied"); > > > + exit(1); > > > + } > > > + > > > + exit(0); > > > +} > > > -- > > > 2.21.0 > > > > > > -- Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02 https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02 Dominick Grift
