[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
Related show

Commit Message

Petr Vorel Dec. 18, 2018, 1:02 a.m.
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. | #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. | #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. | #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. | #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. | #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. | #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. | #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. | #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

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