Hi Joshua,
thanks a lot for the patch! Generally it looks good, just a few minor
comments below...
First, please align the style with the "./tools/check-syntax" tool. It
doesn't always produce a perfect style, but at least it keeps things
consistent and avoids code style arguments :)
On Wed, Mar 11, 2020 at 10:27 PM Joshua Brindle
<joshua.brindle@xxxxxxxxxxxxxxx> wrote:
> selinux-testsuite adds test for default_range glblub
> which can only be inserted into the policy via cil, so add
> CIL_TARGETS to policy and an attempt to detect whether the policy is MCS,
> meaning it only has 1 sensitivity and more need to be added, or MLS
> and therefore fine to test on.
>
> Signed-off-by: Joshua Brindle <joshua.brindle@xxxxxxxxxxxxxxx>
> ---
> policy/Makefile | 15 +++++++--
> policy/test_add_levels.cil | 34 +++++++++++++++++++
> policy/test_glblub.cil | 4 +++
> tests/Makefile | 19 +++++++++--
> tests/glblub/.gitignore | 1 +
> tests/glblub/Makefile | 7 ++++
> tests/glblub/default_range.c | 65 ++++++++++++++++++++++++++++++++++++
> tests/glblub/test | 55 ++++++++++++++++++++++++++++++
> tests/pol_detect | 17 ++++++++++
> 9 files changed, 213 insertions(+), 4 deletions(-)
> create mode 100644 policy/test_add_levels.cil
> create mode 100644 policy/test_glblub.cil
> create mode 100644 tests/glblub/.gitignore
> create mode 100644 tests/glblub/Makefile
> create mode 100644 tests/glblub/default_range.c
> create mode 100755 tests/glblub/test
> create mode 100755 tests/pol_detect
>
[...]
diff --git a/policy/test_add_levels.cil b/policy/test_add_levels.cil
new file mode 100644
index 0000000..09215f4
--- /dev/null
+++ b/policy/test_add_levels.cil
@@ -0,0 +1,34 @@
+(sensitivity s1)
+(sensitivitycategory s1 (range c0 c1023))
+(sensitivity s2)
+(sensitivitycategory s2 (range c0 c1023))
+(sensitivity s3)
+(sensitivitycategory s3 (range c0 c1023))
+(sensitivity s4)
+(sensitivitycategory s4 (range c0 c1023))
+(sensitivity s5)
+(sensitivitycategory s5 (range c0 c1023))
+(sensitivity s6)
+(sensitivitycategory s6 (range c0 c1023))
+(sensitivity s7)
+(sensitivitycategory s7 (range c0 c1023))
+(sensitivity s8)
+(sensitivitycategory s8 (range c0 c1023))
+(sensitivity s9)
+(sensitivitycategory s9 (range c0 c1023))
+(sensitivity s10)
+(sensitivitycategory s10 (range c0 c1023))
+(sensitivity s11)
+(sensitivitycategory s11 (range c0 c1023))
+(sensitivity s12)
+(sensitivitycategory s12 (range c0 c1023))
+(sensitivity s13)
+(sensitivitycategory s13 (range c0 c1023))
+(sensitivity s14)
+(sensitivitycategory s14 (range c0 c1023))
+(sensitivity s15)
+(sensitivitycategory s15 (range c0 c1023))
+(sensitivityorder (s0 s1 s2 s3 s4 s5 s6 s7 s8 s9 s10 s11 s12 s13 s14 s15 ))
This is not important, but you could also consolidate the whitespace
in these two files.
In the above line, I would drop the space from "s15 ))".
+
+(selinuxuser system_u system_u ((s0) (s15((range c0 c1023) ))))
Here, I'd add a space after "s15" and remove the space in the middle
of the closing parentheses.
+(userrange system_u ((s0 ) (s15 (range c0 c1023))))
Here there's an extra space after "s0".
> diff --git a/policy/test_glblub.cil b/policy/test_glblub.cil
> new file mode 100644
> index 0000000..6cd7c95
> --- /dev/null
> +++ b/policy/test_glblub.cil
> @@ -0,0 +1,4 @@
> +(defaultrange db_table glblub)
> +
> +(class new_class_no_default_range (foobar) )
Another extra space here.
> +(classorder (unordered new_class_no_default_range))
> diff --git a/tests/Makefile b/tests/Makefile
> index 46a1641..7dd5ade 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -1,10 +1,21 @@
> -INCLUDEDIR ?= /usr/include
> -POLDEV ?= /usr/share/selinux/devel
> +PREFIX ?= /usr
> +BINDIR ?= $(PREFIX)/bin
> +SBINDIR ?= $(PREFIX)/sbin
> +POLDEV ?= $(PREFIX)/share/selinux/devel
> +INCLUDEDIR ?= ($PREFIX)/include
"(" and "$" are swapped here.
> SELINUXFS ?= /sys/fs/selinux
>
> export CFLAGS+=-g -O0 -Wall -D_GNU_SOURCE
>
> DISTRO=$(shell ./os_detect)
> +SELINUXFS := $(shell cat /proc/mounts | grep selinuxfs | cut -f 2 -d ' ')
> +CHECKPOLICY = $(BINDIR)/checkpolicy
> +CHECKMODULE = $(BINDIR)/checkmodule
> +
> +POL_VERS := $(shell $(CHECKPOLICY) -V |cut -f 1 -d ' ')
> +MOD_POL_VERS := $(shell $(CHECKMODULE) -V |cut -f 2 -d '-')
> +MAX_KERNEL_POLICY := $(shell cat $(SELINUXFS)/policyvers)
> +POL_TYPE := $(shell ./pol_detect $(SELINUXFS))
>
> SUBDIRS:= domain_trans entrypoint execshare exectrace execute_no_trans \
> fdreceive inherit link mkdir msg open ptrace readlink relabel rename \
[...]
> diff --git a/tests/glblub/test b/tests/glblub/test
> new file mode 100755
> index 0000000..090a6d2
> --- /dev/null
> +++ b/tests/glblub/test
> @@ -0,0 +1,55 @@
> +#!/usr/bin/perl
> +
> +use Test;
> +BEGIN { plan tests => 9 }
> +
> +$basedir = $0;
> +$basedir =~ s|(.*)/[^/]*|$1|;
> +
> +# Verify glblub range_transition behavior
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s0:c0.c100-s10:c0.c150 \\
> + system_u:system_r:kernel_t:s5:c50.c100-s15:c0.c149 db_table \\
> + system_u:object_r:kernel_t:s5:c50.c100-s10:c0.c149");
I think these calls would be a bit nicer to read if you made a common
function for them that only takes the MLS ranges and class name.
> +ok($result, 0);
> +
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s5:c50.c100-s15:c0.c149 \\
> + system_u:system_r:kernel_t:s0:c0.c100-s10:c0.c150 db_table \\
> + system_u:object_r:kernel_t:s5:c50.c100-s10:c0.c149");
> +ok($result, 0);
> +
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s0:c0.c100-s10:c0.c150 \\
> + system_u:system_r:kernel_t:s0 db_table \\
> + system_u:object_r:kernel_t:s0");
> +ok($result, 0);
> +
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s5:c512.c550,c552.c1023-s5:c0.c550,c552.c1023 \\
> + system_u:system_r:kernel_t:s5:c512.c550,c553.c1023-s5:c0,c1,c4,c5,c6,c512.c550,c553.c1023 db_table \\
> + system_u:object_r:kernel_t:s5:c512.c550,c553.c1023-s5:c0,c1,c4.c6,c512.c550,c553.c1023");
> +ok($result, 0);
> +
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s5:c50.c100-s15:c0.c149 \\
> + system_u:system_r:kernel_t:s5:c512.c550,c552.c1023-s5:c0.c550,c552.c1023 db_table \\
> + system_u:object_r:kernel_t:s5-s5:c0.c149");
> +ok($result, 0);
> +
> +# Verify incompatible contexts
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s5:c50.c100-s15:c0.c149 \\
> + system_u:system_r:kernel_t:s4-s4:c0.c1023 db_table");
> +ok ($result >> 8, 3);
> +
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s4-s4:c0.c1023 \\
> + system_u:system_r:kernel_t:s5:c50.c100-s15:c0.c149 db_table");
> +ok ($result >> 8, 3);
> +
> +# Verify default (source-low) behavior
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s0:c0.c100-s10:c0.c150 \\
> + system_u:system_r:kernel_t:s5:c50.c100-s15:c0.c149 new_class_no_default_range \\
> + system_u:object_r:kernel_t:s0:c0.c100");
> +ok($result, 0);
> +
> +$result = system("$basedir/default_range system_u:system_r:kernel_t:s5:c50.c100-s15:c0.c149 \\
> + system_u:system_r:kernel_t:s0:c0.c100-s10:c0.c150 new_class_no_default_range \\
> + system_u:object_r:kernel_t:s5:c50.c100");
> +ok($result, 0);
> +
> +
git am complains about these extra empty lines at the end of file -
please also fix that.
> diff --git a/tests/pol_detect b/tests/pol_detect
> new file mode 100755
> index 0000000..ba25b94
> --- /dev/null
> +++ b/tests/pol_detect
> @@ -0,0 +1,17 @@
> +#!/bin/bash
> +
> +if [ $# -ne 1 ]; then
> + echo "Usage $0 <selinuxfs directory>"
> + exit 1
> +fi
> +
> +# This is heuristic but seems unlikely to be wrong,
> +# the kernel initial sid should always be SystemHigh
> +# and SystemHigh is normally s15
> +level=$(cat $1/initial_contexts/kernel | tr '\0' '\n' | cut -d: -f4)
> +
> +if [ -z $level ]; then echo "NON-MLS"
> +elif [ $level = 's0' ]; then echo "MCS"
> +elif [ $level = 's15' ]; then echo "MLS"
> +else echo "UNKNOWN"
> +fi
> --
> 2.21.1
>
Thanks,
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
