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@xxxxxxxxxxxx>
>>>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>>> Cc: linux-api@xxxxxxxxxxxxxxx
>>>> ---
>>>> 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.
??
