On Wed, Feb 14, 2024 at 05:05:38PM +0100, Jann Horn wrote:

security_setselfattr() has an integer overflow bug that leads to
out-of-bounds access when userspace provides bogus input:
`lctx->ctx_len + sizeof(*lctx)` is checked against `lctx->len` (and,
redundantly, also against `size`), but there are no checks on
`lctx->ctx_len`.
Therefore, userspace can provide an `lsm_ctx` with `->ctx_len` set to a
value between `-sizeof(struct lsm_ctx)` and -1, and this bogus `->ctx_len`
will then be passed to an LSM module as a buffer length, causing LSM
modules to perform out-of-bounds accesses.

Ugh. Thanks for catching this. I continue to want to get the unsigned
integer overflow sanitizer rolled out, which would have caught this.

The following reproducer will demonstrate this under ASAN (if AppArmor is
loaded as an LSM):

#define _GNU_SOURCE
#include <unistd.h>
#include <stdint.h>
#include <stdlib.h>
#include <sys/syscall.h>

struct lsm_ctx {
  uint64_t id;
  uint64_t flags;
  uint64_t len;
  uint64_t ctx_len;
  char ctx[];
};

int main(void) {
  size_t size = sizeof(struct lsm_ctx);
  struct lsm_ctx *ctx = malloc(size);
  ctx->id = 104/*LSM_ID_APPARMOR*/;
  ctx->flags = 0;
  ctx->len = size;
  ctx->ctx_len = -sizeof(struct lsm_ctx);
  syscall(
    460/*__NR_lsm_set_self_attr*/,
    /*attr=*/  100/*LSM_ATTR_CURRENT*/,
    /*ctx=*/   ctx,
    /*size=*/  size,
    /*flags=*/ 0
  );
}

(I'm including an ASAN splat in the patch notes sent to the list.)

Fixes: a04a1198088a ("LSM: syscalls for current process attributes")
Signed-off-by: Jann Horn <jannh@google.com>

Reviewed-by: Kees Cook <redacted>

---
ASAN splat from the reproducer:
==================================================================
BUG: KASAN: slab-out-of-bounds in do_setattr (security/apparmor/lsm.c:860) 
Read of size 1 at addr ffff888006163abf by task setselfattr/548

I'd rather prefer that this splat (or some portion of it) stay in the
actual commit log. It makes it easier to scan for sanitizer-related
fixes, etc.

-Kees

-- 
Kees Cook