On Thu, 2013-02-28 at 20:49 -0500, Mimi Zohar wrote:
> On Thu, 2013-02-28 at 17:20 -0500, Eric Paris wrote:
> > The ima_tcb policy was meant to be larger than needed to determine a
> > trusted computing base, but it is clearly not a superset of what he is
> > hoping to accomplish.
The builtin measurement and appraisal policies are different. In order
not to miss a measurement, the measurement policy measures everything
read/executed by root. Userspace can constrain the policy by defining
rules based on LSM labels. The appraisal policy measures everything
owned by root. Userspace might want to add rules to appraise additional
files.
We can not OR the measurement builtin and userspace policies, as the
userspace policy constrains the builtin policy, but for appraisal we
could. Perhaps we should define two rule chains, one for the builtin
appraisal rules and another for all other rules.
When secure boot is defined, instead of having a NULL policy, the
default policy would be the secureboot integrity policy. These rules
would be added to the builtin appraisal rule chain. If the
'ima_appraise_tcb' boot commandline option is specified, these rules
would also be added to the builtin appraisal rule chain, but at the head
of the chain, as they are more restrictive than the secureboot policy
for root owned files.
Vivek, would this work?
thanks,
Mimi
On Fri, Mar 01, 2013 at 07:15:07AM -0500, Mimi Zohar wrote:
> On Thu, 2013-02-28 at 20:49 -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-28 at 17:20 -0500, Eric Paris wrote:
>
> > > The ima_tcb policy was meant to be larger than needed to determine a
> > > trusted computing base, but it is clearly not a superset of what he is
> > > hoping to accomplish.
>
> The builtin measurement and appraisal policies are different. In order
> not to miss a measurement, the measurement policy measures everything
> read/executed by root. Userspace can constrain the policy by defining
> rules based on LSM labels. The appraisal policy measures everything
> owned by root. Userspace might want to add rules to appraise additional
> files.
>
> We can not OR the measurement builtin and userspace policies, as the
> userspace policy constrains the builtin policy, but for appraisal we
> could. Perhaps we should define two rule chains, one for the builtin
> appraisal rules and another for all other rules.
Ok, just to make sure that I understand it right, I will summarize above.
So a user can overide/replace "measure and audit" rules but it can not
overide replace kernel's "appraise" rules and it can only append to
existing appraise rules.
So we internally define two rule chanins. All the appraisal rules
go in one rule chain and all other rules (measure and audit) go in
separate chain.
When user writes an "appraise" rule to "policy" file, it gets *appended*
to internal appraise rule chain and if user writes a "measure or audit"
rule to "policy" file, it replaces the kernel's rules with user's 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).
>
> When secure boot is defined, instead of having a NULL policy, the
> default policy would be the secureboot integrity policy. These rules
> would be added to the builtin appraisal rule chain. If the
> 'ima_appraise_tcb' boot commandline option is specified, these rules
> would also be added to the builtin appraisal rule chain, but at the head
> of the chain, as they are more restrictive than the secureboot policy
> for root owned files.
>
> Vivek, would this work?
This should work except the result caching issue. If we are running a
partially signed user space, then unsigned process can write to disk
directly (of course with right permisions). So secureboot policy can not
cache appraisal results.
In fact thinking more about it, I think ima_appraise_tcb policy also
is vulnerable. This policy will not appraise files which are not
owned by root. And users belonging to group "disk" have write permission
to disks.
So if I create a user "foo" and add it to group "disk", it can now launch
its own processes and write to disk. And write to root owned files and
ima_appraise_tcb policy will not detect the change.
Hence, if ima_appraise_tcb rules are put in front of secureboot rules,
caching appraisal results opens a security hole.
Thanks
Vivek
On Fri, Mar 01, 2013 at 10:28:40AM -0500, Vivek Goyal wrote:
> On Fri, Mar 01, 2013 at 07:15:07AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-28 at 20:49 -0500, Mimi Zohar wrote:
> > > On Thu, 2013-02-28 at 17:20 -0500, Eric Paris wrote:
> >
> > > > The ima_tcb policy was meant to be larger than needed to determine a
> > > > trusted computing base, but it is clearly not a superset of what he is
> > > > hoping to accomplish.
> >
> > The builtin measurement and appraisal policies are different. In order
> > not to miss a measurement, the measurement policy measures everything
> > read/executed by root. Userspace can constrain the policy by defining
> > rules based on LSM labels. The appraisal policy measures everything
> > owned by root. Userspace might want to add rules to appraise additional
> > files.
> >
> > We can not OR the measurement builtin and userspace policies, as the
> > userspace policy constrains the builtin policy, but for appraisal we
> > could. Perhaps we should define two rule chains, one for the builtin
> > appraisal rules and another for all other rules.
>
> Ok, just to make sure that I understand it right, I will summarize above.
>
> So a user can overide/replace "measure and audit" rules but it can not
> overide replace kernel's "appraise" rules and it can only append to
> existing appraise rules.
>
> So we internally define two rule chanins. All the appraisal rules
> go in one rule chain and all other rules (measure and audit) go in
> separate chain.
>
> When user writes an "appraise" rule to "policy" file, it gets *appended*
> to internal appraise rule chain and if user writes a "measure or audit"
> rule to "policy" file, it replaces the kernel's rules with user's 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).
>
> >
> > When secure boot is defined, instead of having a NULL policy, the
> > default policy would be the secureboot integrity policy. These rules
> > would be added to the builtin appraisal rule chain. If the
> > 'ima_appraise_tcb' boot commandline option is specified, these rules
> > would also be added to the builtin appraisal rule chain, but at the head
> > of the chain, as they are more restrictive than the secureboot policy
> > for root owned files.
> >
> > Vivek, would this work?
>
> This should work except the result caching issue. If we are running a
> partially signed user space, then unsigned process can write to disk
> directly (of course with right permisions). So secureboot policy can not
> cache appraisal results.
>
> In fact thinking more about it, I think ima_appraise_tcb policy also
> is vulnerable. This policy will not appraise files which are not
> owned by root. And users belonging to group "disk" have write permission
> to disks.
>
> So if I create a user "foo" and add it to group "disk", it can now launch
> its own processes and write to disk. And write to root owned files and
> ima_appraise_tcb policy will not detect the change.
>
> Hence, if ima_appraise_tcb rules are put in front of secureboot rules,
> caching appraisal results opens a security hole.
To avoid clashes between multiple built-in policies can we keep it
simpler. And that is only one built-in appraisal policy can be effective
a time. So if secureboot policy is effective, one can not use
ima_appraise_tcb.
We can provide one command line option to disable secureboot policy
(which works only if platform has secureboot disabled). So if a user
wants to use ima_appraise_tcb, he needs to pass two command line options.
"ima_apprise_secureboot=disable ima_appraise_tcb".
User can still append its appraise policies using "policy" interface. These
new rules take affect only if existing kernel policy does not apply to the
hook.
Thanks
Vivek
On Fri, 2013-03-01 at 13:40 -0500, Vivek Goyal wrote:
> On Fri, Mar 01, 2013 at 10:28:40AM -0500, Vivek Goyal wrote:
> > On Fri, Mar 01, 2013 at 07:15:07AM -0500, Mimi Zohar wrote:
> > > On Thu, 2013-02-28 at 20:49 -0500, Mimi Zohar wrote:
> > > > On Thu, 2013-02-28 at 17:20 -0500, Eric Paris wrote:
> > >
> > > > > The ima_tcb policy was meant to be larger than needed to determine a
> > > > > trusted computing base, but it is clearly not a superset of what he is
> > > > > hoping to accomplish.
> > >
> > > The builtin measurement and appraisal policies are different. In order
> > > not to miss a measurement, the measurement policy measures everything
> > > read/executed by root. Userspace can constrain the policy by defining
> > > rules based on LSM labels. The appraisal policy measures everything
> > > owned by root. Userspace might want to add rules to appraise additional
> > > files.
> > >
> > > We can not OR the measurement builtin and userspace policies, as the
> > > userspace policy constrains the builtin policy, but for appraisal we
> > > could. Perhaps we should define two rule chains, one for the builtin
> > > appraisal rules and another for all other rules.
> >
> > Ok, just to make sure that I understand it right, I will summarize above.
> >
> > So a user can overide/replace "measure and audit" rules but it can not
> > overide replace kernel's "appraise" rules and it can only append to
> > existing appraise rules.
> >
> > So we internally define two rule chanins. All the appraisal rules
> > go in one rule chain and all other rules (measure and audit) go in
> > separate chain.
> >
> > When user writes an "appraise" rule to "policy" file, it gets *appended*
> > to internal appraise rule chain and if user writes a "measure or audit"
> > rule to "policy" file, it replaces the kernel's rules with user's rules.
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.
> > >
> > > When secure boot is defined, instead of having a NULL policy, the
> > > default policy would be the secureboot integrity policy. These rules
> > > would be added to the builtin appraisal rule chain. If the
> > > 'ima_appraise_tcb' boot commandline option is specified, these rules
> > > would also be added to the builtin appraisal rule chain, but at the head
> > > of the chain, as they are more restrictive than the secureboot policy
> > > for root owned files.
> > >
> > > Vivek, would this work?
> >
> > This should work except the result caching issue. If we are running a
> > partially signed user space, then unsigned process can write to disk
> > directly (of course with right permisions). So secureboot policy can not
> > cache appraisal results.
> >
> > In fact thinking more about it, I think ima_appraise_tcb policy also
> > is vulnerable. This policy will not appraise files which are not
> > owned by root. And users belonging to group "disk" have write permission
> > to disks.
> >
> > So if I create a user "foo" and add it to group "disk", it can now launch
> > its own processes and write to disk. And write to root owned files and
> > ima_appraise_tcb policy will not detect the change.
> >
> > Hence, if ima_appraise_tcb rules are put in front of secureboot rules,
> > caching appraisal results opens a security hole.
We've already spoken about needing an additional hook or moving the
existing bprm hook. Can we defer the memory caching requirements for
now?
> To avoid clashes between multiple built-in policies can we keep it
> simpler. And that is only one built-in appraisal policy can be effective
> a time. So if secureboot policy is effective, one can not use
> ima_appraise_tcb.
After thinking about it some more and discussing it with Dave, the
built-in appraisal policy would be a fixed policy. For now, secureboot
would use it to define their policy. For now, there is no need to
include the ima_appraise_tcb rules in the builtin/fixed chain. They
could continue to be defined in the other chain.
> We can provide one command line option to disable secureboot policy
> (which works only if platform has secureboot disabled). So if a user
> wants to use ima_appraise_tcb, he needs to pass two command line options.
>
> "ima_apprise_secureboot=disable ima_appraise_tcb".
>
> User can still append its appraise policies using "policy" interface.
Right, so with the changes suggested above, this wouldn't be needed.
> These
> new rules take affect only if existing kernel policy does not apply to the
> hook.
As mentioned above, the more restrictive policy would be used.
thanks,
Mimi
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.
Thanks
Vivek
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
On Sun, Mar 03, 2013 at 04:42:24PM -0500, Mimi Zohar wrote:
[..]
> > > 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.
Hi Mimi,
If we decide to merge flags, then practically we modified the
ima_appraise_tcb policy. ima_appraise_tcb policy expects to cache the
results and we will not do that. And this conflict just grows if we
are forced to add more options in future.
Also as you mentioned that in some cases flag merging is OR operation
and in another cases it might be AND operation. And we will most likely
end up hardcoding all this. I think slowly this is getting complicated
and as people add more complex rules things can quickly get out of hand.
I am wondering that why are we trying to make multiple policies work
together. Can we try to keep it simple and say that at one point of
time only one policy can be effective. It could either be a built in
policy or user defined one. In fact that's how things are working right now.
User defined policy replaces built-in policy.
For the sake of backward compatibility "ima_tcb" and "ima_appraise_tcb"
can co-exist together (like today). But ima_secureboot_policy will not
be compatible with other policies. I understand that there might be a
desire to use multiple policies together down the line, but I guess in
that case policies need to specified using "policy" interface. And
ima_secureboot will be odd man out here as it can not trust the root
to specify policy. So practically ima_secureboot will be disabled.
We just have to provide an IMA interface so that caller can query what's
the effective policy currently. Say, IMA_POLICY_SECUREBOOT,
IMA_POLICY_TCB, or IMA_POLICY_USER. Caller of the bprm_check() or
bprm_post_load() can also check for current policy in force and give
CAP_SIGNED only if desired policy is in effect.
This reduces our options but trying to make multiple policies co-exist
together is just making it complicated. We can take it up again when
somebody has a strong use case of using secureboot policy along with
other policies. In fact a user can still define a custom policy which
is mix of multiple policies. Just that it is not compatible with
"secureboot" policy because for that we can't trust "root" to define
policy.
Thanks
Vivek
On Mon, Mar 04, 2013 at 10:29:19AM -0500, Vivek Goyal wrote:
[..]
> This reduces our options but trying to make multiple policies co-exist
> together is just making it complicated. We can take it up again when
> somebody has a strong use case of using secureboot policy along with
> other policies.
Well, I also see the unused hook for module verification. Right now there
is no policy for that but if we ever decide to do module verification
using ima hook, then we will have this question that where does that
rule go in now.
Thanks
Vivek
On Mon, 2013-03-04 at 10:29 -0500, Vivek Goyal wrote:
[...]
> Hi Mimi,
>
> If we decide to merge flags, then practically we modified the
> ima_appraise_tcb policy. ima_appraise_tcb policy expects to cache the
> results and we will not do that. And this conflict just grows if we
> are forced to add more options in future.
>
> Also as you mentioned that in some cases flag merging is OR operation
> and in another cases it might be AND operation. And we will most likely
> end up hardcoding all this. I think slowly this is getting complicated
> and as people add more complex rules things can quickly get out of hand.
>
> I am wondering that why are we trying to make multiple policies work
> together. Can we try to keep it simple and say that at one point of
> time only one policy can be effective. It could either be a built in
> policy or user defined one. In fact that's how things are working right now.
> User defined policy replaces built-in policy.
>
> For the sake of backward compatibility "ima_tcb" and "ima_appraise_tcb"
> can co-exist together (like today). But ima_secureboot_policy will not
> be compatible with other policies. I understand that there might be a
> desire to use multiple policies together down the line, but I guess in
> that case policies need to specified using "policy" interface. And
> ima_secureboot will be odd man out here as it can not trust the root
> to specify policy. So practically ima_secureboot will be disabled.
>
> We just have to provide an IMA interface so that caller can query what's
> the effective policy currently. Say, IMA_POLICY_SECUREBOOT,
> IMA_POLICY_TCB, or IMA_POLICY_USER. Caller of the bprm_check() or
> bprm_post_load() can also check for current policy in force and give
> CAP_SIGNED only if desired policy is in effect.
>
> This reduces our options but trying to make multiple policies co-exist
> together is just making it complicated. We can take it up again when
> somebody has a strong use case of using secureboot policy along with
> other policies. In fact a user can still define a custom policy which
> is mix of multiple policies. Just that it is not compatible with
> "secureboot" policy because for that we can't trust "root" to define
> policy.
Let me get this straight. You're suggesting that distros/users will
need to make a Kconfig build decision of enabling secureboot, including
the secureboot built-in policy, or be allowed to enable other integrity
policies. If RH enables secureboot, then no other integrity policy will
be permitted. Is that what you're saying, and if so, why would I agree
to this?
thanks,
Mimi
I think that is what he was suggesting. It reuses the integrity code
but it loses the integrity flexibility. I don't think it is a good
solution :-(
On Mon, Mar 4, 2013 at 1:59 PM, Mimi Zohar <[email protected]> wrote:
> On Mon, 2013-03-04 at 10:29 -0500, Vivek Goyal wrote:
> [...]
>
>> Hi Mimi,
>>
>> If we decide to merge flags, then practically we modified the
>> ima_appraise_tcb policy. ima_appraise_tcb policy expects to cache the
>> results and we will not do that. And this conflict just grows if we
>> are forced to add more options in future.
>>
>> Also as you mentioned that in some cases flag merging is OR operation
>> and in another cases it might be AND operation. And we will most likely
>> end up hardcoding all this. I think slowly this is getting complicated
>> and as people add more complex rules things can quickly get out of hand.
>>
>> I am wondering that why are we trying to make multiple policies work
>> together. Can we try to keep it simple and say that at one point of
>> time only one policy can be effective. It could either be a built in
>> policy or user defined one. In fact that's how things are working right now.
>> User defined policy replaces built-in policy.
>>
>> For the sake of backward compatibility "ima_tcb" and "ima_appraise_tcb"
>> can co-exist together (like today). But ima_secureboot_policy will not
>> be compatible with other policies. I understand that there might be a
>> desire to use multiple policies together down the line, but I guess in
>> that case policies need to specified using "policy" interface. And
>> ima_secureboot will be odd man out here as it can not trust the root
>> to specify policy. So practically ima_secureboot will be disabled.
>>
>> We just have to provide an IMA interface so that caller can query what's
>> the effective policy currently. Say, IMA_POLICY_SECUREBOOT,
>> IMA_POLICY_TCB, or IMA_POLICY_USER. Caller of the bprm_check() or
>> bprm_post_load() can also check for current policy in force and give
>> CAP_SIGNED only if desired policy is in effect.
>>
>> This reduces our options but trying to make multiple policies co-exist
>> together is just making it complicated. We can take it up again when
>> somebody has a strong use case of using secureboot policy along with
>> other policies. In fact a user can still define a custom policy which
>> is mix of multiple policies. Just that it is not compatible with
>> "secureboot" policy because for that we can't trust "root" to define
>> policy.
>
> Let me get this straight. You're suggesting that distros/users will
> need to make a Kconfig build decision of enabling secureboot, including
> the secureboot built-in policy, or be allowed to enable other integrity
> policies. If RH enables secureboot, then no other integrity policy will
> be permitted. Is that what you're saying, and if so, why would I agree
> to this?
>
> thanks,
>
> Mimi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 04, 2013 at 01:59:41PM -0500, Mimi Zohar wrote:
> On Mon, 2013-03-04 at 10:29 -0500, Vivek Goyal wrote:
> [...]
>
> > Hi Mimi,
> >
> > If we decide to merge flags, then practically we modified the
> > ima_appraise_tcb policy. ima_appraise_tcb policy expects to cache the
> > results and we will not do that. And this conflict just grows if we
> > are forced to add more options in future.
> >
> > Also as you mentioned that in some cases flag merging is OR operation
> > and in another cases it might be AND operation. And we will most likely
> > end up hardcoding all this. I think slowly this is getting complicated
> > and as people add more complex rules things can quickly get out of hand.
> >
> > I am wondering that why are we trying to make multiple policies work
> > together. Can we try to keep it simple and say that at one point of
> > time only one policy can be effective. It could either be a built in
> > policy or user defined one. In fact that's how things are working right now.
> > User defined policy replaces built-in policy.
> >
> > For the sake of backward compatibility "ima_tcb" and "ima_appraise_tcb"
> > can co-exist together (like today). But ima_secureboot_policy will not
> > be compatible with other policies. I understand that there might be a
> > desire to use multiple policies together down the line, but I guess in
> > that case policies need to specified using "policy" interface. And
> > ima_secureboot will be odd man out here as it can not trust the root
> > to specify policy. So practically ima_secureboot will be disabled.
> >
> > We just have to provide an IMA interface so that caller can query what's
> > the effective policy currently. Say, IMA_POLICY_SECUREBOOT,
> > IMA_POLICY_TCB, or IMA_POLICY_USER. Caller of the bprm_check() or
> > bprm_post_load() can also check for current policy in force and give
> > CAP_SIGNED only if desired policy is in effect.
> >
> > This reduces our options but trying to make multiple policies co-exist
> > together is just making it complicated. We can take it up again when
> > somebody has a strong use case of using secureboot policy along with
> > other policies. In fact a user can still define a custom policy which
> > is mix of multiple policies. Just that it is not compatible with
> > "secureboot" policy because for that we can't trust "root" to define
> > policy.
>
> Let me get this straight. You're suggesting that distros/users will
> need to make a Kconfig build decision of enabling secureboot, including
> the secureboot built-in policy, or be allowed to enable other integrity
> policies. If RH enables secureboot, then no other integrity policy will
> be permitted. Is that what you're saying, and if so, why would I agree
> to this?
I was thinking more in terms of being able to disable a policy based on
command line option. So a policy gets compiled in based on config options
and then one should be able to disable that policy. For case of
secureboot, policy will be disabled only if secureboot is not enabled
in the platform.
I am just brain storming and throwing some ideas and see if soemthing
makes sense. I agree that allowing one policy only makes it very
restrictive (while simplifying the implementation).
Thanks
Vivek
On Sun, Mar 03, 2013 at 04:42:24PM -0500, Mimi Zohar wrote:
[..]
> 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;
> }
Hi Mimi,
Based on your code, I have written code to first go through builtin
appraise rule list and then go through ima_rules list. If there is
a conflict of appraise rule in two lists, then conflict is resolved
and a more restrictive rule is picked.
Please have a look. I have yet to test it. Just wanted to show how
it is shaping up.
Also this is on top of some other pathces. So please ignore the code
which looks unfamiliar.
Thanks
Vivek
---
security/integrity/ima/ima_policy.c | 79 ++++++++++++++++++++++++++++++++++--
1 file changed, 75 insertions(+), 4 deletions(-)
Index: linux-2.6/security/integrity/ima/ima_policy.c
===================================================================
--- linux-2.6.orig/security/integrity/ima/ima_policy.c 2013-03-04 14:11:48.000000000 -0500
+++ linux-2.6/security/integrity/ima/ima_policy.c 2013-03-04 16:35:28.582077167 -0500
@@ -120,6 +120,8 @@ static LIST_HEAD(ima_default_rules);
static LIST_HEAD(ima_policy_rules);
static struct list_head *ima_rules;
+static LIST_HEAD(ima_builtin_rules);
+
static DEFINE_MUTEX(ima_rules_mutex);
static bool ima_use_tcb __initdata;
@@ -263,6 +265,40 @@ static int get_subaction(struct ima_rule
}
}
+/*
+ * If two appraise rules from two chains apply, then more restrictive rule
+ * properties are retained
+ * @action: action so far after processing first chain
+ * @entry: new conflicting rule
+ * @func: ima hook function
+ */
+static int fix_appraise_rule_conflict(int action, struct ima_rule_entry *entry,
+ enum ima_hooks func)
+{
+ /* Appraisal optional is set only if both rules are optional */
+ if (action & IMA_APPRAISAL_OPT && !(entry->flags & IMA_APPRAISAL_OPT)) {
+ action &= ~IMA_APPRAISAL_OPT;
+ /*
+ * If first rule is optional and second one is not, then more
+ * restrictive rule is second one. Subaction belongs to more
+ * restrictive rule as effectively that's what is being
+ * enforced.
+ */
+ action &= ~IMA_APPRAISE_SUBMASK;
+ action |= get_subaction(entry, func);
+ }
+
+ /* IMA_DIGSIG_REQUIRED gets set if any of the rules has it */
+ if (entry->flags & IMA_DIGSIG_REQUIRED)
+ action |= IMA_DIGSIG_REQUIRED;
+
+ /*
+ * TODO: If one rule wants result caching and other does not, then
+ * final result should not be cached.
+ */
+ return action;
+}
+
/**
* ima_match_policy - decision based on LSM and other conditions
* @inode: pointer to an inode for which the policy decision is being made
@@ -282,8 +318,8 @@ int ima_match_policy(struct inode *inode
struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
- list_for_each_entry(entry, ima_rules, list) {
-
+ /* First go through builtin appraise rules */
+ list_for_each_entry(entry, &ima_builtin_rules, list) {
if (!(entry->action & actmask))
continue;
@@ -303,6 +339,40 @@ int ima_match_policy(struct inode *inode
if (!actmask)
break;
+
+ }
+
+ /*
+ * Now go through ima_rules list. If there is an appraise rule conflict
+ * from previous list, pick more restrictive rule
+ */
+ actmask = flags | (flags << 1);
+ list_for_each_entry(entry, ima_rules, list) {
+
+ if (!(entry->action & actmask))
+ continue;
+
+ if (!ima_match_rules(entry, inode, func, mask))
+ continue;
+
+ if ((entry->action & IMA_APPRAISE) && (action & IMA_APPRAISE)) {
+ /* appraise rule from two chanins collide */
+ action = fix_appraise_rule_conflict(action, entry,
+ func);
+ } else {
+ action |= entry->flags & IMA_ACTION_FLAGS;
+ action |= entry->action & IMA_DO_MASK;
+ if (entry->action & IMA_APPRAISE)
+ action |= get_subaction(entry, func);
+ }
+
+ if (entry->action & IMA_DO_MASK)
+ actmask &= ~(entry->action | entry->action << 1);
+ else
+ actmask &= ~(entry->action | entry->action >> 1);
+
+ if (!actmask)
+ break;
}
return action;
@@ -335,13 +405,14 @@ void __init ima_init_policy(void)
}
}
+ ima_rules = &ima_default_rules;
+
/* Load other built-in poilcies */
appraise_entries = ARRAY_SIZE(default_memlock_exec_appraise_rules);
for (i = 0; i < appraise_entries; i++) {
list_add_tail(&default_memlock_exec_appraise_rules[i].list,
- &ima_default_rules);
+ &ima_builtin_rules);
}
- ima_rules = &ima_default_rules;
}
/**
On Mon, 2013-03-04 at 14:15 -0500, Vivek Goyal wrote:
> I am just brain storming and throwing some ideas and see if soemthing
> makes sense. I agree that allowing one policy only makes it very
> restrictive (while simplifying the implementation).
Agreed, lets try again ... I think we are actually getting closer.
Without the memory locking or caching issues, would you agree that both
the builtin 'secureboot' and the 'ima_appraise_tcb' policies meet the
secure boot needs? If both policies are acceptable, then we could
define the builtin 'secureboot' policy as the default policy, which
could be replaced with the 'ima_appraise_tcb' policy, if specified on
the boot command line. This would eliminate any need for merging of
rules or rule flags.
To address the memory locking and caching issues, we could define a new
extended attribute type called IMA_XATTR_SECURE_BOOT.
enum evm_ima_xattr_type {
IMA_XATTR_DIGEST = 0x01,
EVM_XATTR_HMAC,
EVM_IMA_XATTR_DIGSIG,
};
and set a corresponding flag in iint->flags. The flag could then be the
bases for setting up any special secureboot requirements, like memory
locking and no caching.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ddeadc7..6ec1575 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -172,6 +172,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
}
status = INTEGRITY_PASS;
break;
+ case IMA_XATTR_SECURE_BOOT:
+ iint->flags |= IMA_SB;
case EVM_IMA_XATTR_DIGSIG:
iint->flags |= IMA_DIGSIG;
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
As originally intended, the policy defines which files are appraised,
not how they are appraised. The extended attribute defines how the file
is to be appraised.
thanks,
Mimi
On Mon, Mar 04, 2013 at 08:21:31PM -0500, Mimi Zohar wrote:
> On Mon, 2013-03-04 at 14:15 -0500, Vivek Goyal wrote:
>
> > I am just brain storming and throwing some ideas and see if soemthing
> > makes sense. I agree that allowing one policy only makes it very
> > restrictive (while simplifying the implementation).
>
> Agreed, lets try again ... I think we are actually getting closer.
>
> Without the memory locking or caching issues, would you agree that both
> the builtin 'secureboot' and the 'ima_appraise_tcb' policies meet the
> secure boot needs?
Hi Mimi,
I want to be little careful while calling it secureboot policy. Ideally
secureboot would want all the software running to be signed. But we are
not planning to go to that extent. We are not planning to sign whole
of the user space. So it is not exactly a "secureboot" policy. I am
not sure what to call it. Say "memlock_exec", which signifies that
verify signatures of memlocked executables.
Very fact that we are planning to sign only select executables, requires
us to lock them in memory and assign special capability. If we had signed
whole of the user space, then ideally no untrusted application should
run and no trusted entity will attack process address space in memory
or on swap disk. So we will not need to memlock executable and we will
not need to disable ptrace for signed executables.
So while ima_appraise_tcb might meet strict secureboot needs partially (it
appraises only root owned files), but it does not meet the requirements
of partially signed user space executables.
Though need of this arises because of secureboot feature in firmware, what
I am trying to implement is not exactly a secureboot policy. It is just
one policy which tries to memlock signed executables, verify signature
and assign them special capability upon successful signature verification.
And there can be multiple variation to this policy based on config
options. For example,
- Whether to appraise only root files or all the executables. I think this
would be a config option.
- Provide config option so that verification is not optional. This will
be needed if one is creating a fully signed user space.
So basically policy can vary depending on what user wants and what kind
of system it is creating (based on config options). So we need to keep
it in mind while coming up with a solution.
> If both policies are acceptable, then we could
> define the builtin 'secureboot' policy as the default policy, which
> could be replaced with the 'ima_appraise_tcb' policy, if specified on
> the boot command line. This would eliminate any need for merging of
> rules or rule flags.
Given the fact that a with the help of config options one might want to
appraise all executables, ima_appraise_tcb will not be exact replacement.
So for my specific use case of "kdump in secureboot" environment, it will
work as /sbin/kexec is root owned. But if somebody decides to extend the
policy a bit to also appraise non-root files, then ima_appraise_tcb will
not be a replacement.
>
> To address the memory locking and caching issues, we could define a new
> extended attribute type called IMA_XATTR_SECURE_BOOT.
>
> enum evm_ima_xattr_type {
> IMA_XATTR_DIGEST = 0x01,
> EVM_XATTR_HMAC,
> EVM_IMA_XATTR_DIGSIG,
> };
>
> and set a corresponding flag in iint->flags. The flag could then be the
> bases for setting up any special secureboot requirements, like memory
> locking and no caching.
I have few concerns here.
- I don't think secureboot mandates that executables should be locked in
memory. It depends on whether all the user space is signed or not and
that will depend on what distributions are shipping or what kind of
image user has created. So hardcoding things behind IMA_XATTR_SECURE_BOOT
might be odd.
- Also memory locking will be done by the caller as binary loader knows
how to load file. IMA just needs to tell loader that this file is
digitally signed (possibly in bprm_check). And then loader can load
the binary, memlock it and can call into post_load hook and ask IMA if
integrity is fine or not.
A pseudo code might look as follows.
bprm_post_check();
if (bprm->unsafe & LSM_UNSAFE_DIGSIG) {
/* Executable is signed. Memlock it */
}
load_various_segemnts;
retval = bprm_post_load();
if (retval) {
/* handle error */
}
/*
* Binary is signed. Signature verification done while locked in
* memory. Raise CAP_SIGNED capability.
*/
cap_raise(CAP_SIGNED);
And all this code can be under various config options. And same config
option will vary the IMA policy accordingly.
- xattr value seem to be reflecting type of signatures. And secureboot as
such is not really a type of signature. So I find it odd to mark such
files as secureboot. That means while signing also user will have to
specify in evmctl that mark the signature as secureboot. And that will
look very ugly. There is no such thing as secureboot signature.
Can we do following. (Just modifying your proposal little bit).
- Implement a new policy say ima_mem_exec. This policy can vary based on
config options. This will be the default policy.
- ima_mem_exec will be default policy and it can be disabled by passing
a command line option ima_mem_exec_disable.
- If user wants to use ima_apprase_tcb policy, they can pass two command
line option. (ima_mem_exec_disable and ima_appraise_tcb).
- Similary if user wants to put its own policy using "policy" interface,
they need to boot kernel with command line option "ima_mem_exec_disable".
In the end, this is again "either A or B" mechanism. Both ima_mem_exec
and ima_appraise_tcb are not co-existing. Comand line option just enables
choosing one over other.
The fact that we are able to replace ima_mem_exec policy using command
line, binary loader will need a way to query IMA to find what's the
current policy. If ima_mem_exec has been replaced, then binary loader
will not memlock files and will not raise extra capability to binary. And
this will disable kdump functionality on secureboot platforms. (Something
which I don't like much).
Thanks
Vivek
>
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ddeadc7..6ec1575 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -172,6 +172,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> }
> status = INTEGRITY_PASS;
> break;
> + case IMA_XATTR_SECURE_BOOT:
> + iint->flags |= IMA_SB;
> case EVM_IMA_XATTR_DIGSIG:
> iint->flags |= IMA_DIGSIG;
> rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>
> As originally intended, the policy defines which files are appraised,
> not how they are appraised. The extended attribute defines how the file
> is to be appraised.
>
> thanks,
>
> Mimi
On Tue, 2013-03-05 at 10:18 -0500, Vivek Goyal wrote:
> Can we do following. (Just modifying your proposal little bit).
>
> - Implement a new policy say ima_mem_exec. This policy can vary based on
> config options. This will be the default policy.
Just to clarify, the default is the existing null policy. When
'secureboot' is enabled, ima_mem_exec will be the default policy.
> - ima_mem_exec will be default policy and it can be disabled by passing
> a command line option ima_mem_exec_disable.
>
> - If user wants to use ima_apprase_tcb policy, they can pass two command
> line option. (ima_mem_exec_disable and ima_appraise_tcb).
Both aren't really needed. Nothing changes for existing users, if
'ima_appraise_tcb' replaces the ima_mem_exec policy.
> - Similary if user wants to put its own policy using "policy" interface,
> they need to boot kernel with command line option "ima_mem_exec_disable".
Not a good idea, as this would be a new requirement for existing users.
Invert the logic.
> In the end, this is again "either A or B" mechanism. Both ima_mem_exec
> and ima_appraise_tcb are not co-existing. Comand line option just enables
> choosing one over other.
Does this impact 'ima_tcb' or only 'ima_appraise_tcb'?
> The fact that we are able to replace ima_mem_exec policy using command
> line, binary loader will need a way to query IMA to find what's the
> current policy. If ima_mem_exec has been replaced, then binary loader
> will not memlock files and will not raise extra capability to binary. And
> this will disable kdump functionality on secureboot platforms. (Something
> which I don't like much).
Ok
thanks,
Mimi
On Tue, Mar 05, 2013 at 03:40:18PM -0500, Mimi Zohar wrote:
> On Tue, 2013-03-05 at 10:18 -0500, Vivek Goyal wrote:
>
> > Can we do following. (Just modifying your proposal little bit).
> >
> > - Implement a new policy say ima_mem_exec. This policy can vary based on
> > config options. This will be the default policy.
>
> Just to clarify, the default is the existing null policy. When
> 'secureboot' is enabled, ima_mem_exec will be the default policy.
Ok, I can do that. I will compile in memlock_exec_appraise policy based on
config option. But this policy will take affect only if.
- user enables it using command line option ima_memlock_exec_appraise
- Or secureboot is enabled.
So that way, systems which don't have secureboot enabled, will not see
the overhead of memlock_exec_appraise policy.
>
> > - ima_mem_exec will be default policy and it can be disabled by passing
> > a command line option ima_mem_exec_disable.
> >
> > - If user wants to use ima_apprase_tcb policy, they can pass two command
> > line option. (ima_mem_exec_disable and ima_appraise_tcb).
>
> Both aren't really needed. Nothing changes for existing users, if
> 'ima_appraise_tcb' replaces the ima_mem_exec policy.
Ok, I can allow ima_appraise_tcb to replace memlock_exec_appraise policy
(even with secureboot enabled). For the time being this sould not be a
problem as only side effect is that kdump will not be available on
secureboot platoforms.
But if people start building more functionality on user executable
signing policy, then we might have some issues. I guess will handle that
once new use cases come up.
>
> > - Similary if user wants to put its own policy using "policy" interface,
> > they need to boot kernel with command line option "ima_mem_exec_disable".
>
> Not a good idea, as this would be a new requirement for existing users.
> Invert the logic.
Sure. I will allow root to override in-built appraise policy using
"policy" interface.
>
> > In the end, this is again "either A or B" mechanism. Both ima_mem_exec
> > and ima_appraise_tcb are not co-existing. Comand line option just enables
> > choosing one over other.
>
> Does this impact 'ima_tcb' or only 'ima_appraise_tcb'?
I think I can keep ima_tcb and ima_appraise_tcb as it is. That is both
the measure and appraise rules go in single policy (like today).
>
> > The fact that we are able to replace ima_mem_exec policy using command
> > line, binary loader will need a way to query IMA to find what's the
> > current policy. If ima_mem_exec has been replaced, then binary loader
> > will not memlock files and will not raise extra capability to binary. And
> > this will disable kdump functionality on secureboot platforms. (Something
> > which I don't like much).
>
> Ok
Mimi, so you like this idea better than the other idea of keeping two
policy chains and running more restrictive rule while resolving flag
conflicts between two rules?
I have written some patches to maintain two rule chains and running
more restrictive rule. I can change it though.
Thanks
Vivek
On Tue, 2013-03-05 at 16:53 -0500, Vivek Goyal wrote:
> On Tue, Mar 05, 2013 at 03:40:18PM -0500, Mimi Zohar wrote:
> > On Tue, 2013-03-05 at 10:18 -0500, Vivek Goyal wrote:
> >
> > > Can we do following. (Just modifying your proposal little bit).
> > >
> > > - Implement a new policy say ima_mem_exec. This policy can vary based on
> > > config options. This will be the default policy.
> >
> > Just to clarify, the default is the existing null policy. When
> > 'secureboot' is enabled, ima_mem_exec will be the default policy.
>
> Ok, I can do that. I will compile in memlock_exec_appraise policy based on
> config option. But this policy will take affect only if.
>
> - user enables it using command line option ima_memlock_exec_appraise
> - Or secureboot is enabled.
>
> So that way, systems which don't have secureboot enabled, will not see
> the overhead of memlock_exec_appraise policy.
>
> >
> > > - ima_mem_exec will be default policy and it can be disabled by passing
> > > a command line option ima_mem_exec_disable.
> > >
> > > - If user wants to use ima_apprase_tcb policy, they can pass two command
> > > line option. (ima_mem_exec_disable and ima_appraise_tcb).
> >
> > Both aren't really needed. Nothing changes for existing users, if
> > 'ima_appraise_tcb' replaces the ima_mem_exec policy.
>
> Ok, I can allow ima_appraise_tcb to replace memlock_exec_appraise policy
> (even with secureboot enabled). For the time being this sould not be a
> problem as only side effect is that kdump will not be available on
> secureboot platoforms.
>
> But if people start building more functionality on user executable
> signing policy, then we might have some issues. I guess will handle that
> once new use cases come up.
>
> >
> > > - Similary if user wants to put its own policy using "policy" interface,
> > > they need to boot kernel with command line option "ima_mem_exec_disable".
> >
> > Not a good idea, as this would be a new requirement for existing users.
> > Invert the logic.
>
> Sure. I will allow root to override in-built appraise policy using
> "policy" interface.
>
> >
> > > In the end, this is again "either A or B" mechanism. Both ima_mem_exec
> > > and ima_appraise_tcb are not co-existing. Comand line option just enables
> > > choosing one over other.
> >
> > Does this impact 'ima_tcb' or only 'ima_appraise_tcb'?
>
> I think I can keep ima_tcb and ima_appraise_tcb as it is. That is both
> the measure and appraise rules go in single policy (like today).
>
> >
> > > The fact that we are able to replace ima_mem_exec policy using command
> > > line, binary loader will need a way to query IMA to find what's the
> > > current policy. If ima_mem_exec has been replaced, then binary loader
> > > will not memlock files and will not raise extra capability to binary. And
> > > this will disable kdump functionality on secureboot platforms. (Something
> > > which I don't like much).
> >
> > Ok
>
> Mimi, so you like this idea better than the other idea of keeping two
> policy chains and running more restrictive rule while resolving flag
> conflicts between two rules?
>
> I have written some patches to maintain two rule chains and running
> more restrictive rule. I can change it though.
Both options overload the file signature with additional meaning to
indicate these files need 'special' handling (eg. memory locking). If
we merge rules, then all files with a signature would be processed with
this special handling; in the other case, the special handling is
limited to a particular policy.
The basic premise, that all files with a valid signature need this
special handling, is flawed. If some other mechanism would be used to
identify these files requiring 'special' handling, then merging of
policy rules would be a non-issue. We wouldn't even need to merge
rules.
My preference would be to define some other mechanism to identify these
files. (Agreed, using the 'security.ima' xattr, is a kludge.) With EVM
protection of LSM labels, you might consider defining a policy based on
LSM labels. Otherwise, consider defining and using a different extended
attribute, or any other file metadata, for this purpose. Once some
method for identifying these files, other than file signature, is
defined, we could then add a new policy option (eg. memlock) or even
action primitive (eg. appraise_memlock).
As the 'special' handling probably doesn't scale very well, we're better
off not requiring it for all signed files. Hopefully, the affects of
not having this special privilege, will be limited to only what has been
discussed (eg. kdump). Even this decision, will require more than my
agreement.
thanks,
Mimi
On Tue, Mar 05, 2013 at 03:40:18PM -0500, Mimi Zohar wrote:
[..]
> > The fact that we are able to replace ima_mem_exec policy using command
> > line, binary loader will need a way to query IMA to find what's the
> > current policy. If ima_mem_exec has been replaced, then binary loader
> > will not memlock files and will not raise extra capability to binary. And
> > this will disable kdump functionality on secureboot platforms. (Something
> > which I don't like much).
>
> Ok
Hi Mimi,
Again, throwing out another idea and see if it has any merit.
I think there are quite a few issues with trying to use IMA only with
the help of policy interface. Different subsystems might have different
needs and we don't have a way to support multiple policies. How about
if we extend IMA to also provide callable function interface and
subsystems can directly call into IMA for integrity verification.
Following are some of the problems I am facing with trying to
scale policy based interface.
- Being able to support only one policy at a time creates conflict
between various users of IMA subsystem. Creating a policy which
meets the needs of all subsystem is not practical. And retaining
on policy only serves select use case and disable rest of the
use cases. What we want is to be able to serve multiple use cases
at the same time.
For example, in above case, very soon I might want to also create
a mechanism to verify signature of bzImage loaded by kexec. Say,
mmap() system call is extended to verify signature of mapped file,
or a new system call is created. In that case there will be another
set of rules and cramming them into mem_exec policy will be odd.
- Because policy can be replaced easily, some of the functionality
will automatically be disabled. (because associated policy is not
there any more). And this can be very unintutive.
For example, even if we do put new rules in mem_exec policy, this
policy can easily be replaced. And kernel loses the capability to
verify signatures of mmaped files. It will be kind of big surprise
where a system call feature disappears because somebody wrote a very
simple policy through "policy" interface. If nothing else, it is
unintutive to me.
- One more issue with "policy" based interface is that rules get
evaluated for every security hook. So though I might want to only
appraise files on exec() but on every security hook (file open, mmap etc),
IMA will go through the rule list and see if some rule matches. This can
add to performance overhead.
- There are other optimization opportunities. For example, in case of
exec() verification, initially I just want to find out if file being
executed has digital signatures or not. But bprm_check() will go ahead
and appraise the file. The appraisal which matters to us is the one
which happens in post_load(). So there is double appraisal for every
executable.
- In an attempt to meet new requirements, we are quickly eating all
the flag space and very few bits are left and further scaling can
become very difficult.
I am beginning to get the feeling that probably using "policy" based
interface is not the best to cater to wide variety of needs. What
if we can provide a set of functions which can be directly called
by various subsystem. That way we will make reuse of IMA code and
at the same time not worry about clash with other policies. Policy
based interface will continue to work as it.
New set of callable functions will primarily be for appraise purposes.
These will retrieive security.ima (and security.evm if EVM is configured),
do integrity verification and return the result to caller. There will
be no caching of results involved.
This should simplify things. It should reduce policy contentions. On
every hook we don't have to go through all the policy rules. Internal
of IMA subsystems remain less complicated etc.
There will be no need for extensions like appraise_type = optional. Caller
of IMA function will decide whether to allow access or not based on
return result. Whether access is optional or not will depend on caller.
IMA will just provide integrity result and will not mandate whether to
allow access or not.
By default interface will not cache results. So there is no need to
worry about providing cache_result=no extensions.
Mimi, what do you think. Does it make any sense?
Thanks
Vivek
On Wed, 2013-03-06 at 10:54 -0500, Vivek Goyal wrote:
> On Tue, Mar 05, 2013 at 03:40:18PM -0500, Mimi Zohar wrote:
>
> [..]
> > > The fact that we are able to replace ima_mem_exec policy using command
> > > line, binary loader will need a way to query IMA to find what's the
> > > current policy. If ima_mem_exec has been replaced, then binary loader
> > > will not memlock files and will not raise extra capability to binary. And
> > > this will disable kdump functionality on secureboot platforms. (Something
> > > which I don't like much).
> >
> > Ok
>
> Hi Mimi,
>
> Again, throwing out another idea and see if it has any merit.
>
> I think there are quite a few issues with trying to use IMA only with
> the help of policy interface. Different subsystems might have different
> needs and we don't have a way to support multiple policies. How about
> if we extend IMA to also provide callable function interface and
> subsystems can directly call into IMA for integrity verification.
The original IMA design included such an interface, but it was rejected.
The conclusion was that LSMs would enforce access control and defer
file integrity enforcement to IMA. IMA policies could be written in
terms of LSM labels. As that decision was made a very long time ago,
and a lot has changed since, perhaps we should to revisit this decision.
> Following are some of the problems I am facing with trying to
> scale policy based interface.
>
> - Being able to support only one policy at a time creates conflict
> between various users of IMA subsystem. Creating a policy which
> meets the needs of all subsystem is not practical. And retaining
> on policy only serves select use case and disable rest of the
> use cases. What we want is to be able to serve multiple use cases
> at the same time.
Our posts must have crossed again. I already addressed this issue in
the post https://lkml.org/lkml/2013/3/6/340.
Your design overloads the existence of a file signature with additional
meaning to indicate these files not only need to be appraised, but also
need 'special' handling. I don't think you can use this example to
claim that only one policy can be supported at a time.
> For example, in above case, very soon I might want to also create
> a mechanism to verify signature of bzImage loaded by kexec.
Absolutely no problem there. Defining a new bzImage hook will allow
measuring, hash audit logging, and integrity appraisal of the bzImage.
> Say,
> mmap() system call is extended to verify signature of mapped file,
> or a new system call is created.
The mmap hook already exists. Just as the new kernel module hook was
recently added, other hooks can be defined just as easily. With the
definition of a new hook, all integrity subystem uses get the benefit.
> In that case there will be another
> set of rules and cramming them into mem_exec policy will be odd.
The mm_exec policy is odd. It mixes identifying a file for appraising
and setting up additional system requirements.
> - Because policy can be replaced easily, some of the functionality
> will automatically be disabled. (because associated policy is not
> there any more). And this can be very unintutive.
Limiting the additional functionality to a single policy, is wrong. A
new policy option (eg. memlock) or even action primitive (eg.
appraise_memlock) should be defined, allowing any policy to achieve the
same results.
> For example, even if we do put new rules in mem_exec policy, this
> policy can easily be replaced. And kernel loses the capability to
> verify signatures of mmaped files.
The capability of verifying the integrity of an mmaped file has nothing
to do with the mem_exec policy, but with the mmap hook.
> It will be kind of big surprise
> where a system call feature disappears because somebody wrote a very
> simple policy through "policy" interface. If nothing else, it is
> unintutive to me.
> - One more issue with "policy" based interface is that rules get
> evaluated for every security hook. So though I might want to only
> appraise files on exec() but on every security hook (file open, mmap etc),
> IMA will go through the rule list and see if some rule matches. This can
> add to performance overhead.
The policies that I'm aware of, have been minimal in size (eg. one
policy doesn't measure anything, but appraises everything). Dmitry has
made a number of performance improvements and has suggested others,
which were rejected for lack of userspace impact. Please feel free to
post performance improvement patches, indicating userspace impact.
> - There are other optimization opportunities. For example, in case of
> exec() verification, initially I just want to find out if file being
> executed has digital signatures or not. But bprm_check() will go ahead
> and appraise the file. The appraisal which matters to us is the one
> which happens in post_load(). So there is double appraisal for every
> executable.
Nothing is stopping you from changing the policy from appraising at
bprm_check() to post_load().
> - In an attempt to meet new requirements, we are quickly eating all
> the flag space and very few bits are left and further scaling can
> become very difficult.np
>
> I am beginning to get the feeling that probably using "policy" based
> interface is not the best to cater to wide variety of needs. What
> if we can provide a set of functions which can be directly called
> by various subsystem. That way we will make reuse of IMA code and
> at the same time not worry about clash with other policies. Policy
> based interface will continue to work as it.
>
> New set of callable functions will primarily be for appraise purposes.
> These will retrieive security.ima (and security.evm if EVM is configured),
> do integrity verification and return the result to caller. There will
> be no caching of results involved.
> This should simplify things. It should reduce policy contentions. On
> every hook we don't have to go through all the policy rules. Internal
> of IMA subsystems remain less complicated etc.
>
> There will be no need for extensions like appraise_type = optional. Caller
> of IMA function will decide whether to allow access or not based on
> return result. Whether access is optional or not will depend on caller.
> IMA will just provide integrity result and will not mandate whether to
> allow access or not.
>
> By default interface will not cache results. So there is no need to
> worry about providing cache_result=no extensions.
>
> Mimi, what do you think. Does it make any sense?
This sounds like your original patches, with a minor difference -
calling the exported IMA functions, instead of calling the crypto
directly. Although it resolves the code duplication, the same issues
that were discussed back then, apply here. Policy should not be
hard-coded into the kernel. Hooks should be defined and accessible for
all uses (eg. measurement, audit hash logging, integrity appraisal) and
any new uses going into the future.
Mimi
On Wed, Mar 06, 2013 at 05:48:01PM -0500, Mimi Zohar wrote:
> On Wed, 2013-03-06 at 10:54 -0500, Vivek Goyal wrote:
> > On Tue, Mar 05, 2013 at 03:40:18PM -0500, Mimi Zohar wrote:
> >
> > [..]
> > > > The fact that we are able to replace ima_mem_exec policy using command
> > > > line, binary loader will need a way to query IMA to find what's the
> > > > current policy. If ima_mem_exec has been replaced, then binary loader
> > > > will not memlock files and will not raise extra capability to binary. And
> > > > this will disable kdump functionality on secureboot platforms. (Something
> > > > which I don't like much).
> > >
> > > Ok
> >
> > Hi Mimi,
> >
> > Again, throwing out another idea and see if it has any merit.
> >
> > I think there are quite a few issues with trying to use IMA only with
> > the help of policy interface. Different subsystems might have different
> > needs and we don't have a way to support multiple policies. How about
> > if we extend IMA to also provide callable function interface and
> > subsystems can directly call into IMA for integrity verification.
>
> The original IMA design included such an interface, but it was rejected.
> The conclusion was that LSMs would enforce access control and defer
> file integrity enforcement to IMA. IMA policies could be written in
> terms of LSM labels. As that decision was made a very long time ago,
> and a lot has changed since, perhaps we should to revisit this decision.
I think it is worth having a discussion on this again. Whether old
decision is still good in the light of new requirements.
>
> > Following are some of the problems I am facing with trying to
> > scale policy based interface.
> >
> > - Being able to support only one policy at a time creates conflict
> > between various users of IMA subsystem. Creating a policy which
> > meets the needs of all subsystem is not practical. And retaining
> > on policy only serves select use case and disable rest of the
> > use cases. What we want is to be able to serve multiple use cases
> > at the same time.
>
> Our posts must have crossed again. I already addressed this issue in
> the post https://lkml.org/lkml/2013/3/6/340.
I did go through that mail and frankly speaking did not understand well.
I will go through it again and respond separately.
>
> Your design overloads the existence of a file signature with additional
> meaning to indicate these files not only need to be appraised, but also
> need 'special' handling. I don't think you can use this example to
> claim that only one policy can be supported at a time.
Well measure and appraise policy co-exist. But writing to "policy"
interface replaces that. So either kernel default policy can exist
or the one specified using "policy" interface.
Even if we allow multiple policies to co-exist, then first matching
rule will be executed. So a IMA subsystem user can never be sure
whether its rule got executed or not. (Because rule imposed by some
other policy will not serve the purpose).
If we go in the direction of running the more restrictive rule, that
quickly becomes complicated and opens possibility for bugs.
>
> > For example, in above case, very soon I might want to also create
> > a mechanism to verify signature of bzImage loaded by kexec.
>
> Absolutely no problem there. Defining a new bzImage hook will allow
> measuring, hash audit logging, and integrity appraisal of the bzImage.
Defining hook does not say anything about rules. So a hook might return
success but there might not be any rule executed (Because a root user
simply replaced the kernel default policy). Now calling code will assume
that file integrity is fine and go ahead and load the unsigned bzImage.
But the fact is that no integrity measurement was done.
So always allowing root user to load policy does not fit well in
secureboot environment.
>
> > Say,
> > mmap() system call is extended to verify signature of mapped file,
> > or a new system call is created.
>
> The mmap hook already exists. Just as the new kernel module hook was
> recently added, other hooks can be defined just as easily. With the
> definition of a new hook, all integrity subystem uses get the benefit.
New module hook is racy. It does file integrity verification before
module get copied into kernel memory. So after integrity verification
there is a window where a root user can change the module.
So far I was assuming that not caching the results itself is sufficient
to avoid the direct writes to disk problem. But hash calculation seems
to be happening in ima_collect_measurement(). So once the measurement
has been collected, even if I call the hook again, file will not be
measured again. And when I call hook second time, I want measurement
to be collected again (as I have locked the file in memory and further
reads will come from page cache and disk).
So current IMA in general has not worried about direct writes to disk
problem and it is getting into the way. IMA is just ensuring integrity
of file on disk and now requirement is that verify integrity of file
after it has been loaded into the memory.
>
> > In that case there will be another
> > set of rules and cramming them into mem_exec policy will be odd.
>
> The mm_exec policy is odd. It mixes identifying a file for appraising
> and setting up additional system requirements.
We can name this policy something else. Additional requirements is
a problem. That's why I think that IMA should expose functions directly
and let the caller decide what to do (based on requirements). IMA
can just limit itself to looking for signature xattr, verify the signature
and return results. It makes things simple.
>
> > - Because policy can be replaced easily, some of the functionality
> > will automatically be disabled. (because associated policy is not
> > there any more). And this can be very unintutive.
>
> Limiting the additional functionality to a single policy, is wrong. A
> new policy option (eg. memlock) or even action primitive (eg.
> appraise_memlock) should be defined, allowing any policy to achieve the
> same results.
Sorry I did not get this part. How does any policy achieve the same
results.
>
> > For example, even if we do put new rules in mem_exec policy, this
> > policy can easily be replaced. And kernel loses the capability to
> > verify signatures of mmaped files.
>
> The capability of verifying the integrity of an mmaped file has nothing
> to do with the mem_exec policy, but with the mmap hook.
hook just calls into IMA. It does not guarantee anything else. So if a
user never enables any policy, then no rule gets executed. And
If we replace the policy with a policy with a rule "don_appraise
bprm_check", then nothing gets appraised. So mmap() hook will call
into IMA but IMA will say no rule matches and IMA will return success.
action = ima_get_action(inode, mask, function);
if (!action)
return 0;
In this new requirement, just calling into the hook is not sufficient.
Success does not mean file integrity was verified. And that's where
the biggest problem is with hook based interface. Hooks can return
success even if nothing was ever verified.
>
> > It will be kind of big surprise
> > where a system call feature disappears because somebody wrote a very
> > simple policy through "policy" interface. If nothing else, it is
> > unintutive to me.
>
> > - One more issue with "policy" based interface is that rules get
> > evaluated for every security hook. So though I might want to only
> > appraise files on exec() but on every security hook (file open, mmap etc),
> > IMA will go through the rule list and see if some rule matches. This can
> > add to performance overhead.
>
> The policies that I'm aware of, have been minimal in size (eg. one
> policy doesn't measure anything, but appraises everything). Dmitry has
> made a number of performance improvements and has suggested others,
> which were rejected for lack of userspace impact. Please feel free to
> post performance improvement patches, indicating userspace impact.
Sure. I think it is much more efficient to not call into hooks at all.
Directly use IMA functions on need basis. If I don't need to verify
integrity of file on mmap() and other hooks, then I should not go
through rules from all hook paths. If we create direct callable functions
then it makes it effiecient automatically.
>
> > - There are other optimization opportunities. For example, in case of
> > exec() verification, initially I just want to find out if file being
> > executed has digital signatures or not. But bprm_check() will go ahead
> > and appraise the file. The appraisal which matters to us is the one
> > which happens in post_load(). So there is double appraisal for every
> > executable.
>
> Nothing is stopping you from changing the policy from appraising at
> bprm_check() to post_load().
Before I call post_load, I need to know whether file is digitally signed
or not. Given the fact IMA stores the signing information I need to call
into IMA for this info. Given the fact that there are no other interface
to IMA, one kludge is that call bprm_check(). It will appraise file,
set IMA_DIGSIG flag in iint->flags. Propogate that flag in bprm->unsafe
And calling code can read it.
If file is signed, then caller can lock the file and call post_load()
again to verify signature.
So not calling bprm_check() is not an option.
Here again, calling into IMA functions will help. There can be one
function just to give information whether file is digitally signed
or not (no appraisal).
>
> > - In an attempt to meet new requirements, we are quickly eating all
> > the flag space and very few bits are left and further scaling can
> > become very difficult.np
> >
> > I am beginning to get the feeling that probably using "policy" based
> > interface is not the best to cater to wide variety of needs. What
> > if we can provide a set of functions which can be directly called
> > by various subsystem. That way we will make reuse of IMA code and
> > at the same time not worry about clash with other policies. Policy
> > based interface will continue to work as it.
> >
> > New set of callable functions will primarily be for appraise purposes.
> > These will retrieive security.ima (and security.evm if EVM is configured),
> > do integrity verification and return the result to caller. There will
> > be no caching of results involved.
>
> > This should simplify things. It should reduce policy contentions. On
> > every hook we don't have to go through all the policy rules. Internal
> > of IMA subsystems remain less complicated etc.
> >
> > There will be no need for extensions like appraise_type = optional. Caller
> > of IMA function will decide whether to allow access or not based on
> > return result. Whether access is optional or not will depend on caller.
> > IMA will just provide integrity result and will not mandate whether to
> > allow access or not.
> >
> > By default interface will not cache results. So there is no need to
> > worry about providing cache_result=no extensions.
> >
> > Mimi, what do you think. Does it make any sense?
>
> This sounds like your original patches, with a minor difference -
> calling the exported IMA functions, instead of calling the crypto
> directly. Although it resolves the code duplication, the same issues
> that were discussed back then, apply here. Policy should not be
> hard-coded into the kernel. Hooks should be defined and accessible for
> all uses (eg. measurement, audit hash logging, integrity appraisal) and
> any new uses going into the future.
Seriously I think this needs to be revisited. First of all this does not
disable any of the current functionality and interfaces. What I am
questioning is that new requirements are making it much harder to use
policy based interface. It probably is much simpler to call into some
IMA functions directly.
IMA is not hardcoding anything here. Only thing caller is doing using
IMA functionality to meet its needs. I think IMA should not be concerned
with what to do if file is not signed. IMA should just return integrity
results and let caller decide what to do with it. Similar, IMA should not
be concerned whether file is memlocked or not. It depends on whehther
all of the user space is signed or not.
I don't know, but more I try to make policy based interface, more I get
the feeling that it is not right thing to do. It is unnecessarily
complicating the IMA code and maintenance of code will become harder. It
will become hard for a user to figure out what's going on. What does
integrity success mean, what file actually appraised and there is no
way to query it.
IMHO, we should continue with current policy based interface but to
meet new requirements in less complicated way, we should also export
some directly callable funcations and they take care of not coming with
a new way to sign a file and reusing existing coe.
Thanks
Vivek
>
> Mimi
On Wed, Mar 06, 2013 at 10:42:31AM -0500, Mimi Zohar wrote:
[..]
> > Mimi, so you like this idea better than the other idea of keeping two
> > policy chains and running more restrictive rule while resolving flag
> > conflicts between two rules?
> >
> > I have written some patches to maintain two rule chains and running
> > more restrictive rule. I can change it though.
>
> Both options overload the file signature with additional meaning to
> indicate these files need 'special' handling (eg. memory locking).
I think memory locking is not part of integrity as such. If user space
is partially signed, then we need to lock files into memory. But if
whole of the user space is signed, we might get away without locking
everything in memory.
So I think we should not build the notion of memory locking into IMA.
Caller knows whether to lock things into memory or not. IMA should
just facilitate integrity verification (before locking and after
locking) and it is left to the caller to decide when is the right
time to do verification.
> If
> we merge rules, then all files with a signature would be processed with
> this special handling; in the other case, the special handling is
> limited to a particular policy.
>
> The basic premise, that all files with a valid signature need this
> special handling, is flawed. If some other mechanism would be used to
> identify these files requiring 'special' handling, then merging of
> policy rules would be a non-issue. We wouldn't even need to merge
> rules.
I am not sure what does "special handling" mean here, but then we are
hardcoding things in file's extended attributes.
In this case kernel needs to decide how to handle file. (possibly based
on a config option). Again going back to memlock example, IMA or file
attribute should not dictate whether file should be memlocked or not.
Whether file's appraisal result should be cached or not. Whether
"measurement" of cache results should be cached or not. This is much
worse hardcoding to me.
IMHO, IMA can provide simple callable functions (like verify_signature())
which does not assume too much and let caller figure out the thigs
around it. This is much more simple.
>
> My preference would be to define some other mechanism to identify these
> files. (Agreed, using the 'security.ima' xattr, is a kludge.)
IMO, it should not a file's attribtue. Caller knows how to handle it.
IMA should just verify the integrity. Caller can choose to lock or not
lock the file in memory depending on its needs and environment it is
operating in. And I don't think this kind of information should be
file specific.
> With EVM
> protection of LSM labels, you might consider defining a policy based on
> LSM labels. Otherwise, consider defining and using a different extended
> attribute, or any other file metadata, for this purpose. Once some
> method for identifying these files, other than file signature, is
> defined, we could then add a new policy option (eg. memlock) or even
> action primitive (eg. appraise_memlock).
>
> As the 'special' handling probably doesn't scale very well, we're better
> off not requiring it for all signed files. Hopefully, the affects of
> not having this special privilege, will be limited to only what has been
> discussed (eg. kdump). Even this decision, will require more than my
> agreement.
IMHO, defining directly callable IMA hooks is much simpler, and much
more maintainable and much more scalable. Atleast we should discuss it
again why it it is not right thing to do. Why it is right thing to
do for security/keys or security/crypto to export callable functions
and then let the caller decide what to do with it. But it is not right for
security/integrity/* or security/integrity/ima*. I just don't get it.
Thanks
Vivek
On Wed, 2013-03-06 at 18:55 -0500, Vivek Goyal wrote:
> On Wed, Mar 06, 2013 at 10:42:31AM -0500, Mimi Zohar wrote:
>
> [..]
> > > Mimi, so you like this idea better than the other idea of keeping two
> > > policy chains and running more restrictive rule while resolving flag
> > > conflicts between two rules?
> > >
> > > I have written some patches to maintain two rule chains and running
> > > more restrictive rule. I can change it though.
> >
> > Both options overload the file signature with additional meaning to
> > indicate these files need 'special' handling (eg. memory locking).
>
> I think memory locking is not part of integrity as such. If user space
> is partially signed, then we need to lock files into memory. But if
> whole of the user space is signed, we might get away without locking
> everything in memory.
>
> So I think we should not build the notion of memory locking into IMA.
> Caller knows whether to lock things into memory or not. IMA should
> just facilitate integrity verification (before locking and after
> locking) and it is left to the caller to decide when is the right
> time to do verification.
Great! So define a hook, in the appropriate place, and IMA will
appraise the file based on policy.
> > If
> > we merge rules, then all files with a signature would be processed with
> > this special handling; in the other case, the special handling is
> > limited to a particular policy.
> >
> > The basic premise, that all files with a valid signature need this
> > special handling, is flawed. If some other mechanism would be used to
> > identify these files requiring 'special' handling, then merging of
> > policy rules would be a non-issue. We wouldn't even need to merge
> > rules.
>
> I am not sure what does "special handling" mean here, but then we are
> hardcoding things in file's extended attributes.
>
> In this case kernel needs to decide how to handle file. (possibly based
> on a config option). Again going back to memlock example, IMA or file
> attribute should not dictate whether file should be memlocked or not.
> Whether file's appraisal result should be cached or not. Whether
> "measurement" of cache results should be cached or not. This is much
> worse hardcoding to me.
>
> IMHO, IMA can provide simple callable functions (like verify_signature())
> which does not assume too much and let caller figure out the thigs
> around it. This is much more simple.
>
> >
> > My preference would be to define some other mechanism to identify these
> > files. (Agreed, using the 'security.ima' xattr, is a kludge.)
>
> IMO, it should not a file's attribtue. Caller knows how to handle it.
> IMA should just verify the integrity. Caller can choose to lock or not
> lock the file in memory depending on its needs and environment it is
> operating in. And I don't think this kind of information should be
> file specific.
>
> > With EVM
> > protection of LSM labels, you might consider defining a policy based on
> > LSM labels. Otherwise, consider defining and using a different extended
> > attribute, or any other file metadata, for this purpose. Once some
> > method for identifying these files, other than file signature, is
> > defined, we could then add a new policy option (eg. memlock) or even
> > action primitive (eg. appraise_memlock).
> >
> > As the 'special' handling probably doesn't scale very well, we're better
> > off not requiring it for all signed files. Hopefully, the affects of
> > not having this special privilege, will be limited to only what has been
> > discussed (eg. kdump). Even this decision, will require more than my
> > agreement.
>
> IMHO, defining directly callable IMA hooks is much simpler, and much
> more maintainable and much more scalable. Atleast we should discuss it
> again why it it is not right thing to do. Why it is right thing to
> do for security/keys or security/crypto to export callable functions
> and then let the caller decide what to do with it. But it is not right for
> security/integrity/* or security/integrity/ima*. I just don't get it.
The purpose of both /crypto and /keys is to provide a callable service
to other parts of the kernel (and expose an interface to userspace).
The original purpose of IMA was to provide a hardware rooted trusted
list of runtime measurements. With the upstreaming of IMA-appraisal
patches, IMA now enforces file integrity as well.
Adding an IMA call to directly appraise the integrity of a file, rather
than adding a hook, prevents other integrity users from being able to
define a rule at that point. I don't have a problem with exposing an
integrity interface, assuming there is a real need.
Mimi
On Wed, 2013-03-06 at 18:38 -0500, Vivek Goyal wrote:
> On Wed, Mar 06, 2013 at 05:48:01PM -0500, Mimi Zohar wrote:
> > On Wed, 2013-03-06 at 10:54 -0500, Vivek Goyal wrote:
[...]
> > > - Because policy can be replaced easily, some of the functionality
> > > will automatically be disabled. (because associated policy is not
> > > there any more). And this can be very unintutive.
> >
> > Limiting the additional functionality to a single policy, is wrong. A
> > new policy option (eg. memlock) or even action primitive (eg.
> > appraise_memlock) should be defined, allowing any policy to achieve the
> > same results.
>
> Sorry I did not get this part. How does any policy achieve the same
> results.
This discussion has gone through many twists and turns - original direct
crypto calls to verify appended signature, 'optional' policy flag,
locking memory, fixing appraisal results, differentiating ima vs. evm
appraisal results, iint caching, merging policies vs. either/or policy,
new policy memory lock option/action, separating policy from locking
memory, and now exporting integrity calls.
Once you resolve the 'special' processing (eg. memory locking issue)
being tied to the policy, either by removing the requirement or by
defining a new policy option/action primitive, you'll be able to resolve
your policy requirements, without merging rules or limiting
functionality for other policies.
Limiting functionality (eg. kexec) to a single builtin policy is
unacceptable. The same mechanism, that the builtin kmem_lock policy
uses to make kexec permissible, should be available to all policies. It
is then up to the system administrator to define an appropriate policy.
thanks,
Mimi
On Wed, Mar 06, 2013 at 08:39:08PM -0500, Mimi Zohar wrote:
> On Wed, 2013-03-06 at 18:55 -0500, Vivek Goyal wrote:
> > On Wed, Mar 06, 2013 at 10:42:31AM -0500, Mimi Zohar wrote:
> >
> > [..]
> > > > Mimi, so you like this idea better than the other idea of keeping two
> > > > policy chains and running more restrictive rule while resolving flag
> > > > conflicts between two rules?
> > > >
> > > > I have written some patches to maintain two rule chains and running
> > > > more restrictive rule. I can change it though.
> > >
> > > Both options overload the file signature with additional meaning to
> > > indicate these files need 'special' handling (eg. memory locking).
> >
> > I think memory locking is not part of integrity as such. If user space
> > is partially signed, then we need to lock files into memory. But if
> > whole of the user space is signed, we might get away without locking
> > everything in memory.
> >
> > So I think we should not build the notion of memory locking into IMA.
> > Caller knows whether to lock things into memory or not. IMA should
> > just facilitate integrity verification (before locking and after
> > locking) and it is left to the caller to decide when is the right
> > time to do verification.
>
> Great! So define a hook, in the appropriate place, and IMA will
> appraise the file based on policy.
As I mentioned, current hooks have issues. They don't guarantee anything.
Caller can't conclude anything based on success because caller does not
know if any rule has been executed.
So far it worked because caller did not have to know anything more. But
now based on integrity results, caller has to take additional decision.
So if a process is given extra capability (because it was signed), hook
returning success does not tell the caller whether process executable
was signed or not.
So just defining hook is not sufficient. Similarly in the above case of
verifying signature of a file after mmap(). Calling a hook does not tell
whether file signature verification was successful or not.
Think of following hypothetical scenario. Say I have defined a flag
in mmap() system call which will verify the integrity of file otherwise
mmap() fails.
mmap(MAP_VERIFY_POST)
Now just defining another hook is not sufficient. Once hook returns
mmap() implementation need to know whether integrity of file was
verified or not. Whether the actual rule was configured or not. Otherwise
bad things willl happen. On success mmap() will say integrity of file
verified and user space will believe that mmaped file's signature are
valid. But that's not the case.
So defining a hook only calls into IMA subsystem. Caller can't deduce much
from the success code. Things worked because I can't see any of the caller
relying on integrity results. But in new use cases we need to know whether
a particular integrity rule was executed or not and then take the
decision. Current hooks are opaque and don't export that info to caller.
And that's why I think defining more hooks or policies is not the right
way to solve this problem.
[..]
> > > With EVM
> > > protection of LSM labels, you might consider defining a policy based on
> > > LSM labels. Otherwise, consider defining and using a different extended
> > > attribute, or any other file metadata, for this purpose. Once some
> > > method for identifying these files, other than file signature, is
> > > defined, we could then add a new policy option (eg. memlock) or even
> > > action primitive (eg. appraise_memlock).
> > >
> > > As the 'special' handling probably doesn't scale very well, we're better
> > > off not requiring it for all signed files. Hopefully, the affects of
> > > not having this special privilege, will be limited to only what has been
> > > discussed (eg. kdump). Even this decision, will require more than my
> > > agreement.
> >
> > IMHO, defining directly callable IMA hooks is much simpler, and much
> > more maintainable and much more scalable. Atleast we should discuss it
> > again why it it is not right thing to do. Why it is right thing to
> > do for security/keys or security/crypto to export callable functions
> > and then let the caller decide what to do with it. But it is not right for
> > security/integrity/* or security/integrity/ima*. I just don't get it.
>
> The purpose of both /crypto and /keys is to provide a callable service
> to other parts of the kernel (and expose an interface to userspace).
And like crypto and keys, IMA can a callable service too. Why not. In fact
IMA calls will be just a wrapper around request_key() and
verify_signature(). With the addition of how to retrieve signatuer. As
IMA knows where signature are stored in what format.
> The original purpose of IMA was to provide a hardware rooted trusted
> list of runtime measurements. With the upstreaming of IMA-appraisal
> patches, IMA now enforces file integrity as well.
IMA file integrity goes only so far. It just makes sure file has not been
modified on disk. But it does not guarantee that file has not been
modified after signature verification. And by the time user uses the
file, it might have been modified.
By exposing IMA functions, a caller can manage this situation well. Based
on need, caller can first load the file in memory and then call IMA. Again
calling hook will not solve problem because it does not guarantee that
file appraisal actually happened. But a simple function will do a small
defined job and return code from it will tell caller whether appraisal
happened or not.
>
> Adding an IMA call to directly appraise the integrity of a file, rather
> than adding a hook, prevents other integrity users from being able to
> define a rule at that point.
We already have security hooks in exec() code and mmap(). And current
integrity callers are happy with it.
Just because somebody calls integrity functions in exec() and mmap(),
why would other integrity users be worried about it. They just
wanted to make sure that in exec() and mmap() file integrity is verified
and that will still happen.
If another caller called into IMA functions to verify file integrity
in a specific way after the file has been loaded, then other existing
integrity users are not affected by it.
Thanks
Vivek
On Thu, Mar 07, 2013 at 08:38:27AM -0500, Mimi Zohar wrote:
> On Wed, 2013-03-06 at 18:38 -0500, Vivek Goyal wrote:
> > On Wed, Mar 06, 2013 at 05:48:01PM -0500, Mimi Zohar wrote:
> > > On Wed, 2013-03-06 at 10:54 -0500, Vivek Goyal wrote:
>
> [...]
> > > > - Because policy can be replaced easily, some of the functionality
> > > > will automatically be disabled. (because associated policy is not
> > > > there any more). And this can be very unintutive.
> > >
> > > Limiting the additional functionality to a single policy, is wrong. A
> > > new policy option (eg. memlock) or even action primitive (eg.
> > > appraise_memlock) should be defined, allowing any policy to achieve the
> > > same results.
> >
> > Sorry I did not get this part. How does any policy achieve the same
> > results.
>
> This discussion has gone through many twists and turns - original direct
> crypto calls to verify appended signature, 'optional' policy flag,
> locking memory, fixing appraisal results, differentiating ima vs. evm
> appraisal results, iint caching, merging policies vs. either/or policy,
> new policy memory lock option/action, separating policy from locking
> memory, and now exporting integrity calls.
>
> Once you resolve the 'special' processing (eg. memory locking issue)
> being tied to the policy, either by removing the requirement
I don't think requirement can be removed. Otherwise one can easily
modify the file after measurement in memory and doing integrity
verification is effectively of no use.
> or by
> defining a new policy option/action primitive,
Memory locking should not belong to IMA subsystem. It is up to the caller
whether it wants to lock file in memory or not. And whether a file should
be locked in memory or not is not a file specific property also.
Think of a signed file being sent on another system (assume cpio has been
modified to take care of xattr). Then this other system might be running
a fully signed user space and one might not have to lock the files. Or
this other system might not have any swap and disabled ptrace()
capability, then there is no need to lock files.
Hence locking a file is not file specific property hence should not be a
file attribute. And it does not fall into the Integrity area too. It all
depends on caller and in what context integrity is being verified. So I
don't think that IMA is right place to take care of this issue. IMA should
just export functions to enable caller to achieve what it wants to.
> you'll be able to resolve
> your policy requirements, without merging rules or limiting
> functionality for other policies.
Even if I define above as policy option, then there will still be another
appraise policy. So how multiple appraise policies will co-exist together?
>
> Limiting functionality (eg. kexec) to a single builtin policy is
> unacceptable.
Sure that's why I am saying trying to build more polices is not a good
idea. Just export functions.
> The same mechanism, that the builtin kmem_lock policy
> uses to make kexec permissible, should be available to all policies.
Locking a file does not fall into IMA area. It is up to the file loader.
In case of binary, it is up to the binary loader. IMA can not dictate
or lock the file based on policy.
Thanks
Vivek
On Thu, 2013-03-07 at 09:36 -0500, Vivek Goyal wrote:
> On Wed, Mar 06, 2013 at 08:39:08PM -0500, Mimi Zohar wrote:
> > On Wed, 2013-03-06 at 18:55 -0500, Vivek Goyal wrote:
> > > On Wed, Mar 06, 2013 at 10:42:31AM -0500, Mimi Zohar wrote:
>
> > Adding an IMA call to directly appraise the integrity of a file, rather
> > than adding a hook, prevents other integrity users from being able to
> > define a rule at that point.
>
> We already have security hooks in exec() code and mmap(). And current
> integrity callers are happy with it.
Exposing integrity calls, resolves the problem of code duplication, but
does not address Rusty's third issue of improving the integrity
subsystem. You have no idea if the existing integrity users are happy
with the status quo. It's there and they're using it. They could want
additional hooks or better located hooks. Each of your complaints about
the integrity subsystem could be addressed and would improve the
subsystem.
thanks,
Mimi
On Thu, Mar 07, 2013 at 10:40:33AM -0500, Mimi Zohar wrote:
> On Thu, 2013-03-07 at 09:36 -0500, Vivek Goyal wrote:
> > On Wed, Mar 06, 2013 at 08:39:08PM -0500, Mimi Zohar wrote:
> > > On Wed, 2013-03-06 at 18:55 -0500, Vivek Goyal wrote:
> > > > On Wed, Mar 06, 2013 at 10:42:31AM -0500, Mimi Zohar wrote:
> >
> > > Adding an IMA call to directly appraise the integrity of a file, rather
> > > than adding a hook, prevents other integrity users from being able to
> > > define a rule at that point.
> >
> > We already have security hooks in exec() code and mmap(). And current
> > integrity callers are happy with it.
>
> Exposing integrity calls, resolves the problem of code duplication, but
> does not address Rusty's third issue of improving the integrity
> subsystem.
I thought rusty wanted me to reuse existing integrity code so that there
is no code duplication. And improve things as needed int the process
so meet new requirements.
And exposing functions calls will just do that. Improve IMA so that
more people can reuse the same code without breaking existing users.
Other improvements will be to improve "cpio" to support xattr and
figure a way out to chain into secureboot root of trust and load
only trusted keys.
So yes, I am improving IMA subsystem. Only thing I am arguing against
that I don't think trying to enahnce policy based interface is right
thing to do here in the light of new requirements. It just makes IMA too
complicated and it is not worth it.
> You have no idea if the existing integrity users are happy
> with the status quo.
If nobody is speaking then I have to assume something. If they are happy,
great, exposing new functions are not going to break any of their
functionality. If they are not happy then there is more reason to believe
that trying to enahance policy based interface is a bad thing. It is not
working.
> It's there and they're using it. They could want
> additional hooks or better located hooks. Each of your complaints about
> the integrity subsystem could be addressed and would improve the
> subsystem.
If they want better located hooks, they are more than welcome to put
it. Exposing some of the functions does not prevent it. They want
to improve IMA subsystem, sure, why not. Exposing some fucntions
does not prevent it.
Thanks
Vivek
On Thu, Mar 7, 2013 at 5:53 PM, Vivek Goyal <[email protected]> wrote:
> On Thu, Mar 07, 2013 at 10:40:33AM -0500, Mimi Zohar wrote:
>> On Thu, 2013-03-07 at 09:36 -0500, Vivek Goyal wrote:
>> > On Wed, Mar 06, 2013 at 08:39:08PM -0500, Mimi Zohar wrote:
>> > > On Wed, 2013-03-06 at 18:55 -0500, Vivek Goyal wrote:
>> > > > On Wed, Mar 06, 2013 at 10:42:31AM -0500, Mimi Zohar wrote:
>> >
>> > > Adding an IMA call to directly appraise the integrity of a file, rather
>> > > than adding a hook, prevents other integrity users from being able to
>> > > define a rule at that point.
>> >
>> > We already have security hooks in exec() code and mmap(). And current
>> > integrity callers are happy with it.
>>
>> Exposing integrity calls, resolves the problem of code duplication, but
>> does not address Rusty's third issue of improving the integrity
>> subsystem.
>
> I thought rusty wanted me to reuse existing integrity code so that there
> is no code duplication. And improve things as needed int the process
> so meet new requirements.
>
> And exposing functions calls will just do that. Improve IMA so that
> more people can reuse the same code without breaking existing users.
>
> Other improvements will be to improve "cpio" to support xattr and
> figure a way out to chain into secureboot root of trust and load
> only trusted keys.
>
> So yes, I am improving IMA subsystem. Only thing I am arguing against
> that I don't think trying to enahnce policy based interface is right
> thing to do here in the light of new requirements. It just makes IMA too
> complicated and it is not worth it.
>
>> You have no idea if the existing integrity users are happy
>> with the status quo.
>
> If nobody is speaking then I have to assume something. If they are happy,
> great, exposing new functions are not going to break any of their
> functionality. If they are not happy then there is more reason to believe
> that trying to enahance policy based interface is a bad thing. It is not
> working.
>
>> It's there and they're using it. They could want
>> additional hooks or better located hooks. Each of your complaints about
>> the integrity subsystem could be addressed and would improve the
>> subsystem.
>
> If they want better located hooks, they are more than welcome to put
> it. Exposing some of the functions does not prevent it. They want
> to improve IMA subsystem, sure, why not. Exposing some fucntions
> does not prevent it.
>
Hello,
Sorry if missed something from this lengthy thread and I repeat something.
I have not noticed what functions you propose to export.
But for your use case you need to know if file was signed and
signature was fine, right?
So you want to export a function which returns, for example
"iint->flags & IMA_DIGSIG".
If it was no xattr or no signature, then this flag will not be set.
If signature verification failed, then hook returns EPERM anyway.
Is it what you want?
- Dmitry
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Thu, Mar 07, 2013 at 07:53:50PM +0200, Kasatkin, Dmitry wrote:
[..]
Hi Dmitry,
> Sorry if missed something from this lengthy thread and I repeat something.
>
> I have not noticed what functions you propose to export.
Actually I have not come up with functions yet. I have yet to write
the code. But I was thinking something along the lines of
verify_signature().
>
> But for your use case you need to know if file was signed and
> signature was fine, right?
Right.
> So you want to export a function which returns, for example
> "iint->flags & IMA_DIGSIG".
Not sure about that but I think you are referring to your patch
of also exporting the iint->DIGSIG by setting a security flag
LSM_UNSAFE_DIGSIG in bprm->unsafe. That helps but then more
issues start cropping up. I will explain issues in detail below.
> If it was no xattr or no signature, then this flag will not be set.
- if iint->DIGSIG is not set then it could mean few things.
- There is no xattr or digital signature
- Or there is signature but ima is disabled or there is no
appraise rule configured.
Now second point can create confusion. It means that a signed file will
be treated as unsigned and any functionality dependent on file being
signed will fail. I think it is uintutive.
If there is no xattr or signature, IMA hook can return failure if appraise
policy is configured. I can't ignore the return code of security hook. So
I need a separate function just to tell me whether file is signed or
not.
> If signature verification failed, then hook returns EPERM anyway.
There are few issues here.
- iint->DIGSIG will be set only if ima is enabled and some appraise
rule/policy has been enabled. Otherwise it will not be set. It might
not be too huge a issue because it just means that a signed file will
be treated unsigned and any functionality dependent on file being
signed will fail. I think it is uintutive.
- iint->DIGSIG could be set even if file is not signed. How?
- Assume system has booted with ima_appraise_tcb policy.
- A binary executes. bprm_check() is called and it will
set iint->DIGSIG.
- root does a direct write to disk blocks where file signature
are stored.
- File executes again. This time iint->DIGSIG is set but there
are no signature on the file.
- File could have invalid signature still iint->DIGSIG could be set and
security hook will return success.
- Assume system has booted with ima_appraise_tcb policy.
- A binary executes. bprm_check() is called and it will
set iint->DIGSIG.
- User goes ahead and replaces appraise policy with some
other policy so no appraisal rule will match for same file.
- User does a direct write to disk on file blocks.
- File executes again. This time iint->DIGSIG is set, and
IMA hook will return success (as there is no matching appraise
rule) and making caller believe file is validly signed.
If we don't cache iint->DIGSIG, I think couple of above issues could be
solved. But then we also need to make sure digest of file and appraisal
results not cached either. Caching of everything is in general a issue
with IMA usage in my scenario. I am not sure why IMA did not address the
issue of somebody writing directly to disk bypassing file system.
If we figure a way out to disable caching of everything, then we also
need to figure out a way to export iint->DIGSIG to callers. Current
security hooks don't allow returning anything other than success/fail
status, that means we probably need to create a new function. Seeting
it in bprm->unsafe alone is not sufficient as I might have to do file
verification in non executable file code also.
In summary, we can still solve the problem we can do few things.
- Provide a reliable way to disable caching of iint->DIGSIG, digest
and appraisal results.
- Provide functions to access iint->DIGSIG after every file execution.
- Create a separate callable IMA function which tells whether file is
signed or not.
- Provide a way to caller to ensure whether caching is disabled or not
in IMA. So that caller can interpret what does result mean.
Thanks
Vivek
On Thu, Mar 7, 2013 at 11:56 PM, Vivek Goyal <[email protected]> wrote:
> On Thu, Mar 07, 2013 at 07:53:50PM +0200, Kasatkin, Dmitry wrote:
>
> [..]
>
> Hi Dmitry,
>
>> Sorry if missed something from this lengthy thread and I repeat something.
>>
>> I have not noticed what functions you propose to export.
>
> Actually I have not come up with functions yet. I have yet to write
> the code. But I was thinking something along the lines of
> verify_signature().
>
>>
>> But for your use case you need to know if file was signed and
>> signature was fine, right?
>
> Right.
>
>> So you want to export a function which returns, for example
>> "iint->flags & IMA_DIGSIG".
>
> Not sure about that but I think you are referring to your patch
> of also exporting the iint->DIGSIG by setting a security flag
> LSM_UNSAFE_DIGSIG in bprm->unsafe. That helps but then more
> issues start cropping up. I will explain issues in detail below.
>
>> If it was no xattr or no signature, then this flag will not be set.
>
> - if iint->DIGSIG is not set then it could mean few things.
> - There is no xattr or digital signature
> - Or there is signature but ima is disabled or there is no
> appraise rule configured.
>
> Now second point can create confusion. It means that a signed file will
> be treated as unsigned and any functionality dependent on file being
> signed will fail. I think it is uintutive.
>
> If there is no xattr or signature, IMA hook can return failure if appraise
> policy is configured. I can't ignore the return code of security hook. So
> I need a separate function just to tell me whether file is signed or
> not.
>
>> If signature verification failed, then hook returns EPERM anyway.
>
> There are few issues here.
>
I fully understand them.
In the past I have implemented attacks to IMA that based on the fact that
verification result is cached...
> - iint->DIGSIG will be set only if ima is enabled and some appraise
> rule/policy has been enabled. Otherwise it will not be set. It might
> not be too huge a issue because it just means that a signed file will
> be treated unsigned and any functionality dependent on file being
> signed will fail. I think it is uintutive.
>
> - iint->DIGSIG could be set even if file is not signed. How?
> - Assume system has booted with ima_appraise_tcb policy.
> - A binary executes. bprm_check() is called and it will
> set iint->DIGSIG.
> - root does a direct write to disk blocks where file signature
> are stored.
> - File executes again. This time iint->DIGSIG is set but there
> are no signature on the file.
>
> - File could have invalid signature still iint->DIGSIG could be set and
> security hook will return success.
> - Assume system has booted with ima_appraise_tcb policy.
> - A binary executes. bprm_check() is called and it will
> set iint->DIGSIG.
> - User goes ahead and replaces appraise policy with some
> other policy so no appraisal rule will match for same file.
Policy can only be replaced once. So if policy has been initialized at
early-user-space,
then it cannot be replaced...
> - User does a direct write to disk on file blocks.
> - File executes again. This time iint->DIGSIG is set, and
> IMA hook will return success (as there is no matching appraise
> rule) and making caller believe file is validly signed.
>
> If we don't cache iint->DIGSIG, I think couple of above issues could be
> solved. But then we also need to make sure digest of file and appraisal
> results not cached either. Caching of everything is in general a issue
> with IMA usage in my scenario. I am not sure why IMA did not address the
> issue of somebody writing directly to disk bypassing file system.
IMA protects against offline attacks.
Runtime protection should be done by DAC/MAC.
But anyway, I fully understand that you want also additional runtime protection.
>
> If we figure a way out to disable caching of everything, then we also
> need to figure out a way to export iint->DIGSIG to callers. Current
> security hooks don't allow returning anything other than success/fail
> status, that means we probably need to create a new function. Seeting
> it in bprm->unsafe alone is not sufficient as I might have to do file
> verification in non executable file code also.
bprm->unsafe was just a very-quick show-case.
>
> In summary, we can still solve the problem we can do few things.
>
> - Provide a reliable way to disable caching of iint->DIGSIG, digest
> and appraisal results.
>
> - Provide functions to access iint->DIGSIG after every file execution.
>
> - Create a separate callable IMA function which tells whether file is
> signed or not.
>
> - Provide a way to caller to ensure whether caching is disabled or not
> in IMA. So that caller can interpret what does result mean.
>
> Thanks
> Vivek
I understand what you want to achieve :)
- Dmitry
On Fri, Mar 08, 2013 at 10:09:48AM +0200, Kasatkin, Dmitry wrote:
[..]
> > - File could have invalid signature still iint->DIGSIG could be set and
> > security hook will return success.
> > - Assume system has booted with ima_appraise_tcb policy.
> > - A binary executes. bprm_check() is called and it will
> > set iint->DIGSIG.
> > - User goes ahead and replaces appraise policy with some
> > other policy so no appraisal rule will match for same file.
>
> Policy can only be replaced once. So if policy has been initialized at
> early-user-space,
> then it cannot be replaced...
Sure, but early user space does not have to initialize the "policy",
isn't. Atleast currently kernel can not enforce it. So root always
can decide to load the policy some time late. assume ima_appraise_tcb is
enabled at kernel command line.
Given that in secureboot environment we are not trusting root, it atleast
gives root a way to deceive IMA due to caching.
[..]
> > In summary, we can still solve the problem we can do few things.
> >
> > - Provide a reliable way to disable caching of iint->DIGSIG, digest
> > and appraisal results.
> >
> > - Provide functions to access iint->DIGSIG after every file execution.
Actually if we have to disbale caching to make it work reliably, then
means we are not storing iint->DIGSIG and that means we can't access it
later with a helper function. So status of iint->DIGSIG has to be returned
with the hook itself and current security hooks don't have any extra
fields to do that.
Thanks
Vivek