A couple comments below...
On Fri, Nov 15, 2019 at 12:44 PM Richard Haines
<richard_c_haines@xxxxxxxxxxxxxx> wrote:
> Test kernel module loading permissions.
>
> Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> ---
> V2 Change:
> Check permission denial module_load versus module_request by using a
> test kernel module for each.
> Note: Rawhide (with secnext kernel) adds built-in.a and built-in.a.cmd when
> building modules, therefore added to Makefile and .gitignore.
>
> policy/Makefile | 4 +
> policy/test_module_load.te | 118 +++++++++++++++++++++
> tests/Makefile | 4 +
> tests/module_load/.gitignore | 11 ++
> tests/module_load/Makefile | 12 +++
> tests/module_load/finit_load.c | 94 +++++++++++++++++
> tests/module_load/init_load.c | 121 ++++++++++++++++++++++
> tests/module_load/setest_module_load.c | 18 ++++
> tests/module_load/setest_module_request.c | 22 ++++
> tests/module_load/test | 62 +++++++++++
> 10 files changed, 466 insertions(+)
> create mode 100644 policy/test_module_load.te
> create mode 100644 tests/module_load/.gitignore
> create mode 100644 tests/module_load/Makefile
> create mode 100644 tests/module_load/finit_load.c
> create mode 100644 tests/module_load/init_load.c
> create mode 100644 tests/module_load/setest_module_load.c
> create mode 100644 tests/module_load/setest_module_request.c
> create mode 100755 tests/module_load/test
>
> diff --git a/policy/Makefile b/policy/Makefile
> index ff65153..545f3b5 100644
> --- a/policy/Makefile
> +++ b/policy/Makefile
> @@ -90,6 +90,10 @@ ifeq ($(shell grep -q all_file_perms.*watch $(POLDEV)/include/support/all_perms.
> TARGETS+=test_notify.te
> endif
>
> +ifeq ($(shell grep -q module_load $(POLDEV)/include/support/all_perms.spt && echo true),true)
> +TARGETS+=test_module_load.te
> +endif
> +
> ifeq (x$(DISTRO),$(filter x$(DISTRO),xRHEL4 xRHEL5 xRHEL6))
> TARGETS:=$(filter-out test_overlayfs.te test_mqueue.te test_ibpkey.te, $(TARGETS))
> endif
> diff --git a/policy/test_module_load.te b/policy/test_module_load.te
> new file mode 100644
> index 0000000..566ddf7
> --- /dev/null
> +++ b/policy/test_module_load.te
> @@ -0,0 +1,118 @@
> +#
> +############################## Define Macro ################################
> +#
> +# Replace domain_type() macro as it hides some relevant denials in audit.log
> +#
> +gen_require(`
> + type setrans_var_run_t, syslogd_t;
> +')
> +
> +define(`module_domain_type',`
> + allow $1 proc_t:dir { search };
> + allow $1 proc_t:lnk_file { read };
> + allow $1 self:dir { search };
> + allow $1 self:file { open read write };
> + dontaudit init_t syslogd_t:fd use;
> + dontaudit $1 security_t:filesystem getattr;
> + dontaudit $1 self:file getattr;
> + dontaudit $1 setrans_var_run_t:dir search;
> + dontaudit unconfined_t $1:process { noatsecure rlimitinh siginh };
> +')
> +
> +#
> +############# Test kernel modules with finitmod_module(2) ###################
> +#
> +attribute finitmoddomain;
> +
> +type test_finitmod_t;
> +module_domain_type(test_finitmod_t)
> +unconfined_runs_test(test_finitmod_t)
> +typeattribute test_finitmod_t testdomain;
> +typeattribute test_finitmod_t finitmoddomain;
> +
> +allow test_finitmod_t self:capability { sys_module };
> +allow test_finitmod_t test_file_t:system { module_load };
> +allow test_finitmod_t kernel_t:system { module_request };
> +
> +############### Deny cap sys_module ######################
> +type test_finitmod_deny_sys_module_t;
> +module_domain_type(test_finitmod_deny_sys_module_t)
> +unconfined_runs_test(test_finitmod_deny_sys_module_t)
> +typeattribute test_finitmod_deny_sys_module_t testdomain;
> +typeattribute test_finitmod_deny_sys_module_t finitmoddomain;
> +
> +neverallow test_finitmod_deny_sys_module_t self:capability { sys_module };
> +
> +############### Deny sys module_load ######################
> +type test_finitmod_deny_module_load_t;
> +module_domain_type(test_finitmod_deny_module_load_t)
> +unconfined_runs_test(test_finitmod_deny_module_load_t)
> +typeattribute test_finitmod_deny_module_load_t testdomain;
> +typeattribute test_finitmod_deny_module_load_t finitmoddomain;
> +
> +allow test_finitmod_deny_module_load_t self:capability { sys_module };
> +neverallow test_finitmod_deny_module_load_t test_file_t:system { module_load };
> +
> +############### Deny sys module_request ######################
> +type test_finitmod_deny_module_request_t;
> +module_domain_type(test_finitmod_deny_module_request_t)
> +unconfined_runs_test(test_finitmod_deny_module_request_t)
> +typeattribute test_finitmod_deny_module_request_t testdomain;
> +typeattribute test_finitmod_deny_module_request_t finitmoddomain;
> +
> +allow test_finitmod_deny_module_request_t self:capability { sys_module };
> +allow test_finitmod_deny_module_request_t test_file_t:system { module_load };
> +neverallow test_finitmod_deny_module_request_t kernel_t:system { module_request };
> +
> +#
> +############# Test kernel modules with initmod_module(2) ###################
> +#
> +attribute initmoddomain;
> +
> +type test_initmod_t;
> +module_domain_type(test_initmod_t)
> +unconfined_runs_test(test_initmod_t)
> +typeattribute test_initmod_t testdomain;
> +typeattribute test_initmod_t initmoddomain;
> +
> +allow test_initmod_t self:capability { sys_module };
> +allow test_initmod_t self:system { module_load };
> +allow test_initmod_t kernel_t:system { module_request };
> +
> +############### Deny cap sys_module ######################
> +type test_initmod_deny_sys_module_t;
> +module_domain_type(test_initmod_deny_sys_module_t)
> +unconfined_runs_test(test_initmod_deny_sys_module_t)
> +typeattribute test_initmod_deny_sys_module_t testdomain;
> +typeattribute test_initmod_deny_sys_module_t initmoddomain;
> +
> +neverallow test_initmod_deny_sys_module_t self:capability { sys_module };
> +
> +############### Deny sys module_load ######################
> +type test_initmod_deny_module_load_t;
> +module_domain_type(test_initmod_deny_module_load_t)
> +unconfined_runs_test(test_initmod_deny_module_load_t)
> +typeattribute test_initmod_deny_module_load_t testdomain;
> +typeattribute test_initmod_deny_module_load_t initmoddomain;
> +
> +allow test_initmod_deny_module_load_t self:capability { sys_module };
> +neverallow test_initmod_deny_module_load_t self:system { module_load };
> +
> +############### Deny sys module_request ######################
> +type test_initmod_deny_module_request_t;
> +module_domain_type(test_initmod_deny_module_request_t)
> +unconfined_runs_test(test_initmod_deny_module_request_t)
> +typeattribute test_initmod_deny_module_request_t testdomain;
> +typeattribute test_initmod_deny_module_request_t initmoddomain;
> +
> +allow test_initmod_deny_module_request_t self:capability { sys_module };
> +allow test_initmod_deny_module_request_t self:system { module_load };
> +neverallow test_initmod_deny_module_request_t kernel_t:system { module_request };
> +
> +#
> +########### Allow these domains to be entered from sysadm domain ############
> +#
> +miscfiles_domain_entry_test_files(finitmoddomain)
> +userdom_sysadm_entry_spec_domtrans_to(finitmoddomain)
> +miscfiles_domain_entry_test_files(initmoddomain)
> +userdom_sysadm_entry_spec_domtrans_to(initmoddomain)
It seems that the finitmoddomain and initmoddomain type sets are
exactly the same except for names - can they be merged into just one
set of types? The AVC denials should be still easily distinguishable
by the comm= field if that's the intended purpose of the separation.
[...]
> diff --git a/tests/module_load/Makefile b/tests/module_load/Makefile
> new file mode 100644
> index 0000000..c561685
> --- /dev/null
> +++ b/tests/module_load/Makefile
> @@ -0,0 +1,12 @@
> +obj-m = setest_module_load.o setest_module_request.o
> +
> +TARGETS = finit_load init_load
> +LDLIBS += -lselinux
> +KDIR = /usr/src/kernels/$(shell uname -r)
I think you should rather use /lib/modules/$(shell uname -r)/build
here, which seems to be more portable (it works at least on Fedora and
Ubuntu, while /usr/src/kernels/... doesn't work at least on Ubuntu).
> +
> +all: $(TARGETS)
> + $(MAKE) -C $(KDIR) M=$(PWD)
> +
> +clean:
> + rm -f $(TARGETS)
> + rm -f *.a *.o *.ko *.cmd *.mod *.mod.c .*.cmd Module.symvers modules.order
> diff --git a/tests/module_load/finit_load.c b/tests/module_load/finit_load.c
> new file mode 100644
> index 0000000..029c698
> --- /dev/null
> +++ b/tests/module_load/finit_load.c
> @@ -0,0 +1,94 @@
> +#define _GNU_SOURCE 1
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <sys/syscall.h>
> +#include <selinux/selinux.h>
> +
> +static void print_usage(char *progfile_name)
> +{
> + fprintf(stderr,
> + "usage: %s [-v] path name\n"
> + "Where:\n\t"
> + "-v Print information.\n\t"
> + "path Kernel module build path.\n\t"
> + "name Name of kernel module to load.\n", progfile_name);
> + exit(-1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + char *context, file_name[PATH_MAX];
> + int opt, result, fd;
> + bool verbose = false;
> +
> + while ((opt = getopt(argc, argv, "v")) != -1) {
> + switch (opt) {
> + case 'v':
> + verbose = true;
> + break;
> + default:
> + print_usage(argv[0]);
> + }
> + }
> +
> + if (optind >= argc)
> + print_usage(argv[0]);
> +
> + result = sprintf(file_name, "%s/%s.ko", argv[optind],
> + argv[optind + 1]);
> + if (result < 0) {
> + fprintf(stderr, "Failed sprintf\n");
> + exit(-1);
> + }
> +
> + fd = open(file_name, O_RDONLY);
> + if (!fd) {
> + fprintf(stderr, "Failed to open %s: %s\n",
> + file_name, strerror(errno));
> + exit(-1);
> + }
> +
> + result = getcon(&context);
> + if (result < 0) {
> + fprintf(stderr, "Failed to obtain process context\n");
> + close(fd);
> + exit(-1);
> + }
> +
> + if (verbose)
> + printf("Process context:\n\t%s\n", context);
> +
> + free(context);
Why not wrap also the getcon() & free() calls under the 'if (verbose)
{ ... }'? The context is not used in the non-verbose case.
> +
> + result = syscall(__NR_finit_module, fd, "", 0);
> + if (result < 0) {
> + fprintf(stderr, "Failed to load '%s' module: %s\n",
> + file_name, strerror(errno));
> + close(fd);
> + /* Denying: sys_module=EPERM, module_load=EACCES */
> + exit(errno);
> + }
> + close(fd);
> +
> + if (verbose)
> + printf("Loaded kernel module: %s\n", file_name);
> +
> + result = syscall(__NR_delete_module, argv[optind + 1], 0);
> + if (result < 0) {
> + fprintf(stderr, "Failed to delete '%s' module: %s\n",
> + argv[optind + 1], strerror(errno));
> + exit(-1);
> + }
> +
> + if (verbose)
> + printf("Deleted kernel module: %s\n", argv[optind + 1]);
> +
> + return 0;
> +}
> diff --git a/tests/module_load/init_load.c b/tests/module_load/init_load.c
> new file mode 100644
> index 0000000..5f9997b
> --- /dev/null
> +++ b/tests/module_load/init_load.c
> @@ -0,0 +1,121 @@
> +#define _GNU_SOURCE 1
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <sys/stat.h>
> +#include <sys/syscall.h>
> +#include <selinux/selinux.h>
> +
> +static void print_usage(char *progfile_name)
> +{
> + fprintf(stderr,
> + "usage: %s [-v] path name\n"
> + "Where:\n\t"
> + "-v Print information.\n\t"
> + "path Kernel module build path.\n\t"
> + "name Name of kernel module to load.\n", progfile_name);
> + exit(-1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + char *context, file_name[PATH_MAX];
> + int opt, result, fd;
> + bool verbose = false;
> + void *elf_image;
> + struct stat st;
> +
> + while ((opt = getopt(argc, argv, "v")) != -1) {
> + switch (opt) {
> + case 'v':
> + verbose = true;
> + break;
> + default:
> + print_usage(argv[0]);
> + }
> + }
> +
> + if (optind >= argc)
> + print_usage(argv[0]);
> +
> + result = sprintf(file_name, "%s/%s.ko", argv[optind],
> + argv[optind + 1]);
> + if (result < 0) {
> + fprintf(stderr, "Failed sprintf\n");
> + exit(-1);
> + }
> +
> + fd = open(file_name, O_RDONLY);
> + if (!fd) {
> + fprintf(stderr, "Failed to open %s: %s\n",
> + file_name, strerror(errno));
> + exit(-1);
> + }
> +
> + result = getcon(&context);
> + if (result < 0) {
> + fprintf(stderr, "Failed to obtain process context\n");
> + close(fd);
> + exit(-1);
> + }
> +
> + if (verbose)
> + printf("Process context:\n\t%s\n", context);
> +
> + free(context);
Ditto here.
> +
> + result = fstat(fd, &st);
> + if (result < 0) {
> + fprintf(stderr, "Failed fstat on %s: %s\n",
> + file_name, strerror(errno));
> + close(fd);
> + exit(-1);
> + }
> +
> + elf_image = malloc(st.st_size);
> + if (!elf_image) {
> + fprintf(stderr, "Failed malloc on %s: %s\n",
> + file_name, strerror(errno));
> + close(fd);
> + exit(-1);
> + }
> +
> + result = read(fd, elf_image, st.st_size);
> + if (result < 0) {
> + fprintf(stderr, "Failed read on %s: %s\n",
> + file_name, strerror(errno));
> + close(fd);
You should free 'elf_image' here.
> + exit(-1);
> + }
> + close(fd);
> +
> + result = syscall(__NR_init_module, elf_image, st.st_size, "");
> + if (result < 0) {
> + fprintf(stderr, "Failed to load '%s' module: %s\n",
> + file_name, strerror(errno));
...and here as well. (In this case you can just move the
'free(elf_image);' below to before the syscall return value check.)1
> + /* Denying: sys_module=EPERM, module_load & request=EACCES */
> + exit(errno);
> + }
> + free(elf_image);
> +
> + if (verbose)
> + printf("Loaded kernel module: %s\n", file_name);
> +
> + result = syscall(__NR_delete_module, argv[optind + 1], 0);
> + if (result < 0) {
> + fprintf(stderr, "Failed to delete '%s' module: %s\n",
> + argv[optind + 1], strerror(errno));
> + exit(-1);
> + }
> +
> + if (verbose)
> + printf("Deleted kernel module: %s\n", argv[optind + 1]);
> +
> + return 0;
> +}
[...]
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
