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
