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

[Simon McVittie <smcv@collabora.com>]

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.

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.

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.

    smcv