diff mbox series

[RFC] selinux-testsuite: Add check for key changes on watch_queue

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

Commit Message

Richard Haines April 17, 2020, 4:38 p.m. UTC
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

Comments

Ondrej Mosnacek May 14, 2020, 8:50 a.m. UTC | #1
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)
[...]
Richard Haines May 14, 2020, 3 p.m. UTC | #2
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 mbox series

Patch

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;
+}