LSM stacking in next for 6.1?

[Casey Schaufler]

On 9/7/2022 8:57 PM, Paul Moore wrote:
> On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>> On 9/7/2022 4:27 PM, Paul Moore wrote:
> ..
>
>>>   I
>>> just want an interface that is clearly defined, has reasonable
>>> capacity to be extended in the future as needed, and is easy(ish) to
>>> use and support over extended periods of time (both from a kernel and
>>> userspace perspective).
>>>
>>> The "smack_label\0apparmor_label\0selinux_label\0future_lsm_label\0"
>>> string interface is none of these things.

That wasn't the proposal. The proposal was

"smack\0smack_label\0apparmor\0apparmor_label\0future_lsm\0future_lsm_label\0"

>> In this we disagree ....
> I think we can both agree that there is a subjective aspect to this
> argument and it may be that we never reach agreement on the "best"
> approach, if there even is such a thing.  What I am trying to do here
> is describe a path that would allow me to be more comfortable merging
> the LSM stacking functionality; I don't think you've had a clearly
> defined path, of any sort, towards getting these patches merged, which
> is a problem and I suspect the source of some of the frustration.  My
> comments, as objectionable as you may find them to be, are intended to
> help you move forward with these patches.

OK. Let's get'er done.

>>>   It is not clearly defined
>>> as it requires other interfaces to associate the labels with the
>>> correct LSMs.
>> The label follows the lsm name directly. What other association is required?
> You need to know the order of the LSMs in order to
> interpret/de-serialize the string.

That's true for the string you included, but not for what I had
actually proposed.

>>>   The ease-of-use quality is a bit subjective, but it does need
>>> another interface to use properly and it requires string parsing which
>>> history has shown to be an issue time and time again (although it is
>>> relatively simple here).
>> There was a lot of discussion regarding that. My proposed
>> apparmor="unconfined",smack="User" format was panned for those same reasons.
>> The nil byte format has been used elsewhere and suggested for that reason.
> Based on what I recall from those discussions, it was my impression
> the nil byte label delimiter was suggested simply because no one was
> entertaining the idea of using something other than the existing
> procfs interface.  It is my opinion that we've taken that interface
> about as far as it can go, and while it needs to stay intact for
> compatibility reasons, the LSM stacking functionality should move to a
> different API that is better suited for it.

It's going to raise its ugly head again with SO_PEERCONTEXT for the
SELinux+Smack case. But we can cross that bridge when we come to it.

>>> Once again, the syscall example I tossed out was a quick strawman, but
>>> it had clearly separated and defined labels conveyed in data
>>> structures that didn't require string parsing to find the label
>>> associated with an LSM.
>> True, but it uses pointers internally and you can't do that in memory
>> you're sending up from the system. What comes from the syscall has to
>> look something like the nil byte format. The strawman would have to do
>> the same sort of parsing in userspace that you are objecting to.
> Then we change the strawman.  That's pretty much the definition of a
> strawman example, it's something you toss out as starting point for
> discussion and future improvements.  If it was something much more
> fully developed I would have submitted a patch .... sheesh.

Fair enough.

> In case it helps spur your imagination, here is a revised strawman:
>
> /**
>  * struct lsm_ctx - LSM context
>  * @id: the LSM id number, see LSM_ID_XXX

A LSM ID hard coded in a kernel header makes it harder to develop new
security modules. The security module can't be self contained. I say
that, but I acknowledge that I've done the same kind of thing with the
definition of the struct lsmblob. That isn't part of an external API
however. It may also interfere with Tetsuo's long standing request that
we don't do things that prevent the possibility of loadable security
modules at some point in the future. I will also mention the out-of-tree
security module objection, not because I care, but because someone most
likely will bring it up.

On the other hand, there's no great way to include two variable length
strings in a structure like this. So unless we adopt something as ugly
as the nil byte scheme this is supposed to replace I expect we're stuck
with an LSM ID.

>  
>  * @flags: LSM specific flags, zero if unused

For an API shouldn't this be a specific size? u32? I'm not really
up to date on the guidance regarding which it should be.

>  * @ctx_len: the size of @ctx
>  * @ctx: the LSM context
>  */
> struct lsm_ctx {
>   unsigned in id;
>   unsigned int flags;
>   size_t ctx_len;
>   unsigned char ctx[];
> };
>
> /**
>  * lsm_current_ctx - Return current tasks's LSM context
>  * @ctx: the LSM contexts
>  * @size: the size of @ctx, 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 minimum required
>  * size is returned via @count.  In all other failure cases, a

s/count/size/ - but I get what you meant.

>  * negative value indicating the error is returned.
>  */
> int lsm_current_ctx(struct lsm_ctx *ctx, size_t *size,
>                     unsigned int flags);

This is a workable interface. Certainly more friendly than the
nil byte format.

>   It was also self-contained in that no other
> interface was needed to interpret the results of the syscall (well,
> beyond the header file definitions I guess).  Finally, it made use of
> flags and "reserved for future use" token values to allow for
> additional data to be conveyed in the future.
>> Can you describe potential flags or additional data? Planning for the future
>> is a good idea, but throwing fields on "just in case" seems contrary to
>> what I'm used to hearing from you.
> Adding flags to a syscall is a fairly common practice.  If you're
> making the mistake of confusing this with the discussion that is
> ongoing audit/fanotify discussion, you can think of the flags as
> similar to adding a type/length field to a struct (which I support).

OK. I can see potential.

I will head in this direction. A couple questions:

Would we want lsm_prev_ctx() as well as lsm_current_ctx(),
or should we use the lsm_ctx->flags to identify the provided
context? If we did that we should have an lsm_ctx() system call
that returns the current, prev, and whatever other security
module specific attributes might be associated with the process,
each identified in the flags field. While the "current" context
is usually what we're after, there may be cases where the other
attributes are desired.