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

[Mimi Z. <zo...@li...>]

On Sat, 2010-04-17 at 12:22 -0400, Eric Paris wrote:
> 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:
> 

Sorry, definitely should have documented it. The IMA ltp testsuite
contains an example of loading a policy.
(<ltp>/testcases/kernel/security/integrity/ima/tests/ima_policy.sh)

> 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 mentioned using 'cat' to load a policy containing comments could cause
problems, as it isn't a rule. The current code would probably reject the
entire policy, thinking the comment was an invalid rule.

> 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)

Agreed, as only the last one is used.

> 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

The assumption was one rule per line.

Mimi