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

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(a)chromium.org&gt; >>> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com&gt; >>> Cc: linux-api(a)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.