IMA: Add test for dm-crypt measurement

PATCH In review Tushar Sugandhi 3 participants 4 messages Aug 29, 2020 – Sep 10, 2020 2 messages not in the archive

Subsystems (2)

Device mapper Keys-trusted

lore

Share & tools

lore

Tushar Sugandhi

author

2020-08-29 01:14 UTC #0 lore raw

PATCH 1/1 IMA: Add test for dm-crypt measurement

IMA subsystem supports measuring data from other kernel components through func=CRITICAL_DATA policy 'critical_kernel_data_sources'. This IMA policy can be set to measure the data coming from device-mapper targets. This scenario needs test coverage.

Add a testcase which verifies that the IMA subsystem correctly measures the data provided by one such DM target - dm-crypt.

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 d012a7190fc1 ("Linux 5.9-rc2")

And the following patch series should be applied in the order below:
1. https://patchwork.kernel.org/patch/11709527/
2. https://patchwork.kernel.org/patch/11742047/
3. https://patchwork.kernel.org/patch/11743265/
4. https://patchwork.kernel.org/patch/11743715/

The kernel code required for this series (the patches above) is still under review. This series will be ready to merge in LTP only after the above patches are merged.

This series is based on the following commit in LTP branch:
commit a277498c08a7 ("IMA/ima_keys.sh: Enhance policy checks")

Signed-off-by Tushar Sugandhi <[email protected]>

---
 runtest/ima                                   |   1 +
 .../kernel/security/integrity/ima/README.md   |  20 +++
 .../integrity/ima/tests/ima_dm_crypt.sh       | 118 ++++++++++++++++++
 3 files changed, 139 insertions(+)
 create mode 100755 testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh

runtest/ima +1 -0

index 5f4b4a7a1..123b6c8b0 100644
@@ -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

testcases/kernel/security/integrity/ima/README.md +20 -0

index 68d046678..663c0b624 100644
@@ -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 critical_kernel_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`

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +118 -0

118 lines not on this page · raw message

Signature · 1 line

2.17.1

A message that is not in the archive (sent off-list or not yet received): 20200829011427.12893-1-tusharsu-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@public.gmane.org

Petr Vorel

2020-09-01 06:07 UTC #2 lore raw

Hi Tushar,

Quoted 4 lines

IMA subsystem supports measuring data from other kernel components through func=CRITICAL_DATA policy 'critical_kernel_data_sources'. This IMA policy can be set to measure the data coming from device-mapper targets. This scenario needs test coverage.

Thank you for your patch.

First, you haven't send this patch to LTP mailing list
(https://lists.linux.it/listinfo/ltp). I Cc it, please do next time.

Quoted 8 lines

Add a testcase which verifies that the IMA subsystem correctly measures the data provided by one such DM target - dm-crypt.
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 d012a7190fc1 ("Linux 5.9-rc2")

Quoted 5 lines

And the following patch series should be applied in the order below:
1. https://patchwork.kernel.org/patch/11709527/
2. https://patchwork.kernel.org/patch/11742047/
3. https://patchwork.kernel.org/patch/11743265/
4. https://patchwork.kernel.org/patch/11743715/

Thanks for a detailed info.

Quoted 30 lines

The kernel code required for this series (the patches above) is still under review. This series will be ready to merge in LTP only after the above patches are merged.
This series is based on the following commit in LTP branch:
commit a277498c08a7 ("IMA/ima_keys.sh: Enhance policy checks")
Signed-off-by Tushar Sugandhi <[email protected]>
---
 runtest/ima                                   |   1 +
 .../kernel/security/integrity/ima/README.md   |  20 +++
 .../integrity/ima/tests/ima_dm_crypt.sh       | 118 ++++++++++++++++++
 3 files changed, 139 insertions(+)
 create mode 100755 testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh
runtest/ima +1 -0
index 5f4b4a7a1..123b6c8b0 100644
@@ -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
testcases/kernel/security/integrity/ima/README.md +0 -0
index 68d046678..663c0b624 100644

testcases/kernel/security/integrity/ima/README.md see example in `kexec.policy`. · line 45

Quoted 9 lines

testcases/kernel/security/integrity/ima/README.md +6 -0

@@ -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 critical_kernel_data_sources=dm-crypt`

Could you please also create ima_dm_crypt.policy file in testcases/kernel/security/integrity/ima/datafiles/ima_dm_crypt/ directory?

Quoted 23 lines

testcases/kernel/security/integrity/ima/README.md +14 -0

+
+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`

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +0 -0

new file mode 100755
index 000000000..ee572ed01

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 8

Quoted 8 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +8 -0

@@ -0,0 +1,118 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2020 Microsoft Corporation
+# Author: Tushar Sugandhi <[email protected]>
+#
+# Verify that DM target dm-crypt is measured correctly based on policy.
+
+TST_NEEDS_CMDS="grep cut sed tr dmsetup"

You don't use tr, and omit xxd (copy paste error).
+ nit: this could be sorted. Thus:
TST_NEEDS_CMDS="cut dmsetup grep sed xxd"

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 21

Quoted 13 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +13 -0

+TST_CNT=1
+TST_NEEDS_DEVICE=1
+TST_SETUP=setup
+
+. 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()
+{
+	tst_res TINFO "inside setup"

This is sort of debugging info. Please remove it.

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 34

Quoted 13 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +13 -0

+	require_ima_policy_content "$REQUIRED_POLICY" '-E' > policy.txt
+}
+
+check_dm_crypt_policy()
+{
+	local pattern="$1"
+
+	if ! grep -E "$pattern" policy.txt; then
+		tst_res TCONF "IMA policy must specify $pattern, $FUNC_CRIT_DATA, $TEMPLATE_BUF"
+		return 1
+	fi
+	return 0
+}

setup() and check_dm_crypt_policy() are the same as check_keys_policy() in ima_keys.sh. We could move $REQUIRED_POLICY handling into ima_setup() in ima_setup.sh. And check_{dm_crypt,keys}_policy() could be also moved to ima_setup.sh as check_test_policy().

NOTE, you need to use $TST_TMPDIR/policy.txt instead of policy.txt: see:
https://patchwork.ozlabs.org/project/ltp/patch/[email protected]/
(I'm going to merge this one today) to fix problems on /tmp on tmpfs. (or simply just add temporary for testing TST_NEEDS_DEVICE=1 to the end of ima_setup.sh).

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 44

Quoted 10 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +10 -0

+
+test1()
+{
+	local dmcheck_lines i dm_targets templates
+	local policy="critical_kernel_data_sources"
+	local pattern='critical_kernel_data_sources=[^[:space:]]+'
+	local tmp_file="tmp.txt"
+	local tokens_file="tokens_file.txt" grep_file="grep_file.txt"
+	local arg cmd key tgt_name
+	local res=0

nit: local res
Later you can check
if [ "$res" = 1 ]; then

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 52

Quoted 8 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +8 -0

+
+	tst_res TINFO "verifying dm target - dmcrypt gets measured correctly."
+
+	check_dm_crypt_policy "$pattern" > $tmp_file || return
+
+	dmcheck_lines=$(cat $tmp_file)
+	dm_targets=$(for i in $dmcheck_lines; do echo "$i" | grep "$policy" | \
+		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')

Again, copy paste from ima_keys.sh. Could this be generalized and moved to ima_setup.sh? See my hint below.

Quoted 12 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +12 -0

+	if [ -z "$dm_targets" ]; then
+		tst_res TCONF "IMA policy has a $policy key-value specifier, but no specified sources."
+		return
+	fi
+
+	templates=$(for i in $dmcheck_lines; do echo "$i" | grep "template" | \
+		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
+
+	tst_res TINFO "dm_targets: '$dm_targets'"
+	tst_res TINFO "templates: '$templates'"
+
+	tgt="crypt"

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 74

Quoted 10 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +10 -0

+	key="faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa"
+
+	arg="'0 1953125 crypt aes-xts-plain64 "
+	arg="$arg $key 0 "
+	arg="$arg /dev/loop0 0 1 allow_discards'"
+	tgt_name="test-crypt"
+	cmd="dmsetup create $tgt_name --table $arg"
+
+	tst_res TINFO "Executing: $cmd"
+	eval $cmd

Please no eval and TINFO. We have ROD() for this:

	ROD dmsetup create $tgt_name --table $arg"

which also prints error if failed.

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 84

Quoted 10 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +10 -0

+
+	grep -E "($templates)*($dm_targets)" $ASCII_MEASUREMENTS > $grep_file
+
+	while read line
+	do
+		local act_digest exp_digest comp_digest algorithm
+
+		act_digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
+		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
+		dmtgt_evtname=$(echo "$line" | cut -d' ' -f5)

Again, whole block is very similar to one from ima_keys.sh. Could this be generalized?

Maybe this function would have callback for verification function?

loop_measurements()

{
	local pattern="$1"
	local target="$2"
	local ver_func="$3"
	shift 3
	local lines line targets

    check_keys_policy "$pattern" > tmp || return
	lines=$(cat tmp)
	targets=$(for i in $lines; do echo "$i" | grep "$target" | \
		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')

	if [ -z "$targets" ]; then
		tst_res TCONF "IMA policy has a $target key-value specifier, but no specified sources."
		return
	fi

	templates=$(for i in $lines; do echo "$i" | grep "template" | \

cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')

	...

	grep -E "($templates).*($targets)" $ASCII_MEASUREMENTS | while read line
	do
	...
	$ver_func $@
	done
}

BTW I plan to send v2 for ima_tpm.sh patch
https://patchwork.ozlabs.org/project/ltp/patch/[email protected]/
Maybe this could use it somehow as well, but not required. Main reason was to use $DIGEST_INDEX (default 4), which would help to use tests also on ima_template_fmt. In the future, when reboot requirement is added into LTP API this could be used to tests more setup. But you can ignore it now.

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 89

Quoted 5 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+
+		echo "$line" | cut -d' ' -f6 | xxd -r -p > $tokens_file
+		plain_text=$(echo "$line" | cut -d' ' -f6 | xxd -r -p)
+
+		#expected digest for $cmd

This is obvious, please remove it.

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 97

Quoted 8 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +8 -0

+		exp_digest="039d8ff71918608d585adca3e5aab2e3f41f84d6"
+		comp_digest="$(compute_digest $algorithm $tokens_file)" || \
+			tst_brk TCONF "cannot compute digest for $algorithm"
+
+		if [ "$act_digest" != "$comp_digest" ]; then
+			tst_res TFAIL "Incorrect digest for ($dmtgt_evtname)."
+			tst_res TFAIL "Expected digest:($comp_digest)."
+			tst_res TFAIL "Actual digest:($act_digest)."

This is wrong. There should be only single tst_res TFAIL message (errors are
counted, thus there would be 3 failures instead of one. Use
tst_res TFAIL "Incorrect digest for ($dmtgt_evtname): '$act_digest' (expected: '$comp_digest')"

(keep it on single line to help grep the message.)
nit: we usually don't put dot at the end of the sentence. And use lower case messages. And you're not consistent (you use both approaches).

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 104

Quoted 7 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +7 -0

+			tst_res TINFO "Removing DM target $tgt_name."
+			dmsetup remove $tgt_name
+			return
+		fi
+
+		if [ "$act_digest" = "$exp_digest" ]; then
+			res=1

Maybe also TFAIL and return here instead of using $res?

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 109

Quoted 5 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+		fi
+
+	done < $grep_file
+
+	if [ $res -eq 1 ]; then

if [ "$res" = 1 ]; then

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 115

Quoted 6 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +6 -0

+		tst_res TPASS "dm-crypt target verification passed."
+	else
+		tst_res TFAIL "dm-crypt target verification failed."
+	fi
+	tst_res TINFO "Removing DM target $tgt_name."
+	dmsetup remove $tgt_name

Duplicity. Please, put it into cleanup function.

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 118

Quoted 3 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +3 -0
+}
+
+tst_run

Kind regards,
Petr

Signature · 1 line

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

Tushar Sugandhi

2020-09-09 21:39 UTC #3 lore raw

Quoted 11 lines · Petr Vorel

Hi Tushar,
Quoted 4 lines
IMA subsystem supports measuring data from other kernel components through func=CRITICAL_DATA policy 'critical_kernel_data_sources'. This IMA policy can be set to measure the data coming from device-mapper targets. This scenario needs test coverage.
Thank you for your patch.
First, you haven't send this patch to LTP mailing list
(https://lists.linux.it/listinfo/ltp). I Cc it, please do next time.

Apologies for my ignorance.
I will include it next time.

Quoted 8 lines

Quoted 7 lines
Add a testcase which verifies that the IMA subsystem correctly measures the data provided by one such DM target - dm-crypt.
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 d012a7190fc1 ("Linux 5.9-rc2")

Quoted 7 lines

Quoted 5 lines
And the following patch series should be applied in the order below:
1. https://patchwork.kernel.org/patch/11709527/
2. https://patchwork.kernel.org/patch/11742047/
3. https://patchwork.kernel.org/patch/11743265/
4. https://patchwork.kernel.org/patch/11743715/
Thanks for a detailed info.

Sure. :)

Quoted 34 lines

Quoted 33 lines
The kernel code required for this series (the patches above) is still under review. This series will be ready to merge in LTP only after the above patches are merged.
This series is based on the following commit in LTP branch:
commit a277498c08a7 ("IMA/ima_keys.sh: Enhance policy checks")
Signed-off-by Tushar Sugandhi <[email protected]>
---
  runtest/ima                                   |   1 +
  .../kernel/security/integrity/ima/README.md   |  20 +++
  .../integrity/ima/tests/ima_dm_crypt.sh       | 118 ++++++++++++++++++
  3 files changed, 139 insertions(+)
create mode 100755 testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh
runtest/ima +1 -0
index 5f4b4a7a1..123b6c8b0 100644
@@ -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
testcases/kernel/security/integrity/ima/README.md +0 -0
index 68d046678..663c0b624 100644
@@ -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>`.

Quoted 9 lines

testcases/kernel/security/integrity/ima/README.md see example in `kexec.policy`. · line 45
Quoted 6 lines
testcases/kernel/security/integrity/ima/README.md +6 -0
+### 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 critical_kernel_data_sources=dm-crypt`
Could you please also create ima_dm_crypt.policy file in testcases/kernel/security/integrity/ima/datafiles/ima_dm_crypt/ directory?

Maybe I should create a directory ‘ima_device_mapper’ rather than ‘ima_dm_crypt’ - because there could be more DM targets like ‘crypt’. And I will place all the dm target policy files there. Does it sound good?

Quoted 26 lines

testcases/kernel/security/integrity/ima/README.md +14 -0

+
+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`

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +3 -0

new file mode 100755
index 000000000..ee572ed01
@@ -0,0 +1,118 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2020 Microsoft Corporation

Quoted 9 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 8
Quoted 5 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+# Author: Tushar Sugandhi <[email protected]>
+#
+# Verify that DM target dm-crypt is measured correctly based on policy.
+
+TST_NEEDS_CMDS="grep cut sed tr dmsetup"
You don't use tr, and omit xxd (copy paste error).
+ nit: this could be sorted. Thus:
TST_NEEDS_CMDS="cut dmsetup grep sed xxd"

Will do. Thanks.

Quoted 6 lines

Quoted 6 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +6 -0
+TST_CNT=1
+TST_NEEDS_DEVICE=1
+TST_SETUP=setup
+
+. ima_setup.sh
+

Quoted 8 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 21
Quoted 7 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +7 -0
+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()
+{
+	tst_res TINFO "inside setup"
This is sort of debugging info. Please remove it.

Will do. I left it there erroneously.

Quoted 10 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +9 -0

+	require_ima_policy_content "$REQUIRED_POLICY" '-E' > policy.txt
+}
+
+check_dm_crypt_policy()
+{
+	local pattern="$1"
+
+	if ! grep -E "$pattern" policy.txt; then
+		tst_res TCONF "IMA policy must specify $pattern, $FUNC_CRIT_DATA, $TEMPLATE_BUF"

Quoted 9 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 34
Quoted 4 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +4 -0
+		return 1
+	fi
+	return 0
+}
setup() and check_dm_crypt_policy() are the same as check_keys_policy() in ima_keys.sh. We could move $REQUIRED_POLICY handling into ima_setup() in ima_setup.sh. And check_{dm_crypt,keys}_policy() could be also moved to ima_setup.sh as check_test_policy().

Will do.

Quoted 6 lines

NOTE, you need to use $TST_TMPDIR/policy.txt instead of policy.txt: see:
https://patchwork.ozlabs.org/project/ltp/patch/[email protected]/
(I'm going to merge this one today) to fix problems on /tmp on tmpfs. (or simply just add temporary for testing TST_NEEDS_DEVICE=1 to the end of ima_setup.sh).

Will do.

Quoted 5 lines

Quoted 5 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+
+test1()
+{
+	local dmcheck_lines i dm_targets templates
+	local policy="critical_kernel_data_sources"

Quoted 9 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 44
Quoted 5 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+	local pattern='critical_kernel_data_sources=[^[:space:]]+'
+	local tmp_file="tmp.txt"
+	local tokens_file="tokens_file.txt" grep_file="grep_file.txt"
+	local arg cmd key tgt_name
+	local res=0
nit: local res
Later you can check
if [ "$res" = 1 ]; then

I believe I do need to initialize res=0.
Please see my comment below.

Quoted 10 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 52
Quoted 8 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +8 -0
+
+	tst_res TINFO "verifying dm target - dmcrypt gets measured correctly."
+
+	check_dm_crypt_policy "$pattern" > $tmp_file || return
+
+	dmcheck_lines=$(cat $tmp_file)
+	dm_targets=$(for i in $dmcheck_lines; do echo "$i" | grep "$policy" | \
+		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
Again, copy paste from ima_keys.sh. Could this be generalized and moved to ima_setup.sh? See my hint below.

Ok. I will see how I can generalize this. Thanks.

Quoted 25 lines

Quoted 13 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +12 -0

+	if [ -z "$dm_targets" ]; then
+		tst_res TCONF "IMA policy has a $policy key-value specifier, but no specified sources."
+		return
+	fi
+
+	templates=$(for i in $dmcheck_lines; do echo "$i" | grep "template" | \
+		cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
+
+	tst_res TINFO "dm_targets: '$dm_targets'"
+	tst_res TINFO "templates: '$templates'"
+
+	tgt="crypt"

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 74

Quoted 10 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +10 -0

+	key="faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa"
+
+	arg="'0 1953125 crypt aes-xts-plain64 "
+	arg="$arg $key 0 "
+	arg="$arg /dev/loop0 0 1 allow_discards'"
+	tgt_name="test-crypt"
+	cmd="dmsetup create $tgt_name --table $arg"
+
+	tst_res TINFO "Executing: $cmd"
+	eval $cmd

Please no eval and TINFO. We have ROD() for this:

Quoted 3 lines

	ROD dmsetup create $tgt_name --table $arg"
which also prints error if failed.

Oh. I wasn’t aware of ROD. Thanks for pointing.
I will use that instead of eval + TINFO.

Quoted 13 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 84
Quoted 11 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +10 -0
+
+	grep -E "($templates)*($dm_targets)" $ASCII_MEASUREMENTS > $grep_file
+
+	while read line
+	do
+		local act_digest exp_digest comp_digest algorithm
+
+		act_digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
+		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
+		dmtgt_evtname=$(echo "$line" | cut -d' ' -f5)
Again, whole block is very similar to one from ima_keys.sh. Could this be generalized?

Will do.

Quoted 33 lines

Maybe this function would have callback for verification function?

loop_measurements()

{
	local pattern="$1"
	local target="$2"
	local ver_func="$3"
	shift 3
	local lines line targets

     check_keys_policy "$pattern" > tmp || return
	lines=$(cat tmp)
	targets=$(for i in $lines; do echo "$i" | grep "$target" | \
		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')

	if [ -z "$targets" ]; then
		tst_res TCONF "IMA policy has a $target key-value specifier, but no specified sources."
		return
	fi

	templates=$(for i in $lines; do echo "$i" | grep "template" | \

cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')

	...

	grep -E "($templates).*($targets)" $ASCII_MEASUREMENTS | while read line
	do
	...
	$ver_func $@
	done
}

Quoted 7 lines

BTW I plan to send v2 for ima_tpm.sh patch
https://patchwork.ozlabs.org/project/ltp/patch/[email protected]/
Maybe this could use it somehow as well, but not required. Main reason was to use $DIGEST_INDEX (default 4), which would help to use tests also on ima_template_fmt. In the future, when reboot requirement is added into LTP API this could be used to tests more setup. But you can ignore it now.

Ok. I will take a look at your v2. But won’t take a dependency on it, for the time being.

Quoted 6 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 89
Quoted 5 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+
+		echo "$line" | cut -d' ' -f6 | xxd -r -p > $tokens_file
+		plain_text=$(echo "$line" | cut -d' ' -f6 | xxd -r -p)
+
+		#expected digest for $cmd
This is obvious, please remove it.

Will do.

Quoted 13 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 97
Quoted 9 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +8 -0
+		exp_digest="039d8ff71918608d585adca3e5aab2e3f41f84d6"
+		comp_digest="$(compute_digest $algorithm $tokens_file)" || \
+			tst_brk TCONF "cannot compute digest for $algorithm"
+
+		if [ "$act_digest" != "$comp_digest" ]; then
+			tst_res TFAIL "Incorrect digest for ($dmtgt_evtname)."
+			tst_res TFAIL "Expected digest:($comp_digest)."
+			tst_res TFAIL "Actual digest:($act_digest)."
This is wrong. There should be only single tst_res TFAIL message (errors are
counted, thus there would be 3 failures instead of one. Use
tst_res TFAIL "Incorrect digest for ($dmtgt_evtname): '$act_digest' (expected: '$comp_digest')"

Ok. Thanks for letting me know. I will stick with a single tst_res TFAIL output.

Quoted 1 line

(keep it on single line to help grep the message.)

Will do.

Quoted 2 lines

nit: we usually don't put dot at the end of the sentence. And use lower case messages. And you're not consistent (you use both approaches).

Will do.

Quoted 9 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 104
Quoted 8 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +7 -0
+			tst_res TINFO "Removing DM target $tgt_name."
+			dmsetup remove $tgt_name
+			return
+		fi
+
+		if [ "$act_digest" = "$exp_digest" ]; then
+			res=1
Maybe also TFAIL and return here instead of using $res?

Thanks for the feedback.
The while loop invokes a sub-shell. And res=1 would mean the hash is
found, and I fail if the hash is *not* found (res=0).
With that, it’s little tricky to do what you are suggesting. But I will see what I can do to simplify the logic.

Quoted 7 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 109
Quoted 6 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+		fi
+
+	done < $grep_file
+
+	if [ $res -eq 1 ]; then
if [ "$res" = 1 ]; then

Will do.

Quoted 8 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 115
Quoted 6 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +6 -0
+		tst_res TPASS "dm-crypt target verification passed."
+	else
+		tst_res TFAIL "dm-crypt target verification failed."
+	fi
+	tst_res TINFO "Removing DM target $tgt_name."
+	dmsetup remove $tgt_name
Duplicity. Please, put it into cleanup function.

Will do.

Quoted 7 lines

testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 118
Quoted 3 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +3 -0
+}
+
+tst_run
Kind regards,
Petr

Signature · 1 line

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

A message that is not in the archive (sent off-list or not yet received): 64b16589-f16a-eff0-47f3-8456853fc205-1pm0nblsJy7Jp67UH1NAhkEOCMrvLtNR@public.gmane.org

Petr Vorel

2020-09-10 07:24 UTC #5 lore raw

Hi Tushar,

Quoted 11 lines

Quoted 8 lines
Quoted 4 lines
IMA subsystem supports measuring data from other kernel components through func=CRITICAL_DATA policy 'critical_kernel_data_sources'. This IMA policy can be set to measure the data coming from device-mapper targets. This scenario needs test coverage.
Thank you for your patch.
First, you haven't send this patch to LTP mailing list
(https://lists.linux.it/listinfo/ltp). I Cc it, please do next time.
Apologies for my ignorance.
I will include it next time.

Thanks! No big deal :).

Quoted 16 lines

Quoted 8 lines
Quoted 7 lines
Add a testcase which verifies that the IMA subsystem correctly measures the data provided by one such DM target - dm-crypt.
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 d012a7190fc1 ("Linux 5.9-rc2")
Quoted 7 lines
Quoted 5 lines
And the following patch series should be applied in the order below:
1. https://patchwork.kernel.org/patch/11709527/
2. https://patchwork.kernel.org/patch/11742047/
3. https://patchwork.kernel.org/patch/11743265/
4. https://patchwork.kernel.org/patch/11743715/
Thanks for a detailed info.
Sure. :)

...

Quoted 9 lines

Quoted 8 lines
testcases/kernel/security/integrity/ima/README.md see example in `kexec.policy`. · line 45
Quoted 6 lines
testcases/kernel/security/integrity/ima/README.md +6 -0
+### 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 critical_kernel_data_sources=dm-crypt`
Could you please also create ima_dm_crypt.policy file in testcases/kernel/security/integrity/ima/datafiles/ima_dm_crypt/ directory?

Quoted 4 lines

Maybe I should create a directory ‘ima_device_mapper’ rather than ‘ima_dm_crypt’ - because there could be more DM targets like ‘crypt’. And I will place all the dm target policy files there. Does it sound good?

The directory must have the same name as the test filename without extension, because that is $TST_ID, see tst_test.sh:

if [ -z "$TST_ID" ]; then

	_tst_filename=$(basename $0) || \
		tst_brk TCONF "Failed to set TST_ID from \$0 ('$0'), fix it with setting TST_ID before sourcing tst_test.sh"
	TST_ID=${_tst_filename%%.*}

fi
export TST_ID="$TST_ID"

...
export TST_DATAROOT="$LTPROOT/testcases/data/$TST_ID"

Unfortunately that's the current limitation for LTP data files.

...

Quoted 13 lines

Quoted 4 lines
Quoted 4 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +4 -0
+test1()
+{
+	local dmcheck_lines i dm_targets templates
+	local policy="critical_kernel_data_sources"
Quoted 8 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 44
Quoted 5 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +5 -0
+	local pattern='critical_kernel_data_sources=[^[:space:]]+'
+	local tmp_file="tmp.txt"
+	local tokens_file="tokens_file.txt" grep_file="grep_file.txt"
+	local arg cmd key tgt_name
+	local res=0
nit: local res
Later you can check
if [ "$res" = 1 ]; then

Quoted 2 lines

I believe I do need to initialize res=0.
Please see my comment below.

I'll reply there.

...

Quoted 6 lines

Quoted 5 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 52
Quoted 3 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +3 -0
+	dmcheck_lines=$(cat $tmp_file)
+	dm_targets=$(for i in $dmcheck_lines; do echo "$i" | grep "$policy" | \
+		sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g')
Again, copy paste from ima_keys.sh. Could this be generalized and moved to ima_setup.sh? See my hint below.
Ok. I will see how I can generalize this. Thanks.

Thanks a lot!

...

Quoted 9 lines

Quoted 6 lines
BTW I plan to send v2 for ima_tpm.sh patch
https://patchwork.ozlabs.org/project/ltp/patch/[email protected]/
Maybe this could use it somehow as well, but not required. Main reason was to use $DIGEST_INDEX (default 4), which would help to use tests also on ima_template_fmt. In the future, when reboot requirement is added into LTP API this could be used to tests more setup. But you can ignore it now.
Ok. I will take a look at your v2. But won’t take a dependency on it, for the time being.

+1 Thanks! (it's really less important than previous generalizations with ima_keys.sh).

...

Quoted 8 lines

Quoted 8 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh line 104
Quoted 7 lines
testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh new file +7 -0
+			tst_res TINFO "Removing DM target $tgt_name."
+			dmsetup remove $tgt_name
+			return
+		fi
+
+		if [ "$act_digest" = "$exp_digest" ]; then
+			res=1
Maybe also TFAIL and return here instead of using $res?

Quoted 5 lines

Thanks for the feedback.
The while loop invokes a sub-shell. And res=1 would mean the hash is found, and I fail if the hash is *not* found (res=0).
With that, it’s little tricky to do what you are suggesting. But I will see what I can do to simplify the logic.

It's just a small detail, you can ignore it. But we started the discussion, thus here is an explanation:
[ "$res" = 1 ] is not true for uninitialized $res. That wouldn't work for [ $res -eq 1 ] and also didn't work [ $res = 1 ] ("bash: [: =: unary operator expected"). The trick is that = operator compares stings but also does numeric evaluation and also "$uninitialized_variable" evaluates as "" (unlike $uninitialized_variable - without quotes).

Thus instead of 0 vs. 1 you can use uninitialized vs 1 (unlike C, where 0 vs 1 must be used e.g. variable must be initialized, unless it's static variable which is automatically initialized to 0). We use it often shell (in LTP), have a look at tst_net.sh and tst_test.sh

Kind regards,
Petr

Signature · 1 line

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