Re: [PATCH 4/6] integrity: IMA policy

[Mimi Zohar]

On Mon, 2009-02-02 at 17:40 -0600, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@xxxxxxxxxxxxxxxxxx):
> > Support for a user loadable policy through securityfs
> > with support for LSM specific policy data.
> > 
> > Based on comments made by: Matt Helsley, Serge Hallyn
> > - replaced policy parsing code with version using strsep and match_token
> > - only replace default policy with a valid policy
> > 
> > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxx>
> 
> Apart from comments below,
> 
> Acked-by: Serge Hallyn <serue@xxxxxxxxxx>
> 
> > ---
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > new file mode 100644
> > index 0000000..6434f0d
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -0,0 +1,61 @@
> > +What:		security/ima/policy
> > +Date:		May 2008
> > +Contact:	Mimi Zohar <zohar@xxxxxxxxxx>
> > +Description:
> > +		The Trusted Computing Group(TCG) runtime Integrity
> > +		Measurement Architecture(IMA) maintains a list of hash
> > +		values of executables and other sensitive system files
> > +		loaded into the run-time of this system.  At runtime,
> > +		the policy can be constrained based on LSM specific data.
> > +		Policies are loaded into the securityfs file ima/policy
> > +		by opening the file, writing the rules one at a time and
> > +		then closing the file.  The new policy takes effect after
> > +		the file ima/policy is closed.
> > +
> > +		rule format: action [condition ...]
> > +
> > +		action: measure | dont_measure
> > +		condition:= base | lsm
> > +			base:	[[func=] [mask=] [fsmagic=] [uid=]]
> > +			lsm:	[[subj_user=] [subj_role=] [subj_type=]
> > +				 [obj_user=] [obj_role=] [obj_type=]]
> > +
> > +		base: 	func:= [BPRM_CHECK][FILE_MMAP][INODE_PERMISSION]
> > +			mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
> > +			fsmagic:= hex value
> > +			uid:= decimal value
> > +		lsm:  	are LSM specific
> > +
> > +		default policy:
> > +			# PROC_SUPER_MAGIC
> > +			dont_measure fsmagic=0x9fa0
> > +			# SYSFS_MAGIC
> > +			dont_measure fsmagic=0x62656572
> > +			# DEBUGFS_MAGIC
> > +			dont_measure fsmagic=0x64626720
> > +			# TMPFS_MAGIC
> > +			dont_measure fsmagic=0x01021994
> > +			# SECURITYFS_MAGIC
> > +			dont_measure fsmagic=0x73636673
> > +
> > +			measure func=BPRM_CHECK
> > +			measure func=FILE_MMAP mask=MAY_EXEC
> > +			measure func=INODE_PERM mask=MAY_READ uid=0
> > +
> > +		The default policy measures all executables in bprm_check,
> > +		all files mmapped executable in file_mmap, and all files
> > +		open for read by root in inode_permission.
> > +
> > +		Examples of LSM specific definitions:
> > +
> > +		SELinux:
> > +			# SELINUX_MAGIC
> > +			dont_measure fsmagic=0xF97CFF8C
> > +
> > +			dont_measure obj_type=var_log_t
> > +			dont_measure obj_type=auditd_log_t
> > +			measure subj_user=system_u func=INODE_PERM mask=MAY_READ
> > +			measure subj_role=system_r func=INODE_PERM mask=MAY_READ
> > +
> > +		Smack:
> > +			measure subj_user=_ func=INODE_PERM mask=MAY_READ
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index 2a761c8..3d2b6ee 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -47,3 +47,9 @@ config IMA_AUDIT
> >  	  auditing messages can be enabled with 'ima_audit=1' on
> >  	  the kernel command line.
> >  
> > +config IMA_LSM_RULES
> > +	bool
> > +	depends on IMA && (SECURITY_SELINUX || SECURITY_SMACK)
> > +	default y
> > +	help
> > +	  Disabling this option will disregard LSM based policy rules
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 236b74e..5b72cdb 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -138,4 +138,28 @@ enum ima_hooks { PATH_CHECK = 1, FILE_MMAP, BPRM_CHECK };
> >  int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
> >  void ima_init_policy(void);
> >  void ima_update_policy(void);
> > +int ima_parse_add_rule(char *);
> > +void ima_delete_rules(void);
> > +
> > +/* LSM based policy rules require audit */
> > +#ifdef CONFIG_IMA_LSM_RULES
> > +
> > +#define security_filter_rule_init security_audit_rule_init
> > +#define security_filter_rule_match security_audit_rule_match
> > +
> > +#else
> > +
> > +static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
> > +					    void **lsmrule)
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> > +					     void *lsmrule,
> > +					     struct audit_context *actx)
> > +{
> > +	return -EINVAL;
> > +}
> > +#endif /* CONFIG_IMA_LSM_RULES */
> >  #endif
> > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > index 5044e4c..752a344 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -19,9 +19,11 @@
> >  #include <linux/seq_file.h>
> >  #include <linux/rculist.h>
> >  #include <linux/rcupdate.h>
> > +#include <linux/parser.h>
> >  
> >  #include "ima.h"
> >  
> > +static int valid_policy = 1;
> >  #define TMPBUFLEN 12
> >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> >  				     loff_t *ppos, atomic_long_t *val)
> > @@ -237,11 +239,66 @@ static struct file_operations ima_ascii_measurements_ops = {
> >  	.release = seq_release,
> >  };
> >  
> > +static ssize_t ima_write_policy(struct file *file, const char __user *buf,
> > +				size_t datalen, loff_t *ppos)
> > +{
> > +	char *data;
> > +	int rc;
> > +
> > +	if (datalen >= PAGE_SIZE)
> > +		return -ENOMEM;
> > +	if (*ppos != 0) {
> > +		/* No partial writes. */
> > +		return -EINVAL;
> > +	}
> > +	data = kmalloc(datalen + 1, GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	if (copy_from_user(data, buf, datalen)) {
> > +		kfree(data);
> > +		return -EFAULT;
> > +	}
> > +	*(data + datalen) = '\0';
> > +	rc = ima_parse_add_rule(data);
> > +	if (rc < 0) {
> > +		datalen = -EINVAL;
> > +		valid_policy = 0;
> > +	}
> > +
> > +	kfree(data);
> > +	return datalen;
> > +}
> > +
> >  static struct dentry *ima_dir;
> >  static struct dentry *binary_runtime_measurements;
> >  static struct dentry *ascii_runtime_measurements;
> >  static struct dentry *runtime_measurements_count;
> >  static struct dentry *violations;
> > +static struct dentry *ima_policy;
> > +
> > +/*
> > + * ima_release_policy - start using the new measure policy rules.
> > + *
> > + * Initially, ima_measure points to the default policy rules, now
> > + * point to the new policy rules, and remove the securityfs policy file.
> > + */
> > +static int ima_release_policy(struct inode *inode, struct file *file)
> > +{
> > +	if (!valid_policy) {
> > +		ima_delete_rules();
> > +		return 0;
> > +	}
> > +	ima_update_policy();
> > +	securityfs_remove(ima_policy);
> > +	ima_policy = NULL;
> > +	return 0;
> > +}
> > +
> > +static struct file_operations ima_measure_policy_ops = {
> > +	.write = ima_write_policy,
> > +	.release = ima_release_policy
> > +};
> >  
> >  int ima_fs_init(void)
> >  {
> > @@ -276,13 +333,20 @@ int ima_fs_init(void)
> >  	if (!violations || IS_ERR(violations))
> >  		goto out;
> >  
> > -	return 0;
> > +	ima_policy = securityfs_create_file("policy",
> > +					    S_IRUSR | S_IRGRP | S_IWUSR,
> > +					    ima_dir, NULL,
> > +					    &ima_measure_policy_ops);
> > +	if (!ima_policy || IS_ERR(ima_policy))
> > +		goto out;
> 
> Of course, James' same comment applies here :)

Yes, got that one too.

> > +/**
> > + * ima_parse_add_rule - add a rule to measure_policy_rules
> > + * @rule - ima measurement policy rule
> > + *
> > + * Uses a mutex to protect the policy list from multiple concurrent writers.
> > + * Returns 0 on success, an error code on failure.
> > + */
> > +int ima_parse_add_rule(char *rule)
> > +{
> > +	const char *op = "add_rule";
> > +	struct ima_measure_rule_entry *entry;
> > +	int result = 0;
> > +	int audit_info = 0;
> > +
> > +	/* Prevent installed policy from changing */
> > +	if (ima_measure != &measure_default_rules) {
> > +		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
> > +				    NULL, op, "already exists",
> > +				    -EACCES, audit_info);
> > +		return -EACCES;
> > +	}
> > +
> > +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > +	if (!entry) {
> > +		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
> > +				    NULL, op, "-ENOMEM", -ENOMEM, audit_info);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	INIT_LIST_HEAD(&entry->list);
> > +
> > +	result = ima_parse_rule(rule, entry);
> > +	if (!result) {
> > +		mutex_lock(&ima_measure_mutex);
> > +		list_add_tail(&entry->list, &measure_policy_rules);
> > +		mutex_unlock(&ima_measure_mutex);
> > +	}
> 
> Should you kfree(entry) if ima_parse_rule() failed?

yes, thanks.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/