[PATCH v2 0/2] IMA: Add test for dm-crypt measurement

[PATCH v2 0/2] IMA: Add test for dm-crypt measurement

[Tushar Sugandhi]

New functionality is being added to IMA to measure data provided by
kernel components. With this feature, IMA policy can be set to enable
measuring data provided by device-mapper targets. Currently one such
device-mapper target - dm-crypt, is being updated to use this
functionality. This new functionality needs test automation in LTP.

Some of the existing functionality in ima_keys.sh can be reused for
this, but it needs to be refactored into generic functions first.

Add a testcase which verifies that the IMA subsystem correctly measures
the data coming from a device-mapper target - dm-crypt.
Refactor common functionality in ima_keys.sh for this, and move the
generic functions to ima_setup.sh.

This series needs a kernel built on the following repo/branch/patches:
 repo: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
 branch: next-integrity
 commit aa662fc04f5b ("ima: Fix NULL pointer dereference in ima_file_hash")

And the following patch series should be applied in the following order:
 1. https://patchwork.kernel.org/patch/11795559/
 2. https://patchwork.kernel.org/patch/11801525/
 3. https://patchwork.kernel.org/patch/11743715/

Change Log v2:
Incorporated feedback from Petr Vorel on v1.
 - Updated TST_NEEDS_CMDS to correctly reflects commands used in tests.
 - Removed unnecessary debugging info.
 - Refactored common functionality in ima_keys.sh, and moved the generic
   functions to ima_setup.sh.
 - Removed the use of eval, and replaced it with the recommended ROD()
   function.
 - All temporary files now get created under $TST_TMPDIR, instead of
   current directory.
 - Removed unnecessary TFAIL, to avoid double counting failures.
 - Updated log messages to be consistent.
 - Moved code to cleanup() to avoid code duplication.

Tushar Sugandhi (2):
  IMA: generalize key measurement tests
  IMA: Add test for dm-crypt measurement

 runtest/ima                                   |  1 +
 .../kernel/security/integrity/ima/README.md   | 20 +++++
 .../integrity/ima/tests/ima_dm_crypt.sh       | 60 ++++++++++++++
 .../security/integrity/ima/tests/ima_keys.sh  | 62 +++------------
 .../security/integrity/ima/tests/ima_setup.sh | 79 +++++++++++++++++++
 5 files changed, 173 insertions(+), 49 deletions(-)
 create mode 100755 testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh

-- 
2.17.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[PATCH v2 1/2] IMA: generalize key measurement tests

[Tushar Sugandhi]

New functionality is being added in IMA to measure data provided by
kernel components. Tests have to be added in LTP to validate this new
feature. The functionality in ima_keys.sh can be reused to test this new
feature if it is made generic.

Refactor check_keys_policy() and test1() implemented in ima_keys.sh to
make it generic, and move the functionality to ima_setup.sh as new
functions - check_policy_pattern() and check_ima_ascii_log_for_policy().

Signed-off-by: Tushar Sugandhi <tusharsu-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@public.gmane.org>
---
 .../security/integrity/ima/tests/ima_keys.sh  | 62 +++------------
 .../security/integrity/ima/tests/ima_setup.sh | 79 +++++++++++++++++++
 2 files changed, 92 insertions(+), 49 deletions(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
index c9eef4b68..c2120358a 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
@@ -6,7 +6,7 @@
 #
 # Verify that keys are measured correctly based on policy.
 
-TST_NEEDS_CMDS="cmp cut grep sed xxd"
+TST_NEEDS_CMDS="cmp cut grep xxd"
 TST_CNT=2
 TST_NEEDS_DEVICE=1
 TST_SETUP=setup
@@ -28,64 +28,28 @@ cleanup()
 	tst_is_num $KEYRING_ID && keyctl clear $KEYRING_ID
 }
 
-check_keys_policy()
-{
-	local pattern="$1"
-
-	if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then
-		tst_res TCONF "IMA policy must specify $pattern, $FUNC_KEYCHECK, $TEMPLATE_BUF"
-		return 1
-	fi
-	return 0
-}
-
 # Based on https://lkml.org/lkml/2019/12/13/564.
 # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys")
 test1()
 {
 	local keycheck_lines i keyrings templates
 	local pattern='keyrings=[^[:space:]]+'
-	local test_file="file.txt" tmp_file="file2.txt"
+	local policy="keyrings"
+	local tmp_file="$TST_TMPDIR/keycheck_tmp_file.txt"
+	local res
 
 	tst_res TINFO "verify key measurement for keyrings and templates specified in IMA policy"
 
-	check_keys_policy "$pattern" > $tmp_file || return
-	keycheck_lines=$(cat $tmp_file)
-	keyrings=$(for i in $keycheck_lines; do echo "$i" | grep "keyrings" | \
-		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
-	if [ -z "$keyrings" ]; then
-		tst_res TCONF "IMA policy has a keyring key-value specifier, but no specified keyrings"
-		return
-	fi
-
-	templates=$(for i in $keycheck_lines; do echo "$i" | grep "template" | \
-		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
-
-	tst_res TINFO "keyrings: '$keyrings'"
-	tst_res TINFO "templates: '$templates'"
-
-	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)
-		keyring=$(echo "$line" | cut -d' ' -f5)
+	check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF > $tmp_file || return
 
-		echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file
+	res="$(check_ima_ascii_log_for_policy $policy $tmp_file)"
 
-		if ! expected_digest="$(compute_digest $algorithm $test_file)"; then
-			tst_res TCONF "cannot compute digest for $algorithm"
-			return
-		fi
-
-		if [ "$digest" != "$expected_digest" ]; then
-			tst_res TFAIL "incorrect digest was found for $keyring keyring"
-			return
-		fi
-	done
+	if [ "$res" = "0" ]; then
+		tst_res TPASS "specified keyrings were measured correctly"
+	else
+		tst_res TFAIL "failed to measure specified keyrings"
+	fi
 
-	tst_res TPASS "specified keyrings were measured correctly"
 }
 
 # Create a new keyring, import a certificate into it, and verify
@@ -97,11 +61,11 @@ test2()
 	local cert_file="$TST_DATAROOT/x509_ima.der"
 	local keyring_name="key_import_test"
 	local pattern="keyrings=[^[:space:]]*$keyring_name"
-	local temp_file="file.txt"
+	local temp_file="$TST_TMPDIR/key_import_test_file.txt"
 
 	tst_res TINFO "verify measurement of certificate imported into a keyring"
 
-	check_keys_policy "$pattern" >/dev/null || return
+	check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF >/dev/null || return
 
 	KEYRING_ID=$(keyctl newring $keyring_name @s) || \
 		tst_brk TBROK "unable to create a new keyring"
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index 1f17aa707..2841d7df5 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -54,6 +54,85 @@ compute_digest()
 	return 1
 }
 
+check_policy_pattern()
+{
+	local pattern="$1"
+	local func="$2"
+	local template="$3"
+
+	if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then
+		tst_res TCONF "IMA policy must specify $pattern, $func, $template"
+		return 1
+	fi
+	return 0
+}
+
+check_ima_ascii_log_for_policy()
+{
+	local test_file="$TST_TMPDIR/ascii_log_test_file.txt"
+	local grep_file="$TST_TMPDIR/ascii_log_grep_file.txt"
+	local func_lines sources templates i src 
+	local input_digest_res=1
+	local policy_option="$1"
+	local input_digest="$3"
+
+	func_lines=$(cat $2)
+
+	sources=$(for i in $func_lines; do echo "$i" | grep "$policy_option" | \
+		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
+	if [ -z "$sources" ]; then
+		tst_res TCONF "IMA policy $policy_option is a key-value specifier, but no values specified"
+		echo "1"
+		return
+	fi
+
+	templates=$(for i in $func_lines; do echo "$i" | grep "template" | \
+		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
+
+	tst_res TINFO "policy sources: '$sources'"
+	tst_res TINFO "templates: '$templates'"
+
+	grep -E "($templates).*($sources)" $ASCII_MEASUREMENTS > $grep_file
+
+	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)
+		src_line=$(echo "$line" | cut -d' ' -f5)
+
+		echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file
+
+		if ! expected_digest="$(compute_digest $algorithm $test_file)"; then
+			tst_res TCONF "cannot compute digest for $algorithm"
+			echo "1"
+			return
+		fi
+
+		if [ "$digest" != "$expected_digest" ]; then
+			tst_res TINFO "incorrect digest was found for $src_line $policy_option"
+			echo "1"	
+			return
+		fi
+
+		if [ "$input_digest" ]; then
+			if [ "$digest" = "$input_digest" ]; then
+				input_digest_res=0
+			fi
+		fi
+
+	done < $grep_file
+
+	if [ "$input_digest" ]; then
+		echo "$input_digest_res"
+		return
+	else
+		echo "0"
+		return
+	fi
+}
+
 check_policy_readable()
 {
 	if [ ! -f $IMA_POLICY ]; then
-- 
2.17.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[PATCH v2 2/2] IMA: Add test for dm-crypt measurement

[Tushar Sugandhi]

New functionality is being added to IMA to measure data provided by
kernel components. With this feature, IMA policy can be set to enable
measuring data provided by device-mapper targets. Currently one such
device-mapper target - dm-crypt, is being updated to use this
functionality. This new functionality needs test automation in LTP.

Add a testcase which verifies that the IMA subsystem correctly measures
the data coming from a device-mapper target - dm-crypt.

Signed-off-by: Tushar Sugandhi <tusharsu-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@public.gmane.org>
---
 runtest/ima                                   |  1 +
 .../kernel/security/integrity/ima/README.md   | 20 +++++++
 .../integrity/ima/tests/ima_dm_crypt.sh       | 60 +++++++++++++++++++
 3 files changed, 81 insertions(+)
 create mode 100755 testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh

diff --git a/runtest/ima b/runtest/ima
index 5f4b4a7a1..123b6c8b0 100644
--- a/runtest/ima
+++ b/runtest/ima
@@ -5,4 +5,5 @@ ima_tpm ima_tpm.sh
 ima_violations ima_violations.sh
 ima_keys ima_keys.sh
 ima_kexec ima_kexec.sh
+ima_dm_crypt ima_dm_crypt.sh
 evm_overlay evm_overlay.sh
diff --git a/testcases/kernel/security/integrity/ima/README.md b/testcases/kernel/security/integrity/ima/README.md
index 68d046678..007662fae 100644
--- a/testcases/kernel/security/integrity/ima/README.md
+++ b/testcases/kernel/security/integrity/ima/README.md
@@ -37,6 +37,26 @@ see example in `kexec.policy`.
 The test attempts to kexec the existing running kernel image.
 To kexec a different kernel image export `IMA_KEXEC_IMAGE=<pathname>`.
 
+### IMA DM target (dm-crypt) measurement test
+
+To enable IMA to measure device-mapper target - dm-crypt,
+`ima_dm_crypt.sh` requires a readable IMA policy, as well as
+a loaded measure policy with
+`func=CRITICAL_DATA data_sources=dm-crypt`
+
+As well as what's required for the IMA tests, dm-crypt measurement test require
+reading the IMA policy allowed in the kernel configuration:
+```
+CONFIG_IMA_READ_POLICY=y
+```
+
+The following kernel configuration is also required. It enables compiling
+the device-mapper target module dm-crypt, which allows to create a device
+that transparently encrypts the data on it.
+```
+CONFIG_DM_CRYPT
+```
+
 ## EVM tests
 
 `evm_overlay.sh` requires a builtin IMA appraise tcb policy (e.g. `ima_policy=appraise_tcb`
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh b/testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh
new file mode 100755
index 000000000..396033f8d
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2020 Microsoft Corporation
+# Author: Tushar Sugandhi <tusharsu-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@public.gmane.org>
+#
+# Verify that DM target dm-crypt are measured correctly based on policy.
+
+TST_NEEDS_CMDS="dmsetup"
+TST_CNT=1
+TST_NEEDS_DEVICE=1
+TST_SETUP=setup
+TST_CLEANUP=cleanup
+
+. ima_setup.sh
+
+FUNC_CRIT_DATA='func=CRITICAL_DATA'
+TEMPLATE_BUF='template=ima-buf'
+REQUIRED_POLICY="^measure.*($FUNC_CRIT_DATA.*$TEMPLATE_BUF|$TEMPLATE_BUF.*$FUNC_CRIT_DATA)"
+
+setup()
+{
+	require_ima_policy_content "$REQUIRED_POLICY" '-E' > $TST_TMPDIR/policy.txt
+}
+
+cleanup()
+{
+	ROD "dmsetup remove test-crypt"
+}
+
+test1()
+{
+	local input_digest="039d8ff71918608d585adca3e5aab2e3f41f84d6"
+	local pattern='data_sources=[^[:space:]]+'
+	local tmp_file="$TST_TMPDIR/dm_crypt_tmp.txt"
+	local policy="data_sources"
+	local arg key res
+
+	tst_res TINFO "verifying dm target - dmcrypt gets measured correctly"
+
+	check_policy_pattern "$pattern" $FUNC_CRIT_DATA $TEMPLATE_BUF > $tmp_file || return
+
+	tgt="crypt"
+	key="faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa"
+
+	arg="'0 1953125 crypt aes-xts-plain64 "
+	arg="$arg $key 0 "
+	arg="$arg /dev/loop0 0 1 allow_discards'"
+
+	ROD "dmsetup create test-crypt --table $arg"
+
+	res="$(check_ima_ascii_log_for_policy $policy $tmp_file $input_digest)"
+
+	if [ $res = "0" ]; then
+		tst_res TPASS "dm-crypt target verification passed"
+	else
+		tst_res TFAIL "dm-crypt target verification failed"
+	fi
+}
+
+tst_run
-- 
2.17.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [dm-devel] [PATCH v2 1/2] IMA: generalize key measurement tests

[Petr Vorel]

Hi Tushar,

I'm very sorry about the delay. I'll finish this review in January,
here just some quick thoughts (minor style nits, I'll fix it before merge).

Generally LGTM, thanks for your work.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> New functionality is being added in IMA to measure data provided by
> kernel components. Tests have to be added in LTP to validate this new
> feature. The functionality in ima_keys.sh can be reused to test this new
> feature if it is made generic.

> Refactor check_keys_policy() and test1() implemented in ima_keys.sh to
> make it generic, and move the functionality to ima_setup.sh as new
> functions - check_policy_pattern() and check_ima_ascii_log_for_policy().

> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  .../security/integrity/ima/tests/ima_keys.sh  | 62 +++------------
>  .../security/integrity/ima/tests/ima_setup.sh | 79 +++++++++++++++++++
>  2 files changed, 92 insertions(+), 49 deletions(-)

> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> index c9eef4b68..c2120358a 100755
> --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> @@ -6,7 +6,7 @@

>  # Verify that keys are measured correctly based on policy.

> -TST_NEEDS_CMDS="cmp cut grep sed xxd"
> +TST_NEEDS_CMDS="cmp cut grep xxd"
It still requires sed, it's just hidden in check_ima_ascii_log_for_policy

Maybe just put at the top of check_ima_ascii_log_for_policy():
tst_require_cmds cut grep sed xxd

And here still keep
TST_NEEDS_CMDS="cmp cut grep tail xxd"

This leads to duplicity in check, but it will not lead to hidden "command not
found".

>  TST_CNT=2
>  TST_NEEDS_DEVICE=1
>  TST_SETUP=setup
> @@ -28,64 +28,28 @@ cleanup()
>  	tst_is_num $KEYRING_ID && keyctl clear $KEYRING_ID
>  }

> -check_keys_policy()
> -{
> -	local pattern="$1"
> -
> -	if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then
> -		tst_res TCONF "IMA policy must specify $pattern, $FUNC_KEYCHECK, $TEMPLATE_BUF"
> -		return 1
> -	fi
> -	return 0
> -}
> -
>  # Based on https://lkml.org/lkml/2019/12/13/564.
>  # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys")
OK, it has been merged in v5.6-rc1. Any more relevant commits, changes since
then?

>  test1()
>  {
>  	local keycheck_lines i keyrings templates
>  	local pattern='keyrings=[^[:space:]]+'
> -	local test_file="file.txt" tmp_file="file2.txt"
> +	local policy="keyrings"
> +	local tmp_file="$TST_TMPDIR/keycheck_tmp_file.txt"
> +	local res
Will be unused, see below.

>  	tst_res TINFO "verify key measurement for keyrings and templates specified in IMA policy"

> -	check_keys_policy "$pattern" > $tmp_file || return
> -	keycheck_lines=$(cat $tmp_file)
> -	keyrings=$(for i in $keycheck_lines; do echo "$i" | grep "keyrings" | \
> -		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
> -	if [ -z "$keyrings" ]; then
> -		tst_res TCONF "IMA policy has a keyring key-value specifier, but no specified keyrings"
> -		return
> -	fi
> -
> -	templates=$(for i in $keycheck_lines; do echo "$i" | grep "template" | \
> -		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
> -
> -	tst_res TINFO "keyrings: '$keyrings'"
> -	tst_res TINFO "templates: '$templates'"
> -
> -	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)
> -		keyring=$(echo "$line" | cut -d' ' -f5)
> +	check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF > $tmp_file || return

> -		echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file
> +	res="$(check_ima_ascii_log_for_policy $policy $tmp_file)"

> -		if ! expected_digest="$(compute_digest $algorithm $test_file)"; then
> -			tst_res TCONF "cannot compute digest for $algorithm"
> -			return
> -		fi
> -
> -		if [ "$digest" != "$expected_digest" ]; then
> -			tst_res TFAIL "incorrect digest was found for $keyring keyring"
> -			return
> -		fi
> -	done
> +	if [ "$res" = "0" ]; then
> +		tst_res TPASS "specified keyrings were measured correctly"
> +	else
> +		tst_res TFAIL "failed to measure specified keyrings"
> +	fi

Instead of:
       res="$(check_ima_ascii_log_for_policy $policy $tmp_file)"
       if [ "$res" = "0" ]; then

I'd prefer to have it as:
       check_ima_ascii_log_for_policy $policy $tmp_file
       if [ $? -eq 0 ]; then


> -	tst_res TPASS "specified keyrings were measured correctly"
>  }

>  # Create a new keyring, import a certificate into it, and verify
> @@ -97,11 +61,11 @@ test2()
>  	local cert_file="$TST_DATAROOT/x509_ima.der"
>  	local keyring_name="key_import_test"
>  	local pattern="keyrings=[^[:space:]]*$keyring_name"
> -	local temp_file="file.txt"
> +	local temp_file="$TST_TMPDIR/key_import_test_file.txt"

>  	tst_res TINFO "verify measurement of certificate imported into a keyring"

> -	check_keys_policy "$pattern" >/dev/null || return
> +	check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF >/dev/null || return

>  	KEYRING_ID=$(keyctl newring $keyring_name @s) || \
>  		tst_brk TBROK "unable to create a new keyring"
> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> index 1f17aa707..2841d7df5 100644
> --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> @@ -54,6 +54,85 @@ compute_digest()
>  	return 1
>  }

> +check_policy_pattern()
> +{
> +	local pattern="$1"
> +	local func="$2"
> +	local template="$3"
> +
> +	if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then
> +		tst_res TCONF "IMA policy must specify $pattern, $func, $template"
> +		return 1
> +	fi
> +	return 0
> +}
Probably ok for now (yes, it removes the duplicity with function used in two
tests, it's very policy specific).

> +
> +check_ima_ascii_log_for_policy()
> +{
> +	local test_file="$TST_TMPDIR/ascii_log_test_file.txt"
> +	local grep_file="$TST_TMPDIR/ascii_log_grep_file.txt"
nit: Since the real description is in variable, I'd just use:

local test_file="$TST_TMPDIR/test.txt"
local grep_file="$TST_TMPDIR/grep.txt"

> +	local func_lines sources templates i src 
> +	local input_digest_res=1
> +	local policy_option="$1"
> +	local input_digest="$3"

tst_require_cmds cut grep sed xxd
> +
> +	func_lines=$(cat $2)
> +
> +	sources=$(for i in $func_lines; do echo "$i" | grep "$policy_option" | \
> +		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
> +	if [ -z "$sources" ]; then
> +		tst_res TCONF "IMA policy $policy_option is a key-value specifier, but no values specified"
> +		echo "1"
> +		return
> +	fi
> +
> +	templates=$(for i in $func_lines; do echo "$i" | grep "template" | \
> +		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
> +
> +	tst_res TINFO "policy sources: '$sources'"
> +	tst_res TINFO "templates: '$templates'"
> +
> +	grep -E "($templates).*($sources)" $ASCII_MEASUREMENTS > $grep_file
> +
> +	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)
> +		src_line=$(echo "$line" | cut -d' ' -f5)
> +
> +		echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file
> +
> +		if ! expected_digest="$(compute_digest $algorithm $test_file)"; then
> +			tst_res TCONF "cannot compute digest for $algorithm"
> +			echo "1"
> +			return
> +		fi
> +
> +		if [ "$digest" != "$expected_digest" ]; then
> +			tst_res TINFO "incorrect digest was found for $src_line $policy_option"
> +			echo "1"	
> +			return
> +		fi
> +
> +		if [ "$input_digest" ]; then
> +			if [ "$digest" = "$input_digest" ]; then
> +				input_digest_res=0
> +			fi
> +		fi
I'd prefer it as single if:
        if [ -n "$input_digest" -a "$digest" = "$input_digest" ]; then
            input_digest_res=0
        fi

> +
> +	done < $grep_file
> +
> +	if [ "$input_digest" ]; then
> +		echo "$input_digest_res"
> +		return
this return is redundant.
> +	else
> +		echo "0"
> +		return
Also this one.

> +	fi

And actually, instead of whole if/else block wouldn't be just this enough?
echo "$input_digest_res"

Isn't it the zero value set in the loop at:

        if [ -n "$input_digest" -a "$digest" = "$input_digest" ]; then
            input_digest_res=0
        fi

Kind regards,
Petr

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 2/2] IMA: Add test for dm-crypt measurement

[Petr Vorel]

Hi Tushar,

...
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh
> @@ -0,0 +1,60 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2020 Microsoft Corporation
> +# Author: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> +#
> +# Verify that DM target dm-crypt are measured correctly based on policy.
> +
> +TST_NEEDS_CMDS="dmsetup"
> +TST_CNT=1
> +TST_NEEDS_DEVICE=1
> +TST_SETUP=setup
> +TST_CLEANUP=cleanup
> +
> +. ima_setup.sh
> +
> +FUNC_CRIT_DATA='func=CRITICAL_DATA'
> +TEMPLATE_BUF='template=ima-buf'
> +REQUIRED_POLICY="^measure.*($FUNC_CRIT_DATA.*$TEMPLATE_BUF|$TEMPLATE_BUF.*$FUNC_CRIT_DATA)"
> +
> +setup()
> +{
> +	require_ima_policy_content "$REQUIRED_POLICY" '-E' > $TST_TMPDIR/policy.txt
> +}
> +
> +cleanup()
> +{
> +	ROD "dmsetup remove test-crypt"
FYI: there should be check that dmsetup create was run.
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#a-word-about-the-cleanup-callback

i.e. here:

[ "$dmsetup_run" ] || return
ROD "dmsetup remove test-crypt"

+ see below.

> +}
> +
> +test1()
> +{
> +	local input_digest="039d8ff71918608d585adca3e5aab2e3f41f84d6"
> +	local pattern='data_sources=[^[:space:]]+'
> +	local tmp_file="$TST_TMPDIR/dm_crypt_tmp.txt"
> +	local policy="data_sources"
> +	local arg key res
> +
> +	tst_res TINFO "verifying dm target - dmcrypt gets measured correctly"
> +
> +	check_policy_pattern "$pattern" $FUNC_CRIT_DATA $TEMPLATE_BUF > $tmp_file || return
> +
> +	tgt="crypt"
> +	key="faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa"
> +
> +	arg="'0 1953125 crypt aes-xts-plain64 "
> +	arg="$arg $key 0 "
> +	arg="$arg /dev/loop0 0 1 allow_discards'"
> +
> +	ROD "dmsetup create test-crypt --table $arg"
adding 
dmsetup_run=1

Kind regards,
Petr

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 1/2] IMA: generalize key measurement tests

[Tushar Sugandhi]

Hi Petr,

On 2020-12-21 3:05 p.m., Petr Vorel wrote:
> Hi Tushar,
> 
> I'm very sorry about the delay. I'll finish this review in January,
> here just some quick thoughts (minor style nits, I'll fix it before merge).
> 
> Generally LGTM, thanks for your work.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 

Thanks for your review.
My sincere apologies for missing this email and not responding in time.

The device mapper measurement work is being revisited - to cover aspects
like more DM targets (not just dm-crypt), better memory management,
more relevant attributes from the DM targets, other corner cases etc.

Therefore, even though this patch, "1/2: generalize key measurement
tests", would be useful for other tests; I will have to revisit the
second patch, "2/2: dm-crypt measurements", to address the DM side 
changes I mentioned above.

I will revisit this series, esp. testing the DM target measurements
part, once the kernel work I mentioned above is close to completion.

I will also address your feedback on patch #1 and #2 from v2 iteration
at that time.

Thanks again for your review and feedback.

Thanks,
Tushar

>> New functionality is being added in IMA to measure data provided by
>> kernel components. Tests have to be added in LTP to validate this new
>> feature. The functionality in ima_keys.sh can be reused to test this new
>> feature if it is made generic.
> 
>> Refactor check_keys_policy() and test1() implemented in ima_keys.sh to
>> make it generic, and move the functionality to ima_setup.sh as new
>> functions - check_policy_pattern() and check_ima_ascii_log_for_policy().
> 
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   .../security/integrity/ima/tests/ima_keys.sh  | 62 +++------------
>>   .../security/integrity/ima/tests/ima_setup.sh | 79 +++++++++++++++++++
>>   2 files changed, 92 insertions(+), 49 deletions(-)
> 
>> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
>> index c9eef4b68..c2120358a 100755
>> --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
>> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
>> @@ -6,7 +6,7 @@
> 
>>   # Verify that keys are measured correctly based on policy.
> 
>> -TST_NEEDS_CMDS="cmp cut grep sed xxd"
>> +TST_NEEDS_CMDS="cmp cut grep xxd"
> It still requires sed, it's just hidden in check_ima_ascii_log_for_policy
> 
> Maybe just put at the top of check_ima_ascii_log_for_policy():
> tst_require_cmds cut grep sed xxd
> 
> And here still keep
> TST_NEEDS_CMDS="cmp cut grep tail xxd"
> 
> This leads to duplicity in check, but it will not lead to hidden "command not
> found".
> 
>>   TST_CNT=2
>>   TST_NEEDS_DEVICE=1
>>   TST_SETUP=setup
>> @@ -28,64 +28,28 @@ cleanup()
>>   	tst_is_num $KEYRING_ID && keyctl clear $KEYRING_ID
>>   }
> 
>> -check_keys_policy()
>> -{
>> -	local pattern="$1"
>> -
>> -	if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then
>> -		tst_res TCONF "IMA policy must specify $pattern, $FUNC_KEYCHECK, $TEMPLATE_BUF"
>> -		return 1
>> -	fi
>> -	return 0
>> -}
>> -
>>   # Based on https://lkml.org/lkml/2019/12/13/564.
>>   # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys")
> OK, it has been merged in v5.6-rc1. Any more relevant commits, changes since
> then?
> 
>>   test1()
>>   {
>>   	local keycheck_lines i keyrings templates
>>   	local pattern='keyrings=[^[:space:]]+'
>> -	local test_file="file.txt" tmp_file="file2.txt"
>> +	local policy="keyrings"
>> +	local tmp_file="$TST_TMPDIR/keycheck_tmp_file.txt"
>> +	local res
> Will be unused, see below.
> 
>>   	tst_res TINFO "verify key measurement for keyrings and templates specified in IMA policy"
> 
>> -	check_keys_policy "$pattern" > $tmp_file || return
>> -	keycheck_lines=$(cat $tmp_file)
>> -	keyrings=$(for i in $keycheck_lines; do echo "$i" | grep "keyrings" | \
>> -		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
>> -	if [ -z "$keyrings" ]; then
>> -		tst_res TCONF "IMA policy has a keyring key-value specifier, but no specified keyrings"
>> -		return
>> -	fi
>> -
>> -	templates=$(for i in $keycheck_lines; do echo "$i" | grep "template" | \
>> -		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
>> -
>> -	tst_res TINFO "keyrings: '$keyrings'"
>> -	tst_res TINFO "templates: '$templates'"
>> -
>> -	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)
>> -		keyring=$(echo "$line" | cut -d' ' -f5)
>> +	check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF > $tmp_file || return
> 
>> -		echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file
>> +	res="$(check_ima_ascii_log_for_policy $policy $tmp_file)"
> 
>> -		if ! expected_digest="$(compute_digest $algorithm $test_file)"; then
>> -			tst_res TCONF "cannot compute digest for $algorithm"
>> -			return
>> -		fi
>> -
>> -		if [ "$digest" != "$expected_digest" ]; then
>> -			tst_res TFAIL "incorrect digest was found for $keyring keyring"
>> -			return
>> -		fi
>> -	done
>> +	if [ "$res" = "0" ]; then
>> +		tst_res TPASS "specified keyrings were measured correctly"
>> +	else
>> +		tst_res TFAIL "failed to measure specified keyrings"
>> +	fi
> 
> Instead of:
>         res="$(check_ima_ascii_log_for_policy $policy $tmp_file)"
>         if [ "$res" = "0" ]; then
> 
> I'd prefer to have it as:
>         check_ima_ascii_log_for_policy $policy $tmp_file
>         if [ $? -eq 0 ]; then
> 
> 
>> -	tst_res TPASS "specified keyrings were measured correctly"
>>   }
> 
>>   # Create a new keyring, import a certificate into it, and verify
>> @@ -97,11 +61,11 @@ test2()
>>   	local cert_file="$TST_DATAROOT/x509_ima.der"
>>   	local keyring_name="key_import_test"
>>   	local pattern="keyrings=[^[:space:]]*$keyring_name"
>> -	local temp_file="file.txt"
>> +	local temp_file="$TST_TMPDIR/key_import_test_file.txt"
> 
>>   	tst_res TINFO "verify measurement of certificate imported into a keyring"
> 
>> -	check_keys_policy "$pattern" >/dev/null || return
>> +	check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF >/dev/null || return
> 
>>   	KEYRING_ID=$(keyctl newring $keyring_name @s) || \
>>   		tst_brk TBROK "unable to create a new keyring"
>> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
>> index 1f17aa707..2841d7df5 100644
>> --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
>> +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
>> @@ -54,6 +54,85 @@ compute_digest()
>>   	return 1
>>   }
> 
>> +check_policy_pattern()
>> +{
>> +	local pattern="$1"
>> +	local func="$2"
>> +	local template="$3"
>> +
>> +	if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then
>> +		tst_res TCONF "IMA policy must specify $pattern, $func, $template"
>> +		return 1
>> +	fi
>> +	return 0
>> +}
> Probably ok for now (yes, it removes the duplicity with function used in two
> tests, it's very policy specific).
> 
>> +
>> +check_ima_ascii_log_for_policy()
>> +{
>> +	local test_file="$TST_TMPDIR/ascii_log_test_file.txt"
>> +	local grep_file="$TST_TMPDIR/ascii_log_grep_file.txt"
> nit: Since the real description is in variable, I'd just use:
> 
> local test_file="$TST_TMPDIR/test.txt"
> local grep_file="$TST_TMPDIR/grep.txt"
> 
>> +	local func_lines sources templates i src
>> +	local input_digest_res=1
>> +	local policy_option="$1"
>> +	local input_digest="$3"
> 
> tst_require_cmds cut grep sed xxd
>> +
>> +	func_lines=$(cat $2)
>> +
>> +	sources=$(for i in $func_lines; do echo "$i" | grep "$policy_option" | \
>> +		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
>> +	if [ -z "$sources" ]; then
>> +		tst_res TCONF "IMA policy $policy_option is a key-value specifier, but no values specified"
>> +		echo "1"
>> +		return
>> +	fi
>> +
>> +	templates=$(for i in $func_lines; do echo "$i" | grep "template" | \
>> +		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
>> +
>> +	tst_res TINFO "policy sources: '$sources'"
>> +	tst_res TINFO "templates: '$templates'"
>> +
>> +	grep -E "($templates).*($sources)" $ASCII_MEASUREMENTS > $grep_file
>> +
>> +	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)
>> +		src_line=$(echo "$line" | cut -d' ' -f5)
>> +
>> +		echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file
>> +
>> +		if ! expected_digest="$(compute_digest $algorithm $test_file)"; then
>> +			tst_res TCONF "cannot compute digest for $algorithm"
>> +			echo "1"
>> +			return
>> +		fi
>> +
>> +		if [ "$digest" != "$expected_digest" ]; then
>> +			tst_res TINFO "incorrect digest was found for $src_line $policy_option"
>> +			echo "1"	
>> +			return
>> +		fi
>> +
>> +		if [ "$input_digest" ]; then
>> +			if [ "$digest" = "$input_digest" ]; then
>> +				input_digest_res=0
>> +			fi
>> +		fi
> I'd prefer it as single if:
>          if [ -n "$input_digest" -a "$digest" = "$input_digest" ]; then
>              input_digest_res=0
>          fi
> 
>> +
>> +	done < $grep_file
>> +
>> +	if [ "$input_digest" ]; then
>> +		echo "$input_digest_res"
>> +		return
> this return is redundant.
>> +	else
>> +		echo "0"
>> +		return
> Also this one.
> 
>> +	fi
> 
> And actually, instead of whole if/else block wouldn't be just this enough?
> echo "$input_digest_res"
> 
> Isn't it the zero value set in the loop at:
> 
>          if [ -n "$input_digest" -a "$digest" = "$input_digest" ]; then
>              input_digest_res=0
>          fi
> 
> Kind regards,
> Petr
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 1/2] IMA: generalize key measurement tests

[Petr Vorel]

Hi Tushar,

> Thanks for your review.
> My sincere apologies for missing this email and not responding in time.

> The device mapper measurement work is being revisited - to cover aspects
> like more DM targets (not just dm-crypt), better memory management,
> more relevant attributes from the DM targets, other corner cases etc.

> Therefore, even though this patch, "1/2: generalize key measurement
> tests", would be useful for other tests; I will have to revisit the
> second patch, "2/2: dm-crypt measurements", to address the DM side changes I
> mentioned above.

> I will revisit this series, esp. testing the DM target measurements
> part, once the kernel work I mentioned above is close to completion.

> I will also address your feedback on patch #1 and #2 from v2 iteration
> at that time.

> Thanks again for your review and feedback.

I updated your patchset a bit and going to send it to ML.

Kind regards,
Petr

> Thanks,
> Tushar

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 2/2] IMA: Add test for dm-crypt measurement

[Petr Vorel]

Hi Tushar, Mimi, Lakshmi,

> New functionality is being added to IMA to measure data provided by
> kernel components. With this feature, IMA policy can be set to enable
> measuring data provided by device-mapper targets. Currently one such
> device-mapper target - dm-crypt, is being updated to use this
> functionality. This new functionality needs test automation in LTP.

> Add a testcase which verifies that the IMA subsystem correctly measures
> the data coming from a device-mapper target - dm-crypt.

I noticed v10 patchset [1] has been merged, including SELinux commit
fdd1ffe8a812 ("selinux: include a consumer of the new IMA critical data hook"),
thus I merge this patchset.

I suppose fdd1ffe8a812 is the commit which should be mentioned in
ima_dm_crypt.sh, right? (the only change to v3 [2])

Kind regards,
Petr

[1] https://lore.kernel.org/linux-integrity/20210108040708.8389-1-tusharsu@linux.microsoft.com/
[2] https://patchwork.ozlabs.org/project/ltp/list/?series=230766&state=*

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel