Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context

From: Casey Schaufler <casey@schaufler-ca.com>
To: John Johansen <john.johansen@canonical.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: linux-audit@redhat.com, keescook@chromium.org,
	penguin-kernel@i-love.sakura.ne.jp, paul@paul-moore.com,
	sds@tycho.nsa.gov, linux-api@vger.kernel.org,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context
Date: Thu, 30 Jul 2020 15:22:47 -0700	[thread overview]
Message-ID: <18fcf6b1-3ef6-bb8b-d2b2-e0fd45bdb68c@schaufler-ca.com> (raw)
In-Reply-To: <97330b2d-5447-cfef-b6d0-444249e671b7@canonical.com>

On 7/30/2020 1:57 PM, John Johansen wrote:
> On 7/30/20 1:44 PM, Casey Schaufler wrote:
>> On 7/30/2020 3:03 AM, John Johansen wrote:
>>> On 7/24/20 1:32 PM, Casey Schaufler wrote:
>>>> Add an entry /proc/.../attr/context which displays the full
>>>> process security "context" in compound format:
>>>>         lsm1\0value\0lsm2\0value\0...
>>>> This entry is not writable.
>>>>
>>>> A security module may decide that its policy does not allow
>>>> this information to be displayed. In this case none of the
>>>> information will be displayed.
>>>>
>>>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> Cc: linux-api@vger.kernel.org
>>>> ---
>>>>  Documentation/security/lsm.rst       | 28 +++++++++++
>>>>  fs/proc/base.c                       |  1 +
>>>>  include/linux/lsm_hooks.h            |  6 +++
>>>>  security/apparmor/include/procattr.h |  2 +-
>>>>  security/apparmor/lsm.c              |  8 +++-
>>>>  security/apparmor/procattr.c         | 22 +++++----
>>>>  security/security.c                  | 70 ++++++++++++++++++++++++++++
>>>>  security/selinux/hooks.c             |  2 +-
>>>>  security/smack/smack_lsm.c           |  2 +-
>>>>  9 files changed, 126 insertions(+), 15 deletions(-)
>> <snip>
>>
>>>>  
>>>>  /**
>>>> diff --git a/security/security.c b/security/security.c
>>>> index d35e578fa45b..bce6be720401 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -754,6 +754,48 @@ static void __init lsm_early_task(struct task_struct *task)
>>>>  		panic("%s: Early task alloc failed.\n", __func__);
>>>>  }
>>>>  
>>>> +/**
>>>> + * append_ctx - append a lsm/context pair to a compound context
>>>> + * @ctx: the existing compound context
>>>> + * @ctxlen: size of the old context, including terminating nul byte
>>>> + * @lsm: new lsm name, nul terminated
>>>> + * @new: new context, possibly nul terminated
>>>> + * @newlen: maximum size of @new
>>>> + *
>>>> + * replace @ctx with a new compound context, appending @newlsm and @new
>>>> + * to @ctx. On exit the new data replaces the old, which is freed.
>>>> + * @ctxlen is set to the new size, which includes a trailing nul byte.
>>>> + *
>>>> + * Returns 0 on success, -ENOMEM if no memory is available.
>>>> + */
>>>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
>>>> +		      int newlen)
>>>> +{
>>>> +	char *final;
>>>> +	size_t llen;
>>>> +
>>>> +	llen = strlen(lsm) + 1;
>>>> +	/*
>>>> +	 * A security module may or may not provide a trailing nul on
>>>> +	 * when returning a security context. There is no definition
>>>> +	 * of which it should be, and there are modules that do it
>>>> +	 * each way.
>>>> +	 */
>>>> +	newlen = strnlen(new, newlen) + 1;
>>>> +
>>>> +	final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
>>>> +	if (final == NULL)
>>>> +		return -ENOMEM;
>>>> +	if (*ctxlen)
>>>> +		memcpy(final, *ctx, *ctxlen);
>>>> +	memcpy(final + *ctxlen, lsm, llen);
>>>> +	memcpy(final + *ctxlen + llen, new, newlen);
>>> if @new doesn't have a newline appended at its end this will read 1 byte
>>> passed the end of the @new buffer. Nor will the result have a trailing
>>> \0 as expected unless we get lucky.
>> @new will never have a newline at the end. The trailing nul comes
>> from the allocation being done with kzalloc(). This function has to
>> be considered in the context of its caller.
>>
> ugh, sorry not trailing newline, I meant trailing \0. The problem isn't
> the kzalloc, the target has the space. It is the source @new. It is
> dangerous to assume that the @new buffer has a null byte after its
> declared length. Which is potentially what we are doing if @new
> doesn't have an embedded null byte. In that case strlen(new, newlen)
> will then return newlen and we add 1 to it.
>
> which means in the memcpy we are copying an extra byte beyond what
> was declared to exist in @new.

You're right. Good point. Fix coming.
??