[PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs

[Stephen Smalley]

On 12/19/19 8:47 AM, Stephen Smalley wrote:
> On 12/19/19 7:19 AM, Simon McVittie wrote:
>> On Wed, 18 Dec 2019 at 15:50:45 -0500, Stephen Smalley wrote:
>>> Ok, this seems to be a lack of support in AppArmor for saving the 
>>> peer info
>>> for unix/local domain sockets, so not your bug.  Doesn't implement the
>>> necessary hooks.
>>
>> Ubuntu kernels have working getsockopt SO_PEERSEC for AppArmor (which
>> definitely works for D-Bus, or did when I tried it in the past), but it
>> seems to be a downstream patch.
>>
>> Is there a combination of LSMs where this works correctly and shows
>> multiple LSMs' labels, either upstream or with downstream patches? D-Bus
>> ought to be an early consumer of this information, but that's going to
>> be difficult if there's nowhere I can test it. If there was a pair of
>> in-tree or out-of-tree test LSMs that automatically labelled process
>> 12345 and all sockets that it created with something like (pseudocode)
>>
>> {
>>    "labeltest1": "labeltest1process12345",
>>    "labeltest2": "labeltest2process12345"
>> }
>>
>> then that would make testing straightforward.
> 
> AFAICT, you can't yet stack Smack+SELinux, and AppArmor requires 
> out-of-tree patches to support SO_PEERSEC so there is no in-tree 
> combination that will demonstrate the new SO_PEERCONTEXT for multiple lsms.
> 
>>  From my user-space developer perspective, I'd really like the kernel
>> to have at least brief documentation of the assumptions that can be
>> made about the compound format, to avoid people like me having to
>> ask the linux-security-module mailing list and use the answers as our
>> documentation:
>>
>> - Can I assume that the LSM names (conceptually, keys in the associative
>>    array) are printable ASCII? If not, can I at least assume that they 
>> are
>>    printable UTF-8, or that LSMs whose names are not printable UTF-8 are
>>    too weird to be worth supporting in user-space APIs?
>>
>>    If this decision hasn't been made yet, I would like to request ASCII,
>>    or UTF-8 if a wider character repertoire is desired - that would make
>>    user-space APIs a lot easier to write.
> 
> ASCII works for me.
> 
>>
>> - What can I assume about the format of the labels (values in the
>>    associative array)?
>>
>>    From previous conversations on linux-security-module it seems the
>>    answer is: they are intended to be printable strings; their encoding
>>    is unspecified (could be ASCII or UTF-8, but equally could be Latin-1
>>    or anything else); there are no reserved or disallowed characters
>>    except '\0'; individual LSMs like SELinux and AppArmor might impose
>>    tighter restrictions, but LSM-agnostic code can't assume those
>>    restrictions are present.
>>
>>    Even if the format is conceptually an unspecified encoding with every
>>    nonzero byte allowed, if LSM and security policy authors establish a
>>    convention that the labels are ASCII or UTF-8 without control 
>> characters
>>    such as newlines, that would make user-space a lot easier to deal 
>> with.
> 
> I believe there is existing userspace code that seems to presume that 
> they are NUL-terminated strings printable using %s format specifiers to 
> printf-like functions in output and log messages _without_ any escaping. 
>   Not just the LSM-specific libraries. systemd (for SO_PEERSEC), 
> procps-ng (for /proc/pid/attr/current), perhaps others.  I think we can 
> rely on that remaining true since the world would break.

Looks like userspace is generally forgiving of whether the terminating 
NUL byte is included or omitted by the kernel (with different behaviors 
for SELinux - always included, Smack - omitted by /proc/pid/attr/current 
but included in SO_PEERSEC, and AppArmor - omitted for 
/proc/pid/attr/current but includes a terminating \n, omitted for 
SO_PEERSEC but no terminating \n), and procps-ng explicitly tests for 
printable characters (but truncates on the first unprintable character). 
If we want consistency for /proc/pid/attr/context and SO_PEERCONTEXT, 
we'd need different hooks for one or both of them; the current stacking 
patches just internally use the existing hooks with their current 
behaviors for current and SO_PEERSEC and then compose them.

> 
>> - Can I assume that the names and labels in /proc/$pid/attr/context
>>    correspond exactly (same label <=> same bytes) to the ones in
>>    SO_PEERCONTEXT?
>>
>>    In particular, a design issue in the previous interfaces with
>>    /proc/$pid/attr/current and SO_PEERSEC was that AppArmor puts
>>    "unconfined\n" in /proc/$pid/attr/current, but "unconfined\0" in
>>    SO_PEERSEC, and implementations of protocols like D-Bus can't know
>>    that these are meant to be the same without LSM-specific knowledge
>>    (namely "AppArmor profile names can't contain newlines").
>>
>>    If this new API is an opportunity to declare that LSMs are expected
>>    to put the same canonical form of a label in 
>> /proc/$pid/attr/context and
>>    SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or
>>    '\0' or similar) exposed in the older /proc/$pid/attr/current and
>>    SO_PEERSEC interfaces for backwards compatibility, then that would 
>> make
>>    life a lot easier for user-space developers like me.
> 
> I'm all for this but the current implementation reuses the same 
> underlying hooks as SO_PEERSEC, so it gets the same result for the 
> per-lsm values.  We'd need a separate hook if we cannot alter the 
> current AppArmor SO_PEERSEC format.  Technically AppArmor seemingly 
> doesn't truly provide SO_PEERSEC in mainline today even though it 
> implements the hook for returning the result because it never actually 
> sets the peer on Unix domain or INET domain connections AFAICT, so they 
> could change the format upstream without a compatibility break but it 
> may break Ubuntu.  So it might require using a new hook for 
> SO_PEERCONTEXT.  Which is fine with me.
> 
>> - Does the order of the names and values matter? Can I assume anything
>>    about them?
>>
>>    It would make the D-Bus API more straightforward if I can assume that
>>    the order doesn't matter, and represent it as an unordered map
>>    (associative array), like a Perl hash or Python dict.
> 
> IMHO order shouldn't matter.
> 
>> I'm asking all this from the D-Bus perspective, but anything that
>> wants to store or forward LSM contexts in an LSM-agnostic way will
>> have essentially the same requirements - I could imagine X11, Wayland,
>> varlink etc. hitting the same problems D-Bus did.
>>
>> Thanks,
>>      smcv
>>
>