Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

[John Johansen]

On 10/02/2018 12:17 PM, Kees Cook wrote:
> On Tue, Oct 2, 2018 at 11:57 AM, John Johansen
> <john.johansen@xxxxxxxxxxxxx> wrote:
>> Under the current scheme
>>
>> lsm.enabled=selinux
>>
>> could actually mean selinux,yama,loadpin,something_else are
>> enabled. If we extend this behavior to when full stacking lands
>>
>> lsm.enabled=selinux,yama
>>
>> might mean selinux,yama,apparmor,loadpin,something_else
>>
>> and what that list is will vary from kernel to kernel, which I think
>> is harder for the user than the lsm.enabled list being what is
>> actually enabled at boot
> 
> Ah, I think I missed this in your earlier emails. What you don't like
> here is that "lsm.enable=" is additive. You want it to be explicit.
> 
> Are you okay with lsm.order= having fallback?
> 

yeah, if we are going to separate order, fallbacks are fine for
anything that isn't specified.

I am still not convinced that separating order from enablement is
right, but its generally something a user should care about so I can
live with it.

> The situation we were trying to solve was with new LSMs getting
> implicitly disabled if someone is booting with an explicit list. For
> example:
> 
> lsm.enable=yama,apparmor
> 
> means when "landlock" gets added to the kernel, it will be implicitly disabled.
> 

And here is the point of contention, I wouldn't call that implicitly
disabled. The user explicitly selected a set of LSMs to enable. Having
other LSMs enable when they aren't specified is confusing to a user,
as now they have to consider what is enabled by default in the
Kconfig.

I think requiring distros/builders to consider Kconfig options is
fine, but its a lot higher hurdle for regular users.


>> If we have to have multiple kernel parameter, I prefer a behvior where
>> if you hav conflicting kernel parameters specified
>>
>>   apparmor=0 lsm.enabled=apparmor
>>
>> that the conflict is logged and the lsm is left disabled, as I think
>> it is easier for users to understand than the overrides scheme of v3,
>> and sans logging of the conflict is effectively what we had in the
>> past
>>
>>   apparmor=0 security=apparmor
>> or
>>   apparmor=1 security=selinux
>>
>> would result in apparmor being disabed
> 
> Okay, so for this part you want per-LSM boot param to have priority
> (which seems to match SELinux's concerns), possibly logging the

hrmmm I wouldn't call it priority :)

If you look at the above logic its a boolean AND operation. The LSM is
only enabled if $LSM=1 AND security=$LSM all other combinations result
in $LSM being disabled

> conflict, but still accepting the apparmor= and selinux= state.

logging is nice for the user but certainly isn't required and is more
than we are doing today

> security= would still driving initialization ordering (so I think the
> behavior I have in the series would be correct).
> 
>> That being said I get we have a mess currently, and there really
>> doesn't seem to be a good way to fix it. I think getting this right
>> for the user is important enough that I am willing to break current
>> apparmor userspace api. While apparmor=0 is documented we have also
>> documented security=X for years and apparmor=0 isn't used too often
>> so I think we can drop it to help clean this mess up abit.
>>
>> I am not going to Nak, or block on v3 behavior if that is considered
>> the best path forward after this discussion/rant.
> 
> I could define CONFIG_LSM_ENABLE as being "additive" to
> SECURITY_APPARMOR_BOOTPARAM_VALUE and
> SECURITY_SELINUX_BOOTPARAM_VALUE?
> 

Oh sure lets deal with my complaint about too many ways to configure
this beast by adding yet another config option :P

seriously though, please no. That just adds another layer of confusion
even if it is only being foisted on the distro/builder