| Message ID | 20210222023421.12576-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: Check for ima-buf template is not required for keys tests | expand |
Hi Lakmasi, > 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. Good catch. But was it alway? IMHO ima-buf as default was added in dea87d0889dd ("ima: select ima-buf template for buffer measurement") in v5.11-rc1. But test1() tests 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys") from v5.6-rc1. Is it safe to ignore it? BTW template=ima-buf requirement was added in commit b0418c93f ("IMA/ima_keys.sh: Require template=ima-buf, fix grep pattern") Also, shouldn't we check that there is none of the other templates (e.g. template=ima-ng, ...)? > Update keys tests to not check for ima template in the policy rule. > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > This patch is based > in https://github.com/pevik/ltp/commits/ima/selinux.v2.draft > in branch ima/selinux.v2.draft. > testcases/kernel/security/integrity/ima/tests/ima_keys.sh | 5 ++--- > 1 file changed, 2 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..a3a7afbf7 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)" nit: remove brackets: REQUIRED_POLICY="^measure.*$FUNC_KEYCHECK" There is testcases/kernel/security/integrity/ima/datafiles/ima_keys/keycheck.policy file, which should be a helper to load proper policy and needs to be updated as well: -measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test template=ima-buf +measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test I was also thinking to move keyrings to REQUIRED_POLICY, e.g.: KEYRINGS="keyrings=\.[a-z]+" REQUIRED_POLICY="^measure.*($FUNC_KEYCHECK.*$KEYRINGS|$KEYRINGS.*$FUNC_KEYCHECK)" Kind regards, Petr > setup() > { > @@ -33,7 +32,7 @@ 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
On 2/23/21 1:24 AM, Petr Vorel wrote: Hi Petr, > >> 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. > Good catch. But was it alway? > IMHO ima-buf as default was added in dea87d0889dd ("ima: select ima-buf template for buffer measurement") in v5.11-rc1. For key measurements ima-buf template was required in the policy rule, but with the above commit (dea87d0889dd) it was changed to ima-buf. So we no longer need to specify the template in the policy. > But test1() tests 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys") from v5.6-rc1. > Is it safe to ignore it? Even when the key is queued for measurement, ima-buf template will be used when the key is dequeued. Not sure if that answers your question. > BTW template=ima-buf requirement was added in commit b0418c93f ("IMA/ima_keys.sh: Require template=ima-buf, fix grep pattern") > > Also, shouldn't we check that there is none of the other templates (e.g. template=ima-ng, ...)? This is a good point - yes: we should check if no other template other than ima-buf is specified in the policy rule for measuring keys. > >> Update keys tests to not check for ima template in the policy rule. > >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >> --- >> This patch is based >> in https://github.com/pevik/ltp/commits/ima/selinux.v2.draft >> in branch ima/selinux.v2.draft. > >> testcases/kernel/security/integrity/ima/tests/ima_keys.sh | 5 ++--- >> 1 file changed, 2 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..a3a7afbf7 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)" > nit: remove brackets: > REQUIRED_POLICY="^measure.*$FUNC_KEYCHECK" Sure - will remove that. > > There is > testcases/kernel/security/integrity/ima/datafiles/ima_keys/keycheck.policy file, > which should be a helper to load proper policy and needs to be updated as well: > -measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test template=ima-buf > +measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test > > I was also thinking to move keyrings to REQUIRED_POLICY, e.g.: > > KEYRINGS="keyrings=\.[a-z]+" > REQUIRED_POLICY="^measure.*($FUNC_KEYCHECK.*$KEYRINGS|$KEYRINGS.*$FUNC_KEYCHECK)" "keyrings=" is optional in the policy. If keyrings is specified it should be checked. thanks, -lakshmi > >> setup() >> { >> @@ -33,7 +32,7 @@ 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
Hi Lakshmi, > On 2/23/21 1:24 AM, Petr Vorel wrote: > Hi Petr, > > > 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. > > Good catch. But was it alway? > > IMHO ima-buf as default was added in dea87d0889dd ("ima: select ima-buf template for buffer measurement") in v5.11-rc1. > For key measurements ima-buf template was required in the policy rule, but > with the above commit (dea87d0889dd) it was changed to ima-buf. So we no > longer need to specify the template in the policy. > > But test1() tests 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys") from v5.6-rc1. > > Is it safe to ignore it? > Even when the key is queued for measurement, ima-buf template will be used > when the key is dequeued. Not sure if that answers your question. IMHO template=ima-buf is required from v5.6-rc1 to v5.10, right? But maybe it's just enough to check that no other template is used as we discuss below. > > BTW template=ima-buf requirement was added in commit b0418c93f ("IMA/ima_keys.sh: Require template=ima-buf, fix grep pattern") > > Also, shouldn't we check that there is none of the other templates (e.g. template=ima-ng, ...)? > This is a good point - yes: we should check if no other template other than > ima-buf is specified in the policy rule for measuring keys. It'd be great if you include it in v2. ... > > > 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)" > > nit: remove brackets: > > REQUIRED_POLICY="^measure.*$FUNC_KEYCHECK" > Sure - will remove that. Thanks! > > There is > > testcases/kernel/security/integrity/ima/datafiles/ima_keys/keycheck.policy file, > > which should be a helper to load proper policy and needs to be updated as well: > > -measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test template=ima-buf > > +measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test > > I was also thinking to move keyrings to REQUIRED_POLICY, e.g.: > > KEYRINGS="keyrings=\.[a-z]+" > > REQUIRED_POLICY="^measure.*($FUNC_KEYCHECK.*$KEYRINGS|$KEYRINGS.*$FUNC_KEYCHECK)" > "keyrings=" is optional in the policy. If keyrings is specified it should be > checked. OK, just optional. Kind regards, Petr > thanks, > -lakshmi
On 2/23/21 9:31 AM, Petr Vorel wrote: >>>> 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. >>> Good catch. But was it alway? > >>> IMHO ima-buf as default was added in dea87d0889dd ("ima: select ima-buf template for buffer measurement") in v5.11-rc1. >> For key measurements ima-buf template was required in the policy rule, but >> with the above commit (dea87d0889dd) it was changed to ima-buf. So we no >> longer need to specify the template in the policy. > >>> But test1() tests 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys") from v5.6-rc1. >>> Is it safe to ignore it? >> Even when the key is queued for measurement, ima-buf template will be used >> when the key is dequeued. Not sure if that answers your question. > IMHO template=ima-buf is required from v5.6-rc1 to v5.10, right? That is correct Petr. > But maybe it's just enough to check that no other template is used as we discuss > below. I agree. > >>> BTW template=ima-buf requirement was added in commit b0418c93f ("IMA/ima_keys.sh: Require template=ima-buf, fix grep pattern") > >>> Also, shouldn't we check that there is none of the other templates (e.g. template=ima-ng, ...)? >> This is a good point - yes: we should check if no other template other than >> ima-buf is specified in the policy rule for measuring keys. > It'd be great if you include it in v2. Will do. > > ... >>>> 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)" >>> nit: remove brackets: >>> REQUIRED_POLICY="^measure.*$FUNC_KEYCHECK" >> Sure - will remove that. > Thanks! > >>> There is >>> testcases/kernel/security/integrity/ima/datafiles/ima_keys/keycheck.policy file, >>> which should be a helper to load proper policy and needs to be updated as well: >>> -measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test template=ima-buf >>> +measure func=KEY_CHECK keyrings=.ima|.evm|.builtin_trusted_keys|.blacklist|key_import_test > >>> I was also thinking to move keyrings to REQUIRED_POLICY, e.g.: > >>> KEYRINGS="keyrings=\.[a-z]+" >>> REQUIRED_POLICY="^measure.*($FUNC_KEYCHECK.*$KEYRINGS|$KEYRINGS.*$FUNC_KEYCHECK)" >> "keyrings=" is optional in the policy. If keyrings is specified it should be >> checked. > OK, just optional. > I'll see how to validate an optional field and update the test. thanks, -lakshmi
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh index c9eef4b68..a3a7afbf7 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,7 +32,7 @@ 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
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. Update keys tests to not check for ima template in the policy rule. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> --- This patch is based in https://github.com/pevik/ltp/commits/ima/selinux.v2.draft in branch ima/selinux.v2.draft. testcases/kernel/security/integrity/ima/tests/ima_keys.sh | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)