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

[John Johansen]

On 12/19/19 8: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.
> 
that is true if any value except \0 is allowed, which is/was the case. I am
not opposed to the LSM defining it otherwise.

I would also love to ditch the trailing \n from /proc/pid/attr/current,
we tried that before and ran into problems with something in userspace.
I can look into it again.


> 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?
> 
>>>>    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.
> 
Right, I am not opposed to a new interface. Ditching the trailing \n
would be even better if we can get rid of it

> 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.
> 

yeah

>     smcv
>