[PATCH v2 0/8] LTP tests: load predefined policy, enhancements

[PATCH v2 0/8] LTP tests: load predefined policy, enhancements

[Petr Vorel]

Changes v1->v2:
* Removed ont_measure fsmagic=0x1021994 from TCB example policy
* More reasons to fail when uploading policy
  (testcases/kernel/security/integrity/ima/README.md)
* New commits:
  - tst_test.sh: IMA: Allow to disable LSM warnings and use it for IMA
  - ima_setup: Print warning when policy not readable
  - ima_kexec.sh: Move checking policy if readable to ima_setup.sh
  - IMA: Add example policy for ima_violations.sh
  - ima_violations.sh: Check for a required policy
  - [RFC] ima_kexec.sh: Relax result on unreadable policy to TCONF

TODO:
* ima_measurements.sh: check for example policy as an variant to
  ima_policy=tcb command line parameter.
* Use LTP shell loader for ima_boot_aggregate.c and ima_mmap.c


Petr Vorel (8):
  IMA: Add TCB policy as an example for ima_measurements.sh
  ima_setup.sh: Allow to load predefined policy
  tst_test.sh: IMA: Allow to disable LSM warnings and use it for IMA
  ima_setup: Print warning when policy not readable
  ima_kexec.sh: Move checking policy if readable to ima_setup.sh
  IMA: Add example policy for ima_violations.sh
  ima_violations.sh: Check for a required policy
  [RFC] ima_kexec.sh: Relax result on unreadable policy to TCONF

 .../kernel/security/integrity/ima/README.md   | 12 ++++
 .../ima/datafiles/ima_measurements/tcb.policy | 19 +++++
 .../ima_violations/violations.policy          |  1 +
 .../security/integrity/ima/tests/ima_kexec.sh | 10 +--
 .../integrity/ima/tests/ima_measurements.sh   | 17 ++++-
 .../security/integrity/ima/tests/ima_setup.sh | 72 ++++++++++++++++---
 .../integrity/ima/tests/ima_violations.sh     |  5 +-
 testcases/lib/tst_test.sh                     |  2 +-
 8 files changed, 118 insertions(+), 20 deletions(-)
 create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy
 create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy

-- 
2.47.1

[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh

[Petr Vorel]

Taken from IMA docs [1], removed dont_measure fsmagic=0x1021994 (tmpfs)
as suggested by Mimi.

[1] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#ima-tcb

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
I would like to check in ima_measurements.sh for this policy as an
variant to ima_policy=tcb command line parameter. Do I need to check for
all of these (suppose all are in ima_policy=tcb).

 .../ima/datafiles/ima_measurements/tcb.policy | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy

diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy
new file mode 100644
index 0000000000..1c919f7260
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy
@@ -0,0 +1,19 @@
+dont_measure fsmagic=0x9fa0
+dont_measure fsmagic=0x62656572
+dont_measure fsmagic=0x64626720
+dont_measure fsmagic=0x1cd1
+dont_measure fsmagic=0x42494e4d
+dont_measure fsmagic=0x73636673
+dont_measure fsmagic=0xf97cff8c
+dont_measure fsmagic=0x43415d53
+dont_measure fsmagic=0x27e0eb
+dont_measure fsmagic=0x63677270
+dont_measure fsmagic=0x6e736673
+dont_measure fsmagic=0xde5e81e4
+measure func=MMAP_CHECK mask=MAY_EXEC
+measure func=BPRM_CHECK mask=MAY_EXEC
+measure func=FILE_CHECK mask=^MAY_READ euid=0
+measure func=FILE_CHECK mask=^MAY_READ uid=0
+measure func=MODULE_CHECK
+measure func=FIRMWARE_CHECK
+measure func=POLICY_CHECK
-- 
2.47.1

[PATCH v2 2/8] ima_setup.sh: Allow to load predefined policy

[Petr Vorel]

environment variable LTP_IMA_LOAD_POLICY=1 tries to load example policy
if available. This should be used only if tooling running LTP tests
allows to reboot afterwards because policy may be writable only once,
e.g. missing CONFIG_IMA_WRITE_POLICY=y, or policies can influence each
other.

Loading may fail due various reasons (e.g. previously mentioned missing
CONFIG_IMA_WRITE_POLICY=y and policy already loaded or when secure boot is
enabled and the kernel is configured with CONFIG_IMA_ARCH_POLICY enabled, an
appraise func=POLICY_CHECK appraise_type=imasig rule is loaded, requiring the
IMA policy itself to be signed).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/security/integrity/ima/README.md   | 12 +++++
 .../integrity/ima/tests/ima_measurements.sh   | 17 +++++-
 .../security/integrity/ima/tests/ima_setup.sh | 54 ++++++++++++++++---
 3 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/testcases/kernel/security/integrity/ima/README.md b/testcases/kernel/security/integrity/ima/README.md
index 5b261a1914..c5b3db1a5a 100644
--- a/testcases/kernel/security/integrity/ima/README.md
+++ b/testcases/kernel/security/integrity/ima/README.md
@@ -8,6 +8,18 @@ CONFIG_INTEGRITY=y
 CONFIG_IMA=y
 ```
 
+### Loading policy for testing (optional)
+Setting environment variable `LTP_IMA_LOAD_POLICY=1` tries to load example
+policy if available. This should be used only if tooling running LTP tests
+allows to reboot afterwards because policy may be writable only once, e.g.
+missing `CONFIG_IMA_WRITE_POLICY=y`, or policies can influence each other.
+
+Loading may fail due various reasons (e.g. previously mentioned missing
+`CONFIG_IMA_WRITE_POLICY=y` and policy already loaded or when secure boot is
+enabled and the kernel is configured with `CONFIG_IMA_ARCH_POLICY` enabled, an
+`appraise func=POLICY_CHECK appraise_type=imasig` rule is loaded, requiring the
+IMA policy itself to be signed).
+
 ### IMA measurement tests
 `ima_measurements.sh` require builtin IMA tcb policy to be loaded
 (`ima_policy=tcb` kernel parameter).
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
index 1da2aa6a51..2c95aeb990 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2009 IBM Corporation
-# Copyright (c) 2018-2021 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
 # Author: Mimi Zohar <zohar@linux.ibm.com>
 #
 # Verify that measurements are added to the measurement list based on policy.
@@ -12,10 +12,23 @@ TST_CNT=3
 
 setup()
 {
-	require_ima_policy_cmdline "tcb"
+	local policy="tcb"
 
 	TEST_FILE="$PWD/test.txt"
 	[ -f "$IMA_POLICY" ] || tst_res TINFO "not using default policy"
+
+	if [ "$LTP_IMA_LOAD_POLICY" != 1 ]; then
+		require_ima_policy_cmdline $policy
+		return
+	elif check_ima_policy_cmdline $policy; then
+		return
+	fi
+
+	if ! check_ima_policy_cmdline $policy &&
+		! require_ima_policy_content '^measure func=FILE_CHECK mask=^MAY_READ uid=0' &&
+		! require_ima_policy_content 'measure func=POLICY_CHECK'; then
+			tst_brk TCONF "IMA measurement tests require builtin IMA $policy policy (e.g. ima_policy=$policy kernel parameter) or it's equivalent (try LTP_IMA_LOAD_POLICY=1)"
+	fi
 }
 
 check_iversion_support()
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index df3fc5603f..7afb1a0967 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2009 IBM Corporation
-# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
 # Author: Mimi Zohar <zohar@linux.ibm.com>
 
 TST_TESTFUNC="test"
@@ -72,14 +72,20 @@ require_policy_readable()
 	fi
 }
 
-require_policy_writable()
+check_policy_writable()
 {
-	local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
-
-	[ -f $IMA_POLICY ] || tst_brk TCONF "$err"
-	# CONFIG_IMA_READ_POLICY
+	[ -f $IMA_POLICY ] || return 1
+	# workaround for kernels < v4.18 without fix
+	# ffb122de9a60b ("ima: Reflect correct permissions for policy")
 	echo "" 2> log > $IMA_POLICY
-	grep -q "Device or resource busy" log && tst_brk TCONF "$err"
+	grep -q "Device or resource busy" log && return 1
+	return 0
+}
+
+require_policy_writable()
+{
+	check_policy_writable || tst_brk TCONF \
+		"IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
 }
 
 check_ima_policy_content()
@@ -158,6 +164,34 @@ print_ima_config()
 	tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
 }
 
+load_ima_policy()
+{
+	local policy="$(ls $TST_DATAROOT/*.policy 2>/dev/null)"
+
+	if [ "$LTP_IMA_LOAD_POLICY" != 1 -a "$policy" -a -f "$policy" ]; then
+		tst_res TINFO "NOTE: set LTP_IMA_LOAD_POLICY=1 to load policy for this test"
+		return
+	fi
+
+	if [ -z "$policy" -o ! -f "$policy" ]; then
+		tst_res TINFO "no policy for this test"
+		LTP_IMA_LOAD_POLICY=
+		return
+	fi
+
+	tst_res TINFO "trying to load '$policy' policy:"
+	cat $policy
+	if ! check_policy_writable; then
+		tst_res TINFO "WARNING: IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y), reboot required"
+		LTP_IMA_LOAD_POLICY=
+		return
+	fi
+
+	cat "$policy" 2> log > $IMA_POLICY
+	if grep -q "Device or resource busy" log; then
+		tst_brk TBROK "Loading policy failed"
+	fi
+}
 ima_setup()
 {
 	SECURITYFS="$(mount_helper securityfs $SYSFS/kernel/security)"
@@ -180,6 +214,8 @@ ima_setup()
 		cd "$TST_MNTPOINT"
 	fi
 
+	load_ima_policy
+
 	[ -n "$TST_SETUP_CALLER" ] && $TST_SETUP_CALLER
 }
 
@@ -192,6 +228,10 @@ ima_cleanup()
 	for dir in $UMOUNT; do
 		umount $dir
 	done
+
+	if [ "$LTP_IMA_LOAD_POLICY" = 1 ]; then
+		tst_res TINFO "WARNING: policy loaded via LTP_IMA_LOAD_POLICY=1, reboot recommended"
+	fi
 }
 
 set_digest_index()
-- 
2.47.1

[PATCH v2 3/8] tst_test.sh: IMA: Allow to disable LSM warnings and use it for IMA

[Petr Vorel]

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
@Cyril: or should we use the opposite approach - by default unused and
declare tests where should be used? I guess tests for typical userspace
tools should use it (e.g. runtest/commands or tests which use
tst_net.sh).

 testcases/kernel/security/integrity/ima/tests/ima_setup.sh | 1 +
 testcases/lib/tst_test.sh                                  | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index 7afb1a0967..cf769ac751 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -11,6 +11,7 @@ TST_CLEANUP_CALLER="$TST_CLEANUP"
 TST_CLEANUP="ima_cleanup"
 TST_NEEDS_ROOT=1
 TST_MOUNT_DEVICE=1
+TST_SKIP_LSM_WARNINGS=1
 
 # TST_MOUNT_DEVICE can be unset, therefore specify explicitly
 TST_NEEDS_TMPDIR=1
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index cfdae02300..3e03a1717f 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -81,7 +81,7 @@ _tst_do_exit()
 	fi
 
 	if [ $TST_BROK -gt 0 -o $TST_FAIL -gt 0 -o $TST_WARN -gt 0 ]; then
-		_tst_check_security_modules
+		[ -z "$TST_SKIP_LSM_WARNINGS" ] && _tst_check_security_modules
 	fi
 
 	cat >&2 << EOF
-- 
2.47.1

[PATCH v2 4/8] ima_setup: Print warning when policy not readable

[Petr Vorel]

check_ima_policy_content() now prints TINFO message when policy is not
readable and it does not return 1 in this case. Therefore
"'func=KEXEC_KERNEL_CHECK' appraise policy loaded, kernel image may not
be signed" TWARN message in ima_kexec.sh is not printed when policy is
not readable.

This is better because in previous case test always failed due TWARN but
result is actually unknown (e.g. don't expect missing policy, return 1
as failure only when policy is readable and checking with grep failed).

Fixes: 3843e2d6fb ("IMA: Add policy related helpers")
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/security/integrity/ima/tests/ima_setup.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index cf769ac751..e958dd3334 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -94,8 +94,11 @@ check_ima_policy_content()
 	local pattern="$1"
 	local grep_params="${2--q}"
 
-	check_policy_readable || return 1
-	grep $grep_params "$pattern" $IMA_POLICY
+	if check_policy_readable; then
+		grep $grep_params "$pattern" $IMA_POLICY
+	else
+		tst_res TINFO "WARNING: policy not readable, can't check policy for '$pattern' (possible false positives)"
+	fi
 }
 
 require_ima_policy_content()
-- 
2.47.1

[PATCH v2 5/8] ima_kexec.sh: Move checking policy if readable to ima_setup.sh

[Petr Vorel]

It will be reused.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/security/integrity/ima/tests/ima_kexec.sh   |  8 ++------
 .../kernel/security/integrity/ima/tests/ima_setup.sh   | 10 ++++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh b/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
index 3446bc24bf..df8658655d 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
@@ -47,10 +47,7 @@ setup()
 		tst_brk TCONF "kernel image not found, specify path in \$IMA_KEXEC_IMAGE"
 	fi
 
-	if check_policy_readable; then
-		require_ima_policy_content "$REQUIRED_POLICY"
-		policy_readable=1
-	fi
+	require_ima_policy_content_if_readable "$REQUIRED_POLICY"
 }
 
 kexec_failure_hint()
@@ -97,8 +94,7 @@ kexec_test()
 
 	ROD kexec -su
 	if ! measure "$cmdline"; then
-		if [ "$policy_readable" != 1 ]; then
-			tst_res TWARN "policy not readable, it might not contain required policy '$REQUIRED_POLICY'"
+		if ! check_policy_readable; then
 			res=TBROK
 		fi
 		tst_brk $res "unable to find a correct measurement"
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index e958dd3334..9a05a31c31 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -101,6 +101,16 @@ check_ima_policy_content()
 	fi
 }
 
+require_ima_policy_content_if_readable()
+{
+	local pattern="$1"
+	local grep_params="${2--q}"
+
+	if ! check_ima_policy_content "$pattern" "$grep_params"; then
+		tst_brk TCONF "IMA policy does not specify '$pattern'"
+	fi
+}
+
 require_ima_policy_content()
 {
 	local pattern="$1"
-- 
2.47.1

[PATCH v2 6/8] IMA: Add example policy for ima_violations.sh

[Petr Vorel]

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../integrity/ima/datafiles/ima_violations/violations.policy     | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy

diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
new file mode 100644
index 0000000000..5734c7617f
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
@@ -0,0 +1 @@
+func=FILE_CHECK
-- 
2.47.1

[PATCH v2 7/8] ima_violations.sh: Check for a required policy

[Petr Vorel]

Add check for ^func=FILE_CHECK'

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

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/security/integrity/ima/tests/ima_violations.sh    | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
index 0f710dea2e..73b9fe6f30 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2009 IBM Corporation
-# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
 # Author: Mimi Zohar <zohar@linux.ibm.com>
 #
 # Test whether ToMToU and open_writer violations invalidatethe PCR and are logged.
@@ -9,6 +9,7 @@
 TST_SETUP="setup"
 TST_CLEANUP="cleanup"
 TST_CNT=3
+REQUIRED_POLICY='^func=FILE_CHECK'
 
 setup()
 {
@@ -17,6 +18,8 @@ setup()
 	LOG="/var/log/messages"
 	PRINTK_RATE_LIMIT=
 
+	require_ima_policy_content_if_readable "$REQUIRED_POLICY"
+
 	if status_daemon auditd; then
 		LOG="/var/log/audit/audit.log"
 	elif tst_check_cmds sysctl; then
-- 
2.47.1

[PATCH v2 8/8] [RFC] ima_kexec.sh: Relax result on unreadable policy to TCONF

[Petr Vorel]

Although d1e29adca6, which set minimal version fixed some false
positives, it might be better to be optimistic and exit with TCONF
when result is unknown due policy not being readable than "fail" with
TBROK and TWARN.

Fixes: 731aae8121 ("IMA: Add test for kexec cmdline measurement")
Reported-by: Martin Doucha <mdoucha@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/security/integrity/ima/tests/ima_kexec.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh b/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
index df8658655d..c52d767fe7 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_kexec.sh
@@ -95,7 +95,7 @@ kexec_test()
 	ROD kexec -su
 	if ! measure "$cmdline"; then
 		if ! check_policy_readable; then
-			res=TBROK
+			res=TCONF
 		fi
 		tst_brk $res "unable to find a correct measurement"
 	fi
-- 
2.47.1

Re: [PATCH v2 2/8] ima_setup.sh: Allow to load predefined policy

[Mimi Zohar]

Hi Petr,

On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
[snip]

> --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> @@ -1,7 +1,7 @@
>  #!/bin/sh
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  # Copyright (c) 2009 IBM Corporation
> -# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
> +# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
>  # Author: Mimi Zohar <zohar@linux.ibm.com>
>  
>  TST_TESTFUNC="test"
> @@ -72,14 +72,20 @@ require_policy_readable()
>  	fi
>  }
>  
> -require_policy_writable()
> +check_policy_writable()
>  {
> -	local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
> -
> -	[ -f $IMA_POLICY ] || tst_brk TCONF "$err"
> -	# CONFIG_IMA_READ_POLICY
> +	[ -f $IMA_POLICY ] || return 1
> +	# workaround for kernels < v4.18 without fix
> +	# ffb122de9a60b ("ima: Reflect correct permissions for policy")
>  	echo "" 2> log > $IMA_POLICY
> -	grep -q "Device or resource busy" log && tst_brk TCONF "$err"
> +	grep -q "Device or resource busy" log && return 1
> +	return 0
> +}
> +
> +require_policy_writable()
> +{
> +	check_policy_writable || tst_brk TCONF \
> +		"IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
>  }
>  
>  check_ima_policy_content()
> @@ -158,6 +164,34 @@ print_ima_config()
>  	tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
>  }
>  
> +load_ima_policy()
> +{
> +	local policy="$(ls $TST_DATAROOT/*.policy 2>/dev/null)"
> +
> +	if [ "$LTP_IMA_LOAD_POLICY" != 1 -a "$policy" -a -f "$policy" ]; then
> +		tst_res TINFO "NOTE: set LTP_IMA_LOAD_POLICY=1 to load policy for this test"
> +		return
> +	fi
> +
> +	if [ -z "$policy" -o ! -f "$policy" ]; then
> +		tst_res TINFO "no policy for this test"
> +		LTP_IMA_LOAD_POLICY=
> +		return
> +	fi
> +
> +	tst_res TINFO "trying to load '$policy' policy:"
> +	cat $policy
> +	if ! check_policy_writable; then
> +		tst_res TINFO "WARNING: IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y), reboot required"
> +		LTP_IMA_LOAD_POLICY=
> +		return
> +	fi
> +
> +	cat "$policy" 2> log > $IMA_POLICY
> +	if grep -q "Device or resource busy" log; then
> +		tst_brk TBROK "Loading policy failed"
> +	fi

To write to the IMA securityfs policy file, check_policy_writable() used "echo",
while here it's using "cat".  "cat" fails when signed policies are required.
Perhaps add something like:
+
+       if grep -q "write error: Permission denied" log; then
+               tst_brk TBROK "Loading unsigned policy failed"
+       fi

> +}

Mimi

Re: [PATCH v2 7/8] ima_violations.sh: Check for a required policy

[Mimi Zohar]

Hi Petr,

On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> Add check for ^func=FILE_CHECK'
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  .../kernel/security/integrity/ima/tests/ima_violations.sh    | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> index 0f710dea2e..73b9fe6f30 100755
> --- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> @@ -1,7 +1,7 @@
>  #!/bin/sh
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  # Copyright (c) 2009 IBM Corporation
> -# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
> +# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
>  # Author: Mimi Zohar <zohar@linux.ibm.com>
>  #
>  # Test whether ToMToU and open_writer violations invalidatethe PCR and are logged.
> @@ -9,6 +9,7 @@
>  TST_SETUP="setup"
>  TST_CLEANUP="cleanup"
>  TST_CNT=3
> +REQUIRED_POLICY='^func=FILE_CHECK'

The first field of an IMA policy rule is the 'action', followed by the
condition. Use "func=FILE_CHECK" instead.

thanks,

Mimi

>  
>  setup()
>  {
> @@ -17,6 +18,8 @@ setup()
>  	LOG="/var/log/messages"
>  	PRINTK_RATE_LIMIT=
>  
> +	require_ima_policy_content_if_readable "$REQUIRED_POLICY"
> +
>  	if status_daemon auditd; then
>  		LOG="/var/log/audit/audit.log"
>  	elif tst_check_cmds sysctl; then

Re: [PATCH v2 6/8] IMA: Add example policy for ima_violations.sh

[Mimi Zohar]

Hi Petr,

On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  .../integrity/ima/datafiles/ima_violations/violations.policy     | 1 +
>  1 file changed, 1 insertion(+)
>  create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> 
> diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> new file mode 100644
> index 0000000000..5734c7617f
> --- /dev/null
> +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> @@ -0,0 +1 @@
> +func=FILE_CHECK

"[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh"
contains two rules to measure files opened by root on file open.

measure func=FILE_CHECK mask=^MAY_READ euid=0
measure func=FILE_CHECK mask=^MAY_READ uid=0

If the 'tcb' or equivalent policy is loaded, there is no need to load another
policy rule. 

Thanks,

Mimi

Re: [PATCH v2 2/8] ima_setup.sh: Allow to load predefined policy

[Petr Vorel]

> Hi Petr,

> On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> [snip]

> > --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > @@ -1,7 +1,7 @@
> >  #!/bin/sh
> >  # SPDX-License-Identifier: GPL-2.0-or-later
> >  # Copyright (c) 2009 IBM Corporation
> > -# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
> > +# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
> >  # Author: Mimi Zohar <zohar@linux.ibm.com>

> >  TST_TESTFUNC="test"
> > @@ -72,14 +72,20 @@ require_policy_readable()
> >  	fi
> >  }

> > -require_policy_writable()
> > +check_policy_writable()
> >  {
> > -	local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
> > -
> > -	[ -f $IMA_POLICY ] || tst_brk TCONF "$err"
> > -	# CONFIG_IMA_READ_POLICY
> > +	[ -f $IMA_POLICY ] || return 1
> > +	# workaround for kernels < v4.18 without fix
> > +	# ffb122de9a60b ("ima: Reflect correct permissions for policy")
> >  	echo "" 2> log > $IMA_POLICY
> > -	grep -q "Device or resource busy" log && tst_brk TCONF "$err"
> > +	grep -q "Device or resource busy" log && return 1
> > +	return 0
> > +}
> > +
> > +require_policy_writable()
> > +{
> > +	check_policy_writable || tst_brk TCONF \
> > +		"IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
> >  }

> >  check_ima_policy_content()
> > @@ -158,6 +164,34 @@ print_ima_config()
> >  	tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
> >  }

> > +load_ima_policy()
> > +{
> > +	local policy="$(ls $TST_DATAROOT/*.policy 2>/dev/null)"
> > +
> > +	if [ "$LTP_IMA_LOAD_POLICY" != 1 -a "$policy" -a -f "$policy" ]; then
> > +		tst_res TINFO "NOTE: set LTP_IMA_LOAD_POLICY=1 to load policy for this test"
> > +		return
> > +	fi
> > +
> > +	if [ -z "$policy" -o ! -f "$policy" ]; then
> > +		tst_res TINFO "no policy for this test"
> > +		LTP_IMA_LOAD_POLICY=
> > +		return
> > +	fi
> > +
> > +	tst_res TINFO "trying to load '$policy' policy:"
> > +	cat $policy
> > +	if ! check_policy_writable; then
> > +		tst_res TINFO "WARNING: IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y), reboot required"
> > +		LTP_IMA_LOAD_POLICY=
> > +		return
> > +	fi
> > +
> > +	cat "$policy" 2> log > $IMA_POLICY
> > +	if grep -q "Device or resource busy" log; then
> > +		tst_brk TBROK "Loading policy failed"
> > +	fi

> To write to the IMA securityfs policy file, check_policy_writable() used "echo",
> while here it's using "cat".  "cat" fails when signed policies are required.
> Perhaps add something like:
> +
> +       if grep -q "write error: Permission denied" log; then
> +               tst_brk TBROK "Loading unsigned policy failed"
> +       fi

+1, I'll add this extra check to v3.

I suppose echo "" > /sys/kernel/security/ima/policy does not need this check.

Do I understand correctly you talk about policy containing func=POLICY_CHECK [1]?
Maybe there could be a test based on example [2].

echo /home/user/tmpfile > /sys/kernel/security/ima/policy
cp tmpfile /sys/kernel/security/ima/policy
cat tmpfile > /sys/kernel/security/ima/policy

Kind regards,
Petr

[1] https://ima-doc.readthedocs.io/en/latest/policy-syntax.html#func-policy-check
[2] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#runtime-custom-policy

> > +}

> Mimi

Re: [PATCH v2 6/8] IMA: Add example policy for ima_violations.sh

[Petr Vorel]

> Hi Petr,

> On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  .../integrity/ima/datafiles/ima_violations/violations.policy     | 1 +
> >  1 file changed, 1 insertion(+)
> >  create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy

> > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > new file mode 100644
> > index 0000000000..5734c7617f
> > --- /dev/null
> > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > @@ -0,0 +1 @@
> > +func=FILE_CHECK

> "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh"
> contains two rules to measure files opened by root on file open.

> measure func=FILE_CHECK mask=^MAY_READ euid=0
> measure func=FILE_CHECK mask=^MAY_READ uid=0

My bad of course "func=FILE_CHECK" is not enough. Thanks for providing a correct
example policy (required part of 'tcb' policy).

> If the 'tcb' or equivalent policy is loaded, there is no need to load another
> policy rule. 

Yes, I'll fix the next commit to avoid loading example policy when
ima_policy=tcb.

Kind regards,
Petr

> Thanks,

> Mimi

Re: [PATCH v2 6/8] IMA: Add example policy for ima_violations.sh

[Petr Vorel]

Hi Mimi,

> Hi Petr,

> On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  .../integrity/ima/datafiles/ima_violations/violations.policy     | 1 +
> >  1 file changed, 1 insertion(+)
> >  create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy

> > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > new file mode 100644
> > index 0000000000..5734c7617f
> > --- /dev/null
> > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > @@ -0,0 +1 @@
> > +func=FILE_CHECK

> "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh"
> contains two rules to measure files opened by root on file open.

> measure func=FILE_CHECK mask=^MAY_READ euid=0
> measure func=FILE_CHECK mask=^MAY_READ uid=0

> If the 'tcb' or equivalent policy is loaded, there is no need to load another
> policy rule. 

I guess I'll move check for builtin policy loaded via kernel command line
parameter also to ima_setup.sh to avoid loading example policy when there is a
required builtin policy loaded. I also wonder what is a common approach - don't
try to load custom example policy when there is builtin policy loaded?

My goal was to allow more broad IMA testing based on different setup:

* running tests with ima_policy=tcb builtin policy (current approach). Many
tests will be skipped due missing required policy content.
* running tests without any builtin policy + load a custom policy + reboot via
LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only
if required (or even none) builtin policy is loaded.
* Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it
(but then it is hard to detect whether failures are real bugs or just false
positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF if
policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and
custom policy with proper content was not loaded).

But you may have an idea what is more useful (brings more test coverage).

Kind regards,
Petr

> Thanks,

> Mimi

Re: [PATCH v2 2/8] ima_setup.sh: Allow to load predefined policy

[Mimi Zohar]

On Tue, 2024-12-31 at 11:00 +0100, Petr Vorel wrote:
> > Hi Petr,
> 
> > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > [snip]
> 
> > > --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > > @@ -1,7 +1,7 @@
> > >  #!/bin/sh
> > >  # SPDX-License-Identifier: GPL-2.0-or-later
> > >  # Copyright (c) 2009 IBM Corporation
> > > -# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
> > > +# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
> > >  # Author: Mimi Zohar <zohar@linux.ibm.com>
> 
> > >  TST_TESTFUNC="test"
> > > @@ -72,14 +72,20 @@ require_policy_readable()
> > >  	fi
> > >  }
> 
> > > -require_policy_writable()
> > > +check_policy_writable()
> > >  {
> > > -	local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
> > > -
> > > -	[ -f $IMA_POLICY ] || tst_brk TCONF "$err"
> > > -	# CONFIG_IMA_READ_POLICY
> > > +	[ -f $IMA_POLICY ] || return 1
> > > +	# workaround for kernels < v4.18 without fix
> > > +	# ffb122de9a60b ("ima: Reflect correct permissions for policy")
> > >  	echo "" 2> log > $IMA_POLICY
> > > -	grep -q "Device or resource busy" log && tst_brk TCONF "$err"
> > > +	grep -q "Device or resource busy" log && return 1
> > > +	return 0
> > > +}
> > > +
> > > +require_policy_writable()
> > > +{
> > > +	check_policy_writable || tst_brk TCONF \
> > > +		"IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
> > >  }
> 
> > >  check_ima_policy_content()
> > > @@ -158,6 +164,34 @@ print_ima_config()
> > >  	tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
> > >  }
> 
> > > +load_ima_policy()
> > > +{
> > > +	local policy="$(ls $TST_DATAROOT/*.policy 2>/dev/null)"
> > > +
> > > +	if [ "$LTP_IMA_LOAD_POLICY" != 1 -a "$policy" -a -f "$policy" ]; then
> > > +		tst_res TINFO "NOTE: set LTP_IMA_LOAD_POLICY=1 to load policy for this test"
> > > +		return
> > > +	fi
> > > +
> > > +	if [ -z "$policy" -o ! -f "$policy" ]; then
> > > +		tst_res TINFO "no policy for this test"
> > > +		LTP_IMA_LOAD_POLICY=
> > > +		return
> > > +	fi
> > > +
> > > +	tst_res TINFO "trying to load '$policy' policy:"
> > > +	cat $policy
> > > +	if ! check_policy_writable; then
> > > +		tst_res TINFO "WARNING: IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y), reboot required"
> > > +		LTP_IMA_LOAD_POLICY=
> > > +		return
> > > +	fi
> > > +
> > > +	cat "$policy" 2> log > $IMA_POLICY
> > > +	if grep -q "Device or resource busy" log; then
> > > +		tst_brk TBROK "Loading policy failed"
> > > +	fi
> 
> > To write to the IMA securityfs policy file, check_policy_writable() used "echo",
> > while here it's using "cat".  "cat" fails when signed policies are required.
> > Perhaps add something like:
> > +
> > +       if grep -q "write error: Permission denied" log; then
> > +               tst_brk TBROK "Loading unsigned policy failed"
> > +       fi
> 
> +1, I'll add this extra check to v3.
> 
> I suppose echo "" > /sys/kernel/security/ima/policy does not need this check.

The original method for loading an IMA policy was by cat'ing the policy rules. 
Commit 7429b092811f ("ima: load policy using path") introduced the ability of
verifying the integrity of the policy itself.

echo <policy filepath>  > /sys/kernel/security/ima/policy

> 
> Do I understand correctly you talk about policy containing func=POLICY_CHECK [1]?

Yes.  On a secure boot enabled system, the architecture specific policy might
require the IMA policy itself to be signed.

Snippet from ima_fs.c:

#if IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) &&
IS_ENABLED(CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY)
        "appraise func=POLICY_CHECK appraise_type=imasig",
#endif

> Maybe there could be a test based on example [2].
> 
> echo /home/user/tmpfile > /sys/kernel/security/ima/policy
> cp tmpfile /sys/kernel/security/ima/policy
> cat tmpfile > /sys/kernel/security/ima/policy

All of the above will load a policy, assuming the policy itself doesn't need to
be signed.  Only "echo /home/user/tmpfile > /sys/kernel/security/ima/policy" can
load a signed policy.

Loading a CA key (mokutil), signing (evmctl)[1] and loading (keyctl) an IMA
policy is probably beyond LTP.  The purpose of this test would be to detect
whether policies need to be signed.

Going forward what's probably needed is a new package containing a set of pre-
defined sample custom policies, which are signed by the distro.

[1] Directions for signing and loading a custom policy,
https://ima-doc.readthedocs.io/en/latest/ima-utilities.html#sign-and-install-a-custom-policy

Thanks,

Mimi

> 
> Kind regards,
> Petr
> 
> [1] https://ima-doc.readthedocs.io/en/latest/policy-syntax.html#func-policy-check
> [2] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#runtime-custom-policy
> 
> > > +}
> 
> > Mimi
> 
> 

Re: [PATCH v2 6/8] IMA: Add example policy for ima_violations.sh

[Mimi Zohar]

On Tue, 2024-12-31 at 13:23 +0100, Petr Vorel wrote:
> Hi Mimi,
> 
> > Hi Petr,
> 
> > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > ---
> > >  .../integrity/ima/datafiles/ima_violations/violations.policy     | 1 +
> > >  1 file changed, 1 insertion(+)
> > >  create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> 
> > > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > > new file mode 100644
> > > index 0000000000..5734c7617f
> > > --- /dev/null
> > > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > > @@ -0,0 +1 @@
> > > +func=FILE_CHECK
> 
> > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh"
> > contains two rules to measure files opened by root on file open.
> 
> > measure func=FILE_CHECK mask=^MAY_READ euid=0
> > measure func=FILE_CHECK mask=^MAY_READ uid=0
> 
> > If the 'tcb' or equivalent policy is loaded, there is no need to load another
> > policy rule. 
> 
> I guess I'll move check for builtin policy loaded via kernel command line
> parameter also to ima_setup.sh to avoid loading example policy when there is a
> required builtin policy loaded.
> 

Between the builtin and arch specific policies, most of the rules are already
defined.  The exception is measuring the boot command line.  Perhaps we should
update the arch specific policy to include it with the other kexec rules.

The arch specific policy may include the rule that requires the IMA policy to be
signed.

> I also wonder what is a common approach - don't
> try to load custom example policy when there is builtin policy loaded?

How about first checking if the rule exists when there is a builtin or
equivalent custom policy loaded, before loading the example test policy?

> 
> My goal was to allow more broad IMA testing based on different setup:

Very good.
> 
> * running tests with ima_policy=tcb builtin policy (current approach). Many
> tests will be skipped due missing required policy content.

Ok.  Remember even with "ima_policy=tcb" specified on the boot command line, the
results will differ depending on whether the arch specific policy is loaded.

> * running tests without any builtin policy + load a custom policy + reboot via
> LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only
> if required (or even none) builtin policy is loaded.

Good.  The first patch introduces the equivalent custom policy to
"ima_policy=tcb".  By "load a custom policy" are you referring to this policy or
a specific policy test rule?

> * Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it
> (but then it is hard to detect whether failures are real bugs or just false
> positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF if
> policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and
> custom policy with proper content was not loaded).

Probably the latter option of converting from TBROK/TFAIL to TCONF is
preferable.  Why fail a test without knowing it will fail.

> But you may have an idea what is more useful (brings more test coverage).

There are two main problems:
- Not being able to read the policy.
- Only being able to load a signed policy.

I think between your above ordering and a new test to see if the policy needs to
be signed, it's the best we can do for now.

As mentioned in my 2/8 response, a new package containing pre-defined custom
policies that are signed by the distro would resolve the latter problem.


Thanks,

Mimi

Re: [PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh

[Mimi Zohar]

Hi Petr,

On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> Taken from IMA docs [1], removed dont_measure fsmagic=0x1021994 (tmpfs)
> as suggested by Mimi.
> 
> [1] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#ima-tcb
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>

After thinking about it some more, anyone interested in constraining the
"measure func=FILE_CHECK" rules based on LSM labels to avoid integrity
violations would need to reboot the system anyway. [1]

For this reason, please include the new dont_measure tmpfs rule as proposed in
"[PATCH] ima: limit the builtin 'tcb' dont_measure tmpfs policy rule". [2]

[1] Integrity violations -
https://ima-doc.readthedocs.io/en/latest/event-log-format.html#template-data-hash

[2]
https://lore.kernel.org/linux-integrity/20241230142333.1309623-2-zohar@linux.ibm.com/

thanks,

Mimi

> ---
> I would like to check in ima_measurements.sh for this policy as an
> variant to ima_policy=tcb command line parameter. Do I need to check for
> all of these (suppose all are in ima_policy=tcb).
> 
>  .../ima/datafiles/ima_measurements/tcb.policy | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy
> 
> diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy
> new file mode 100644
> index 0000000000..1c919f7260
> --- /dev/null
> +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_measurements/tcb.policy
> @@ -0,0 +1,19 @@
> +dont_measure fsmagic=0x9fa0
> +dont_measure fsmagic=0x62656572
> +dont_measure fsmagic=0x64626720
> +dont_measure fsmagic=0x1cd1
> +dont_measure fsmagic=0x42494e4d
> +dont_measure fsmagic=0x73636673
> +dont_measure fsmagic=0xf97cff8c
> +dont_measure fsmagic=0x43415d53
> +dont_measure fsmagic=0x27e0eb
> +dont_measure fsmagic=0x63677270
> +dont_measure fsmagic=0x6e736673
> +dont_measure fsmagic=0xde5e81e4
> +measure func=MMAP_CHECK mask=MAY_EXEC
> +measure func=BPRM_CHECK mask=MAY_EXEC
> +measure func=FILE_CHECK mask=^MAY_READ euid=0
> +measure func=FILE_CHECK mask=^MAY_READ uid=0
> +measure func=MODULE_CHECK
> +measure func=FIRMWARE_CHECK
> +measure func=POLICY_CHECK

Re: [PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh

[Petr Vorel]

Hi Mimi,

> Hi Petr,

> On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > Taken from IMA docs [1], removed dont_measure fsmagic=0x1021994 (tmpfs)
> > as suggested by Mimi.

> > [1] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#ima-tcb

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

> After thinking about it some more, anyone interested in constraining the
> "measure func=FILE_CHECK" rules based on LSM labels to avoid integrity
> violations would need to reboot the system anyway. [1]

> For this reason, please include the new dont_measure tmpfs rule as proposed in
> "[PATCH] ima: limit the builtin 'tcb' dont_measure tmpfs policy rule". [2]

Sure, I'll add in v3:

dont_measure fsmagic=0x1021994 func=FILE_CHECK

Kind regards,
Petr

> [1] Integrity violations -
> https://ima-doc.readthedocs.io/en/latest/event-log-format.html#template-data-hash

> [2]
> https://lore.kernel.org/linux-integrity/20241230142333.1309623-2-zohar@linux.ibm.com/

> thanks,

> Mimi

Re: [PATCH v2 2/8] ima_setup.sh: Allow to load predefined policy

[Petr Vorel]

Hi Mimi,

> On Tue, 2024-12-31 at 11:00 +0100, Petr Vorel wrote:
> > > Hi Petr,

> > > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > > [snip]

> > > > --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > > > @@ -1,7 +1,7 @@
> > > >  #!/bin/sh
> > > >  # SPDX-License-Identifier: GPL-2.0-or-later
> > > >  # Copyright (c) 2009 IBM Corporation
> > > > -# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
> > > > +# Copyright (c) 2018-2024 Petr Vorel <pvorel@suse.cz>
> > > >  # Author: Mimi Zohar <zohar@linux.ibm.com>

> > > >  TST_TESTFUNC="test"
> > > > @@ -72,14 +72,20 @@ require_policy_readable()
> > > >  	fi
> > > >  }

> > > > -require_policy_writable()
> > > > +check_policy_writable()
> > > >  {
> > > > -	local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
> > > > -
> > > > -	[ -f $IMA_POLICY ] || tst_brk TCONF "$err"
> > > > -	# CONFIG_IMA_READ_POLICY
> > > > +	[ -f $IMA_POLICY ] || return 1
> > > > +	# workaround for kernels < v4.18 without fix
> > > > +	# ffb122de9a60b ("ima: Reflect correct permissions for policy")
> > > >  	echo "" 2> log > $IMA_POLICY
> > > > -	grep -q "Device or resource busy" log && tst_brk TCONF "$err"
> > > > +	grep -q "Device or resource busy" log && return 1
> > > > +	return 0
> > > > +}
> > > > +
> > > > +require_policy_writable()
> > > > +{
> > > > +	check_policy_writable || tst_brk TCONF \
> > > > +		"IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)"
> > > >  }

> > > >  check_ima_policy_content()
> > > > @@ -158,6 +164,34 @@ print_ima_config()
> > > >  	tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
> > > >  }

> > > > +load_ima_policy()
> > > > +{
> > > > +	local policy="$(ls $TST_DATAROOT/*.policy 2>/dev/null)"
> > > > +
> > > > +	if [ "$LTP_IMA_LOAD_POLICY" != 1 -a "$policy" -a -f "$policy" ]; then
> > > > +		tst_res TINFO "NOTE: set LTP_IMA_LOAD_POLICY=1 to load policy for this test"
> > > > +		return
> > > > +	fi
> > > > +
> > > > +	if [ -z "$policy" -o ! -f "$policy" ]; then
> > > > +		tst_res TINFO "no policy for this test"
> > > > +		LTP_IMA_LOAD_POLICY=
> > > > +		return
> > > > +	fi
> > > > +
> > > > +	tst_res TINFO "trying to load '$policy' policy:"
> > > > +	cat $policy
> > > > +	if ! check_policy_writable; then
> > > > +		tst_res TINFO "WARNING: IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y), reboot required"
> > > > +		LTP_IMA_LOAD_POLICY=
> > > > +		return
> > > > +	fi
> > > > +
> > > > +	cat "$policy" 2> log > $IMA_POLICY
> > > > +	if grep -q "Device or resource busy" log; then
> > > > +		tst_brk TBROK "Loading policy failed"
> > > > +	fi

> > > To write to the IMA securityfs policy file, check_policy_writable() used "echo",
> > > while here it's using "cat".  "cat" fails when signed policies are required.
> > > Perhaps add something like:
> > > +
> > > +       if grep -q "write error: Permission denied" log; then
> > > +               tst_brk TBROK "Loading unsigned policy failed"
> > > +       fi

> > +1, I'll add this extra check to v3.

> > I suppose echo "" > /sys/kernel/security/ima/policy does not need this check.

> The original method for loading an IMA policy was by cat'ing the policy rules. 
> Commit 7429b092811f ("ima: load policy using path") introduced the ability of
> verifying the integrity of the policy itself.

> echo <policy filepath>  > /sys/kernel/security/ima/policy

Thanks, I completely missed this already quite old method (v4.6).

I guess I could use

cat < /dev/null > /sys/kernel/security/ima/policy

instead of echo "" > /sys/kernel/security/ima/policy

Then "write error: Permission denied" check would not be needed, right?

> > Do I understand correctly you talk about policy containing func=POLICY_CHECK [1]?

> Yes.  On a secure boot enabled system, the architecture specific policy might
> require the IMA policy itself to be signed.

> Snippet from ima_fs.c:

> #if IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) &&
> IS_ENABLED(CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY)
>         "appraise func=POLICY_CHECK appraise_type=imasig",
> #endif

+1

> > Maybe there could be a test based on example [2].

> > echo /home/user/tmpfile > /sys/kernel/security/ima/policy
> > cp tmpfile /sys/kernel/security/ima/policy
> > cat tmpfile > /sys/kernel/security/ima/policy

> All of the above will load a policy, assuming the policy itself doesn't need to
> be signed.  Only "echo /home/user/tmpfile > /sys/kernel/security/ima/policy" can
> load a signed policy.

> Loading a CA key (mokutil), signing (evmctl)[1] and loading (keyctl) an IMA
> policy is probably beyond LTP.  The purpose of this test would be to detect
> whether policies need to be signed.

> Going forward what's probably needed is a new package containing a set of pre-
> defined sample custom policies, which are signed by the distro.

> [1] Directions for signing and loading a custom policy,
> https://ima-doc.readthedocs.io/en/latest/ima-utilities.html#sign-and-install-a-custom-policy

Hopefully I find time to do some experiments with it soon.

Kind regards,
Petr

> Thanks,

> Mimi


> > Kind regards,
> > Petr

> > [1] https://ima-doc.readthedocs.io/en/latest/policy-syntax.html#func-policy-check
> > [2] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#runtime-custom-policy

> > > > +}

> > > Mimi

Re: [PATCH v2 2/8] ima_setup.sh: Allow to load predefined policy

[Petr Vorel]

Hi Mimi,

...
> > Do I understand correctly you talk about policy containing func=POLICY_CHECK [1]?

> Yes.  On a secure boot enabled system, the architecture specific policy might
> require the IMA policy itself to be signed.

> Snippet from ima_fs.c:

> #if IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) &&
> IS_ENABLED(CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY)
>         "appraise func=POLICY_CHECK appraise_type=imasig",
> #endif

> > Maybe there could be a test based on example [2].

> > echo /home/user/tmpfile > /sys/kernel/security/ima/policy
> > cp tmpfile /sys/kernel/security/ima/policy
> > cat tmpfile > /sys/kernel/security/ima/policy

> All of the above will load a policy, assuming the policy itself doesn't need to
> be signed.  Only "echo /home/user/tmpfile > /sys/kernel/security/ima/policy" can
> load a signed policy.

> Loading a CA key (mokutil), signing (evmctl)[1] and loading (keyctl) an IMA
> policy is probably beyond LTP.  The purpose of this test would be to detect
> whether policies need to be signed.

The most advanced for LTP is currently solving reboot [3].

FYI we plan to add support [4] to our kirk tool [5] (currently supports running LTP,
kselftest and liburing, testing via SSH, qemu).

I suppose given how sparse is IMA/EVM testing in LTP this can wait (there are
more basic features not covered by testing). I suppose most of the testing you
have in ima-evm-utils repo (at least I found only IMA related code in kselftest
in BPF tests).

> Going forward what's probably needed is a new package containing a set of pre-
> defined sample custom policies, which are signed by the distro.

Please let me now once you or other IMA devs are doing any work in this.

Kind regards,
Petr

> [1] Directions for signing and loading a custom policy,
> https://ima-doc.readthedocs.io/en/latest/ima-utilities.html#sign-and-install-a-custom-policy

> Thanks,

> Mimi


> > Kind regards,
> > Petr

> > [1] https://ima-doc.readthedocs.io/en/latest/policy-syntax.html#func-policy-check
> > [2] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#runtime-custom-policy

[3] https://github.com/linux-test-project/ltp/issues/868
[4] https://github.com/linux-test-project/kirk/issues/12
[5] https://github.com/linux-test-project/kirk

> > > > +}

> > > Mimi

Re: [PATCH v2 2/8] ima_setup.sh: Allow to load predefined policy

[Mimi Zohar]

Hi Petr,

...
> > Going forward what's probably needed is a new package containing a set of pre-
> > defined sample custom policies, which are signed by the distro.
> 
> Please let me now once you or other IMA devs are doing any work in this.

Yes, of course we'll let you know.

Mimi

Re: [PATCH v2 6/8] IMA: Add example policy for ima_violations.sh

[Petr Vorel]

> On Tue, 2024-12-31 at 13:23 +0100, Petr Vorel wrote:
> > Hi Mimi,

> > > Hi Petr,

> > > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > > ---
> > > >  .../integrity/ima/datafiles/ima_violations/violations.policy     | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >  create mode 100644 testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy

> > > > diff --git a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > > > new file mode 100644
> > > > index 0000000000..5734c7617f
> > > > --- /dev/null
> > > > +++ b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations.policy
> > > > @@ -0,0 +1 @@
> > > > +func=FILE_CHECK

> > > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh"
> > > contains two rules to measure files opened by root on file open.

> > > measure func=FILE_CHECK mask=^MAY_READ euid=0
> > > measure func=FILE_CHECK mask=^MAY_READ uid=0

> > > If the 'tcb' or equivalent policy is loaded, there is no need to load another
> > > policy rule. 

> > I guess I'll move check for builtin policy loaded via kernel command line
> > parameter also to ima_setup.sh to avoid loading example policy when there is a
> > required builtin policy loaded.


> Between the builtin and arch specific policies, most of the rules are already
> defined.  The exception is measuring the boot command line.  Perhaps we should
> update the arch specific policy to include it with the other kexec rules.

> The arch specific policy may include the rule that requires the IMA policy to be
> signed.

> > I also wonder what is a common approach - don't
> > try to load custom example policy when there is builtin policy loaded?

> How about first checking if the rule exists when there is a builtin or
> equivalent custom policy loaded, before loading the example test policy?


> > My goal was to allow more broad IMA testing based on different setup:

> Very good.

> > * running tests with ima_policy=tcb builtin policy (current approach). Many
> > tests will be skipped due missing required policy content.

> Ok.  Remember even with "ima_policy=tcb" specified on the boot command line, the
> results will differ depending on whether the arch specific policy is loaded.

> > * running tests without any builtin policy + load a custom policy + reboot via
> > LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only
> > if required (or even none) builtin policy is loaded.

> Good.  The first patch introduces the equivalent custom policy to
> "ima_policy=tcb".  By "load a custom policy" are you referring to this policy or
> a specific policy test rule?

I refer to this policy. Maybe better would be "policy content required by the test"
or "test example policy".

My point is to allow testing without forcing ima_policy=tcb setup (some tooling
might not allow easily to add kernel cmdline parameters). Also, mixing test
example policy with ima_policy=tcb may result a different measurements, right?

If the above assumption is correct I would like to have testing *with*
ima_policy=tcb without loading any test example policy and *without*
ima_policy=tcb but loading test example policy via LTP_IMA_LOAD_POLICY=1.

> > * Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it
> > (but then it is hard to detect whether failures are real bugs or just false
> > positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF if

I'm sorry, I was wrong here, I meant to ask: convert TFAIL to either TBROK or TCONF,
e.g. my patch [1].

> > policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and
> > custom policy with proper content was not loaded).

> Probably the latter option of converting from TBROK/TFAIL to TCONF is
> preferable.  Why fail a test without knowing it will fail.

Because on distros without CONFIG_IMA_READ_POLICY=y we never get notified about
the failure (maybe kernel is broken when it fails but nobody notices TCONF).
But although there is a slight difference between TFAIL and TBROK [2], I agree
that TCONF is probably the best (nobody wants to deal with false positives),
which is handled in my patch [1].

But instead of this I'll try for all tests which require to have certain policy
content (currently all but ima_conditionals.sh): if LTP_IMA_LOAD_POLICY=1 set
try to load example policy even policy content cannot be checked (TCONF when
policy fails to be loaded or if LTP_IMA_LOAD_POLICY not set).

Kind regards,
Petr

> > But you may have an idea what is more useful (brings more test coverage).

> There are two main problems:
> - Not being able to read the policy.
> - Only being able to load a signed policy.

> I think between your above ordering and a new test to see if the policy needs to
> be signed, it's the best we can do for now.

> As mentioned in my 2/8 response, a new package containing pre-defined custom
> policies that are signed by the distro would resolve the latter problem.


> Thanks,

> Mimi

[1] https://patchwork.ozlabs.org/project/ltp/patch/20241213222014.1580991-9-pvorel@suse.cz/
[2] https://linux-test-project.readthedocs.io/en/latest/developers/api_c_tests.html#tst-res-flags-constants

Re: [PATCH v2 6/8] IMA: Add example policy for ima_violations.sh

[Mimi Zohar]

On Fri, 2025-01-03 at 20:02 +0100, Petr Vorel wrote:
> > On Tue, 2024-12-31 at 13:23 +0100, Petr Vorel wrote:
> > > Hi Mimi,
> 
> > > > Hi Petr,
> 
> > > > On Fri, 2024-12-13 at 23:20 +0100, Petr Vorel wrote:
> > > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > > > ---
> > > > >  .../integrity/ima/datafiles/ima_violations/violations.policy     | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >  create mode 100644
> > > > > testcases/kernel/security/integrity/ima/datafiles/ima_violations/violations
> > > > > .policy
> 
> > > > > diff --git
> > > > > a/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violatio
> > > > > ns.policy
> > > > > b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violatio
> > > > > ns.policy
> > > > > new file mode 100644
> > > > > index 0000000000..5734c7617f
> > > > > --- /dev/null
> > > > > +++
> > > > > b/testcases/kernel/security/integrity/ima/datafiles/ima_violations/violatio
> > > > > ns.policy
> > > > > @@ -0,0 +1 @@
> > > > > +func=FILE_CHECK
> 
> > > > "[PATCH v2 1/8] IMA: Add TCB policy as an example for ima_measurements.sh"
> > > > contains two rules to measure files opened by root on file open.
> 
> > > > measure func=FILE_CHECK mask=^MAY_READ euid=0
> > > > measure func=FILE_CHECK mask=^MAY_READ uid=0
> 
> > > > If the 'tcb' or equivalent policy is loaded, there is no need to load another
> > > > policy rule. 
> 
> > > I guess I'll move check for builtin policy loaded via kernel command line
> > > parameter also to ima_setup.sh to avoid loading example policy when there is a
> > > required builtin policy loaded.
> 
> 
> > Between the builtin and arch specific policies, most of the rules are already
> > defined.  The exception is measuring the boot command line.  Perhaps we should
> > update the arch specific policy to include it with the other kexec rules.
> 
> > The arch specific policy may include the rule that requires the IMA policy to be
> > signed.
> 
> > > I also wonder what is a common approach - don't
> > > try to load custom example policy when there is builtin policy loaded?
> 
> > How about first checking if the rule exists when there is a builtin or
> > equivalent custom policy loaded, before loading the example test policy?
> 
> 
> > > My goal was to allow more broad IMA testing based on different setup:
> 
> > Very good.
> 
> > > * running tests with ima_policy=tcb builtin policy (current approach). Many
> > > tests will be skipped due missing required policy content.
> 
> > Ok.  Remember even with "ima_policy=tcb" specified on the boot command line, the
> > results will differ depending on whether the arch specific policy is loaded.
> 
> > > * running tests without any builtin policy + load a custom policy + reboot via
> > > LTP_IMA_LOAD_POLICY=1 (this patchset), but this should be probably be done only
> > > if required (or even none) builtin policy is loaded.
> 
> > Good.  The first patch introduces the equivalent custom policy to
> > "ima_policy=tcb".  By "load a custom policy" are you referring to this policy or
> > a specific policy test rule?
> 
> I refer to this policy. Maybe better would be "policy content required by the test"
> or "test example policy".
> 
> My point is to allow testing without forcing ima_policy=tcb setup (some tooling
> might not allow easily to add kernel cmdline parameters). Also, mixing test
> example policy with ima_policy=tcb may result a different measurements, right?

Only if the file matches multiple, different measurement rules.  The ordering of the
policy rules impacts the measurement and might even prevent the measurement.

> 
> If the above assumption is correct I would like to have testing *with*
> ima_policy=tcb without loading any test example policy

I assume the purpose is to simplify testing.

However,
- Not all of the policy rules needed by the tests are included in the builtin "tcb"
measurement policy.  Without loading test specific example policy rules, the testing
would be incomplete.

- There's no guarantee that the builtin "tcb" measurement policy has not been
replaced with a custom policy.

> and *without*
> ima_policy=tcb but loading test example policy via LTP_IMA_LOAD_POLICY=1.

Ok, but this assumes the ability of loading an unsigned policy.

> 
> > > * Ideally not require CONFIG_IMA_READ_POLICY=y as some distros does not have it
> > > (but then it is hard to detect whether failures are real bugs or just false
> > > positives due not having a proper policy). Maybe convert TBROK/TFAIL to TCONF
> > > if
> 
> I'm sorry, I was wrong here, I meant to ask: convert TFAIL to either TBROK or
> TCONF,
> e.g. my patch [1].
> 
> > > policy content is required but cannot be read due CONFIG_IMA_READ_POLICY (and
> > > custom policy with proper content was not loaded).
> 
> > Probably the latter option of converting from TBROK/TFAIL to TCONF is
> > preferable.  Why fail a test without knowing it will fail.
> 
> Because on distros without CONFIG_IMA_READ_POLICY=y we never get notified about
> the failure (maybe kernel is broken when it fails but nobody notices TCONF).
> But although there is a slight difference between TFAIL and TBROK [2], I agree
> that TCONF is probably the best (nobody wants to deal with false positives),
> which is handled in my patch [1].
> 
> But instead of this I'll try for all tests which require to have certain policy
> content (currently all but ima_conditionals.sh): if LTP_IMA_LOAD_POLICY=1 set
> try to load example policy even policy content cannot be checked (TCONF when
> policy fails to be loaded or if LTP_IMA_LOAD_POLICY not set).

Sounds good.

Mimi