LSM stacking in next for 6.1?

[Casey Schaufler]

On 9/8/2022 12:32 PM, Paul Moore wrote:
> On Thu, Sep 8, 2022 at 2:05 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>> 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:
> ..
>
>>>>>   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.
> There are also problems with IP_PASSSEC/SCM_SECURITY that we've never
> fully resolved (and have gotten a bit lucky over the years); it very
> well could be time to add support for IP_SECURITY as the multi-LSM
> replacement for SCM_SECURITY.  We could leverage the same LSM context
> structures as in the other multi-LSM interfaces.  Existing
> applications could continue to use SCM_SECURITY; in fact I believe we
> could have both SCM_SECURITY and IP_SECURITY in the same message for
> maximum compatibility.

Adding IP_SECURITY seems sensible. Having both at the same time leaves
us with the question of which security module's value to put in SCM_SECURITY
when there are multiple choices. I assume we'd use the first available
value, as determined by the LSM list order, or the interface_lsm if we're
supporting that concept. And again, that's a problem for the complete
stacking round.

> https://github.com/SELinuxProject/selinux-kernel/issues/24
>
> For SO_PEERSEC, we should probably just introduce SO_PEERSEC2 or
> similar, using the same multi-LSM context structures as the other
> interfaces.

Also sensible, although I think SO_PEERCONTEXT or SO_PEERCTX is a better
name than SO_PEERSEC2. Also a problem for complete stacking and it is
way to early for me to get into bikeshedding.

>>> 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.
> There is so much precedence for defining a token scalar value to
> represent a "thing" that I don't know where to begin.  Look at IANA,
> there are entire organizations that exist to map "things" to numbers.
>
> If you're objecting to assigning LSMs fixed integer numbers you've got
> to give me some very explicit reasons (complete with examples) as to
> why that would be a mistake.

I talked myself out of the objection below. Thanks for listening.

>> 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.
> I'm not following you here.  See my comment above about better examples.
>
>> 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 already replied to Tetsuo's email, and while this particular point
> about LSM ID numbers wasn't directly addressed, my response there
> seems to apply equally well here: it's not so much about loadable
> LSMs, it's about out-of-tree LSMs.  These are two very different
> things, with different solutions.

Agreed.

>> 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.
> I don't like making general comments, but when in doubt, consider me
> not-a-fan of string-based identifiers in APIs.  Give me token scalar
> values instead.

Works for me.

>>>  * @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.
> Enh, sure, whatever.  You'll remember my initial comment about not
> being a syscall stylist; if the discussion has moved to seriously
> discussing the syscall prototypes we should likely start a new thread
> and bring in the syscall folks ... I vaguely remember there was a
> mailing list for syscalls and API changes ...

Good idea. I'm reading the official how-to-write-a-syscall documentation.


>> I will head in this direction. A couple questions:
>>
>> Would we want lsm_prev_ctx() as well as lsm_current_ctx(),
> I'm not sure I'm following your thinking, what would lsm_prev_ctx() do?

Instead of the values in /proc/.../attr/current you'd get the
values from /proc/.../attr/prev.


>> 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.
> I don't understand what you mean by "prev"{ious} attributes.  I'm
> thinking of lsm_current_ctx() as a multi-LSM replacement for
> /proc/self/attr/current.  If, for example, we wanted something for
> /proc/self/attr/exec I imagine we would create lsm_current_exec(), or
> something similarly named, with a similar prototype.
>
> Or perhaps we try to stick a bit closer to the procfs naming and go
> with lsm_self_cur(...) and lsm_self_exec(...).  All things to discuss.

I'm thinking that a syscall lsm_self_attr() would get all of the attributes
that are available from /proc/.../attr today. Flags would identify what
kind of attribute they are:

	LSM_ATTR_CURRENT	0x0001 /* Current security context */
	LSM_ATTR_PREV		0x0002 /* Previous security context */
	LSM_ATTR_EXEC		0x0004 /* Exec security context */
	...
	LSM_ATTR_SOCREATE	0x0020 /* Socket creation context */

As you suggest above (with a touch of modification) lsm_self_current()
gets just the LSM_ATTR_CURRENT values, lsm_self_exec() gets the
LSM_ATTR_EXEC values. It would be easy to add new attributes and implement
library functions to filter them out rather than have to create new
syscalls every time a new attribute is added.

It might also be useful to have a flag value (See? You were right) for
the syscalls LSM_ATTR_INTERFACE_LSM which instructs the syscall to only
return the attribute values for the interface_lsm. Hmm. How about allowing
the caller to specify which security module they want the values for by
LSM ID? That could be useful although certainly not necessary. It could
make the user space processing more efficient. It would make converting
libselinux and libsmack to use the syscalls a little bit easier. I'm not
wedded to the LSM ID filter as the caller can always ignore data from
security modules it doesn't care about.

What about lsm_self_attr_set()? This seem rife with peril. Setting a group
of security attributes on a process could fail halfway through and then
require unwinding the preceding successes. I could see having this syscall
if it was only allowed to set a single attribute at a time.

lsm_pid_attr() would be like lsm_self_attr(), but would take a process id as
an additional parameter and get the values for the specified process. This
could make a bunch of dbus' /proc accesses unnecessary. It would have to be
quite a bit more complicated than lsm_self_attr() because it would have to
verify that the caller has appropriate access to the target process. We can
debate whether Smack can deny access to another process' SELinux context.
We can also debate whether an incomplete operation (you can get the SELinux
context, but not the AppArmor context) is an error.

I am going to start playing with these syscalls. Please help me understand
where I have suggested something stoopid.

Thank you.