[PATCH v20 05/23] net: Prepare UDS for security module stacking

[John Johansen]

On 9/9/20 6:19 AM, Stephen Smalley wrote:
> On Tue, Sep 8, 2020 at 8:21 PM John Johansen
> <john.johansen at canonical.com> wrote:
>>
>> On 9/8/20 4:37 PM, Casey Schaufler wrote:
>>> On 9/8/2020 6:35 AM, Stephen Smalley wrote:
>>>> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
>>>> <stephen.smalley.work at gmail.com> wrote:
>>>>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
>>>>> <john.johansen at canonical.com> wrote:
>>>>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
>>>>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
>>>>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>>>>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>>>>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>>>>>>> ...
>>>>>>>>
>>>>>>>>>> I understand the concerns you mention, they are all valid as far as
>>>>>>>>>> I'm concerned, but I think we are going to get burned by this code as
>>>>>>>>>> it currently stands.
>>>>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
>>>>>>>>> of secids. It will take someone smarter than me to figure out how to
>>>>>>>>> fit N secids into 32bits without danger of either failure or memory
>>>>>>>>> allocation.
>>>>>>>> Sooo what are the next steps here?  It sounds like there is some
>>>>>>>> agreement that the currently proposed unix_skb_params approach is a
>>>>>>>> problem, but it also sounds like you just want to merge it anyway?
>>>>>>> There are real problems with all the approaches. This is by far the
>>>>>>> least invasive of the lot. If this is acceptable for now I will commit
>>>>>>> to including the dynamic allocation version in the full stacking
>>>>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
>>>>>>> to take even longer than it already has. Sigh.
>>>>>>>
>>>>>>>
>>>>>>>> I was sorta hoping for something a bit better.
>>>>>>> I will be looking at alternatives. I am very much open to suggestions.
>>>>>>> I'm not even 100% convinced that Stephen's objections to my separate
>>>>>>> allocation strategy outweigh its advantages. If you have an opinion on
>>>>>>> that, I'd love to hear it.
>>>>>>>
>>>>>> fwiw I prefer the separate allocation strategy, but as you have already
>>>>>> said it trading off one set of problems for another. I would rather see
>>>>>> this move forward and one set of trade offs isn't significantly worse
>>>>>> than the other to me so, either wfm.
>>>>> I remain unclear that AppArmor needs this patch at all even when
>>>>> support for SO_PEERSEC lands.
>>>>> Contrary to the patch description, it is about supporting SCM_SECURITY
>>>>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
>>>>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
>>>> I remembered that systemd once tried using SCM_SECURITY but that was a
>>>> bug since systemd was using it with stream sockets and that wasn't
>>>> supported by the kernel at the time,
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
>>>> switched over to using SO_PEERSEC.  Subsequently I did fix
>>>> SCM_SECURITY to work with stream sockets via kernel commit
>>>> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
>>>> preferred.  Looking around, I see that there is still one usage of
>>>> SCM_SECURITY in systemd-journald but it doesn't seem to be required
>>>> (if provided, journald will pass the label along but nothing seems to
>>>> depend on it AFAICT).  In any event, I don't believe this patch is
>>>> needed to support stacking AppArmor.
>>>
>>> Stephen is, as is so often the case, correct. AppArmor has a stub
>>> socket_getpeersec_dgram() that gets removed in patch 23. If I remove
>>
>> right but as I said before this is coming, I have been playing with
>> it and have code. So the series doesn't need it today but sooner than
>> later it will be needed
> 
> I don't understand why.  Is there a userspace component that relies on
> SCM_SECURITY today for anything real (more than just blindly passing
> it along and maybe writing to a log somewhere)?  And this doesn't
> provide support for a composite SCM_SECURITY or SCM_CONTEXT, so it
> doesn't really solve the stacking problem for it anyway.  What am I
> missing?  Why do you care about this patch?
> 


personally I don't atm, but there are people who do care about this in
there logs, whether they should or shouldn't is an entirely different
question.

Long term there may be some uses for it that I care about or "have to
care about." For now Casey can drop it from this series.