[PATCH v4 1/2] IMA: Fix policy readability check

[PATCH v4 1/2] IMA: Fix policy readability check

[Petr Vorel]

Check with cat, because file attributes were fixed in
ffb122de9a60 ("ima: Reflect correct permissions for policy")
in v4.18.

Added into ima_setup.sh as it'll be used next commit in another test.

Fixes: d2768c84e ("IMA: Add a test to verify measurement of keys")

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/security/integrity/ima/tests/ima_keys.sh | 4 +---
 .../kernel/security/integrity/ima/tests/ima_setup.sh      | 8 ++++++++
 2 files changed, 9 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 904b7515b..26b8eaf84 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
@@ -19,9 +19,7 @@ test1()
 
 	tst_res TINFO "verifying key measurement for keyrings and templates specified in IMA policy file"
 
-	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
-
-	[ -r $IMA_POLICY ] || tst_brk TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)"
+	check_policy_readable
 
 	keycheck_lines=$(grep "func=KEY_CHECK" $IMA_POLICY)
 	if [ -z "$keycheck_lines" ]; then
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index 8ae477c1c..458f67c30 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -111,6 +111,14 @@ print_ima_config()
 	tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
 }
 
+check_policy_readable()
+{
+	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
+
+	cat $IMA_POLICY > /dev/null 2>/dev/null || \
+		tst_brk TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)"
+}
+
 ima_setup()
 {
 	SECURITYFS="$(mount_helper securityfs $SYSFS/kernel/security)"
-- 
2.27.0

[PATCH v4 2/2] IMA: Add test for kexec cmdline measurement

[Petr Vorel]

From: Lachlan Sneff <t-josne@linux.microsoft.com>

IMA policy can be set to measure the command line passed in the kexec
system call. Add a testcase that verifies that the IMA subsystem
correctly measure the cmdline specified during a kexec.

Note that this test does not actually reboot.

Ideally, test shouldn't even require an image, since it doesn't actually
reboot, but the IMA cmdline measurement occurs after the image is parsed
and verified, so we must pass a valid kernel image.

There is a possibility of putting together a dummy kernel image that has
the right headers and appears to be signed correctly, but doesn't
actually contain any code, but, after investigating that possibility, it
appears to be quite difficult (and would require a dummy kernel for each
arch).

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi,

sent version with few my fixes.
@Mimi: could you please have a quick look? (I know you reviewed previous
version.)

Kind regards,
Petr

 runtest/ima                                   |   1 +
 .../kernel/security/integrity/ima/README.md   |  11 ++
 .../security/integrity/ima/tests/ima_kexec.sh | 100 ++++++++++++++++++
 3 files changed, 112 insertions(+)
 create mode 100755 testcases/kernel/security/integrity/ima/tests/ima_kexec.sh

diff --git a/runtest/ima b/runtest/ima
index 309d47420..5f4b4a7a1 100644
--- a/runtest/ima
+++ b/runtest/ima
@@ -4,4 +4,5 @@ ima_policy ima_policy.sh
 ima_tpm ima_tpm.sh
 ima_violations ima_violations.sh
 ima_keys ima_keys.sh
+ima_kexec ima_kexec.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 732cd912f..f28bc8e8c 100644
--- a/testcases/kernel/security/integrity/ima/README.md
+++ b/testcases/kernel/security/integrity/ima/README.md
@@ -36,6 +36,17 @@ CONFIG_SYSTEM_TRUSTED_KEYS="/etc/keys/ima-local-ca.pem"
 
 Test also requires loaded policy with `func=KEY_CHECK`, see example in `keycheck.policy`.
 
+### IMA kexec test
+
+This test requires that the ima policy contains:
+```
+measure func=KEXEC_CMDLINE
+```
+
+Even though the test does not actually reboot, it does require a valid,
+signed kernel image (defined with `$IMA_KEXEC_IMAGE` environment variable,
+default `/boot/vmlinuz-$(uname r)`).
+
 ## 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_kexec.sh b/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
new file mode 100755
index 000000000..15bbca03d
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2020 Microsoft Corporation
+# Author: Lachlan Sneff <t-josne@linux.microsoft.com>
+#
+# Verify that kexec cmdline is measured correctly.
+
+TST_NEEDS_CMDS="grep kexec sed tr xargs"
+TST_CNT=1
+TST_NEEDS_DEVICE=1
+
+. ima_setup.sh
+
+IMA_KEXEC_IMAGE="${IMA_KEXEC_IMAGE:-/boot/vmlinuz-$(uname -r)}"
+
+measure() {
+	local algorithm digest expected_digest found
+	local temp_file="file.txt" temp_file2="file2.txt"
+
+	echo -n "$1" > $temp_file
+	grep "kexec-cmdline" $ASCII_MEASUREMENTS > $temp_file2
+
+	while read found
+	do
+		algorithm=$(echo "$found" | cut -d' ' -f4 | cut -d':' -f1)
+		digest=$(echo "$found" | cut -d' ' -f4 | cut -d':' -f2)
+
+		expected_digest=$(compute_digest $algorithm $temp_file)
+
+		if [ "$digest" = "$expected_digest" ]; then
+			return 0
+		fi
+	done < $temp_file2
+
+	return 1
+}
+
+# Test that the kexec cmdline is measured correctly.
+# NOTE: This does *not* actually reboot.
+test1() {
+	if [ ! -f "$IMA_KEXEC_IMAGE" ]; then
+		tst_brk TCONF "Kernel image not found ('$IMA_KEXEC_IMAGE'), specify it in \$IMA_KEXEC_IMAGE"
+	fi
+
+	# Strip the `BOOT_IMAGE=...` part from the cmdline.
+	local cmdline="$(sed 's/BOOT_IMAGE=[^ ]* //' /proc/cmdline)"
+
+	if ! kexec -sl $IMA_KEXEC_IMAGE --reuse-cmdline; then
+		tst_res TCONF "kexec failed: $?"
+
+		local sb_status="$(bootctl status 2>/dev/null | grep 'Secure Boot' \
+			| tr -d ' ' | sed 's/SecureBoot:*//')"
+
+		if [ "$sb_status" = "enabled" ]; then
+			tst_res TINFO "secure boot is enabled, the specified kernel image may not be signed"
+		fi
+
+		return
+	fi
+
+	kexec -su
+
+	if ! measure "$cmdline"; then
+		tst_res TFAIL "unable to find a correct entry in the IMA log"
+
+		check_policy_readable
+
+		if ! grep "measure func=KEXEC_CMDLINE" $IMA_POLICY >/dev/null; then
+			tst_brk TCONF "The IMA policy does not specify 'measure func=KEXEC_CMDLINE', see IMA test README"
+		fi
+
+		return
+	fi
+
+	cmdline="foo"
+	if ! kexec -sl $IMA_KEXEC_IMAGE --append=$cmdline; then
+		tst_brk TCONF "kexec failed: $?"
+	fi
+
+	kexec -su
+
+	if ! measure "$cmdline"; then
+		tst_brk TFAIL "unable to find a correct entry in the IMA log"
+	fi
+
+	cmdline="bar"
+	if ! kexec -sl $IMA_KEXEC_IMAGE --command-line=$cmdline; then
+		tst_brk TCONF "kexec failed: $?"
+	fi
+
+	kexec -su
+
+	if ! measure "$cmdline"; then
+		tst_brk TFAIL "unable to find a correct entry in the IMA log"
+	fi
+
+	tst_res TPASS "kexec cmdline was measured correctly"
+}
+
+tst_run
-- 
2.27.0

Re: [PATCH v4 2/2] IMA: Add test for kexec cmdline measurement

[Mimi Zohar]

On Fri, 2020-07-24 at 09:00 +0200, Petr Vorel wrote:
> From: Lachlan Sneff <t-josne@linux.microsoft.com>
> 
> IMA policy can be set to measure the command line passed in the kexec
> system call. Add a testcase that verifies that the IMA subsystem
> correctly measure the cmdline specified during a kexec.
> 
> Note that this test does not actually reboot.
> 
> Ideally, test shouldn't even require an image, since it doesn't actually
> reboot, but the IMA cmdline measurement occurs after the image is parsed
> and verified, so we must pass a valid kernel image.
> 
> There is a possibility of putting together a dummy kernel image that has
> the right headers and appears to be signed correctly, but doesn't
> actually contain any code, but, after investigating that possibility, it
> appears to be quite difficult (and would require a dummy kernel for each
> arch).

This test attempts to kexec the existing running kernel image.  To
kexec a different kernel image export IMA_KEXEC_IMAGE=<pathname>.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Hi,
> sent version with few my fixes.
> @Mimi: could you please have a quick look? (I know you reviewed previous
> version.)

Thanks, Petr.  Just a couple of comments ...

 
<snip>

> +# Test that the kexec cmdline is measured correctly.
> +# NOTE: This does *not* actually reboot.
> +test1() {
> +	if [ ! -f "$IMA_KEXEC_IMAGE" ]; then
> +		tst_brk TCONF "Kernel image not found ('$IMA_KEXEC_IMAGE'), specify it in \$IMA_KEXEC_IMAGE"
> +	fi
> +
> +	# Strip the `BOOT_IMAGE=...` part from the cmdline.
> +	local cmdline="$(sed 's/BOOT_IMAGE=[^ ]* //' /proc/cmdline)"
> +
> +	if ! kexec -sl $IMA_KEXEC_IMAGE --reuse-cmdline; then
> +		tst_res TCONF "kexec failed: $?"
> +
> +		local sb_status="$(bootctl status 2>/dev/null | grep 'Secure Boot' \
> +			| tr -d ' ' | sed 's/SecureBoot:*//')"
> +
> +		if [ "$sb_status" = "enabled" ]; then
> +			tst_res TINFO "secure boot is enabled, the specified kernel image may not be signed"
> +		fi

Independently of the secure boot status, the IMA policy itself could require a signature.

For example, a recent software update is preventing one of my test
laptops from booting with secure boot enabled, but the custom IMA
policy still requires the kexec kernel image to be signed.

Search the IMA policy for an appraise "func=KEXEC_KERNEL_CHECK" policy
rule.

> +
> +		return
> +	fi
> +
> +	kexec -su
> +
> +	if ! measure "$cmdline"; then
> +		tst_res TFAIL "unable to find a correct entry in the IMA log"
> +
> +		check_policy_readable
> +
> +		if ! grep "measure func=KEXEC_CMDLINE" $IMA_POLICY >/dev/null; then
> +			tst_brk TCONF "The IMA policy does not specify 'measure func=KEXEC_CMDLINE', see IMA test README"
> +		fi
> +

Other than the policy "action" - measure/dont_measure,
audit/dont_audit, appraise/dont_appraise - being the first keyword,
the ordering of the policy options and flags is flexible.  Most
policies do provide the "func" option immediately following the
"action".  This would normally work.


> +		return
> +	fi
> +
> +	cmdline="foo"
> +	if ! kexec -sl $IMA_KEXEC_IMAGE --append=$cmdline; then
> +		tst_brk TCONF "kexec failed: $?"
> +	fi
> +

The error messages are exactly the same here and below.  Should a hint
be provided as to which one failed?

Mimi

> +	kexec -su
> +
> +	if ! measure "$cmdline"; then
> +		tst_brk TFAIL "unable to find a correct entry in the IMA log"
> +	fi
> +
> +	cmdline="bar"
> +	if ! kexec -sl $IMA_KEXEC_IMAGE --command-line=$cmdline; then
> +		tst_brk TCONF "kexec failed: $?"
> +	fi
> +
> +	kexec -su
> +
> +	if ! measure "$cmdline"; then
> +		tst_brk TFAIL "unable to find a correct entry in the IMA log"
> +	fi
> +
> +	tst_res TPASS "kexec cmdline was measured correctly"
> +}
> +
> +tst_run

Re: [PATCH v4 2/2] IMA: Add test for kexec cmdline measurement

[Petr Vorel]

Hi all,

> On Fri, 2020-07-24 at 09:00 +0200, Petr Vorel wrote:
> > From: Lachlan Sneff <t-josne@linux.microsoft.com>

> > IMA policy can be set to measure the command line passed in the kexec
> > system call. Add a testcase that verifies that the IMA subsystem
> > correctly measure the cmdline specified during a kexec.

> > Note that this test does not actually reboot.

> > Ideally, test shouldn't even require an image, since it doesn't actually
> > reboot, but the IMA cmdline measurement occurs after the image is parsed
> > and verified, so we must pass a valid kernel image.

> > There is a possibility of putting together a dummy kernel image that has
> > the right headers and appears to be signed correctly, but doesn't
> > actually contain any code, but, after investigating that possibility, it
> > appears to be quite difficult (and would require a dummy kernel for each
> > arch).
Maybe I'll omit these 2 paragraphs from commit message.

> This test attempts to kexec the existing running kernel image.  To
> kexec a different kernel image export IMA_KEXEC_IMAGE=<pathname>.
+1, that's a correct description for all test description, commit message and
README.md. Thanks!

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > Hi,
> > sent version with few my fixes.
> > @Mimi: could you please have a quick look? (I know you reviewed previous
> > version.)

> Thanks, Petr.  Just a couple of comments ...


> <snip>

> > +# Test that the kexec cmdline is measured correctly.
> > +# NOTE: This does *not* actually reboot.
> > +test1() {
> > +	if [ ! -f "$IMA_KEXEC_IMAGE" ]; then
> > +		tst_brk TCONF "Kernel image not found ('$IMA_KEXEC_IMAGE'), specify it in \$IMA_KEXEC_IMAGE"
> > +	fi
> > +
> > +	# Strip the `BOOT_IMAGE=...` part from the cmdline.
> > +	local cmdline="$(sed 's/BOOT_IMAGE=[^ ]* //' /proc/cmdline)"
> > +
> > +	if ! kexec -sl $IMA_KEXEC_IMAGE --reuse-cmdline; then
> > +		tst_res TCONF "kexec failed: $?"
I overlooked that TCONF is bad, I guess it should be TFAIL (failure of something
what we test).

Meaning of TCONF/TBROK/TFAIL is described in
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#222-basic-test-interface
IMHO only missing kernel image ($IMA_KEXEC_IMAGE) and checking
func=KEXEC_CMDLINE in loaded policy should be marked as TCONF, the rest should
be TFAIL. TBROK is used for unexpected errors in preparation phase (often this
code is in setup).

Thus here should be:
tst_res TFAIL "kexec failed: $?"

> > +
> > +		local sb_status="$(bootctl status 2>/dev/null | grep 'Secure Boot' \
> > +			| tr -d ' ' | sed 's/SecureBoot:*//')"
> > +
> > +		if [ "$sb_status" = "enabled" ]; then
> > +			tst_res TINFO "secure boot is enabled, the specified kernel image may not be signed"
> > +		fi

> Independently of the secure boot status, the IMA policy itself could require a signature.

> For example, a recent software update is preventing one of my test
> laptops from booting with secure boot enabled, but the custom IMA
> policy still requires the kexec kernel image to be signed.

> Search the IMA policy for an appraise "func=KEXEC_KERNEL_CHECK" policy
> rule.
I guess you mean keep to keep current bootctl status based search

That also mean, that check_policy_readable needs to be searched earlier.
Probably just to set:
TST_SETUP="check_policy_readable"

> > +
> > +		return
> > +	fi
> > +
> > +	kexec -su
> > +
> > +	if ! measure "$cmdline"; then
> > +		tst_res TFAIL "unable to find a correct entry in the IMA log"
> > +
> > +		check_policy_readable
> > +
> > +		if ! grep "measure func=KEXEC_CMDLINE" $IMA_POLICY >/dev/null; then
> > +			tst_brk TCONF "The IMA policy does not specify 'measure func=KEXEC_CMDLINE', see IMA test README"
> > +		fi
> > +

> Other than the policy "action" - measure/dont_measure,
> audit/dont_audit, appraise/dont_appraise - being the first keyword,
> the ordering of the policy options and flags is flexible.  Most
> policies do provide the "func" option immediately following the
> "action".  This would normally work.
OK, is this correct?
if ! grep '^measure.*func=KEXEC_CMDLINE' $IMA_POLICY >/dev/null; then

> > +		return
> > +	fi
> > +
> > +	cmdline="foo"
> > +	if ! kexec -sl $IMA_KEXEC_IMAGE --append=$cmdline; then
> > +		tst_brk TCONF "kexec failed: $?"
> > +	fi
> > +

Instead of whole block please use:
EXPECT_PASS_BRK kexec -sl $IMA_KEXEC_IMAGE --append=$cmdline


> The error messages are exactly the same here and below.  Should a hint
> be provided as to which one failed?
+1, thanks for spotting this. Using EXPECT_PASS_BRK will solve this (commands
are different).

> Mimi

> > +	kexec -su
Maybe, if we expect kexec to exit 0, we could run it with ROD (that exit test
with tst_brk TBROK if non-zero exit code)
ROD kexec -su

FYI we don't use ROD everywhere, but where hidden failure of some command
complicates debugging failing test or even lead to wrong test result (false
positive or negative).

> > +
> > +	if ! measure "$cmdline"; then
> > +		tst_brk TFAIL "unable to find a correct entry in the IMA log"
> > +	fi
> > +
> > +	cmdline="bar"
> > +	if ! kexec -sl $IMA_KEXEC_IMAGE --command-line=$cmdline; then
EXPECT_PASS_BRK kexec -sl $IMA_KEXEC_IMAGE --command-line=$cmdline
> > +		tst_brk TCONF "kexec failed: $?"
> > +	fi
> > +
> > +	kexec -su
And here as well.

> > +
> > +	if ! measure "$cmdline"; then
> > +		tst_brk TFAIL "unable to find a correct entry in the IMA log"
> > +	fi
> > +
> > +	tst_res TPASS "kexec cmdline was measured correctly"
> > +}
> > +
> > +tst_run

@Mimi, @Lachlan: if you both ack my changes, I can fix the code before merging.
If there are more changes needed and thus Lachlan plan new version, please use
my patches from https://patchwork.ozlabs.org/project/ltp/list/?series=191990&state=*
(download https://patchwork.ozlabs.org/series/191990/mbox/ and import it with "git am")
or clone my github fork and use kexec.v3.fixes branch:
https://github.com/pevik/ltp/tree/ima/kexec.v3.fixes).

Kind regards,
Petr

Re: [PATCH v4 2/2] IMA: Add test for kexec cmdline measurement

[Mimi Zohar]

On Fri, 2020-07-24 at 15:18 +0200, Petr Vorel wrote:

> 
> > Other than the policy "action" - measure/dont_measure,
> > audit/dont_audit, appraise/dont_appraise - being the first keyword,
> > the ordering of the policy options and flags is flexible.  Most
> > policies do provide the "func" option immediately following the
> > "action".  This would normally work.
> OK, is this correct?
> if ! grep '^measure.*func=KEXEC_CMDLINE' $IMA_POLICY >/dev/null; then

Yes, that works.

> 
> @Mimi, @Lachlan: if you both ack my changes, I can fix the code before merging.

That's fine. Feel free to add my Reviewed-by.

Mimi

> If there are more changes needed and thus Lachlan plan new version, please use
> my patches from https://patchwork.ozlabs.org/project/ltp/list/?series=191990&state=*
> (download https://patchwork.ozlabs.org/series/191990/mbox/ and import it with "git am")
> or clone my github fork and use kexec.v3.fixes branch:
> https://github.com/pevik/ltp/tree/ima/kexec.v3.fixes).

Re: [PATCH v4 2/2] IMA: Add test for kexec cmdline measurement

[Lachlan Sneff]

Hi Petr,

On 7/24/20 9:18 AM, Petr Vorel wrote:
> Hi all,
>
>> On Fri, 2020-07-24 at 09:00 +0200, Petr Vorel wrote:
>>> From: Lachlan Sneff <t-josne@linux.microsoft.com>
>>> IMA policy can be set to measure the command line passed in the kexec
>>> system call. Add a testcase that verifies that the IMA subsystem
>>> correctly measure the cmdline specified during a kexec.
>>> Note that this test does not actually reboot.
>>>
>>>
>>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>>> Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
>>> Signed-off-by: Petr Vorel <pvorel@suse.cz>
>>> ---
>>> Hi,
>>> sent version with few my fixes.
>>> @Mimi: could you please have a quick look? (I know you reviewed previous
>>> version.)
> @Mimi, @Lachlan: if you both ack my changes, I can fix the code before merging.
> If there are more changes needed and thus Lachlan plan new version, please use
> my patches from https://patchwork.ozlabs.org/project/ltp/list/?series=191990&state=*
> (download https://patchwork.ozlabs.org/series/191990/mbox/ and import it with "git am")
> or clone my github fork and use kexec.v3.fixes branch:
> https://github.com/pevik/ltp/tree/ima/kexec.v3.fixes).
Hi Petr, these changes look good to me. I'm not exactly sure the 
convention here,
since I originally wrote the patch, but feel free to mark me as 
Signed-off-by.
>
> Kind regards,
> Petr
Thank you for working on this!
Lachlan

Re: [PATCH v4 2/2] IMA: Add test for kexec cmdline measurement

[Petr Vorel]

Hi Mimi, Lachlan,

...
Thanks both for ack of the changes, I'll merge it on Monday.

> Hi Petr, these changes look good to me. I'm not exactly sure the convention
> here,
> since I originally wrote the patch, but feel free to mark me as
> Signed-off-by.
Of course I'll merge with you as the author of the patch :).

> Thank you for working on this!
> Lachlan

Kind regards,
Petr