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

[John Johansen]

On 12/19/19 9:02 AM, Stephen Smalley wrote:
> On 12/19/19 11:48 AM, Simon McVittie wrote:
>> On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote:
>>> 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).
>>
>> Because LSM people have told me in the past that the '\0' is not
>> conceptually part of the label, the D-Bus specification and reference
>> implementation already assume that its presence or absence is irrelevant,
>> and normalize to a canonical form (which happens to be that it appends a
>> '\0' if missing, to be nice to C-like languages, but I could equally
>> have chosen to strip the '\0' and rely on an out-of-band length count).
>>
>> By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!)
>> preserve whether the label originally ended with '\0' or not (because
>> they are designed to use '\0' as a terminator for each label), so these
>> new kernel interfaces are already a bit closer than the old kernel
>> interfaces to how D-Bus represents this information.
>>
>> The problematic case is AppArmor's terminating '\n' on
>> /proc/pid/attr/current, because when I asked in the past, I was told
>> that it would be (unwise but) valid to have a LSM where "foo" and "foo\n"
>> are distinct labels.
> 
> I don't agree with that stance, but obviously others may differ.
> 
Its not so much a stance as a reality. The LSM allowed anything except
\0 values as part of the interface and there was no documentation
to set expectations beyond what the code allowed.

This could be tightened.

>> If that hypothetical LSM would make procps-ng lose information (because
>> procps-ng truncates at the first unprintable character), does that change
>> the situation any? Would that make it acceptable for other LSM-agnostic
>> user-space components, like the reference implementation of D-Bus, to
>> assume that stripping a trailing newline from /proc/pid/attr/context
>> or from one of the component strings of /proc/pid/attr/current is a
>> non-lossy operation?
> 
> IMHO, yes.  In fact, looking further, I see that systemd's src/libsystemd/sd-bus/bus-creds.c:bus_creds_add_more() reads /proc/pid/attr/current with its read_one_line_file() helper which ultimately uses read_line_full() and treats EOF, \n, \r, or \0 as terminators and truncates on first such occurrence.
> 

fun

>>
>>>>>     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.
>>
>> If AppArmor was going to change the format of one of its interfaces
>> (or deviate from it when implementing new interfaces), I'd actually
>> prefer it to be /proc/pid/attr/current that changed or was superseded,
>> because /proc/pid/attr/current is the one that contains a newline that
>> consumers are meant to ignore.
>>
>> For what it's worth, libapparmor explicitly removes the newline, so this
>> only matters to LSM-agnostic readers like D-Bus implementations, and to
>> lower-level AppArmor-aware readers that use the kernel interfaces directly
>> in preference to using libapparmor.
> 
> Deferring to the AA maintainer(s) to speak to this.

I will look into what I can do. If we can ditch the trailing \n, that would
be best. I tried to do that once before and we ran into some problems and
I had to revert the change. But that was a long time ago and we can probably
get away with doing it now.