diff mbox series

[v3,2/6] shell: Add tst_security.sh helper

Message ID 20181218010220.2446-3-pvorel@suse.cz
State Accepted
Headers show
Series DHCP tests and AppArmor/SELinux improvements | expand

Commit Message

Petr Vorel Dec. 18, 2018, 1:02 a.m. UTC 
It prints info about AppArmor and SELinux and allows to disable it.
This is due some false positives because improper usage or bugs
in AppArmor profiles (e.g. traceroute, dnsmasq).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
NOTE: some of functions in tst_security.sh are meant to be used
also in tests (when AppArmor and SELinux has different paths).
---
 testcases/lib/tst_security.sh | 124 ++++++++++++++++++++++++++++++++++
 testcases/lib/tst_test.sh     |  17 +++--
 2 files changed, 136 insertions(+), 5 deletions(-)
 create mode 100644 testcases/lib/tst_security.sh

Comments

Alexey Kodanev Dec. 19, 2018, 12:38 p.m. UTC | #1 
Hi Petr,
On 12/18/2018 04:02 AM, Petr Vorel wrote:
> It prints info about AppArmor and SELinux and allows to disable it.
> This is due some false positives because improper usage or bugs
> in AppArmor profiles (e.g. traceroute, dnsmasq).
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> NOTE: some of functions in tst_security.sh are meant to be used
> also in tests (when AppArmor and SELinux has different paths).
> ---
>  testcases/lib/tst_security.sh | 124 ++++++++++++++++++++++++++++++++++
>  testcases/lib/tst_test.sh     |  17 +++--
>  2 files changed, 136 insertions(+), 5 deletions(-)
>  create mode 100644 testcases/lib/tst_security.sh
> 
> diff --git a/testcases/lib/tst_security.sh b/testcases/lib/tst_security.sh
> new file mode 100644
> index 000000000..68b47347f
> --- /dev/null
> +++ b/testcases/lib/tst_security.sh
> @@ -0,0 +1,124 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2018 Petr Vorel <pvorel@suse.cz>
> +
> +if [ -z "$TST_LIB_LOADED" ]; then
> +	echo "please load tst_test.sh first" >&2
> +	exit 1
> +fi
> +
> +[ -n "$TST_SECURITY_LOADED" ] && return 0
> +TST_SECURITY_LOADED=1
> +
> +_tst_check_security_modules()
> +{
> +	local cmd
> +	local profiles
> +
> +	if tst_apparmor_enabled; then
> +		tst_res TINFO "AppArmor enabled, this may affect test results. Disable it with TST_DISABLE_APPARMOR=1 (requires super/root)"
> +		profiles=
> +		for cmd in $TST_NEEDS_CMDS; do
> +			tst_apparmor_used_profile $cmd && profiles="$cmd $profiles"
> +		done
> +		[ -z "$profiles" ] && profiles="none"
> +		tst_res TINFO "loaded AppArmor profiles: $profiles"
> +	fi
> +
> +	if tst_selinux_enabled; then
> +		tst_res TINFO "SELinux enabled in enforcing mode, this may affect test results. Disable it with TST_DISABLE_SELINUX=1 (requires super/root)"
> +		profiles=
> +		for cmd in $TST_NEEDS_CMDS; do
> +			tst_selinux_used_profile $cmd && profiles="$cmd $profiles"
> +		done
> +		[ -z "$profiles" ] && profiles="none"
> +		tst_res TINFO "loaded SELinux profiles: $profiles"
> +	fi
> +}
> +
> +# Detect whether AppArmor profiles are loaded
> +# Return 0: profiles loaded, 1: none profile loaded or AppArmor disabled
> +tst_apparmor_enabled()
> +{
> +	local f="/sys/module/apparmor/parameters/enabled"
> +	[ -f "$f" ] && [ "$(cat $f)" = "Y" ]
> +}
> +
> +# Detect whether AppArmor profile for command is enforced
> +# tst_apparmor_used_profile CMD
> +# Return 0: loaded profile for CMD
> +# Return 1: no profile CMD
> +tst_apparmor_used_profile()
> +{
> +	[ $# -eq 1 ] && tst_brk TCONF "usage tst_apparmor_used_profile CMD"
              ^
Should be "-ne", because you expect exactly one argument or change the return check to ||.

> +	local cmd="$1"
> +	grep -q "$cmd .*(enforce)" /sys/kernel/security/apparmor/profiles 2>/dev/null
> +}
> +
> +# Detect whether SELinux is enabled in enforcing mode
> +# Return 0: enabled in enforcing mode
> +# Return 1: enabled in permissive mode or disabled
> +tst_selinux_enabled()
> +{
> +	local f="$(_tst_get_enforce)"
> +
> +	[ -f "$f" ] && [ "$(cat $f)" = "1" ]
> +}
> +
> +# Detect whether SELinux profile for command is enforced
> +# tst_selinux_used_profile CMD
> +# Return 0: loaded profile for CMD
> +# Return 1: profile for CMD not loaded or seinfo not available
> +tst_selinux_used_profile()
> +{
> +	[ $# -eq 1 ] && tst_brk TCONF "usage tst_selinux_used_profile CMD"

              ^
     The same is here.


> +	local cmd="$1"
> +
> +	if ! tst_cmd_available seinfo; then
> +		if [ -z "$seinfo_warn_printed" ]; then
> +			tst_res "install seinfo to find used SELinux profiles"
                               ^
                   Please add TINFO.


> +			export seinfo_warn_printed=1
> +		fi
> +		return
                       ^
Missed "return 1" here as you're checking the return value in
_tst_check_security_modules().

> +	fi
> +	seinfo -t 2>/dev/null | grep -q $cmd
> +}
> +
> +# Try disable AppArmor
> +# Return 0: AppArmor disabled
> +# Return > 0: failed to disable AppArmor
> +tst_disable_apparmor()
> +{
> +	_tst_require_root
> +	local f="aa-teardown"
> +	local action
> +
> +	tst_cmd_available $f && { $f; return; }
> +	f="/etc/init.d/apparmor"
> +	if [ -f "$f" ]; then
> +		for action in teardown kill stop; do
> +			$f $action >/dev/null 2>&1 && return
> +		done
> +	fi
> +}
> +
> +# Try disable SELinux
> +# Return 0: SELinux disabled
> +# Return > 0: failed to disable SELinux
> +tst_disable_selinux()
> +{
> +	_tst_require_root
> +	local f="$(_tst_get_enforce)"
> +
> +	[ -f "$f" ] && cat 0 > $f
> +}
> +
> +# Get SELinux enforce file path
> +_tst_get_enforce()
> +{
> +	local dir="/sys/fs/selinux"
> +
> +	[ -d "$dir" ] || dir="/selinux"
> +	local f="$dir/enforce"
> +	[ -f "$f" ] && echo "$f"
> +}
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index e3770d005..333061028 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -34,6 +34,7 @@ export TST_TMPDIR_RHOST=0
>  export TST_LIB_LOADED=1
>  
>  . tst_ansi_color.sh
> +. tst_security.sh
>  
>  # default trap function
>  trap "tst_brk TBROK 'test interrupted'" INT
> @@ -67,6 +68,7 @@ _tst_do_exit()
>  
>  	if [ $TST_FAIL -gt 0 ]; then
>  		ret=$((ret|1))
> +		_tst_check_security_modules
>  	fi
>  
>  	if [ $TST_BROK -gt 0 ]; then
> @@ -376,6 +378,11 @@ _tst_setup_timer()
>  	_tst_setup_timer_pid=$!
>  }
>  
> +_tst_require_root()
> +{
> +	[ "$(id -ru)" != 0 ] && tst_brk TCONF "Must be super/root for this test!"
> +}
> +
>  tst_run()
>  {
>  	local _tst_i
> @@ -386,6 +393,7 @@ tst_run()
>  	if [ -n "$TST_TEST_PATH" ]; then
>  		for _tst_i in $(grep TST_ "$TST_TEST_PATH" | sed 's/.*TST_//; s/[="} \t\/:`].*//'); do
>  			case "$_tst_i" in
> +			DISABLE_APPARMOR|DISABLE_SELINUX);;
>  			SETUP|CLEANUP|TESTFUNC|ID|CNT|MIN_KVER);;
>  			OPTS|USAGE|PARSE_ARGS|POS_ARGS);;
>  			NEEDS_ROOT|NEEDS_TMPDIR|TMPDIR|NEEDS_DEVICE|DEVICE);;
> @@ -421,11 +429,10 @@ tst_run()
>  		tst_brk TBROK "Number of iterations (-i) must be > 0"
>  	fi
>  
> -	if [ "$TST_NEEDS_ROOT" = 1 ]; then
> -		if [ "$(id -ru)" != 0 ]; then
> -			tst_brk TCONF "Must be super/root for this test!"
> -		fi
> -	fi
> +	[ "$TST_NEEDS_ROOT" = 1 ] && _tst_require_root
> +
> +	[ "$TST_DISABLE_APPARMOR" = 1 ] && tst_disable_apparmor
> +	[ "$TST_DISABLE_SELINUX" = 1 ] && tst_disable_selinux
>  
>  	tst_test_cmds $TST_NEEDS_CMDS
>  	tst_test_drivers $TST_NEEDS_DRIVERS
> 

The rest in the patch-set looks fine to me.

Thanks,
Alexey
Petr Vorel Dec. 19, 2018, 1:33 p.m. UTC | #2 
Hi Alexey,

> The rest in the patch-set looks fine to me.
thank you for review and pointing out errors, I'll add your ack.
Waiting for Cyril to add his ack for this commit.

> Thanks,
> Alexey


Kind regards,
Petr
Cyril Hrubis Jan. 16, 2019, 1:12 p.m. UTC | #3 
Hi!
> It prints info about AppArmor and SELinux and allows to disable it.
> This is due some false positives because improper usage or bugs
> in AppArmor profiles (e.g. traceroute, dnsmasq).

Looks good, the only piece I'm not 100% sure about is if this should be
sourced by default in tst_test.sh. Shouldn't we just include this in
network tests?
Petr Vorel Jan. 16, 2019, 3:20 p.m. UTC | #4 
Hi Cyril,

> > It prints info about AppArmor and SELinux and allows to disable it.
> > This is due some false positives because improper usage or bugs
> > in AppArmor profiles (e.g. traceroute, dnsmasq).

> Looks good, the only piece I'm not 100% sure about is if this should be
> sourced by default in tst_test.sh. Shouldn't we just include this in
> network tests?
Understand, most of user space related tests are network tests.

I decided to add it for all tests as AppArmor default [1] and non-default [2]
contains various non-networking tools (syslog-ng, syslogd, klogd, cron, passwd,
useradd, userdel); SELinux has some kernel [3] and system [4] related modules.

Kind regards,
Petr

[1] https://gitlab.com/apparmor/apparmor/tree/master/profiles/apparmor.d
[2] https://gitlab.com/apparmor/apparmor/tree/master/profiles/apparmor/profiles/extras
[3] https://github.com/SELinuxProject/refpolicy/tree/master/policy/modules/kernel
[4] https://github.com/SELinuxProject/refpolicy/tree/master/policy/modules/system
Cyril Hrubis Jan. 23, 2019, 2:08 p.m. UTC | #5 
Hi!
> > > It prints info about AppArmor and SELinux and allows to disable it.
> > > This is due some false positives because improper usage or bugs
> > > in AppArmor profiles (e.g. traceroute, dnsmasq).
> 
> > Looks good, the only piece I'm not 100% sure about is if this should be
> > sourced by default in tst_test.sh. Shouldn't we just include this in
> > network tests?
> Understand, most of user space related tests are network tests.
> 
> I decided to add it for all tests as AppArmor default [1] and non-default [2]
> contains various non-networking tools (syslog-ng, syslogd, klogd, cron, passwd,
> useradd, userdel); SELinux has some kernel [3] and system [4] related modules.

Okay then. I checked the messages generated in a case of a test failure
and hopefully these cannot be misinterpreted as they say "may affect".
Maybe it would be a bit better if the second sentence would be "You can
try to disable it with ..."
Petr Vorel Jan. 23, 2019, 4:20 p.m. UTC | #6 
Hi,

I decided to print warning on all TBROK/TCONF/TFAIL/TWARN (originally it was only on TFAIL).
This is needed for dnsmasq tests, which TBROK due AppArmor profile.

And split long messages into 2 (besides obvious fixes reported by Alexey).

Kind regards,
Petr

Diff of this commit with posted version:

diff --git testcases/lib/tst_security.sh testcases/lib/tst_security.sh
index 2c8c30f42..25e085d3c 100644
--- testcases/lib/tst_security.sh
+++ testcases/lib/tst_security.sh
@@ -16,7 +16,8 @@ _tst_check_security_modules()
 	local profiles
 
 	if tst_apparmor_enabled; then
-		tst_res TINFO "AppArmor enabled, this may affect test results. Disable it with TST_DISABLE_APPARMOR=1 (requires super/root)"
+		tst_res TINFO "AppArmor enabled, this may affect test results"
+		tst_res TINFO "You can try to disable it with TST_DISABLE_APPARMOR=1 (requires super/root)"
 		profiles=
 		for cmd in $TST_NEEDS_CMDS; do
 			tst_apparmor_used_profile $cmd && profiles="$cmd $profiles"
@@ -26,7 +27,8 @@ _tst_check_security_modules()
 	fi
 
 	if tst_selinux_enabled; then
-		tst_res TINFO "SELinux enabled in enforcing mode, this may affect test results. Disable it with TST_DISABLE_SELINUX=1 (requires super/root)"
+		tst_res TINFO "SELinux enabled in enforcing mode, this may affect test results"
+		tst_res TINFO "You can try to disable it with TST_DISABLE_SELINUX=1 (requires super/root)"
 		profiles=
 		for cmd in $TST_NEEDS_CMDS; do
 			tst_selinux_used_profile $cmd && profiles="$cmd $profiles"
diff --git testcases/lib/tst_test.sh testcases/lib/tst_test.sh
index 333061028..e69301e54 100644
--- testcases/lib/tst_test.sh
+++ testcases/lib/tst_test.sh
@@ -68,7 +68,6 @@ _tst_do_exit()
 
 	if [ $TST_FAIL -gt 0 ]; then
 		ret=$((ret|1))
-		_tst_check_security_modules
 	fi
 
 	if [ $TST_BROK -gt 0 ]; then
@@ -83,6 +82,10 @@ _tst_do_exit()
 		ret=$((ret|32))
 	fi
 
+	if [ $TST_BROK -gt 0 -o $TST_CONF -gt 0 -o $TST_FAIL -gt 0 -o $TST_WARN -gt 0 ]; then
+		_tst_check_security_modules
+	fi
+
 	echo
 	echo "Summary:"
 	echo "passed   $TST_PASS"
Cyril Hrubis Jan. 23, 2019, 4:20 p.m. UTC | #7 
Hi!
>  
> +	if [ $TST_BROK -gt 0 -o $TST_CONF -gt 0 -o $TST_FAIL -gt 0 -o $TST_WARN -gt 0 ]; then
> +		_tst_check_security_modules
> +	fi

Not sure about the TCONF here but apart of that it looks good.
Petr Vorel Jan. 29, 2019, 6:24 p.m. UTC | #8 
Hi,

> > +	if [ $TST_BROK -gt 0 -o $TST_CONF -gt 0 -o $TST_FAIL -gt 0 -o $TST_WARN -gt 0 ]; then
> > +		_tst_check_security_modules
> > +	fi

> Not sure about the TCONF here but apart of that it looks good.
Whole patchset merged, without TCONF.

Kind regards,
Petr
diff mbox series

Patch

diff --git a/testcases/lib/tst_security.sh b/testcases/lib/tst_security.sh
new file mode 100644
index 000000000..68b47347f
--- /dev/null
+++ b/testcases/lib/tst_security.sh
@@ -0,0 +1,124 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2018 Petr Vorel <pvorel@suse.cz>
+
+if [ -z "$TST_LIB_LOADED" ]; then
+	echo "please load tst_test.sh first" >&2
+	exit 1
+fi
+
+[ -n "$TST_SECURITY_LOADED" ] && return 0
+TST_SECURITY_LOADED=1
+
+_tst_check_security_modules()
+{
+	local cmd
+	local profiles
+
+	if tst_apparmor_enabled; then
+		tst_res TINFO "AppArmor enabled, this may affect test results. Disable it with TST_DISABLE_APPARMOR=1 (requires super/root)"
+		profiles=
+		for cmd in $TST_NEEDS_CMDS; do
+			tst_apparmor_used_profile $cmd && profiles="$cmd $profiles"
+		done
+		[ -z "$profiles" ] && profiles="none"
+		tst_res TINFO "loaded AppArmor profiles: $profiles"
+	fi
+
+	if tst_selinux_enabled; then
+		tst_res TINFO "SELinux enabled in enforcing mode, this may affect test results. Disable it with TST_DISABLE_SELINUX=1 (requires super/root)"
+		profiles=
+		for cmd in $TST_NEEDS_CMDS; do
+			tst_selinux_used_profile $cmd && profiles="$cmd $profiles"
+		done
+		[ -z "$profiles" ] && profiles="none"
+		tst_res TINFO "loaded SELinux profiles: $profiles"
+	fi
+}
+
+# Detect whether AppArmor profiles are loaded
+# Return 0: profiles loaded, 1: none profile loaded or AppArmor disabled
+tst_apparmor_enabled()
+{
+	local f="/sys/module/apparmor/parameters/enabled"
+	[ -f "$f" ] && [ "$(cat $f)" = "Y" ]
+}
+
+# Detect whether AppArmor profile for command is enforced
+# tst_apparmor_used_profile CMD
+# Return 0: loaded profile for CMD
+# Return 1: no profile CMD
+tst_apparmor_used_profile()
+{
+	[ $# -eq 1 ] && tst_brk TCONF "usage tst_apparmor_used_profile CMD"
+	local cmd="$1"
+	grep -q "$cmd .*(enforce)" /sys/kernel/security/apparmor/profiles 2>/dev/null
+}
+
+# Detect whether SELinux is enabled in enforcing mode
+# Return 0: enabled in enforcing mode
+# Return 1: enabled in permissive mode or disabled
+tst_selinux_enabled()
+{
+	local f="$(_tst_get_enforce)"
+
+	[ -f "$f" ] && [ "$(cat $f)" = "1" ]
+}
+
+# Detect whether SELinux profile for command is enforced
+# tst_selinux_used_profile CMD
+# Return 0: loaded profile for CMD
+# Return 1: profile for CMD not loaded or seinfo not available
+tst_selinux_used_profile()
+{
+	[ $# -eq 1 ] && tst_brk TCONF "usage tst_selinux_used_profile CMD"
+	local cmd="$1"
+
+	if ! tst_cmd_available seinfo; then
+		if [ -z "$seinfo_warn_printed" ]; then
+			tst_res "install seinfo to find used SELinux profiles"
+			export seinfo_warn_printed=1
+		fi
+		return
+	fi
+	seinfo -t 2>/dev/null | grep -q $cmd
+}
+
+# Try disable AppArmor
+# Return 0: AppArmor disabled
+# Return > 0: failed to disable AppArmor
+tst_disable_apparmor()
+{
+	_tst_require_root
+	local f="aa-teardown"
+	local action
+
+	tst_cmd_available $f && { $f; return; }
+	f="/etc/init.d/apparmor"
+	if [ -f "$f" ]; then
+		for action in teardown kill stop; do
+			$f $action >/dev/null 2>&1 && return
+		done
+	fi
+}
+
+# Try disable SELinux
+# Return 0: SELinux disabled
+# Return > 0: failed to disable SELinux
+tst_disable_selinux()
+{
+	_tst_require_root
+	local f="$(_tst_get_enforce)"
+
+	[ -f "$f" ] && cat 0 > $f
+}
+
+# Get SELinux enforce file path
+_tst_get_enforce()
+{
+	local dir="/sys/fs/selinux"
+
+	[ -d "$dir" ] || dir="/selinux"
+	local f="$dir/enforce"
+	[ -f "$f" ] && echo "$f"
+}
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index e3770d005..333061028 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -34,6 +34,7 @@  export TST_TMPDIR_RHOST=0
 export TST_LIB_LOADED=1
 
 . tst_ansi_color.sh
+. tst_security.sh
 
 # default trap function
 trap "tst_brk TBROK 'test interrupted'" INT
@@ -67,6 +68,7 @@  _tst_do_exit()
 
 	if [ $TST_FAIL -gt 0 ]; then
 		ret=$((ret|1))
+		_tst_check_security_modules
 	fi
 
 	if [ $TST_BROK -gt 0 ]; then
@@ -376,6 +378,11 @@  _tst_setup_timer()
 	_tst_setup_timer_pid=$!
 }
 
+_tst_require_root()
+{
+	[ "$(id -ru)" != 0 ] && tst_brk TCONF "Must be super/root for this test!"
+}
+
 tst_run()
 {
 	local _tst_i
@@ -386,6 +393,7 @@  tst_run()
 	if [ -n "$TST_TEST_PATH" ]; then
 		for _tst_i in $(grep TST_ "$TST_TEST_PATH" | sed 's/.*TST_//; s/[="} \t\/:`].*//'); do
 			case "$_tst_i" in
+			DISABLE_APPARMOR|DISABLE_SELINUX);;
 			SETUP|CLEANUP|TESTFUNC|ID|CNT|MIN_KVER);;
 			OPTS|USAGE|PARSE_ARGS|POS_ARGS);;
 			NEEDS_ROOT|NEEDS_TMPDIR|TMPDIR|NEEDS_DEVICE|DEVICE);;
@@ -421,11 +429,10 @@  tst_run()
 		tst_brk TBROK "Number of iterations (-i) must be > 0"
 	fi
 
-	if [ "$TST_NEEDS_ROOT" = 1 ]; then
-		if [ "$(id -ru)" != 0 ]; then
-			tst_brk TCONF "Must be super/root for this test!"
-		fi
-	fi
+	[ "$TST_NEEDS_ROOT" = 1 ] && _tst_require_root
+
+	[ "$TST_DISABLE_APPARMOR" = 1 ] && tst_disable_apparmor
+	[ "$TST_DISABLE_SELINUX" = 1 ] && tst_disable_selinux
 
 	tst_test_cmds $TST_NEEDS_CMDS
 	tst_test_drivers $TST_NEEDS_DRIVERS