Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[John Johansen]

On 09/17/2018 12:23 PM, Casey Schaufler wrote:
> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>
>>> Keep security=$lsm with the existing exclusive behavior.
>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>
>>> If you want to be fancy (I don't!) you could add
>>>
>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>> We've got two issues: ordering and enablement. It's been strongly
>> suggested that we should move away from per-LSM enable/disable flags
>> (to which I agree).
> 
> I also agree. There are way too many ways to turn off some LSMs.
> 
I wont disagree, but its largely because we didn't have this discussion
when we should have. 

>> If ordering should be separate from enablement (to
>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>> didn't include it so it's disabled case), then I think we need to
>> split the logic (otherwise we just reinvented "security=" with similar
>> problems).
> 
> We could reduce the problem by declaring that LSM ordering is
> not something you can specify on the boot line. I can see value
> in specifying it when you build the kernel, but your circumstances
> would have to be pretty strange to change it at boot time.
> 

if there is LSM ordering the getting

  lsm=B,A,C

is not the behavior I would expect from specifying

  lsm=A,B,C


>> Should "lsm=" allow arbitrary ordering? (I think yes.)
> 
> I say no. Assume you can specify it at build time. When would
> you want to change the order? Why would you?
> 

because maybe you care about the denial message from one LSM more than
you do from another. Since stacking is bail on first fail the order
could be important from an auditing POV

Auditing is why apparmor's internal stacking is not bail on first
fail.

>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>> LSMs are implicitly auto-appended to the explicit list)
> 
> If you want to add something that isn't there instead of making
> it explicit you want "lsm.enable=" not "lsm=".
> 
>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>
>> If builtin list was:
>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>> then:
>>
>>     lsm.disable=loadpin lsm=smack
> 
> Methinks this should be lsm.disable=loadpin lsm.enable=smack
> 

that would only work if order is not important

>> becomes
>>
>>     capability,smack,yama,integrity
>>
>> and
>>
>>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
> 
> Do you mean
> 	selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo lsm.enable=integrity
> 	selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
> 	selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity lsm.disable=smack lsm.disable=tomoyo
> 
>> becomes
>>
>>     capability,integrity,yama,loadpin,apparmor
>>
>>
>> If "lsm=" _does_ imply enablement, then how does it interact with
>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
> 
> There should either be one option "lsm=", which is an explicit list or
> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
> 
maybe but this breaks with current behavior as their is a mismatch between
how the major lsms do selection/enablement and the minor ones.

I personally would prefer

  lsm=

but that breaks how the minor lsms are currently enable

> In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
> apparmor off the list, but it's up to the AppArmor code to do that.
>> If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
> of the security module is used, but it's up to the AppArmor code to do that.
> 
current behavior

> If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
> should have shut down AppArmor before it looked to see the "apparmor.enabled=1",
> so it will remain disabled.
> 
yep, current behavior

> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
> specified is used giving "lsm.disable=apparmor".
> 
makes sense