diff mbox series

[1/2] IMA: Add a test to verify measurment of keys

Message ID 1591808483-22040-1-git-send-email-t-josne@linux.microsoft.com
State Changes Requested
Headers show
Series [1/2] IMA: Add a test to verify measurment of keys | expand

Commit Message

Lachlan Sneff June 10, 2020, 5:01 p.m. UTC
Add a testcase that verifies that the IMA subsystem has correctly
measured keys added to keyrings specified in the IMA policy file.

Additionally, add support for handling a new IMA template descriptor,
namely ima-buf[1], in the IMA measurement tests.

[1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use

Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
---
 runtest/ima                                   |  1 +
 .../integrity/ima/tests/compute_digest.sh     | 38 ++++++++++
 .../security/integrity/ima/tests/ima_keys.sh  | 72 +++++++++++++++++++
 .../integrity/ima/tests/ima_measurements.sh   | 37 +---------
 4 files changed, 113 insertions(+), 35 deletions(-)
 create mode 100644 testcases/kernel/security/integrity/ima/tests/compute_digest.sh
 create mode 100644 testcases/kernel/security/integrity/ima/tests/ima_keys.sh

Comments

Petr Vorel June 11, 2020, 3:30 p.m. UTC | #1
Hi Lachlan,

thank you for updating LTP!

I have few comments below.
Mostly just tiny details, which I could fix before merge.

> Add a testcase that verifies that the IMA subsystem has correctly
> measured keys added to keyrings specified in the IMA policy file.

> Additionally, add support for handling a new IMA template descriptor,
> namely ima-buf[1], in the IMA measurement tests.
Great, thanks!

> [1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use

...
> +++ b/runtest/ima
> @@ -4,3 +4,4 @@ ima_policy ima_policy.sh
>  ima_tpm ima_tpm.sh
>  ima_violations ima_violations.sh
>  evm_overlay evm_overlay.sh
> +ima_keys ima_keys.sh
Please move evm_overlay after ima_keys.
EVM tests require specific configuration, that's why I put it on the end.
Or before ima_policy.sh (see comment below).

> diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
> new file mode 100644
Please use 755 (shell libraries which are sources can be 644, but this is
meant to be executed).

> index 000000000..85f6bf3da
> --- /dev/null
> +++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
Could you please move this to ima_setup.sh? That's the file we keep helper
functions. (BTW in LTP shell based tests it's usually foo_lib.sh, I named this
ima_setup.sh instead of ima_lib.sh, because it was first meant as tmpfs related
setup and still it does, although I later I added other helper functions there.

...
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> @@ -0,0 +1,72 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2020 Microsoft Corporation
> +# Author: Lachlan Sneff <t-josne@linux.microsoft.com>
> +#
> +# Verify that keys are measured correctly based on policy.
> +
> +TST_NEEDS_CMDS="awk cut"
You missed xxd, which is IMHO less common than awk and cut, therefore it should
be specified.

> +TST_SETUP="setup"
> +TST_CNT=1
> +TST_NEEDS_DEVICE=1
> +
> +. ima_setup.sh
> +. compute_digest.sh
> +
> +setup()
> +{
> +    TEST_FILE="$PWD/test.txt"
This suggest that file tested in test2 (second commit) requires to be the
same. Is it true? If not, I'd set just local variable in each test function.
> +}
> +
> +# Based on https://lkml.org/lkml/2019/12/13/564.
Maybe also mention the commit?
450d0fd51564 ("IMA: Call workqueue functions to measure queued keys")

> +test1()
> +{
> +	local keyrings keycheck_line templates
> +
> +	tst_res TINFO "verifying key measurement for keyrings and \
> +templates specified in ima policy file"
Could you please keep string on single line? We prefer it over to 80 chars per
line rule (although I admit that super long string in check_policy_writable is awfull).
> +
> +	IMA_POLICY="$IMA_DIR/policy"
Could you please move IMA_POLICY to ima_setup.sh and remove the same definition
from ima_policy.sh?
> +	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
BTW to read IMA policy requires CONFIG_IMA_READ_POLICY=y. Maybe there could be
just some hint, because 

grep: /sys/kernel/security/ima/policy: Permission denied
ima_keys 1 TCONF: ima policy does not specify "func=KEY_CHECK"

Could we have here something like
cat $IMA_POLICY > /dev/null || tst_res TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)"


> +
> +	keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY)
> +	if [ -z "$keycheck_line" ]; then
> +		tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\""
> +	fi
Could we prepare policy example in
testcases/kernel/security/integrity/ima/datafiles/keycheck.policy?

I'm trying IMA tests to prepare themselves, otherwise most of the tests would be
TCONF. And the idea is that test has everything prepared when run as part of
runtest file or also on it's own. But here is getting complicated. Unless there is
CONFIG_IMA_WRITE_POLICY=y, we cannot load it in the test. And if we require
loading it before, then ima_policy.sh TCONF.

Not sure if we should have 2 versions of correct IMA policy.
in test2 in ima_policy.sh try to load first keycheck.policy and if it fails then
the old measure.policy. That way we would also avoid testing kernel version
(this functionality can be backported). We could also print support for new
feature. And try load keycheck.policy in ima_keys.sh, in case it's run on it's
own (not part of other tests in runtest file).

I understand if you don't want to play with this, I can add this after merging
your test. But providing keycheck.policy would help.

> +
> +	if echo "$keycheck_line" | grep -q "*keyrings*"; then
> +		tst_brk TCONF "ima policy does not specify a keyrings to check"
> +	fi
> +
> +	keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \
> +		sed "s/\./\\\./g" | cut -d'=' -f2)
> +	if [ -z "$keyrings" ]; then
> +		tst_brk TCONF "ima policy has a keyring key-value specifier, \
> +but no specified keyrings"
+ also here put on single line.
> +	fi
> +
> +	templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \
> +		cut -d'=' -f2)
> +
> +	grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line
> +	do
> +		local digest expected_digest algorithm
> +
> +		digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
> +		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
> +
> +		echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE
> +
> +		expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \
> +			tst_brk TCONF "cannot compute digest for $algorithm"
> +
> +		if [ "$digest" != "$expected_digest" ]; then
> +			tst_res TFAIL "incorrect digest was found for the \
> +$(echo "$line" | cut -d' ' -f5) keyring"
Here as well. Maybe add $keyring and use it as variable.
> +		fi
> +	done
> +
> +	tst_res TPASS "specified keyrings were measured correctly"
> +}
...

Kind regards,
Petr
Lachlan Sneff June 11, 2020, 8:58 p.m. UTC | #2
Thank you for your review!

On 6/11/20 11:30 AM, Petr Vorel wrote:
> Hi Lachlan,
>
> thank you for updating LTP!
>
> I have few comments below.
> Mostly just tiny details, which I could fix before merge.
>
>> Add a testcase that verifies that the IMA subsystem has correctly
>> measured keys added to keyrings specified in the IMA policy file.
>> Additionally, add support for handling a new IMA template descriptor,
>> namely ima-buf[1], in the IMA measurement tests.
> Great, thanks!
:)
>> [1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use
> ...
>> +++ b/runtest/ima
>> @@ -4,3 +4,4 @@ ima_policy ima_policy.sh
>>   ima_tpm ima_tpm.sh
>>   ima_violations ima_violations.sh
>>   evm_overlay evm_overlay.sh
>> +ima_keys ima_keys.sh
> Please move evm_overlay after ima_keys.
> EVM tests require specific configuration, that's why I put it on the end.
> Or before ima_policy.sh (see comment below).
Okay
>> diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
>> new file mode 100644
> Please use 755 (shell libraries which are sources can be 644, but this is
> meant to be executed).
This makes sense, sorry!
>> index 000000000..85f6bf3da
>> --- /dev/null
>> +++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
> Could you please move this to ima_setup.sh? That's the file we keep helper
> functions. (BTW in LTP shell based tests it's usually foo_lib.sh, I named this
> ima_setup.sh instead of ima_lib.sh, because it was first meant as tmpfs related
> setup and still it does, although I later I added other helper functions there.

I wasn't sure what to do with compute_digest. I didn't want to duplicate 
it, so I added a

a new file for it. Adding it to ima_setup.sh is a solid idea.

> ...
>> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
>> @@ -0,0 +1,72 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +# Copyright (c) 2020 Microsoft Corporation
>> +# Author: Lachlan Sneff <t-josne@linux.microsoft.com>
>> +#
>> +# Verify that keys are measured correctly based on policy.
>> +
>> +TST_NEEDS_CMDS="awk cut"
> You missed xxd, which is IMHO less common than awk and cut, therefore it should
> be specified.
Good call.
>> +TST_SETUP="setup"
>> +TST_CNT=1
>> +TST_NEEDS_DEVICE=1
>> +
>> +. ima_setup.sh
>> +. compute_digest.sh
>> +
>> +setup()
>> +{
>> +    TEST_FILE="$PWD/test.txt"
> This suggest that file tested in test2 (second commit) requires to be the
> same. Is it true? If not, I'd set just local variable in each test function.
They don't need to be the same file. I'll just create a new one in each 
test.
>> +}
>> +
>> +# Based on https://lkml.org/lkml/2019/12/13/564.
> Maybe also mention the commit?
> 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys")
Good call. I'll expand this comment.
>> +test1()
>> +{
>> +	local keyrings keycheck_line templates
>> +
>> +	tst_res TINFO "verifying key measurement for keyrings and \
>> +templates specified in ima policy file"
> Could you please keep string on single line? We prefer it over to 80 chars per
> line rule (although I admit that super long string in check_policy_writable is awfull).
Sounds good. I wasn't sure what to do with the long strings.
>> +
>> +	IMA_POLICY="$IMA_DIR/policy"
> Could you please move IMA_POLICY to ima_setup.sh and remove the same definition
> from ima_policy.sh?
Yep.
>> +	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
> BTW to read IMA policy requires CONFIG_IMA_READ_POLICY=y. Maybe there could be
> just some hint, because
>
> grep: /sys/kernel/security/ima/policy: Permission denied
> ima_keys 1 TCONF: ima policy does not specify "func=KEY_CHECK"
>
> Could we have here something like
> cat $IMA_POLICY > /dev/null || tst_res TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)"
>
Didn't realize it wasn't necessarily readable. Will add that check.
>> +
>> +	keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY)
>> +	if [ -z "$keycheck_line" ]; then
>> +		tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\""
>> +	fi
> Could we prepare policy example in
> testcases/kernel/security/integrity/ima/datafiles/keycheck.policy?
>
> I'm trying IMA tests to prepare themselves, otherwise most of the tests would be
> TCONF. And the idea is that test has everything prepared when run as part of
> runtest file or also on it's own. But here is getting complicated. Unless there is
> CONFIG_IMA_WRITE_POLICY=y, we cannot load it in the test. And if we require
> loading it before, then ima_policy.sh TCONF.
>
> Not sure if we should have 2 versions of correct IMA policy.
> in test2 in ima_policy.sh try to load first keycheck.policy and if it fails then
> the old measure.policy. That way we would also avoid testing kernel version
> (this functionality can be backported). We could also print support for new
> feature. And try load keycheck.policy in ima_keys.sh, in case it's run on it's
> own (not part of other tests in runtest file).
>
> I understand if you don't want to play with this, I can add this after merging
> your test. But providing keycheck.policy would help.

I'll supply a keycheck.policy in my updated patch, but I need to think 
more about what you've written here. Is the idea to have an example 
policy that the test can load if the kernel was built with 
CONFIG_IMA_WRITE_POLICY=y?

What should the test do in that case if the kernel wasn't build with a 
writable IMA policy?

>> +
>> +	if echo "$keycheck_line" | grep -q "*keyrings*"; then
>> +		tst_brk TCONF "ima policy does not specify a keyrings to check"
>> +	fi
>> +
>> +	keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \
>> +		sed "s/\./\\\./g" | cut -d'=' -f2)
>> +	if [ -z "$keyrings" ]; then
>> +		tst_brk TCONF "ima policy has a keyring key-value specifier, \
>> +but no specified keyrings"
> + also here put on single line.
Yep
>> +	fi
>> +
>> +	templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \
>> +		cut -d'=' -f2)
>> +
>> +	grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line
>> +	do
>> +		local digest expected_digest algorithm
>> +
>> +		digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
>> +		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
>> +
>> +		echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE
>> +
>> +		expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \
>> +			tst_brk TCONF "cannot compute digest for $algorithm"
>> +
>> +		if [ "$digest" != "$expected_digest" ]; then
>> +			tst_res TFAIL "incorrect digest was found for the \
>> +$(echo "$line" | cut -d' ' -f5) keyring"
> Here as well. Maybe add $keyring and use it as variable.
Okay
>> +		fi
>> +	done
>> +
>> +	tst_res TPASS "specified keyrings were measured correctly"
>> +}
> ...
>
> Kind regards,
> Petr

Thanks,

Lachlan
diff mbox series

Patch

diff --git a/runtest/ima b/runtest/ima
index f3ea88cf0..939fb40f0 100644
--- a/runtest/ima
+++ b/runtest/ima
@@ -4,3 +4,4 @@  ima_policy ima_policy.sh
 ima_tpm ima_tpm.sh
 ima_violations ima_violations.sh
 evm_overlay evm_overlay.sh
+ima_keys ima_keys.sh
diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
new file mode 100644
index 000000000..85f6bf3da
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
@@ -0,0 +1,38 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2009 IBM Corporation
+# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
+# Author: Mimi Zohar <zohar@linux.ibm.com>
+
+# TODO: find support for rmd128 rmd256 rmd320 wp256 wp384 tgr128 tgr160
+compute_digest()
+{
+	local algorithm="$1"
+	local file="$2"
+	local digest
+
+	digest="$(${algorithm}sum $file 2>/dev/null | cut -f1 -d ' ')"
+	if [ -n "$digest" ]; then
+		echo "$digest"
+		return 0
+	fi
+
+	digest="$(openssl $algorithm $file 2>/dev/null | cut -f2 -d ' ')"
+	if [ -n "$digest" ]; then
+		echo "$digest"
+		return 0
+	fi
+
+	# uncommon ciphers
+	local arg="$algorithm"
+	case "$algorithm" in
+	tgr192) arg="tiger" ;;
+	wp512) arg="whirlpool" ;;
+	esac
+
+	digest="$(rdigest --$arg $file 2>/dev/null | cut -f1 -d ' ')"
+	if [ -n "$digest" ]; then
+		echo "$digest"
+		return 0
+	fi
+	return 1
+}
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
new file mode 100644
index 000000000..1b0dd0aed
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
@@ -0,0 +1,72 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2020 Microsoft Corporation
+# Author: Lachlan Sneff <t-josne@linux.microsoft.com>
+#
+# Verify that keys are measured correctly based on policy.
+
+TST_NEEDS_CMDS="awk cut"
+TST_SETUP="setup"
+TST_CNT=1
+TST_NEEDS_DEVICE=1
+
+. ima_setup.sh
+. compute_digest.sh
+
+setup()
+{
+    TEST_FILE="$PWD/test.txt"
+}
+
+# Based on https://lkml.org/lkml/2019/12/13/564.
+test1()
+{
+	local keyrings keycheck_line templates
+
+	tst_res TINFO "verifying key measurement for keyrings and \
+templates specified in ima policy file"
+
+	IMA_POLICY="$IMA_DIR/policy"
+	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
+
+	keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY)
+	if [ -z "$keycheck_line" ]; then
+		tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\""
+	fi
+
+	if echo "$keycheck_line" | grep -q "*keyrings*"; then
+		tst_brk TCONF "ima policy does not specify a keyrings to check"
+	fi
+
+	keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \
+		sed "s/\./\\\./g" | cut -d'=' -f2)
+	if [ -z "$keyrings" ]; then
+		tst_brk TCONF "ima policy has a keyring key-value specifier, \
+but no specified keyrings"
+	fi
+
+	templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \
+		cut -d'=' -f2)
+
+	grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line
+	do
+		local digest expected_digest algorithm
+
+		digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
+		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
+
+		echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE
+
+		expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \
+			tst_brk TCONF "cannot compute digest for $algorithm"
+
+		if [ "$digest" != "$expected_digest" ]; then
+			tst_res TFAIL "incorrect digest was found for the \
+$(echo "$line" | cut -d' ' -f5) keyring"
+		fi
+	done
+
+	tst_res TPASS "specified keyrings were measured correctly"
+}
+
+tst_run
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
index 54237d688..0a58d021d 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
@@ -12,6 +12,7 @@  TST_CNT=3
 TST_NEEDS_DEVICE=1
 
 . ima_setup.sh
+. compute_digest.sh
 
 setup()
 {
@@ -28,7 +29,7 @@  setup()
 	# parse digest index
 	# https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use
 	case "$template" in
-	ima|ima-ng|ima-sig) DIGEST_INDEX=4 ;;
+	ima|ima-ng|ima-sig|ima-buf) DIGEST_INDEX=4 ;;
 	*)
 		# using ima_template_fmt kernel parameter
 		local IFS="|"
@@ -46,40 +47,6 @@  setup()
 		"Cannot find digest index (template: '$template')"
 }
 
-# TODO: find support for rmd128 rmd256 rmd320 wp256 wp384 tgr128 tgr160
-compute_digest()
-{
-	local algorithm="$1"
-	local file="$2"
-	local digest
-
-	digest="$(${algorithm}sum $file 2>/dev/null | cut -f1 -d ' ')"
-	if [ -n "$digest" ]; then
-		echo "$digest"
-		return 0
-	fi
-
-	digest="$(openssl $algorithm $file 2>/dev/null | cut -f2 -d ' ')"
-	if [ -n "$digest" ]; then
-		echo "$digest"
-		return 0
-	fi
-
-	# uncommon ciphers
-	local arg="$algorithm"
-	case "$algorithm" in
-	tgr192) arg="tiger" ;;
-	wp512) arg="whirlpool" ;;
-	esac
-
-	digest="$(rdigest --$arg $file 2>/dev/null | cut -f1 -d ' ')"
-	if [ -n "$digest" ]; then
-		echo "$digest"
-		return 0
-	fi
-	return 1
-}
-
 ima_check()
 {
 	local delimiter=':'