Message-ID: <88b0cc69-cd42-1798-6ce4-d3cfbbc79d83@canonical.com>
Date:   Wed, 3 Oct 2018 23:18:01 -0700
From:   John Johansen <john.johansen@...onical.com>
To:     Kees Cook <keescook@...omium.org>, James Morris <jmorris@...ei.org>
Cc:     Jordan Glover <Golden_Miller83@...tonmail.ch>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        Paul Moore <paul@...l-moore.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        "Schaufler, Casey" <casey.schaufler@...el.com>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On 10/03/2018 04:55 PM, Kees Cook wrote:
> On Wed, Oct 3, 2018 at 2:34 PM, James Morris <jmorris@...ei.org> wrote:
>> On Wed, 3 Oct 2018, Kees Cook wrote:
>>
>>> On Wed, Oct 3, 2018 at 11:28 AM, James Morris <jmorris@...ei.org> wrote:
>>>> On Wed, 3 Oct 2018, Kees Cook wrote:
>>>>
>>>>> On Wed, Oct 3, 2018 at 11:17 AM, James Morris <jmorris@...ei.org> wrote:
>>>>>> On Tue, 2 Oct 2018, John Johansen wrote:
>>>>>>> To me a list like
>>>>>>>   lsm.enable=X,Y,Z
>>>>>>
>>>>>> What about even simpler:
>>>>>>
>>>>>> lsm=selinux,!apparmor,yama
>>>>>
>>>>> We're going to have lsm.order=, so I'd like to keep it with a dot
>>>>> separator (this makes it more like module parameters, too). You want
>>>>> to mix enable/disable in the same string? That implies you'd want
>>>>> implicit enabling (i.e. it complements the builtin enabling), which is
>>>>> opposite from what John wanted.
>>>>>
>>>>
>>>> Why can't this be the order as well?
>>>
>>> That was covered extensively in the earlier threads. It boils down to
>>> making sure we do not create a pattern of leaving LSMs disabled by
>>> default when they are added to the kernel. The v1 series used
>>> security= like this:
>>>
>>> +       security=       [SECURITY] An ordered comma-separated list of
>>> +                       security modules to attempt to enable at boot. If
>>> +                       this boot parameter is not specified, only the
>>> +                       security modules asking for initialization will be
>>> +                       enabled (see CONFIG_DEFAULT_SECURITY). Duplicate
>>> +                       or invalid security modules will be ignored. The
>>> +                       capability module is always loaded first, without
>>> +                       regard to this parameter.
>>>
>>> This meant booting "security=apparmor" would disable all the other
>>> LSMs, which wasn't friendly at all. So "security=" was left alone (to
>>> leave it to only select the "major" LSM: all major LSMs not matching
>>> "security=" would be disabled). So I proposed "lsm.order=" to specify
>>> the order things were going to be initialized in, but to avoid kernels
>>> booting with newly added LSMs forced-off due to not being listed in
>>> "lsm.order=", it had to have implicit fall-back for unlisted LSMs.
>>> (i.e. anything missing from lsm.order would then follow their order in
>>> CONFIG_LSM_ORDER, and anything missing there would fall back to
>>> link-time ordering.) However, then the objection was raised that this
>>> didn't provide a way to explicitly disable an LSM. So I proposed
>>> lsm.enable/disable, and John argued for CONFIG_LSM_ENABLE over
>>> CONFIG_LSM_DISABLE.
>>
>> Ok, but it may end up being clearer, simpler, and thus more secure to just
>> have a single way to configure LSM.
>>
>> For example:
>>
>>   - All LSMs which are built are NOT enabled by default
>>
>>   - You specify enablement and order via a Kconfig:
>>
>>         CONFIG_LSM="selinux,yama"
>>
>>   - This can be entirely overridden by a boot param:
>>
>>         lsm="apparmor,landlock"
> 
> This doesn't work with how SELinux and AppArmor do their bootparams,
> unfortunately. (And Paul and Stephen have expressed that the
> documented selinux on/off must continue to work.) For example, let's
> say you've built an Ubuntu kernel with:
> 
> CONFIG_SELINUX=y
> ...
> CONFIG_LSM="yama,apparmor"
> 
> (i.e. you want SELinux available, but not enabled, so it's left out of
> CONFIG_LSM)
> 
> Then someone boots the system with:
> 
> selinux=1 security=selinux
> 
> In what order does selinux get initialized relative to yama?
> (apparmor, flagged as a "legacy major", would have been disabled by
> the "security=" not matching it.)
> 
> 
> The LSM order needs to be defined externally to enablement because
> something may become enabled when not listed in the order.
> 
it doesn't have to be that way. The only argument I can see for
ordering being separate from enablement are
- the order doesn't matter to some LSMs but other LSMs (like landlock)
  need to be in a specific order.

  Separating the ordering from enablement  means the user doesn't
  have to take this into account when overriding a default enablement.

> Now, maybe I misunderstood your earlier suggestion, and what you meant
> was to do something like:
> 
> CONFIG_LSM="yama,apparmor,!selinux"
> 
> to mean "put selinux here in the order, but don't enable it". Then the
> problem becomes what happens to an LSM that has been built in but not
> listed in CONFIG_LSM?
> 
it should be disabled. I know that is not what we have today but
does current behavior of splitting how minor and major LSM enablement
is done was a mistake

> Related to that, this means that when new LSMs are added, they will
> need to be added to any custom CONFIG_LSM= or lsm= parameters. If
> that's really how we have to go, I'll accept it, but I think it's a
> bit unfriendly. :P
> 
That is exactly was will happen in at least some distros. Distros
are committed to maintaining a certain configuration. If some new
LSM comes along it shouldn't change the support config until the
distro can evaluate any interaction will the current supported
configuration.

As a concrete example, the Ubuntu hwe kernels (newer kernels on
LTS releases, are going to want to specify the hwe kernel config
is the same as the release kernel except for few well vetted
exceptions. 

> Another reason I don't like it is because it requires users to know
> about all the LSMs to make changes. One LSM can't be added/removed

The don't have to know about all LSMs, but they do need to know
what the distro has as default set.

> without specifying ALL of the LSMs. (i.e. there is no trivial way to
> enable/disable a single LSM without it growing its own enable/disable
> code as in SELinux/AppArmor. I'd hoped to make that easier for both
> users and developers.) Again, I can live with it, but I think it's
> unfriendly.
> 

well unfriendly to who, and define users :)
- its probably unfriendly to the developer wanting to work with
  a specific LSM.
- a user messing with the different LSMs, yeah probably
- a regular user, generally shouldn't have to know.
- a distro trying to support a specific configuration, very much
  wants to define what is enabled/supported. Whether through
  individual LSM logic, or a centralized place

> I just want to have a direct I can go that meets all the requirements.
> :) I'm fine to ignore my sense of aesthetics if everyone can agree on
> the code.
> 
>> And that's it.
>>
>> Of course, capabilities is always enabled and not be visible to kconfig or
>> boot params.
> 
> Correct. I've made sure that's true in all the versions.
> 
> BTW, there doesn't seem to be disagreement about the earlier part of
> the series, though (patches 1-10). Could these go into -next just so I
> don't have to keep sending them? :)
> 
> LSM: Correctly announce start of LSM initialization
> vmlinux.lds.h: Avoid copy/paste of security_init section
> LSM: Rename .security_initcall section to .lsm_info
> LSM: Remove initcall tracing
> LSM: Convert from initcall to struct lsm_info
> vmlinux.lds.h: Move LSM_TABLE into INIT_DATA
> LSM: Convert security_initcall() into DEFINE_LSM()
> LSM: Record LSM name in struct lsm_info
> LSM: Provide init debugging infrastructure
> LSM: Don't ignore initialization failures
> 
> Thanks!
> 
> -Kees
> 

Powered by blists - more mailing lists