linux-kernel - Re: UBSAN: array-index-out-of-bounds in alg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Oct 2020 20:49:38 -0700
From:   Kees Cook <keescook@...omium.org>
To:     herbert@...dor.apana.org.au
Cc:     syzbot <syzbot+92ead4eb8e26a26d465e@...kaller.appspotmail.com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com, linux-hardening@...r.kernel.org,
        Elena Petrova <lenaptr@...gle.com>
Subject: Re: UBSAN: array-index-out-of-bounds in alg_bind

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@...kaller.appspotmail.com
> [...]
> 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.

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).

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.

-- 
Kees Cook