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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>


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@xxxxxxxxxxxxxxxxxxx>
---
  .../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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux