[PATCH ltp v2] IMA: Add tests for uid, gid, fowner, and fgroup options

[PATCH ltp v2] IMA: Add tests for uid, gid, fowner, and fgroup options

[Alex Henrie]

Requires "ima: add gid support".

Signed-off-by: Alex Henrie <alexh@vpitech.com>
---
v2:
- Add sudo to list of required commands
- Check policy writability
- Check kernel version
- Use `sudo sg` to test the gid option
- Don't try to restore the original policy after testing
---
 .../integrity/ima/tests/ima_measurements.sh   | 37 +++++++++++++++++--
 .../integrity/ima/tests/ima_policy.sh         | 14 +------
 .../security/integrity/ima/tests/ima_setup.sh | 10 +++++
 3 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
index 1927e937c..d685fc161 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
@@ -6,7 +6,7 @@
 #
 # Verify that measurements are added to the measurement list based on policy.
 
-TST_NEEDS_CMDS="awk cut sed"
+TST_NEEDS_CMDS="awk cut sed sg sudo"
 TST_SETUP="setup"
 TST_CNT=3
 TST_NEEDS_DEVICE=1
@@ -20,6 +20,8 @@ setup()
 	TEST_FILE="$PWD/test.txt"
 	POLICY="$IMA_DIR/policy"
 	[ -f "$POLICY" ] || tst_res TINFO "not using default policy"
+
+	require_policy_writable
 }
 
 ima_check()
@@ -103,7 +105,7 @@ test3()
 	local file="$dir/test.txt"
 
 	# Default policy does not measure user files
-	tst_res TINFO "verify not measuring user files"
+	tst_res TINFO "verify not measuring user files by default"
 	tst_check_cmds sudo || return
 
 	if ! id $user >/dev/null 2>/dev/null; then
@@ -116,9 +118,38 @@ test3()
 	cd $dir
 	# need to read file to get updated $ASCII_MEASUREMENTS
 	sudo -n -u $user sh -c "echo $(date) user file > $file; cat $file > /dev/null"
+	EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS"
 	cd ..
 
-	EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS"
+	tst_res TINFO "verify measuring user files when requested via uid"
+	ROD echo "measure uid=$(id -u $user)" \> $IMA_POLICY
+	ROD echo "$(date) uid test" \> $TEST_FILE
+	sudo -n -u $user sh -c "cat $TEST_FILE > /dev/null"
+	ima_check
+
+	tst_res TINFO "verify measuring user files when requested via fowner"
+	ROD echo "measure fowner=$(id -u $user)" \> $IMA_POLICY
+	ROD echo "$(date) fowner test" \> $TEST_FILE
+	chown $user $TEST_FILE
+	cat $TEST_FILE > /dev/null
+	ima_check
+
+	if tst_kvcmp -lt 5.15; then
+		tst_brk TCONF "gid and fgroup options require kernel 5.15 or newer"
+	fi
+
+	tst_res TINFO "verify measuring user files when requested via gid"
+	ROD echo "measure gid=$(id -g $user)" \> $IMA_POLICY
+	ROD echo "$(date) gid test" \> $TEST_FILE
+	sudo sg $user "sh -c 'cat $TEST_FILE > /dev/null'"
+	ima_check
+
+	tst_res TINFO "verify measuring user files when requested via fgroup"
+	ROD echo "measure fgroup=$(id -g $user)" \> $IMA_POLICY
+	ROD echo "$(date) fgroup test" \> $TEST_FILE
+	chgrp $user $TEST_FILE
+	cat $TEST_FILE > /dev/null
+	ima_check
 }
 
 tst_run
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_policy.sh b/testcases/kernel/security/integrity/ima/tests/ima_policy.sh
index 244cf081d..f1d3b6074 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_policy.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_policy.sh
@@ -11,19 +11,9 @@ TST_CNT=2
 
 . ima_setup.sh
 
-check_policy_writable()
-{
-	local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
-
-	[ -f $IMA_POLICY ] || tst_brk TCONF "$err"
-	# CONFIG_IMA_READ_POLICY
-	echo "" 2> log > $IMA_POLICY
-	grep -q "Device or resource busy" log && tst_brk TCONF "$err"
-}
-
 setup()
 {
-	check_policy_writable
+	require_policy_writable
 
 	VALID_POLICY="$TST_DATAROOT/measure.policy"
 	[ -f $VALID_POLICY ] || tst_brk TCONF "missing $VALID_POLICY"
@@ -55,7 +45,6 @@ test1()
 
 	local p1
 
-	check_policy_writable
 	load_policy $INVALID_POLICY & p1=$!
 	wait "$p1"
 	if [ $? -ne 0 ]; then
@@ -71,7 +60,6 @@ test2()
 
 	local p1 p2 rc1 rc2
 
-	check_policy_writable
 	load_policy $VALID_POLICY & p1=$!
 	load_policy $VALID_POLICY & p2=$!
 	wait "$p1"; rc1=$?
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index 565f0bc3e..9c25d634d 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -73,6 +73,16 @@ require_policy_readable()
 	fi
 }
 
+require_policy_writable()
+{
+	local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
+
+	[ -f $IMA_POLICY ] || tst_brk TCONF "$err"
+	# CONFIG_IMA_READ_POLICY
+	echo "" 2> log > $IMA_POLICY
+	grep -q "Device or resource busy" log && tst_brk TCONF "$err"
+}
+
 check_ima_policy_content()
 {
 	local pattern="$1"
-- 
2.33.0

Re: [PATCH ltp v2] IMA: Add tests for uid, gid, fowner, and fgroup options

[Petr Vorel]

Hi Alex,

> Requires "ima: add gid support".

> v2:
> - Add sudo to list of required commands
> - Check policy writability
> - Check kernel version
> - Use `sudo sg` to test the gid option
> - Don't try to restore the original policy after testing

...
> -	check_policy_writable
> +	require_policy_writable
Good point to fix function name. But could you please do the rename and move to
ima_setup.sh in separate commit?

Also, why do you extending test3()? Wouldn't be more readable to add test4()?
See notes below.

...
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
...
> -TST_NEEDS_CMDS="awk cut sed"
> +TST_NEEDS_CMDS="awk cut sed sg sudo"
I'm sorry, I was wrong. sudo is needed just in the last test, thus
original check "tst_check_cmds sudo || return" is enough.
Having it TST_NEEDS_CMDS it requires it also for old kernels, which is necessary.

chgrp and sg you newly introduced, should be also tested by tst_check_cmds,
after checking kernel version.

>  TST_SETUP="setup"
>  TST_CNT=3
>  TST_NEEDS_DEVICE=1
> @@ -20,6 +20,8 @@ setup()
>  	TEST_FILE="$PWD/test.txt"
>  	POLICY="$IMA_DIR/policy"
>  	[ -f "$POLICY" ] || tst_res TINFO "not using default policy"
> +
> +	require_policy_writable
This changes test to require CONFIG_IMA_WRITE_POLICY=y. Most distributions does
not have it, thus you'd disable testing for most distros. Not having policy
readable and writeable everywhere greatly complicates IMA testing.
...
>  }

> @@ -103,7 +105,7 @@ test3()
>  	local file="$dir/test.txt"

>  	# Default policy does not measure user files
> -	tst_res TINFO "verify not measuring user files"
> +	tst_res TINFO "verify not measuring user files by default"
>  	tst_check_cmds sudo || return

>  	if ! id $user >/dev/null 2>/dev/null; then
> @@ -116,9 +118,38 @@ test3()
>  	cd $dir
>  	# need to read file to get updated $ASCII_MEASUREMENTS
>  	sudo -n -u $user sh -c "echo $(date) user file > $file; cat $file > /dev/null"
> +	EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS"
>  	cd ..

> -	EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS"
> +	tst_res TINFO "verify measuring user files when requested via uid"
> +	ROD echo "measure uid=$(id -u $user)" \> $IMA_POLICY
This is the reason for require_policy_writable.
Previously it was possible to run it without:

ima_measurements 1 TPASS: correct digest found
ima_measurements 2 TINFO: verify updating record in the IMA measurement list
ima_measurements 2 TINFO: computing digest for sha256 algorithm
ima_measurements 2 TPASS: correct digest found
ima_measurements 3 TINFO: verify not measuring user files
ima_measurements 3 TPASS: grep /tmp/LTP_ima_measurements.6nhS7ScgBn/user/test.txt /sys/kernel/security/ima/ascii_runtime_measurements failed as expected

I'd keep the old EXPECT_FAIL variant (suppose it's still valid and don't require
writable policy) and definitely separate new tests.
Remember, test should run the same on older kernels (we don't want to drop
coverage on older distros / enterprise distros).

> +	ROD echo "$(date) uid test" \> $TEST_FILE
> +	sudo -n -u $user sh -c "cat $TEST_FILE > /dev/null"
> +	ima_check
> +
> +	tst_res TINFO "verify measuring user files when requested via fowner"
> +	ROD echo "measure fowner=$(id -u $user)" \> $IMA_POLICY
> +	ROD echo "$(date) fowner test" \> $TEST_FILE
> +	chown $user $TEST_FILE
> +	cat $TEST_FILE > /dev/null
> +	ima_check
> +
> +	if tst_kvcmp -lt 5.15; then
> +		tst_brk TCONF "gid and fgroup options require kernel 5.15 or newer"
> +	fi
> +
> +	tst_res TINFO "verify measuring user files when requested via gid"
> +	ROD echo "measure gid=$(id -g $user)" \> $IMA_POLICY
> +	ROD echo "$(date) gid test" \> $TEST_FILE
> +	sudo sg $user "sh -c 'cat $TEST_FILE > /dev/null'"
> +	ima_check
> +
> +	tst_res TINFO "verify measuring user files when requested via fgroup"
> +	ROD echo "measure fgroup=$(id -g $user)" \> $IMA_POLICY
> +	ROD echo "$(date) fgroup test" \> $TEST_FILE
> +	chgrp $user $TEST_FILE
> +	cat $TEST_FILE > /dev/null
> +	ima_check
>  }
...

Kind regards,
Petr