diff mbox series

[v13,26/25] Audit: Multiple LSM support in audit rules

Message ID ee5e4cea-b6c1-fa12-30de-8fc9007d69e9@schaufler-ca.com (mailing list archive)
State Superseded
Headers show
Series LSM: Module stacking for AppArmor | expand

Commit Message

Casey Schaufler Jan. 3, 2020, 6:53 p.m. UTC
With multiple possible security modules supporting audit rule
it is necessary to keep separate data for each module in the
audit rules. This affects IMA as well, as it re-uses the audit
rule list mechanisms.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-integrity@vger.kernel.org
Cc: linux-audit@redhat.com
---
 include/linux/audit.h               |  4 +++-
 include/linux/security.h            |  8 +++----
 kernel/auditfilter.c                | 26 +++++++++++----------
 kernel/auditsc.c                    | 12 +++++-----
 security/integrity/ima/ima_policy.c | 36 +++++++++++++++++++----------
 security/security.c                 | 34 +++++++++++++++++++++++----
 6 files changed, 80 insertions(+), 40 deletions(-)

Comments

Mimi Zohar Jan. 9, 2020, 4:33 p.m. UTC | #1
Hi Casey,

On Fri, 2020-01-03 at 10:53 -0800, Casey Schaufler wrote:
> With multiple possible security modules supporting audit rule
> it is necessary to keep separate data for each module in the
> audit rules. This affects IMA as well, as it re-uses the audit
> rule list mechanisms.

While reviewing this patch, I realized there was a bug in the base IMA
code.  With Janne's bug fix, that he just posted, I think this patch
can now be simplified.

My main concern is the number of warning messages that will be
generated.  Any time a new LSM policy is loaded, the labels will be
re-evaulated whether or not they are applicable to the particular LSM,
causing unnecessary warnings.

Mimi
Casey Schaufler Jan. 10, 2020, 7:40 p.m. UTC | #2
On 1/9/2020 8:33 AM, Mimi Zohar wrote:
> Hi Casey,
>
> On Fri, 2020-01-03 at 10:53 -0800, Casey Schaufler wrote:
>> With multiple possible security modules supporting audit rule
>> it is necessary to keep separate data for each module in the
>> audit rules. This affects IMA as well, as it re-uses the audit
>> rule list mechanisms.
> While reviewing this patch, I realized there was a bug in the base IMA
> code.  With Janne's bug fix, that he just posted, I think this patch
> can now be simplified.

How and when do you plan to get Janne's fix in? It's looking like
stacking won't be in for 5.6.

> My main concern is the number of warning messages that will be
> generated.  Any time a new LSM policy is loaded, the labels will be
> re-evaulated whether or not they are applicable to the particular LSM,
> causing unnecessary warnings.

Uhg. 

>
> Mimi
>
Mimi Zohar Jan. 12, 2020, 3:37 p.m. UTC | #3
On Fri, 2020-01-10 at 11:40 -0800, Casey Schaufler wrote:
> On 1/9/2020 8:33 AM, Mimi Zohar wrote:
> > Hi Casey,
> >
> > On Fri, 2020-01-03 at 10:53 -0800, Casey Schaufler wrote:
> >> With multiple possible security modules supporting audit rule
> >> it is necessary to keep separate data for each module in the
> >> audit rules. This affects IMA as well, as it re-uses the audit
> >> rule list mechanisms.
> > While reviewing this patch, I realized there was a bug in the base IMA
> > code.  With Janne's bug fix, that he just posted, I think this patch
> > can now be simplified.
> 
> How and when do you plan to get Janne's fix in? It's looking like
> stacking won't be in for 5.6.

The patch is now in the next-integrity-testing branch.  We'll see how
it goes.

> 
> > My main concern is the number of warning messages that will be
> > generated.  Any time a new LSM policy is loaded, the labels will be
> > re-evaulated whether or not they are applicable to the particular LSM,
> > causing unnecessary warnings.
> 
> Uhg.
diff mbox series

Patch

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2ce0e8da3922..d4213c471801 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -11,6 +11,7 @@ 
 
 #include <linux/sched.h>
 #include <linux/ptrace.h>
+#include <linux/security.h>
 #include <uapi/linux/audit.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
@@ -64,8 +65,9 @@  struct audit_field {
 		kuid_t			uid;
 		kgid_t			gid;
 		struct {
+			bool		lsm_isset;
 			char		*lsm_str;
-			void		*lsm_rule;
+			void		*lsm_rules[LSMBLOB_ENTRIES];
 		};
 	};
 	u32				op;
diff --git a/include/linux/security.h b/include/linux/security.h
index 26967055a002..0bf71dd74a9c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1887,8 +1887,8 @@  static inline int security_key_getsecurity(struct key *key, char **_buffer)
 int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
 int security_audit_rule_known(struct audit_krule *krule);
 int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
-			      void *lsmrule);
-void security_audit_rule_free(void *lsmrule);
+			      void **lsmrule);
+void security_audit_rule_free(void **lsmrule);
 
 #else
 
@@ -1904,12 +1904,12 @@  static inline int security_audit_rule_known(struct audit_krule *krule)
 }
 
 static inline int security_audit_rule_match(struct lsmblob *blob, u32 field,
-					    u32 op, void *lsmrule)
+					    u32 op, void **lsmrule)
 {
 	return 0;
 }
 
-static inline void security_audit_rule_free(void *lsmrule)
+static inline void security_audit_rule_free(void **lsmrule)
 { }
 
 #endif /* CONFIG_SECURITY */
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index bf28bb599b6d..0f351d1f6ef9 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -74,7 +74,7 @@  static void audit_free_lsm_field(struct audit_field *f)
 	case AUDIT_OBJ_LEV_LOW:
 	case AUDIT_OBJ_LEV_HIGH:
 		kfree(f->lsm_str);
-		security_audit_rule_free(f->lsm_rule);
+		security_audit_rule_free(f->lsm_rules);
 	}
 }
 
@@ -517,7 +517,7 @@  static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			entry->rule.buflen += f->val;
 
 			err = security_audit_rule_init(f->type, f->op, str,
-						       (void **)&f->lsm_rule);
+						       f->lsm_rules);
 			/* Keep currently invalid fields around in case they
 			 * become valid after a policy reload. */
 			if (err == -EINVAL) {
@@ -528,8 +528,10 @@  static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			if (err) {
 				kfree(str);
 				goto exit_free;
-			} else
+			} else {
+				f->lsm_isset = true;
 				f->lsm_str = str;
+			}
 			break;
 		case AUDIT_WATCH:
 			str = audit_unpack_string(&bufp, &remain, f->val);
@@ -767,7 +769,7 @@  static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
 	return 0;
 }
 
-/* Duplicate LSM field information.  The lsm_rule is opaque, so must be
+/* Duplicate LSM field information.  The lsm_rules is opaque, so must be
  * re-initialized. */
 static inline int audit_dupe_lsm_field(struct audit_field *df,
 					   struct audit_field *sf)
@@ -781,9 +783,9 @@  static inline int audit_dupe_lsm_field(struct audit_field *df,
 		return -ENOMEM;
 	df->lsm_str = lsm_str;
 
-	/* our own (refreshed) copy of lsm_rule */
+	/* our own (refreshed) copy of lsm_rules */
 	ret = security_audit_rule_init(df->type, df->op, df->lsm_str,
-				       (void **)&df->lsm_rule);
+				       df->lsm_rules);
 	/* Keep currently invalid fields around in case they
 	 * become valid after a policy reload. */
 	if (ret == -EINVAL) {
@@ -835,7 +837,7 @@  struct audit_entry *audit_dupe_rule(struct audit_krule *old)
 	new->tree = old->tree;
 	memcpy(new->fields, old->fields, sizeof(struct audit_field) * fcount);
 
-	/* deep copy this information, updating the lsm_rule fields, because
+	/* deep copy this information, updating the lsm_rules fields, because
 	 * the originals will all be freed when the old rule is freed. */
 	for (i = 0; i < fcount; i++) {
 		switch (new->fields[i].type) {
@@ -1354,11 +1356,11 @@  int audit_filter(int msgtype, unsigned int listtype)
 			case AUDIT_SUBJ_TYPE:
 			case AUDIT_SUBJ_SEN:
 			case AUDIT_SUBJ_CLR:
-				if (f->lsm_rule) {
+				if (f->lsm_isset) {
 					security_task_getsecid(current, &blob);
 					result = security_audit_rule_match(
 							&blob, f->type,
-							f->op, f->lsm_rule);
+							f->op, f->lsm_rules);
 				}
 				break;
 			case AUDIT_EXE:
@@ -1385,7 +1387,7 @@  int audit_filter(int msgtype, unsigned int listtype)
 	return ret;
 }
 
-static int update_lsm_rule(struct audit_krule *r)
+static int update_lsm_rules(struct audit_krule *r)
 {
 	struct audit_entry *entry = container_of(r, struct audit_entry, rule);
 	struct audit_entry *nentry;
@@ -1417,7 +1419,7 @@  static int update_lsm_rule(struct audit_krule *r)
 	return err;
 }
 
-/* This function will re-initialize the lsm_rule field of all applicable rules.
+/* This function will re-initialize the lsm_rules field of all applicable rules.
  * It will traverse the filter lists serarching for rules that contain LSM
  * specific filter fields.  When such a rule is found, it is copied, the
  * LSM field is re-initialized, and the old rule is replaced with the
@@ -1432,7 +1434,7 @@  int audit_update_lsm_rules(void)
 
 	for (i = 0; i < AUDIT_NR_FILTERS; i++) {
 		list_for_each_entry_safe(r, n, &audit_rules_list[i], list) {
-			int res = update_lsm_rule(r);
+			int res = update_lsm_rules(r);
 			if (!err)
 				err = res;
 		}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 28fea2e73040..b9f81ef64c39 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -638,7 +638,7 @@  static int audit_filter_rules(struct task_struct *tsk,
 			   match for now to avoid losing information that
 			   may be wanted.   An error message will also be
 			   logged upon error */
-			if (f->lsm_rule) {
+			if (f->lsm_isset) {
 				if (need_sid) {
 					security_task_getsecid(tsk, &blob);
 					need_sid = 0;
@@ -646,7 +646,7 @@  static int audit_filter_rules(struct task_struct *tsk,
 				result = security_audit_rule_match(&blob,
 								   f->type,
 								   f->op,
-								   f->lsm_rule);
+								   f->lsm_rules);
 			}
 			break;
 		case AUDIT_OBJ_USER:
@@ -656,21 +656,21 @@  static int audit_filter_rules(struct task_struct *tsk,
 		case AUDIT_OBJ_LEV_HIGH:
 			/* The above note for AUDIT_SUBJ_USER...AUDIT_SUBJ_CLR
 			   also applies here */
-			if (f->lsm_rule) {
+			if (f->lsm_isset) {
 				/* Find files that match */
 				if (name) {
 					result = security_audit_rule_match(
 								&name->oblob,
 								f->type,
 								f->op,
-								f->lsm_rule);
+								f->lsm_rules);
 				} else if (ctx) {
 					list_for_each_entry(n, &ctx->names_list, list) {
 						if (security_audit_rule_match(
 								&n->oblob,
 								f->type,
 								f->op,
-								f->lsm_rule)) {
+								f->lsm_rules)) {
 							++result;
 							break;
 						}
@@ -681,7 +681,7 @@  static int audit_filter_rules(struct task_struct *tsk,
 					break;
 				if (security_audit_rule_match(&ctx->ipc.oblob,
 							      f->type, f->op,
-							      f->lsm_rule))
+							      f->lsm_rules))
 					++result;
 			}
 			break;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1c617ae74558..227993b8422d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -74,7 +74,7 @@  struct ima_rule_entry {
 	bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
 	int pcr;
 	struct {
-		void *rule;	/* LSM file metadata specific */
+		void *rules[LSMBLOB_ENTRIES];
 		void *args_p;	/* audit value */
 		int type;	/* audit type */
 	} lsm[MAX_LSM_RULES];
@@ -82,6 +82,16 @@  struct ima_rule_entry {
 	struct ima_template_desc *template;
 };
 
+static inline bool ima_lsm_isset(void *rules[])
+{
+	int i;
+
+	for (i = 0; i < LSMBLOB_ENTRIES; i++)
+		if (rules[i])
+			return true;
+	return false;
+}
+
 /*
  * Without LSM specific knowledge, the default policy can only be
  * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
@@ -252,9 +262,11 @@  __setup("ima_appraise_tcb", default_appraise_policy_setup);
 static void ima_lsm_free_rule(struct ima_rule_entry *entry)
 {
 	int i;
+	int r;
 
 	for (i = 0; i < MAX_LSM_RULES; i++) {
-		kfree(entry->lsm[i].rule);
+		for (r = 0; r < LSMBLOB_ENTRIES; r++)
+			kfree(entry->lsm[i].rules[r]);
 		kfree(entry->lsm[i].args_p);
 	}
 	kfree(entry);
@@ -277,7 +289,7 @@  static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 	memset(nentry->lsm, 0, sizeof_field(struct ima_rule_entry, lsm));
 
 	for (i = 0; i < MAX_LSM_RULES; i++) {
-		if (!entry->lsm[i].rule)
+		if (!ima_lsm_isset(entry->lsm[i].rules))
 			continue;
 
 		nentry->lsm[i].type = entry->lsm[i].type;
@@ -289,7 +301,7 @@  static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 		result = security_filter_rule_init(nentry->lsm[i].type,
 						   Audit_equal,
 						   nentry->lsm[i].args_p,
-						   &nentry->lsm[i].rule);
+						   nentry->lsm[i].rules);
 		if (result == -EINVAL)
 			pr_warn("ima: rule for LSM \'%d\' is undefined\n",
 				entry->lsm[i].type);
@@ -329,7 +341,7 @@  static void ima_lsm_update_rules(void)
 	list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
 		needs_update = 0;
 		for (i = 0; i < MAX_LSM_RULES; i++) {
-			if (entry->lsm[i].rule) {
+			if (ima_lsm_isset(entry->lsm[i].rules)) {
 				needs_update = 1;
 				break;
 			}
@@ -415,7 +427,7 @@  static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		int rc = 0;
 		struct lsmblob blob;
 
-		if (!rule->lsm[i].rule)
+		if (!ima_lsm_isset(rule->lsm[i].rules))
 			continue;
 
 		switch (i) {
@@ -426,7 +438,7 @@  static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			rc = security_filter_rule_match(&blob,
 							rule->lsm[i].type,
 							Audit_equal,
-							rule->lsm[i].rule);
+							rule->lsm[i].rules);
 			break;
 		case LSM_SUBJ_USER:
 		case LSM_SUBJ_ROLE:
@@ -434,7 +446,7 @@  static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			rc = security_filter_rule_match(&blob,
 							rule->lsm[i].type,
 							Audit_equal,
-							rule->lsm[i].rule);
+							rule->lsm[i].rules);
 		default:
 			break;
 		}
@@ -811,7 +823,7 @@  static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 {
 	int result;
 
-	if (entry->lsm[lsm_rule].rule)
+	if (ima_lsm_isset(entry->lsm[lsm_rule].rules))
 		return -EINVAL;
 
 	entry->lsm[lsm_rule].args_p = match_strdup(args);
@@ -822,8 +834,8 @@  static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 	result = security_filter_rule_init(entry->lsm[lsm_rule].type,
 					   Audit_equal,
 					   entry->lsm[lsm_rule].args_p,
-					   &entry->lsm[lsm_rule].rule);
-	if (!entry->lsm[lsm_rule].rule) {
+					   entry->lsm[lsm_rule].rules);
+	if (!ima_lsm_isset(entry->lsm[lsm_rule].rules)) {
 		kfree(entry->lsm[lsm_rule].args_p);
 		return -EINVAL;
 	}
@@ -1470,7 +1482,7 @@  int ima_policy_show(struct seq_file *m, void *v)
 	}
 
 	for (i = 0; i < MAX_LSM_RULES; i++) {
-		if (entry->lsm[i].rule) {
+		if (ima_lsm_isset(entry->lsm[i].rules)) {
 			switch (i) {
 			case LSM_OBJ_USER:
 				seq_printf(m, pt(Opt_obj_user),
diff --git a/security/security.c b/security/security.c
index e94de64e114c..4be490512ed2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2831,7 +2831,24 @@  int security_key_getsecurity(struct key *key, char **_buffer)
 
 int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
 {
-	return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
+	struct security_hook_list *hp;
+	bool one_is_good = false;
+	int rc = 0;
+	int trc;
+
+	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
+		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+			continue;
+		trc = hp->hook.audit_rule_init(field, op, rulestr,
+					       &lsmrule[hp->lsmid->slot]);
+		if (trc == 0)
+			one_is_good = true;
+		else
+			rc = trc;
+	}
+	if (one_is_good)
+		return 0;
+	return rc;
 }
 
 int security_audit_rule_known(struct audit_krule *krule)
@@ -2839,13 +2856,19 @@  int security_audit_rule_known(struct audit_krule *krule)
 	return call_int_hook(audit_rule_known, 0, krule);
 }
 
-void security_audit_rule_free(void *lsmrule)
+void security_audit_rule_free(void **lsmrule)
 {
-	call_void_hook(audit_rule_free, lsmrule);
+	struct security_hook_list *hp;
+
+	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
+		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+			continue;
+		hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
+	}
 }
 
 int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
-			      void *lsmrule)
+			      void **lsmrule)
 {
 	struct security_hook_list *hp;
 	int rc;
@@ -2854,7 +2877,8 @@  int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
 		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
 			continue;
 		rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
-					       field, op, lsmrule);
+					       field, op,
+					       &lsmrule[hp->lsmid->slot]);
 		if (rc != 0)
 			return rc;
 	}