3:57 p.m.
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>
>> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com>
>> 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.
>
>
>> + kfree(*ctx);
>> + *ctx = final;
>> + *ctxlen = *ctxlen + llen + newlen;
>> + return 0;
>> +}
>> +
>> /*
>> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
>> * can be accessed with:
>> @@ -2124,6 +2166,10 @@ int security_getprocattr(struct task_struct *p, const char
*lsm, char *name,
>> char **value)
>> {
>> struct security_hook_list *hp;
>> + char *final = NULL;
>> + char *cp;
>> + int rc = 0;
>> + int finallen = 0;
> these are only used by context so they could be moved under its if, this
> is really just a style comment and I'll leave it up to you
Old coding habits die hard. Unless there's value to gain, I'll leave it
as is.
>
>> int display = lsm_task_display(current);
>> int slot = 0;
>>
>> @@ -2151,6 +2197,30 @@ int security_getprocattr(struct task_struct *p, const char
*lsm, char *name,
>> return -ENOMEM;
>> }
>>
>> + if (!strcmp(name, "context")) {
>> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
>> + list) {
>> + rc = hp->hook.getprocattr(p, "context", &cp);
>> + if (rc == -EINVAL)
>> + continue;
>> + if (rc < 0) {
>> + kfree(final);
>> + return rc;
>> + }
>> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>> + cp, rc);
>> + kfree(cp);
>> + if (rc < 0) {
>> + kfree(final);
>> + return rc;
>> + }
>> + }
>> + if (final == NULL)
>> + return -EINVAL;
>> + *value = final;
>> + return finallen;
>> + }
>> +
>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index c13c207c5da1..43d5c09b9a9e 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6288,7 +6288,7 @@ static int selinux_getprocattr(struct task_struct *p,
>> goto bad;
>> }
>>
>> - if (!strcmp(name, "current"))
>> + if (!strcmp(name, "current") || !strcmp(name, "context"))
>> sid = __tsec->sid;
>> else if (!strcmp(name, "prev"))
>> sid = __tsec->osid;
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 6f0cdb40addc..d7bb6442f192 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -3463,7 +3463,7 @@ static int smack_getprocattr(struct task_struct *p, char
*name, char **value)
>> char *cp;
>> int slen;
>>
>> - if (strcmp(name, "current") != 0)
>> + if (strcmp(name, "current") != 0 && strcmp(name,
"context") != 0)
>> return -EINVAL;
>>
>> cp = kstrdup(skp->smk_known, GFP_KERNEL);
>>