Re: [PATCH v24 04/25] IMA: avoid label collisions with stacked LSMs

Hi Casey, On Tue, 2021-01-26 at 08:40 -0800, Casey Schaufler wrote: > Integrity measurement may filter on security module information > and needs to be clear in the case of multiple active security > modules which applies. Provide a boot option ima_rules_lsm= to > allow the user to specify an active securty module to apply > filters to. If not specified, use the first registered module > that supports the audit_rule_match() LSM hook. Allow the user > to specify in the IMA policy an lsm= option to specify the > security module to use for a particular rule. Thanks, Casey. (This patch description line length seems short.) > Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com&gt; > To: Mimi Zohar <zohar(a)linux.ibm.com&gt; > To: linux-integrity(a)vger.kernel.org > --- > Documentation/ABI/testing/ima_policy | 8 +++- > security/integrity/ima/ima_policy.c | 64 ++++++++++++++++++++++------ > 2 files changed, 57 insertions(+), 15 deletions(-) > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > index e35263f97fc1..a7943d40466f 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -25,7 +25,7 @@ Description: > base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=] > [euid=] [fowner=] [fsname=]] > lsm: [[subj_user=] [subj_role=] [subj_type=] > - [obj_user=] [obj_role=] [obj_type=]] > + [obj_user=] [obj_role=] [obj_type=] [lsm=]] "[lsm=]" either requires all LSM rules types (e.g. {subj/obj}_user, role, type) to be exactly the same for multiple LSMs or all of the LSM rule types are applicable to only a single LSM. Supporting multiple LSMs with exactly the same LSM labels doesn't seem worth the effort. Keep it simple - a single rule, containing any LSM rule types, is applicable to a single LSM.

> option: [[appraise_type=]] [template=] [permit_directio] > [appraise_flag=] [keyrings=] > base: > @@ -114,6 +114,12 @@ Description: > > measure subj_user=_ func=FILE_CHECK mask=MAY_READ > > + It is possible to explicitly specify which security > + module a rule applies to using lsm=. If the security > + modules specified is not active on the system the rule > + will be rejected. If lsm= is not specified the first > + security module registered on the system will be assumed. > + > Example of measure rules using alternate PCRs:: > > measure func=KEXEC_KERNEL_CHECK pcr=4 > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 8002683003e6..de72b719c90c 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -82,6 +82,7 @@ struct ima_rule_entry { > void *rules[LSMBLOB_ENTRIES]; /* LSM file metadata specific */ > char *args_p; /* audit value */ > int type; /* audit type */ > + int which_lsm; /* which of the rules to use */ > } lsm[MAX_LSM_RULES]; Even if we wanted to support multiple LSMs within the same rule having both "rules[LSMBLOB_ENTRIES]" and "which_lsm" shouldn't be necessary. The LSMBLOB_ENTRIES should already identify the LSM. To support a single LSM per policy rule, "which_lsm" should be defined outside of lsm[MAX_LSM_RULES]. This will simplify the rest of the code (e.g. matching/freeing rules). int which_lsm; /* which of the rules to use */ struct { void *rule; /* LSM file metadata specific */ char *args_p; /* audit value */ int type; /* audit type */ } lsm[MAX_LSM_RULES];

> char *fsname; > struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */ > @@ -90,17 +91,15 @@ struct ima_rule_entry { > > /** > * ima_lsm_isset - Is a rule set for any of the active security modules > - * @rules: The set of IMA rules to check > + * @entry: the rule entry to examine > + * @lsm_rule: the specific rule type in question > * > - * If a rule is set for any LSM return true, otherwise return false. > + * If a rule is set return true, otherwise return false. > */ > -static inline bool ima_lsm_isset(void *rules[]) > +static inline bool ima_lsm_isset(struct ima_rule_entry *entry, int lsm_rule) > { > - int i; > - > - for (i = 0; i < LSMBLOB_ENTRIES; i++) > - if (rules[i]) > - return true; > + if (entry->lsm[lsm_rule].rules[entry->lsm[lsm_rule].which_lsm]) > + return true; If each IMA policy rule is limited to a specific LSM, then the test would be "entry->which_lsm".

> return false; > } > > @@ -273,6 +272,20 @@ static int __init default_appraise_policy_setup(char *str) > } > __setup("ima_appraise_tcb", default_appraise_policy_setup); > > +static int ima_rule_lsm __ro_after_init; > + > +static int __init ima_rule_lsm_init(char *str) > +{ > + ima_rule_lsm = lsm_name_to_slot(str); > + if (ima_rule_lsm < 0) { > + ima_rule_lsm = 0; > + pr_err("rule lsm \"%s\" not registered", str); > + } > + > + return 1; > +} > +__setup("ima_rule_lsm=", ima_rule_lsm_init); The patch description refers to "ima_rules_lsm=". Please update one or the other.

thanks, Mimi