LSM stacking in next for 6.1?

[Casey Schaufler]

On 9/2/2022 2:30 PM, Paul Moore wrote:
> On Tue, Aug 2, 2022 at 8:56 PM Paul Moore <paul at paul-moore.com> wrote:
>> On Tue, Aug 2, 2022 at 8:01 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>>> I would like very much to get v38 or v39 of the LSM stacking for Apparmor
>>> patch set in the LSM next branch for 6.1. The audit changes have polished
>>> up nicely and I believe that all comments on the integrity code have been
>>> addressed. The interface_lsm mechanism has been beaten to a frothy peak.
>>> There are serious binder changes, but I think they address issues beyond
>>> the needs of stacking. Changes outside these areas are pretty well limited
>>> to LSM interface improvements.
>> The LSM stacking patches are near the very top of my list to review
>> once the merge window clears, the io_uring fixes are in (bug fix), and
>> SCTP is somewhat sane again (bug fix).  I'm hopeful that the io_uring
>> and SCTP stuff can be finished up in the next week or two.
>>
>> Since I'm the designated first stuckee now for the stacking stuff I
>> want to go back through everything with fresh eyes, which probably
>> isn't a bad idea since it has been a while since I looked at the full
>> patchset from bottom to top.  I can tell you that I've never been
>> really excited about the /proc changes, and believe it or not I've
>> been thinking about those a fair amount since James asked me to start
>> maintaining the LSM.  I don't want to get into any detail until I've
>> had a chance to look over everything again, but just a heads-up that
>> I'm not too excited about those bits.
> As I mentioned above, I don't really like the stuff that one has to do
> to support LSM stacking on the existing /proc interfaces, the
> "label1\0label2\labelN\0" hack is probably the best (only?) option we
> have for retrofitting multiple LSMs into those interfaces and I think
> we can all agree it's not a great API.  Considering that applications
> that wish to become simultaneous multi-LSM aware are going to need
> modification anyway, let's take a step back and see if we can do this
> with a more sensible API.

This is a compound problem. Some applications, including systemd and dbus,
will require modification to completely support multiple concurrent LSMs
in the long term. This will certainly be the case should someone be wild
and crazy enough to use Smack and SELinux together. Even with the (Smack or
SELinux) and AppArmor case the ps(1) command should be educated about the
possibility of multiple "current" values. However, in a container world,
where an Android container can run on an Ubuntu system, the presence of
AppArmor on the base system is completely uninteresting to the SELinux
aware applications in the container. This is a real use case.

I have chosen to use /proc interfaces for the stacking work for several
reasons. First and foremost, there are a painfully large number of system
applications that will never be modified to support multiple concurrent
LSMs, but that can be used successfully with the interface_lsm "hack".
Another is that it will take years to get a significant number of the
applications that can be changed updated. No one is even going to start
on that until kernel support is upstream.

> I think it's time to think about a proper set of LSM syscalls.

At the very least we need a liblsm that preforms a number of useful
functions, like identifying what security modules are available,
validating "contexts", fetching "contexts" from files and processes
and that sort of thing. Whether it is built on syscalls or /proc and
getxattr() is a matter of debate and taste.

>   We
> have avoided this in the past for several reasons, but over the past
> couple of decades the LSMs have established themselves as a core part
> of Linux with many (all?) major Linux distributions shipping and
> supporting at least one LSM; I think we can justify a handful of well
> designed syscalls, and with Landlock we have some precedence too.
> While I realize syscalls are not the only kernel/userspace API option,
> but given the popularity of namespaces I believe a syscall based
> kernel/userspace LSM API has a number of advantages over the other
> options, e.g. procfs/sysfs, netlink, etc.

You can't script syscalls. A syscall interface is fine if you can also
update the entire system service application base for your distribution.
I don't see that as an option.

> Further, I think we can add the new syscall API separately from the
> LSM stacking changes as they do have standalone value.

I agree, but unless the new system calls take stacking into account
from their inception they're just going to be another interface that
makes stacking harder to accomplish.

>   This would
> help reduce the size and complexity of the stacking patchset, which I
> believe would be a very good thing.

The /proc interfaces interface_lsm and context are really pretty simple.

The addition of multiple subject labels to audit would be the same regardless
of /proc or syscall interfaces. We'd still need multiple LSM data in most
security blobs. The conversion of LSM hook interfaces from secids to lsmblobs
would still be required. As would the conversion from string+len pairs to
lsmcontext structures.

> Introducing the syscall API
> sooner would also allow any applications wanting to make use of the
> crazy new stacked-LSM world a head start as they could be modified
> while the kernel patches were making their way through the
> review/update/merge/release process.

A liblsm based on the /proc interfaces would address that as well.
Just as libselinux abstracts the /proc interfaces now.

> Thoughts?

I wish you'd suggested this three years ago, when I could have done
something with it. If stacking has to go on a two year redesign because
of this it is dead. We've spent years polishing the /proc interfaces.
Changed the names, the content, even bent over backwards to accommodate
the security module that refused to adopt an attr/subdir strategy. 

> To help make things a bit more concrete, I put together a quick
> strawman this afternoon to get the discussion started.  I'm definitely
> not a syscall stylist so please consider this more as an idea and
> discussion starter at this point; if we agree there is value in going
> this direction I can put together a proper patchset to introduce the
> new API ...

I'm not objecting to this proposed API. I am objecting to the idea that
stacking can't progress without it.

>
> /* LSM_ID_XXX values 32 and below are reserved for future use */
> #define LSM_ID_SELINUX 33
> #define LSM_ID_SMACK 34
> #define LSM_ID_TOMOYO 35
> #define LSM_ID_IMA 36
> #define LSM_ID_APPARMOR 37
> #define LSM_ID_YAMA 38
> #define LSM_ID_LOADPIN 39
> #define LSM_ID_SAFESETID 40
> #define LSM_ID_LOCKDOWN 41
> #define LSM_ID_BPF 42
> #define LSM_ID_LANDLOCK 43
>
> /**
>  * struct lsm_mod - LSM module information
>  * @id: the LSM id number, see LSM_ID_XXX
>  * @flags: LSM specific flags, zero if unused
>  */
> struct lsm_mod {
>   unsigned int id;
>   unsigned int flags;
> };
>
> /**
>  * struct lsm_ctx - LSM context
>  * @id: the LSM id number, see LSM_ID_XXX
>  * @flags: LSM specific flags, zero if unused
>  * @ctx_str: the LSM context string
>  */
> struct lsm_ctx {
>   unsigned in id;
>   unsigned int flags;
>   char *ctx_str;

   const char *ctx_str;

or even

   const char *ctx;

err, and there are problems with passing this to a syscall.

> };
>
> /**
>  * lsm_enabled - Return information on the enabled LSMs
>  * @lsm: individual LSM definitions
>  * @count: the number of @lsm elements, updated on return
>  * @flags: reserved for future use, must be zero
>  *
>  * Return information on the different LSMs enabled in the kernel.
>  * On success, this function returns a positive number representing
>  * the number of @lsm array elements, which may be zero if none are
>  * enabled.  If the size of @lsm is insufficient, -E2BIG is returned
>  * and the number of enabled LSMs is returned via @count.  In all
>  * other failure cases, a negative value indicating the error is
>  * returned.
>  */
> int lsm_enabled(struct lsm_mod *lsm, size_t *count,
>   unsigned int flags);

Easy to implement in liblsm by parsing /sys/kernel/security/lsm.

> /**
>  * lsm_current_ctx - Return current tasks's LSM context
>  * @ctx: the LSM contexts
>  * @count: the number of @ctx elements, updated on return
>  * @flags: reserved for future use, must be zero
>  *
>  * Returns the calling task's LSM contexts.  On success this
>  * function returns a positive number representing the number of
>  * @ctx array elements, which may be zero if there are no LSM
>  * contexts assigned to the caller.  If the size of @ctx is
>  * insufficient, -E2BIG is returned and the required number @ctx
>  * elements is returned via @count.  In all other failure cases, a
>  * negative value indicating the error is returned.
>  */
> int lsm_current_ctx(struct lsm_ctx *ctx, size_t *count,
>   unsigned int flags);

Your lsm_ctx struct won't do here. Where do the context strings go?
You can't have an unallocated pointer in a structure you pass to a syscall. 
This is the problem that led to the "lsm\0context\0lsm2\0context2\0"
version of attr/context.

... and you can fill this from /sys/kernel/security/lsm and
/proc/self/attr without using interface_lsm, so it can be implemented
today.

So while I agree that syscalls might be better, they are unnecessary
to support application updates. A liblsm that uses the file based interfaces
that are already there will work as well.