Re: IMA: How to manage user space signing policy with others

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

On Fri, 2013-03-01 at 16:33 -0500, Vivek Goyal wrote:
> On Fri, Mar 01, 2013 at 02:39:13PM -0500, Mimi Zohar wrote:
> 
> [..]
> > I was suggesting that a builtin appraise rule chain and everything else
> > on the other chain.  Userspace could replace the other chain with
> > whatever they wanted, including additional appraisal rules.
> > 
> > > > Given the fact that policy file ABI is still in testing we should be
> > > > able to change semantics. (As currently user's appraise rules override
> > > > kernel's appraisal rules).
> > 
> > The userspace policy could only extend the appraisal rules.  We OR the
> > result of both chains, and use the more restrictive rule.
> 
> 
> So secureboot rules will go in builtin policy. tcb appraise rules and
> others will go in other policy. This other policy is replacable by
> user.
> 
> We OR the results of both chains and instead of using first matching
> rule, we choose a rule which is more restrictive and use that.
> 
> Is there always a clear relationship between rules. I mean one is more
> restrictive than other. There can not be part-overlapping rules?
> 
> [..]
> > We've already spoken about needing an additional hook or moving the
> > existing bprm hook.  Can we defer the memory caching requirements for
> > now?
> 
> Sure, additional hook is not a concern.
> 
> I can defer caching discussion but I think it is important to discuss
> it now. Because it might very well affect how do we decide to handle
> multiple appraise rules/policies. So please, if possible, let us not
> defer the caching requirement discussion.
> 
> My biggest concern is what if we decide to rule based caching option
> and rule gets skipped because of more restrictive rule present.
> 
> appraise func=bprm_check cache_status=no
> appraise fowner=root 
> 
> In above case second rule will override first one and that's not what
> we want.

I was thinking more in terms of merging flags.  Merging the flags in
your example would work.

appraise func=bprm_check appraise_type=optional cache_status=no
appraise fowner=root
example 2:  merging the flags results in the 'optional' flag being set

Unfortunately, in some cases, like in your example, the flag needs to be
set if either rule enables it.  In other cases, like in the second
example, the flag should be set only if both rules enable it.

As the 'ima_tcb' and 'ima_appraise_tcb' policies are also builtin, we
should probably use a different term to identify these new rules.  This
code snippet is only for illustration.

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 399433a..acc455b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -288,6 +288,15 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 			break;
 	}
 
+	list_for_each_entry(entry, ima_builtin_rules, list) {
+		if (!ima_match_rules(entry, inode, func, mask))
+			continue;
+		action |= entry->flags & IMA_ACTION_FLAGS;      <=== can't do blindly
+		action |= IMA_APPRAISE;
+		action &= ~IMA_FILE_APPRAISE; /* remove default subaction */
+		action |= get_subaction(entry, func);
+	}
+
 	return action;
 }
 

thanks,

Mimi