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

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

[Matthew Garrett]

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".
 			pcr:= decimal value
 
 		default policy:
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..18b48a6d0b80 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -147,6 +147,7 @@ int ima_init_crypto(void);
 void ima_putc(struct seq_file *m, void *data, int datalen);
 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
 struct ima_template_desc *ima_template_desc_current(void);
+struct ima_template_desc *lookup_template_desc(const char *name);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
@@ -193,7 +194,8 @@ enum ima_hooks {
 
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
-		   int mask, enum ima_hooks func, int *pcr);
+		   int mask, enum ima_hooks func, int *pcr,
+		   struct ima_template_desc **template_desc);
 int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
 			    struct file *file, void *buf, loff_t size,
@@ -201,11 +203,13 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
-			   int xattr_len, int pcr);
+			   int xattr_len, int pcr,
+			   struct ima_template_desc *template_desc);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
-			    struct ima_template_entry **entry);
+			    struct ima_template_entry **entry,
+			    struct ima_template_desc *template_desc);
 int ima_store_template(struct ima_template_entry *entry, int violation,
 		       struct inode *inode,
 		       const unsigned char *filename, int pcr);
@@ -214,7 +218,8 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
 
 /* IMA policy related functions */
 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);
 void ima_init_policy(void);
 void ima_update_policy(void);
 void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7505fb122d4..78eb11c7ac07 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -38,11 +38,17 @@ void ima_free_template_entry(struct ima_template_entry *entry)
  * ima_alloc_init_template - create and initialize a new template entry
  */
 int ima_alloc_init_template(struct ima_event_data *event_data,
-			    struct ima_template_entry **entry)
+			    struct ima_template_entry **entry,
+			    struct ima_template_desc *desc)
 {
-	struct ima_template_desc *template_desc = ima_template_desc_current();
+	struct ima_template_desc *template_desc;
 	int i, result = 0;
 
+	if (desc)
+		template_desc = desc;
+	else
+		template_desc = ima_template_desc_current();
+
 	*entry = kzalloc(sizeof(**entry) + template_desc->num_fields *
 			 sizeof(struct ima_field_data), GFP_NOFS);
 	if (!*entry)
@@ -141,7 +147,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
 	/* can overflow, only indicator */
 	atomic_long_inc(&ima_htable.violations);
 
-	result = ima_alloc_init_template(&event_data, &entry);
+	result = ima_alloc_init_template(&event_data, &entry, NULL);
 	if (result < 0) {
 		result = -ENOMEM;
 		goto err_out;
@@ -164,6 +170,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  *        MAY_APPEND)
  * @func: caller identifier
  * @pcr: pointer filled in if matched measure policy sets pcr=
+ * @template_desc: pointer filled in if matched measure policy sets template=
  *
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
@@ -176,13 +183,15 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  *
  */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
-		   int mask, enum ima_hooks func, int *pcr)
+		   int mask, enum ima_hooks func, int *pcr,
+		   struct ima_template_desc **template_desc)
 {
 	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
 
 	flags &= ima_policy_flag;
 
-	return ima_match_policy(inode, cred, secid, func, mask, flags, pcr);
+	return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
+				template_desc);
 }
 
 /*
@@ -277,7 +286,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 void ima_store_measurement(struct integrity_iint_cache *iint,
 			   struct file *file, const unsigned char *filename,
 			   struct evm_ima_xattr_data *xattr_value,
-			   int xattr_len, int pcr)
+			   int xattr_len, int pcr,
+			   struct ima_template_desc *template_desc)
 {
 	static const char op[] = "add_template_measure";
 	static const char audit_cause[] = "ENOMEM";
@@ -291,7 +301,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 	if (iint->measured_pcrs & (0x1 << pcr))
 		return;
 
-	result = ima_alloc_init_template(&event_data, &entry);
+	result = ima_alloc_init_template(&event_data, &entry, template_desc);
 	if (result < 0) {
 		integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
 				    op, audit_cause, result, 0);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..2f6536ab69e8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -57,7 +57,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
 
 	security_task_getsecid(current, &secid);
 	return ima_match_policy(inode, current_cred(), secid, func, mask,
-				IMA_APPRAISE | IMA_HASH, NULL);
+				IMA_APPRAISE | IMA_HASH, NULL, NULL);
 }
 
 static int ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 6c9295449751..993d0f1915ff 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -72,7 +72,7 @@ static int __init ima_add_boot_aggregate(void)
 		}
 	}
 
-	result = ima_alloc_init_template(&event_data, &entry);
+	result = ima_alloc_init_template(&event_data, &entry, NULL);
 	if (result < 0) {
 		audit_cause = "alloc_entry";
 		goto err_out;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..83c5dea0d939 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -174,7 +174,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 {
 	struct inode *inode = file_inode(file);
 	struct integrity_iint_cache *iint = NULL;
-	struct ima_template_desc *template_desc;
+	struct ima_template_desc *template_desc = NULL;
 	char *pathbuf = NULL;
 	char filename[NAME_MAX];
 	const char *pathname = NULL;
@@ -192,7 +192,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	 * bitmask based on the appraise/audit/measurement policy.
 	 * Included is the appraise submask.
 	 */
-	action = ima_get_action(inode, cred, secid, mask, func, &pcr);
+	action = ima_get_action(inode, cred, secid, mask, func, &pcr,
+				&template_desc);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -275,7 +276,6 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		goto out_locked;
 	}
 
-	template_desc = ima_template_desc_current();
 	if ((action & IMA_APPRAISE_SUBMASK) ||
 		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
 		/* read 'security.ima' */
@@ -292,7 +292,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
 
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
-				      xattr_value, xattr_len, pcr);
+				      xattr_value, xattr_len, pcr,
+				      template_desc);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
 		inode_lock(inode);
 		rc = ima_appraise_measurement(func, iint, file, pathname,
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();
+
 		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
 };
 
 static const match_table_t policy_tokens = {
@@ -710,6 +718,7 @@ static const match_table_t policy_tokens = {
 	{Opt_appraise_type, "appraise_type=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
+	{Opt_template, "template=%s"},
 	{Opt_err, NULL}
 };
 
@@ -763,6 +772,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 	char *from;
 	char *p;
 	bool uid_token;
+	struct ima_template_desc *template_desc;
 	int result = 0;
 
 	ab = integrity_audit_log_start(audit_context(), GFP_KERNEL,
@@ -1059,6 +1069,18 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->flags |= IMA_PCR;
 
 			break;
+		case Opt_template:
+			ima_log_string(ab, "template", args[0].from);
+			if (entry->action != MEASURE) {
+				result = -EINVAL;
+				break;
+			}
+			template_desc = lookup_template_desc(args[0].from);
+			if (!template_desc || entry->template)
+				result = -EINVAL;
+			else
+				entry->template = template_desc;
+			break;
 		case Opt_err:
 			ima_log_string(ab, "UNKNOWN", p);
 			result = -EINVAL;
@@ -1331,6 +1353,8 @@ int ima_policy_show(struct seq_file *m, void *v)
 			}
 		}
 	}
+	if (entry->template)
+		seq_printf(m, "template=%s ", entry->template->name);
 	if (entry->flags & IMA_DIGSIG_REQUIRED)
 		seq_puts(m, "appraise_type=imasig ");
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index b631b8bc7624..e6e892f31cbd 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -47,7 +47,6 @@ static const struct ima_template_field supported_fields[] = {
 #define MAX_TEMPLATE_NAME_LEN 15
 
 static struct ima_template_desc *ima_template;
-static struct ima_template_desc *lookup_template_desc(const char *name);
 static int template_desc_init_fields(const char *template_fmt,
 				     const struct ima_template_field ***fields,
 				     int *num_fields);
@@ -108,7 +107,7 @@ static int __init ima_template_fmt_setup(char *str)
 }
 __setup("ima_template_fmt=", ima_template_fmt_setup);
 
-static struct ima_template_desc *lookup_template_desc(const char *name)
+struct ima_template_desc *lookup_template_desc(const char *name)
 {
 	struct ima_template_desc *template_desc;
 	int found = 0;
@@ -117,6 +116,13 @@ static struct ima_template_desc *lookup_template_desc(const char *name)
 	list_for_each_entry_rcu(template_desc, &defined_templates, list) {
 		if ((strcmp(template_desc->name, name) == 0) ||
 		    (strcmp(template_desc->fmt, name) == 0)) {
+			/*
+			 * template_desc_init_fields() will return immediately
+			 * if the template is already initialised
+			 */
+			template_desc_init_fields(template_desc->fmt,
+						 &(template_desc->fields),
+						 &(template_desc->num_fields));
 			found = 1;
 			break;
 		}
-- 
2.22.0.rc1.311.g5d7573a151-goog

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

[Mimi Zohar]

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.

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
>  };
> 

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

[Mimi Zohar]

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
> >  };
> > 

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

[Roberto Sassu]

On 6/4/2019 3:51 AM, 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".

The patch is correct. lookup_template_desc() also considers the format.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

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

[Roberto Sassu]

On 6/3/2019 10:13 PM, 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".
>   			pcr:= decimal value
>   
>   		default policy:
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d213e835c498..18b48a6d0b80 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -147,6 +147,7 @@ int ima_init_crypto(void);
>   void ima_putc(struct seq_file *m, void *data, int datalen);
>   void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
>   struct ima_template_desc *ima_template_desc_current(void);
> +struct ima_template_desc *lookup_template_desc(const char *name);
>   int ima_restore_measurement_entry(struct ima_template_entry *entry);
>   int ima_restore_measurement_list(loff_t bufsize, void *buf);
>   int ima_measurements_show(struct seq_file *m, void *v);
> @@ -193,7 +194,8 @@ enum ima_hooks {
>   
>   /* LIM API function definitions */
>   int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
> -		   int mask, enum ima_hooks func, int *pcr);
> +		   int mask, enum ima_hooks func, int *pcr,
> +		   struct ima_template_desc **template_desc);
>   int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
>   int ima_collect_measurement(struct integrity_iint_cache *iint,
>   			    struct file *file, void *buf, loff_t size,
> @@ -201,11 +203,13 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
>   			   const unsigned char *filename,
>   			   struct evm_ima_xattr_data *xattr_value,
> -			   int xattr_len, int pcr);
> +			   int xattr_len, int pcr,
> +			   struct ima_template_desc *template_desc);
>   void ima_audit_measurement(struct integrity_iint_cache *iint,
>   			   const unsigned char *filename);
>   int ima_alloc_init_template(struct ima_event_data *event_data,
> -			    struct ima_template_entry **entry);
> +			    struct ima_template_entry **entry,
> +			    struct ima_template_desc *template_desc);
>   int ima_store_template(struct ima_template_entry *entry, int violation,
>   		       struct inode *inode,
>   		       const unsigned char *filename, int pcr);
> @@ -214,7 +218,8 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
>   
>   /* IMA policy related functions */
>   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);
>   void ima_init_policy(void);
>   void ima_update_policy(void);
>   void ima_update_policy_flag(void);
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c7505fb122d4..78eb11c7ac07 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -38,11 +38,17 @@ void ima_free_template_entry(struct ima_template_entry *entry)
>    * ima_alloc_init_template - create and initialize a new template entry
>    */
>   int ima_alloc_init_template(struct ima_event_data *event_data,
> -			    struct ima_template_entry **entry)
> +			    struct ima_template_entry **entry,
> +			    struct ima_template_desc *desc)
>   {
> -	struct ima_template_desc *template_desc = ima_template_desc_current();
> +	struct ima_template_desc *template_desc;
>   	int i, result = 0;
>   
> +	if (desc)
> +		template_desc = desc;
> +	else
> +		template_desc = ima_template_desc_current();
> +
>   	*entry = kzalloc(sizeof(**entry) + template_desc->num_fields *
>   			 sizeof(struct ima_field_data), GFP_NOFS);
>   	if (!*entry)
> @@ -141,7 +147,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>   	/* can overflow, only indicator */
>   	atomic_long_inc(&ima_htable.violations);
>   
> -	result = ima_alloc_init_template(&event_data, &entry);
> +	result = ima_alloc_init_template(&event_data, &entry, NULL);
>   	if (result < 0) {
>   		result = -ENOMEM;
>   		goto err_out;
> @@ -164,6 +170,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>    *        MAY_APPEND)
>    * @func: caller identifier
>    * @pcr: pointer filled in if matched measure policy sets pcr=
> + * @template_desc: pointer filled in if matched measure policy sets template=
>    *
>    * The policy is defined in terms of keypairs:
>    *		subj=, obj=, type=, func=, mask=, fsmagic=
> @@ -176,13 +183,15 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>    *
>    */
>   int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
> -		   int mask, enum ima_hooks func, int *pcr)
> +		   int mask, enum ima_hooks func, int *pcr,
> +		   struct ima_template_desc **template_desc)
>   {
>   	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
>   
>   	flags &= ima_policy_flag;
>   
> -	return ima_match_policy(inode, cred, secid, func, mask, flags, pcr);
> +	return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
> +				template_desc);
>   }
>   
>   /*
> @@ -277,7 +286,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   void ima_store_measurement(struct integrity_iint_cache *iint,
>   			   struct file *file, const unsigned char *filename,
>   			   struct evm_ima_xattr_data *xattr_value,
> -			   int xattr_len, int pcr)
> +			   int xattr_len, int pcr,
> +			   struct ima_template_desc *template_desc)
>   {
>   	static const char op[] = "add_template_measure";
>   	static const char audit_cause[] = "ENOMEM";
> @@ -291,7 +301,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
>   	if (iint->measured_pcrs & (0x1 << pcr))
>   		return;
>   
> -	result = ima_alloc_init_template(&event_data, &entry);
> +	result = ima_alloc_init_template(&event_data, &entry, template_desc);
>   	if (result < 0) {
>   		integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
>   				    op, audit_cause, result, 0);
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 5fb7127bbe68..2f6536ab69e8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -57,7 +57,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
>   
>   	security_task_getsecid(current, &secid);
>   	return ima_match_policy(inode, current_cred(), secid, func, mask,
> -				IMA_APPRAISE | IMA_HASH, NULL);
> +				IMA_APPRAISE | IMA_HASH, NULL, NULL);
>   }
>   
>   static int ima_fix_xattr(struct dentry *dentry,
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 6c9295449751..993d0f1915ff 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -72,7 +72,7 @@ static int __init ima_add_boot_aggregate(void)
>   		}
>   	}
>   
> -	result = ima_alloc_init_template(&event_data, &entry);
> +	result = ima_alloc_init_template(&event_data, &entry, NULL);
>   	if (result < 0) {
>   		audit_cause = "alloc_entry";
>   		goto err_out;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 357edd140c09..83c5dea0d939 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -174,7 +174,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   {
>   	struct inode *inode = file_inode(file);
>   	struct integrity_iint_cache *iint = NULL;
> -	struct ima_template_desc *template_desc;
> +	struct ima_template_desc *template_desc = NULL;
>   	char *pathbuf = NULL;
>   	char filename[NAME_MAX];
>   	const char *pathname = NULL;
> @@ -192,7 +192,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   	 * bitmask based on the appraise/audit/measurement policy.
>   	 * Included is the appraise submask.
>   	 */
> -	action = ima_get_action(inode, cred, secid, mask, func, &pcr);
> +	action = ima_get_action(inode, cred, secid, mask, func, &pcr,
> +				&template_desc);
>   	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
>   			   (ima_policy_flag & IMA_MEASURE));
>   	if (!action && !violation_check)
> @@ -275,7 +276,6 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   		goto out_locked;
>   	}
>   
> -	template_desc = ima_template_desc_current();
>   	if ((action & IMA_APPRAISE_SUBMASK) ||
>   		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
>   		/* read 'security.ima' */
> @@ -292,7 +292,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   
>   	if (action & IMA_MEASURE)
>   		ima_store_measurement(iint, file, pathname,
> -				      xattr_value, xattr_len, pcr);
> +				      xattr_value, xattr_len, pcr,
> +				      template_desc);
>   	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
>   		inode_lock(inode);
>   		rc = ima_appraise_measurement(func, iint, file, pathname,
> 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();
> +
>   		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
>   };
>   
>   static const match_table_t policy_tokens = {
> @@ -710,6 +718,7 @@ static const match_table_t policy_tokens = {
>   	{Opt_appraise_type, "appraise_type=%s"},
>   	{Opt_permit_directio, "permit_directio"},
>   	{Opt_pcr, "pcr=%s"},
> +	{Opt_template, "template=%s"},
>   	{Opt_err, NULL}
>   };
>   
> @@ -763,6 +772,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   	char *from;
>   	char *p;
>   	bool uid_token;
> +	struct ima_template_desc *template_desc;
>   	int result = 0;
>   
>   	ab = integrity_audit_log_start(audit_context(), GFP_KERNEL,
> @@ -1059,6 +1069,18 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   				entry->flags |= IMA_PCR;
>   
>   			break;
> +		case Opt_template:
> +			ima_log_string(ab, "template", args[0].from);
> +			if (entry->action != MEASURE) {
> +				result = -EINVAL;
> +				break;
> +			}
> +			template_desc = lookup_template_desc(args[0].from);
> +			if (!template_desc || entry->template)
> +				result = -EINVAL;
> +			else
> +				entry->template = template_desc;
> +			break;
>   		case Opt_err:
>   			ima_log_string(ab, "UNKNOWN", p);
>   			result = -EINVAL;
> @@ -1331,6 +1353,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>   			}
>   		}
>   	}
> +	if (entry->template)
> +		seq_printf(m, "template=%s ", entry->template->name);

Name could be empty if entry->template is the last element of the
builtin_templates array. In this case, you should print ->fmt (check
ima_measurement_show()).

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

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

[Mimi Zohar]

On Tue, 2019-06-04 at 16:03 +0200, Roberto Sassu wrote:
> On 6/4/2019 3:51 AM, 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".
> 
> The patch is correct. lookup_template_desc() also considers the format.

Specifying the template format works if it is defined in
builtin_templates[], but seems to fail if it isn't.

Mimi

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

[Roberto Sassu]

On 6/4/2019 4:32 PM, Mimi Zohar wrote:
> On Tue, 2019-06-04 at 16:03 +0200, Roberto Sassu wrote:
>> On 6/4/2019 3:51 AM, 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".
>>
>> The patch is correct. lookup_template_desc() also considers the format.
> 
> Specifying the template format works if it is defined in
> builtin_templates[], but seems to fail if it isn't.

Yes, the original patch set supports the definition of new templates.
That part is not included in this patch.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

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

[Mimi Zohar]

On Tue, 2019-06-04 at 16:40 +0200, Roberto Sassu wrote:
> On 6/4/2019 4:32 PM, Mimi Zohar wrote:
> > On Tue, 2019-06-04 at 16:03 +0200, Roberto Sassu wrote:
> >> On 6/4/2019 3:51 AM, 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".
> >>
> >> The patch is correct. lookup_template_desc() also considers the format.
> > 
> > Specifying the template format works if it is defined in
> > builtin_templates[], but seems to fail if it isn't.
> 
> Yes, the original patch set supports the definition of new templates.
> That part is not included in this patch.

There are three patch sets waiting for this new feature.  For now,
let's remove the reference to the template definition in this patch.
 The missing support could be added as a separate patch.

Roberto, are you ok with that?

Mimi

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

[Roberto Sassu]

On 6/4/2019 4:47 PM, Mimi Zohar wrote:
> On Tue, 2019-06-04 at 16:40 +0200, Roberto Sassu wrote:
>> On 6/4/2019 4:32 PM, Mimi Zohar wrote:
>>> On Tue, 2019-06-04 at 16:03 +0200, Roberto Sassu wrote:
>>>> On 6/4/2019 3:51 AM, 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".
>>>>
>>>> The patch is correct. lookup_template_desc() also considers the format.
>>>
>>> Specifying the template format works if it is defined in
>>> builtin_templates[], but seems to fail if it isn't.
>>
>> Yes, the original patch set supports the definition of new templates.
>> That part is not included in this patch.
> 
> There are three patch sets waiting for this new feature.  For now,
> let's remove the reference to the template definition in this patch.
>   The missing support could be added as a separate patch.
> 
> Roberto, are you ok with that?

Yes, it is fine for me.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

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

[Matthew Garrett]

On Mon, Jun 3, 2019 at 6:52 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Mon, 2019-06-03 at 13:13 -0700, Matthew Garrett wrote:
> > +                     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".

It supports specifying the template format, as long as the template
format is already defined. I can remove the example, but it'll still
work.

> >       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.

Ok.

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

[Mimi Zohar]

On Tue, 2019-06-04 at 13:03 -0700, Matthew Garrett wrote:
> On Mon, Jun 3, 2019 at 6:52 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Mon, 2019-06-03 at 13:13 -0700, Matthew Garrett wrote:
> > > +                     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".
> 
> It supports specifying the template format, as long as the template
> format is already defined. I can remove the example, but it'll still
> work.

Correct, as mentioned in subsequent posts.  Until full support for
template field descriptions is added, please remove the field format
example.

> 
> > >       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.
> 
> Ok

Subsequent posts pointed out that either all callers to
ima_match_policy() need to pass a pointer to template_desc or
*template_desc needs to be tested.

thanks!

Mimi