On 12/19/19 12:51 PM, Casey Schaufler wrote: > On 12/19/2019 11:27 AM, John Johansen wrote: >> 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. > > I would be happy to define and document that the compound context does > not include trailing \n. There are no existing security modules or > user space that would be impacted. No one uses /proc/self/attr/context > or SO_PEERCONTEXT yet. If AppArmor is happy with stripping the \n I > think we're good to go. > yes, we can certainly remove it from peer context, and I expect we can remove it from current as well
