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.
