On 7/8/2020 6:30 PM, Jann Horn wrote:
> On Thu, Jul 9, 2020 at 2:42 AM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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.
>>
>> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>> Cc: linux-api@xxxxxxxxxxxxxxx
> [...]
>> diff --git a/security/security.c b/security/security.c
> [...]
>> +/**
>> + * 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;
>> + int llen;
> Please use size_t to represent object sizes
OK.
> , instead of implicitly
> truncating them and assuming that that doesn't wrap. Using "int" here
> not only makes it harder to statically reason about this code, it
> actually can also make the generated code worse:
>
>
> $ cat numtrunc.c
> #include <stddef.h>
>
> size_t my_strlen(char *p);
> void *my_alloc(size_t len);
>
> void *blah_trunc(char *p) {
> int len = my_strlen(p) + 1;
> return my_alloc(len);
> }
>
> void *blah_notrunc(char *p) {
> size_t len = my_strlen(p) + 1;
> return my_alloc(len);
> }
> $ gcc -O2 -c -o numtrunc.o numtrunc.c
> $ objdump -d numtrunc.o
> [...]
> 0000000000000000 <blah_trunc>:
> 0: 48 83 ec 08 sub $0x8,%rsp
> 4: e8 00 00 00 00 callq 9 <blah_trunc+0x9>
> 9: 48 83 c4 08 add $0x8,%rsp
> d: 8d 78 01 lea 0x1(%rax),%edi
> 10: 48 63 ff movslq %edi,%rdi <<<<<<<<unnecessary instruction
> 13: e9 00 00 00 00 jmpq 18 <blah_trunc+0x18>
> [...]
> 0000000000000020 <blah_notrunc>:
> 20: 48 83 ec 08 sub $0x8,%rsp
> 24: e8 00 00 00 00 callq 29 <blah_notrunc+0x9>
> 29: 48 83 c4 08 add $0x8,%rsp
> 2d: 48 8d 78 01 lea 0x1(%rax),%rdi
> 31: e9 00 00 00 00 jmpq 36 <blah_notrunc+0x16>
> $
>
> This is because GCC documents
> (https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html) that
> for integer conversions where the value does not fit into the signed
> target type, "the value is reduced modulo 2^N to be within range of
> the type"; so the compiler has to assume that you are actually
> intentionally trying to truncate the more significant bits from the
> length, and therefore may have to insert extra code to ensure that
> this truncation happens.
>
>
>> + llen = strlen(lsm) + 1;
>> + newlen = strnlen(new, newlen) + 1;
> This strnlen() call seems dodgy. If an LSM can return a string that
> already contains null bytes, shouldn't that be considered a bug, given
> that it can't be displayed properly? Would it be more appropriate to
> have a WARN_ON(memchr(new, '\0', newlen)) check here and bail out if
> that happens?
Whether or not a security module should include a trailing nul has
been a matter of some discussion. Alas, the discussion has not reached
conscensus. The strnlen() is here to allow modules their own convention.
>
>> + 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);
>> + 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:
>> @@ -2109,6 +2145,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;
>> int display = lsm_task_display(current);
>> int slot = 0;
>>
> [...]
>> 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;
>> + }
> This means that if SELinux refuses to give the caller PROCESS__GETATTR
> access to the target process, the entire "context" file will refuse to
> show anything, even if e.g. an AppArmor label would be visible through
> the LSM-specific attribute directory, right?
That is correct.
> That seems awkward.
Sure is.
> Can
> you maybe omit context elements for which the access check failed
> instead, or embed an extra flag byte to signal for each element
> whether the lookup failed, or something along those lines?
The SELinux team seems convinced that if their check fails, the
whole thing must fail.
> If this is an intentional design limitation, it should probably be
> documented in the commit message or so.
Point.
>
>> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>> + cp, rc);
>> + if (rc < 0) {
>> + kfree(final);
>> + return rc;
>> + }
> Isn't there a memory leak here?
Why yes, there is.
> `cp` points to memory that was
> allocated by hp->hook.getprocattr(), and you're not freeing it after
> append_ctx(). (And append_ctx() also doesn't free it.)
>
>> + }
>> + 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;
