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

On 12/19/19 11:48 AM, Simon McVittie wrote:

On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote:

quoted

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.

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.

quoted

quoted

quoted

    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.