[PATCH] IMA: Allow only ima-buf template for key measurement

[PATCH] IMA: Allow only ima-buf template for key measurement

[Lakshmi Ramasubramanian]

ima-buf is the default IMA template used for all buffer measurements.
Therefore, IMA policy rule for measuring keys need not specify
an IMA template.  But if a template is specified for key measurement
rule then it must be only ima-buf.

Update keys tests to not require a template to be specified for
key measurement rule, but if a template is specified verify it is
only ima-buf.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 .../security/integrity/ima/tests/ima_keys.sh     | 10 +++++++---
 .../security/integrity/ima/tests/ima_setup.sh    | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 3 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..8b214b413 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
@@ -15,8 +15,7 @@ TST_CLEANUP=cleanup
 . ima_setup.sh
 
 FUNC_KEYCHECK='func=KEY_CHECK'
-TEMPLATE_BUF='template=ima-buf'
-REQUIRED_POLICY="^measure.*($FUNC_KEYCHECK.*$TEMPLATE_BUF|$TEMPLATE_BUF.*$FUNC_KEYCHECK)"
+REQUIRED_POLICY="^measure.*$FUNC_KEYCHECK"
 
 setup()
 {
@@ -33,9 +32,10 @@ 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"
+		tst_res TCONF "IMA policy must specify $pattern, $FUNC_KEYCHECK"
 		return 1
 	fi
+
 	return 0
 }
 
@@ -49,6 +49,8 @@ test1()
 
 	tst_res TINFO "verify key measurement for keyrings and templates specified in IMA policy"
 
+	check_policy_template "template=ima-buf" $FUNC_KEYCHECK || return
+
 	check_keys_policy "$pattern" > $tmp_file || return
 	keycheck_lines=$(cat $tmp_file)
 	keyrings=$(for i in $keycheck_lines; do echo "$i" | grep "keyrings" | \
@@ -101,6 +103,8 @@ test2()
 
 	tst_res TINFO "verify measurement of certificate imported into a keyring"
 
+	check_policy_template "template=ima-buf" $FUNC_KEYCHECK || return
+
 	check_keys_policy "$pattern" >/dev/null || return
 
 	KEYRING_ID=$(keyctl newring $keyring_name @s) || \
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index 59a7ffeac..01ebec2b6 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -107,6 +107,22 @@ check_ima_policy_cmdline()
 	return 1
 }
 
+check_policy_template()
+{
+	local template="$1"
+	local func="$2"
+	grep -E "template=" $TST_TMPDIR/policy.txt | while read line
+	do
+		ima_template=$(echo $line | grep $template)
+		if [ -z "$ima_template" ]; then
+			tst_res TCONF "Only $template can be specified for $func"
+			return 1
+		fi
+	done
+
+	return 0
+}
+
 require_ima_policy_cmdline()
 {
 	local policy="$1"
-- 
2.30.0

Re: [PATCH] IMA: Allow only ima-buf template for key measurement

[Petr Vorel]

Hi Lakshmi,

for my record: previous version was
https://patchwork.ozlabs.org/project/ltp/patch/20210222023421.12576-1-nramas@linux.microsoft.com/

> ima-buf is the default IMA template used for all buffer measurements.
> Therefore, IMA policy rule for measuring keys need not specify
> an IMA template.  But if a template is specified for key measurement
> rule then it must be only ima-buf.

> Update keys tests to not require a template to be specified for
> key measurement rule, but if a template is specified verify it is
> only ima-buf.
Good, but there are some issues, see below.

...
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
...
> +	check_policy_template "template=ima-buf" $FUNC_KEYCHECK || return
> +
>  	check_keys_policy "$pattern" > $tmp_file || return
>  	keycheck_lines=$(cat $tmp_file)
>  	keyrings=$(for i in $keycheck_lines; do echo "$i" | grep "keyrings" | \
> @@ -101,6 +103,8 @@ test2()

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

> +	check_policy_template "template=ima-buf" $FUNC_KEYCHECK || return
> +
>  	check_keys_policy "$pattern" >/dev/null || return

>  	KEYRING_ID=$(keyctl newring $keyring_name @s) || \
> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> index 59a7ffeac..01ebec2b6 100644
> --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> @@ -107,6 +107,22 @@ check_ima_policy_cmdline()
>  	return 1
>  }

> +check_policy_template()
> +{
> +	local template="$1"
> +	local func="$2"
> +	grep -E "template=" $TST_TMPDIR/policy.txt | while read line
> +	do
> +		ima_template=$(echo $line | grep $template)
> +		if [ -z "$ima_template" ]; then
instead of putting it into variable, why not just using grep?
if ! echo $line | grep -q $template; then

> +			tst_res TCONF "Only $template can be specified for $func"
> +			return 1
Have you test it? This will not work. There is ${PIPESTATUS[@]} bash/zsh
array, thus 1 is in $pipestatus[1]. But that's bashism, which will not work on
dash busybox ash, ...

You need to do:
while read line; do
	if ! echo $line | grep -q $template; then
		tst_res TCONF "only $template can be specified for $func"
		return 1
	fi
done < $TST_TMPDIR/policy.txt
return 0

*BUT* on vanilla 5.11 with and SLES 5.3.18-47-default with many backports when
testing with this wrong policy:
measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test template=ima-ng


ima_keys 1 TINFO: verify key measurement for keyrings and templates specified in IMA policy
ima_keys 1 TCONF: Only template=ima-buf can be specified for func=KEY_CHECK
ima_keys 1 TINFO: keyrings: '\.ima|\.evm|\.builtin_trusted_keys|\.blacklist|key_import_test'
ima_keys 1 TINFO: templates: 'ima-ng'
ima_keys 1 TPASS: specified keyrings were measured correctly
           ^
first test passes. Why? Is that correct?
I haven't tested any other templates.

ima_keys 2 TINFO: verify measurement of certificate imported into a keyring
ima_keys 2 TCONF: Only template=ima-buf can be specified for func=KEY_CHECK
errno: No such file or directory (2)
ima_keys 2 TBROK: unable to import a certificate into key_import_test keyring

> +		fi
> +	done

Besides that, I'd like to put check_policy_template() into ima_keys.sh because
1) is so far needed only in ima_keys.sh 2) it expects $TST_TMPDIR/policy.txt.
Functions in ima_setup.sh which are used for more tests should not expect any
function was called before.

dm-crypt measurement tests from Tushar Sugandhi will require these, I'll put it
into ima_setup.sh during rebase and probably add policy file as a function parameter.

Kind regards,
Petr

Re: [PATCH] IMA: Allow only ima-buf template for key measurement

[Lakshmi Ramasubramanian]

On 3/5/21 8:15 AM, Petr Vorel wrote:

Hi Petr,

> 
> for my record: previous version was
> https://patchwork.ozlabs.org/project/ltp/patch/20210222023421.12576-1-nramas@linux.microsoft.com/
> 
>> ima-buf is the default IMA template used for all buffer measurements.
>> Therefore, IMA policy rule for measuring keys need not specify
>> an IMA template.  But if a template is specified for key measurement
>> rule then it must be only ima-buf.
> 
>> Update keys tests to not require a template to be specified for
>> key measurement rule, but if a template is specified verify it is
>> only ima-buf.
> Good, but there are some issues, see below.
> 
> ...
>> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> ...
>> +	check_policy_template "template=ima-buf" $FUNC_KEYCHECK || return
>> +
>>   	check_keys_policy "$pattern" > $tmp_file || return
>>   	keycheck_lines=$(cat $tmp_file)
>>   	keyrings=$(for i in $keycheck_lines; do echo "$i" | grep "keyrings" | \
>> @@ -101,6 +103,8 @@ test2()
> 
>>   	tst_res TINFO "verify measurement of certificate imported into a keyring"
> 
>> +	check_policy_template "template=ima-buf" $FUNC_KEYCHECK || return
>> +
>>   	check_keys_policy "$pattern" >/dev/null || return
> 
>>   	KEYRING_ID=$(keyctl newring $keyring_name @s) || \
>> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
>> index 59a7ffeac..01ebec2b6 100644
>> --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
>> +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
>> @@ -107,6 +107,22 @@ check_ima_policy_cmdline()
>>   	return 1
>>   }
> 
>> +check_policy_template()
>> +{
>> +	local template="$1"
>> +	local func="$2"
>> +	grep -E "template=" $TST_TMPDIR/policy.txt | while read line
>> +	do
>> +		ima_template=$(echo $line | grep $template)
>> +		if [ -z "$ima_template" ]; then
> instead of putting it into variable, why not just using grep?
> if ! echo $line | grep -q $template; then
> 
Sure - will make this change.

>> +			tst_res TCONF "Only $template can be specified for $func"
>> +			return 1
> Have you test it? This will not work. There is ${PIPESTATUS[@]} bash/zsh
> array, thus 1 is in $pipestatus[1]. But that's bashism, which will not work on
> dash busybox ash, ...
> 

I tested it by running "/opt/ltp/run -f ima -s keys" to run the keys 
test in IMA. I followed the pattern in check_keys_policy() to check for 
the template and validate the change. I will check this again and 
update. Sorry about that.

> You need to do:
> while read line; do
> 	if ! echo $line | grep -q $template; then
> 		tst_res TCONF "only $template can be specified for $func"
> 		return 1
> 	fi
> done < $TST_TMPDIR/policy.txt
> return 0

Sure - will make this change.

> 
> *BUT* on vanilla 5.11 with and SLES 5.3.18-47-default with many backports when
> testing with this wrong policy:
> measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test template=ima-ng
> 
> 
> ima_keys 1 TINFO: verify key measurement for keyrings and templates specified in IMA policy
> ima_keys 1 TCONF: Only template=ima-buf can be specified for func=KEY_CHECK
> ima_keys 1 TINFO: keyrings: '\.ima|\.evm|\.builtin_trusted_keys|\.blacklist|key_import_test'
> ima_keys 1 TINFO: templates: 'ima-ng'
> ima_keys 1 TPASS: specified keyrings were measured correctly
>             ^
> first test passes. Why? Is that correct?
> I haven't tested any other templates.
No - the test should not pass if an incorrect template is specified. I 
will check this again and update.

> 
> ima_keys 2 TINFO: verify measurement of certificate imported into a keyring
> ima_keys 2 TCONF: Only template=ima-buf can be specified for func=KEY_CHECK
> errno: No such file or directory (2)
> ima_keys 2 TBROK: unable to import a certificate into key_import_test keyring
> 
>> +		fi
>> +	done
> 
> Besides that, I'd like to put check_policy_template() into ima_keys.sh because
> 1) is so far needed only in ima_keys.sh 2) it expects $TST_TMPDIR/policy.txt.
> Functions in ima_setup.sh which are used for more tests should not expect any
> function was called before.
Agreed. I'll move check_policy_template() to ima_keys.sh.

> 
> dm-crypt measurement tests from Tushar Sugandhi will require these, I'll put it
> into ima_setup.sh during rebase and probably add policy file as a function parameter.
That sounds good.

Thanks a lot Petr.

  -lakshmi

Re: [PATCH] IMA: Allow only ima-buf template for key measurement

[Lakshmi Ramasubramanian]

On 3/5/21 8:15 AM, Petr Vorel wrote:

Hi Petr,

A small change is needed:

In the while loop, for each line of the KEY_CHECK policy, we need to 
check if a "template" is specified, and if it is then verify if it is 
"ima-buf".

> You need to do:
> while read line; do
> 	if ! echo $line | grep -q $template; then
> 		tst_res TCONF "only $template can be specified for $func"
> 		return 1
> 	fi
> done < $TST_TMPDIR/policy.txt
> return 0

Please see the change below:

while read line; do
	if echo $line | grep -q 'template=' && ! echo $line | grep -q $template 
; then
		tst_res TCONF "only $template can be specified for $func"
		return 1
	fi
done < $TST_TMPDIR/policy.txt
return 0

With check_policy_template() moved from ima_setup.sh to ima_keys.sh, the 
test works fine

When the policy contains the following
    measure func=KEY_CHECK keyrings=key_import_test template=ima-buf
    measure func=KEY_CHECK keyrings=.builtin_trusted_keys

the test passes:

ima_keys 1 TINFO: verify key measurement for keyrings and templates 
specified in IMA policy
ima_keys 1 TINFO: keyrings: 'key_import_test|\.builtin_trusted_keys'
ima_keys 1 TINFO: templates: 'ima-buf'
ima_keys 1 TPASS: specified keyrings were measured correctly


But if the policy is changed to below:
    measure func=KEY_CHECK keyrings=key_import_test template=ima-buf
    measure func=KEY_CHECK keyrings=.builtin_trusted_keys template=ima-sig

the test fails as expected.

ima_keys 1 TINFO: verify key measurement for keyrings and templates 
specified in IMA policy
ima_keys 1 TCONF: only template=ima-buf can be specified for func=KEY_CHECK

I'll post the updated patch shortly.

thanks,
  -lakshmi

Re: [PATCH] IMA: Allow only ima-buf template for key measurement

[Petr Vorel]

Hi Lakshmi,

> On 3/5/21 8:15 AM, Petr Vorel wrote:

> Hi Petr,

> A small change is needed:

> In the while loop, for each line of the KEY_CHECK policy, we need to check
> if a "template" is specified, and if it is then verify if it is "ima-buf".

> > You need to do:
> > while read line; do
> > 	if ! echo $line | grep -q $template; then
> > 		tst_res TCONF "only $template can be specified for $func"
> > 		return 1
> > 	fi
> > done < $TST_TMPDIR/policy.txt
> > return 0

> Please see the change below:

> while read line; do
> 	if echo $line | grep -q 'template=' && ! echo $line | grep -q $template ;
> then
> 		tst_res TCONF "only $template can be specified for $func"
> 		return 1
> 	fi
> done < $TST_TMPDIR/policy.txt
> return 0
Good catch!

> With check_policy_template() moved from ima_setup.sh to ima_keys.sh, the
> test works fine

> When the policy contains the following
>    measure func=KEY_CHECK keyrings=key_import_test template=ima-buf
>    measure func=KEY_CHECK keyrings=.builtin_trusted_keys

I guess we should update datafiles/ima_keys/keycheck.policy to contain both
lines.

> the test passes:

> ima_keys 1 TINFO: verify key measurement for keyrings and templates
> specified in IMA policy
> ima_keys 1 TINFO: keyrings: 'key_import_test|\.builtin_trusted_keys'
> ima_keys 1 TINFO: templates: 'ima-buf'
> ima_keys 1 TPASS: specified keyrings were measured correctly


> But if the policy is changed to below:
>    measure func=KEY_CHECK keyrings=key_import_test template=ima-buf
>    measure func=KEY_CHECK keyrings=.builtin_trusted_keys template=ima-sig

> the test fails as expected.

> ima_keys 1 TINFO: verify key measurement for keyrings and templates
> specified in IMA policy
> ima_keys 1 TCONF: only template=ima-buf can be specified for func=KEY_CHECK

> I'll post the updated patch shortly.

Thanks a lot!

> thanks,
>  -lakshmi

Kind regards,
Petr