Re: [PATCH V3] IMA: Allow profiles to define the desired IMA template

[Mimi Zohar <zohar@linux.ibm.com>]

On Mon, 2019-06-03 at 21:51 -0400, Mimi Zohar wrote:
> On Mon, 2019-06-03 at 13:13 -0700, Matthew Garrett wrote:
> > Admins may wish to log different measurements using different IMA
> > templates. Add support for overriding the default template on a per-rule
> > basis.
> > 
> > Signed-off-by: Matthew Garrett <mjg59@google.com>
> > ---
> > 
> > Updated based on review feedback, verified that I can generate an event
> > log that contains multiple different templates.
> > 
> >  Documentation/ABI/testing/ima_policy  |  6 ++++--
> >  security/integrity/ima/ima.h          | 13 +++++++++----
> >  security/integrity/ima/ima_api.c      | 24 ++++++++++++++++-------
> >  security/integrity/ima/ima_appraise.c |  2 +-
> >  security/integrity/ima/ima_init.c     |  2 +-
> >  security/integrity/ima/ima_main.c     |  9 +++++----
> >  security/integrity/ima/ima_policy.c   | 28 +++++++++++++++++++++++++--
> >  security/integrity/ima/ima_template.c | 10 ++++++++--
> >  8 files changed, 71 insertions(+), 23 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index 74c6702de74e..4ded0668a22d 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -24,8 +24,7 @@ Description:
> >  				[euid=] [fowner=] [fsname=]]
> >  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
> >  				 [obj_user=] [obj_role=] [obj_type=]]
> > -			option:	[[appraise_type=]] [permit_directio]
> > -
> > +			option:	[[appraise_type=]] [template=] [permit_directio]
> >  		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> >  				[FIRMWARE_CHECK]
> >  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> > @@ -38,6 +37,9 @@ Description:
> >  			fowner:= decimal value
> >  		lsm:  	are LSM specific
> >  		option:	appraise_type:= [imasig]
> > +			template:= name or format of a defined IMA template
> > +			type (eg,ima-ng or d-ng|n-ng). Only valid when action
> > +			is "measure".
> 
> This patch only supports specifying the template name, not the
> template format description.  Please remove "d-ng|n-ng".
> 
> >  			pcr:= decimal value
> > 
> >  		default policy:
> 
> <snip>
> 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 0f6fe53cef09..cbae2a3a9c5b 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -80,6 +80,7 @@ struct ima_rule_entry {
> >  		int type;	/* audit type */
> >  	} lsm[MAX_LSM_RULES];
> >  	char *fsname;
> > +	struct ima_template_desc *template;
> >  };
> > 
> >  /*
> > @@ -397,6 +398,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
> >   * @func: IMA hook identifier
> >   * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
> >   * @pcr: set the pcr to extend
> > + * @template_desc: the template that should be used for this rule
> >   *
> >   * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
> >   * conditions.
> > @@ -406,7 +408,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
> >   * than writes so ima_match_policy() is classical RCU candidate.
> >   */
> >  int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> > -		     enum ima_hooks func, int mask, int flags, int *pcr)
> > +		     enum ima_hooks func, int mask, int flags, int *pcr,
> > +		     struct ima_template_desc **template_desc)
> >  {
> >  	struct ima_rule_entry *entry;
> >  	int action = 0, actmask = flags | (flags << 1);
> > @@ -438,6 +441,11 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> >  		if ((pcr) && (entry->flags & IMA_PCR))
> >  			*pcr = entry->pcr;
> > 
> > +		if (template_desc && entry->template)
> > +			*template_desc = entry->template;
> > +		else
> > +			*template_desc = ima_template_desc_current();
> > +
> 
> This code is finding the template format, but is subsequently being
> replaced with the current description.  One way of fixing this, is by
> initializing the template_desc before walking the list.

Perhaps not, but the "else" clause also needs to test template_desc.
 The other option is for all ima_match_policy callers to provide
template_desc.

Mimi

> 
> >  		if (!actmask)
> >  			break;
> >  	}
> > @@ -676,7 +684,7 @@ enum {
> >  	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
> >  	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> >  	Opt_appraise_type, Opt_permit_directio,
> > -	Opt_pcr, Opt_err
> > +	Opt_pcr, Opt_template, Opt_err
> >  };
> >