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

From: John Johansen <john.johansen@canonical.com>
To: Casey Schaufler <casey@schaufler-ca.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Simon McVittie <smcv@collabora.com>
Cc: casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	keescook@chromium.org, penguin-kernel@i-love.sakura.ne.jp,
	paul@paul-moore.com
Subject: Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
Date: Thu, 19 Dec 2019 13:41:38 -0800	[thread overview]
Message-ID: <9c000fcc-7d83-622d-c9fb-faa9e0815161@canonical.com> (raw)
In-Reply-To: <abea0322-cf44-c37f-35c3-955adb8a455d@schaufler-ca.com>

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