Re: [Linux-ima-user] [RFC][PATCH][0/2] IMA

[Eric P. <ep...@pa...>]

On Fri, Apr 16, 2010 at 11:12 AM, Roberto Sassu <rob...@po...> wrote:
> On Thursday 15 April 2010 00:20:00 Mimi Zohar wrote:
>> On Wed, 2010-04-14 at 15:32 -0400, Eric Paris wrote:
>> > On Wed, Apr 14, 2010 at 5:51 AM, Roberto Sassu <rob...@po...> wrote:
>> > > Description of the issue:
>> >
>> > > The parsing function is used only when custom rules must be loaded to replace
>> > > the default ones, but a different behavior can be observed in respect to the
>> > > first case: only one element is allocated and it is populated by the parsing
>> > > function that recognizes the tokens ' ' and '\n'.
>> > > So, having multiple lines in the file passed through the exported interface
>> > > doesn't lead to the same number of rules in the list, since only one allocated
>> > > item is available. If a directive spans two different lines, only the last
>> > > value is taken and the first one is overwritten.
>> >
>> > I admit I haven't looked at the loading code before and it does seem
>> > 'special.'   It looks to me on first glance like the kernel accepts
>> > one rule per write() rather than one rule per line.  If you send it
>> > two rules in a single write you are going to get a rather
>> > unpredictable result.
>>
>> Similar to smack, one rule per write.  Keeps it nice and simple.
>>
>> > Your change is a good start to allow single writes which contain
>> > multiple rules.  I'm guessing that's a good idea, I though I think the
>> > parser should get a little smarter.  Does it ever make sense to have 2
>> > actions in a single rule, with the last one winning?  You also added
>> > support for some sort of commenting in the rule set didn't you?  That
>> > probably needs to be a lot stronger too....
>> >
>> > I'm not acking or nacking here, just trying to understand exactly the
>> > problem we are trying to solve.  Loading rules using
>> >
>> > cat rule > /sys/kernel/security/ima/policy
>> >
>> > seems like a good direction to head.
>>
>> hm, I was under the impression we wanted to minimize any
>> policy/configuration parsing done in the kernel.
>>
>
> In my thinking, actually the behaviour of the code that loads the policy through the /sys interface
> is different from the one that handles hardcoded rules. In the last case these are stored as array of structures
> and each element is added to the list by the "list_add_tail" function. I past here the few lines of the code
> involved:
>
> void __init ima_init_policy(void)
> {
>        int i, entries;
>
>        /* if !ima_use_tcb set entries = 0 so we load NO default rules */
>        if (ima_use_tcb)
>                entries = ARRAY_SIZE(default_rules);
>        else
>                entries = 0;
>
>        for (i = 0; i < entries; i++)
>                list_add_tail(&default_rules[i].list, &measure_default_rules);
>        ima_measure = &measure_default_rules;
> }

You get the same behaviour with your loaded rules if you load them the
'right' way (which is non-obvious, IMHO)  You need to

open(sysfs file)
write(rule one)
write(rule two)
write(rule three)
write(rule four)
close(sysfs_file)

I'm guessing you used cat file > sysfs_file which resulted in

open(sysfs_file)
write(rule one rule two rule th);
write(ree rule four)
close(sysfs_file)

Which resulted in the crap behaviour you got.  I suggested to Mimi off
list that maybe the best solution would be to change the parser a
little such that we stop parsing on \n on return a short write equal
to the length of the one rule we parsed.  Thus the way cat handles
things we would get:

open(sysfs_file)
write(rule one rule two rule_th)
which would return 8 (length of "rule one") so cat would next try
write (rule two rule three rul)
which would return 8 (length of "rule two") so cat would next try
write (rule three rule four)
which would return 10 (length of "rule three") so cat would next try
write (rule four)
which would return a full write 9 (length of "rule four)
close (sysfs_file)

I think that solves the problem you were trying to suggest by relying
on a properly implemented userspace.

Mimi suggest we also add parsing for comments, which I'm ok with.
I'm also think if we go this way we should check 2 other things:

1) make sure that we never have the same options set twice (aka can't
set action to both measure and don't measure in the same rule)
2) make sure that rules ALWAYS end in \n, so if userspace doesn't
write the full rule we have some clue and can reject it.

I don't know if #2 is backwards compatible with how whatever utility
IBM uses to loads rules works, but seeing as how noone else has hit
this until now, I'm guess it can't bite many people in the rump.....

-Eric