Re: UBSAN: array-index-out-of-bounds in alg_bind

[Jann Horn]

+linux-api because this is about fixing UAPI without breaking userspace

On Sat, Oct 17, 2020 at 8:02 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Fri, Oct 16, 2020 at 01:12:24AM -0700, syzbot wrote:
> > dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e
> > [...]
> > Reported-by: syzbot+92ead4eb8e26a26d465e@xxxxxxxxxxxxxxxxxxxxxxxxx
> > [...]
> > UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2
> > index 91 is out of range for type '__u8 [64]'
>
> This seems to be an "as intended", if very odd. false positive (the actual
> memory area is backed by the on-stack _K_SS_MAXSIZE-sized sockaddr_storage
> "address" variable in __sys_bind. But yes, af_alg's salg_name member
> size here doesn't make sense. The origin appears to be that 3f69cc60768b
> ("crypto: af_alg - Allow arbitrarily long algorithm names") intentionally
> didn't extend the kernel structure (which is actually just using the UAPI
> structure). I don't see a reason the UAPI couldn't have been extended:
> it's a sockaddr implementation, so the size is always passed in as part
> of the existing API.

If you e.g. recompiled the wrong parts of the "btcheck" project with
such changed UAPI headers, I think you'd get OOB writes, because they
have this in a header
(https://sources.debian.org/src/btcheck/2.1-4/src/kernelcryptoapi.h/?hl=29#L29):

typedef struct {
  struct sockaddr_alg sa;
  int safd;
  int fd;
} lkca_hash_ctx;

so if you rebuilt e.g. kernelcryptoapi.o (which uses the struct)
without also rebuilding hash.o (which allocates the struct), code in
kernelcryptoapi.o would write beyond the end of lkca_hash_ctx, I
think.

Sure, there aren't many places that do this kind of thing for this
struct. But at least in theory, you can't change the size of UAPI
structs because someone might be passing instances of that struct
around between object files.

> At the very least the kernel needs to switch to using a correctly-sized
> structure: I expected UBSAN_BOUNDS to be enabled globally by default at
> some point in the future (with the minimal runtime -- the
> instrumentation is tiny and catches real issues).

Yeah, the kernel should probably use a struct that looks different
from the userspace one. :/ I guess we'll probably end up with some
ugly hack with "#ifdef __KERNEL__", where the same struct has
different sizes between kernel and userspace? Or am I being too
puritan about UAPI consistency?

> Reproduction:
>
> struct sockaddr_alg sa = {
>     .salg_family = AF_ALG,
>     .salg_type = "skcipher",
>     .salg_name = "cbc(aes)"
> };
> fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
> bind(fd, (void *)&sa, sizeof(sa));
>
> Replace "sizeof(sa)" with x where 64<x<=128.

I think you mean 88<x<=128 ?