Re: LSM stacking in next for 6.1?

[John Johansen <john.johansen@canonical.com>]

On 9/8/22 11:05, Casey Schaufler wrote:
> On 9/7/2022 8:57 PM, Paul Moore wrote:
>> On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler <casey@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.
> 

AppArmor too, I am working on revising the out of tree af_unix mediation


>>>> 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.
> 

well at a minimum we shouldn't export the kernel internal LSM_ID if its
exposed to userspace it needs to be something that can live with for a
long time

- Fixed length strings, which really are just a long LSM ID, Say 8 bytes.
   Can still even look human readable. For most* LSMs this could just
   be their name.

   * only safesetid and capability don't fit atm

- and certainly uglier, for variable length use an index for one of the
   variable length strings, with an embedded \0 inside the variable length
   string

{
   size_t lsm_id_len;
   size_t lsm_id_ctx_index;
   size_t ctx_len;
   unsigned char ctx[];
}

with access to lsm id being ctx[lsm_id_ctx_index]