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

[Casey Schaufler]

On 12/19/2019 11:27 AM, John Johansen wrote:
> 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.

I would be happy to define and document that the compound context does
not include trailing \n. There are no existing security modules or
user space that would be impacted. No one uses /proc/self/attr/context
or SO_PEERCONTEXT yet. If AppArmor is happy with stripping the \n I
think we're good to go.