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

[Petr Vorel <pvorel@suse.cz>]

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