Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

On Mon, Oct 26, 2020 at 10:57 PM Eric Biggers [off-list ref] wrote:

On Mon, Oct 26, 2020 at 10:23:35PM +0100, 'Jann Horn' via syzkaller-bugs wrote:

quoted

On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers [off-list ref] wrote:

quoted

Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm
names") made the kernel start accepting arbitrarily long algorithm names
in sockaddr_alg.

That's not true; it's still limited by the size of struct
sockaddr_storage (128 bytes total for the entire address).

Interesting, so the actual limit is 104 bytes.  It seems like the intent of that
commit was to make it unlimited, though...

quoted

If you make it longer, __copy_msghdr_from_user() will silently truncate the
size.

That's used for sys_sendmsg(), which AFAICT isn't relevant here.  sockaddr_alg
is used with sys_bind(), which fails with EINVAL if the address is longer than
sizeof(struct sockaddr_storage).

Ugh, of course you're right, sorry.

However, since sys_sendmsg() is truncating overly-long addresses, it's probably
the case that sizeof(struct sockaddr_storage) can never be increased in the
future...

Eh, I think there'd probably be bigger issues with that elsewhere.

quoted

quoted

This is broken because the kernel can access indices >= 64 in salg_name,
which is undefined behavior -- even though the memory that is accessed
is still located within the sockaddr structure.  It would only be
defined behavior if the array were properly marked as arbitrary-length
(either by making it a flexible array, which is the recommended way
these days, or by making it an array of length 0 or 1).

We can't simply change salg_name into a flexible array, since that would
break source compatibility with userspace programs that embed
sockaddr_alg into another struct, or (more commonly) declare a
sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'.

One solution would be to change salg_name into a flexible array only
when '#ifdef __KERNEL__'.  However, that would keep userspace without an
easy way to actually use the longer algorithm names.

Instead, add a new structure 'sockaddr_alg_new' that has the flexible
array field, and expose it to both userspace and the kernel.
Make the kernel use it correctly in alg_bind().

[...]

quoted

@@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
        const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY;
        struct sock *sk = sock->sk;
        struct alg_sock *ask = alg_sk(sk);
-       struct sockaddr_alg *sa = (void *)uaddr;
+       struct sockaddr_alg_new *sa = (void *)uaddr;
        const struct af_alg_type *type;
        void *private;
        int err;

@@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
        if (sock->state == SS_CONNECTED)
                return -EINVAL;

-       if (addr_len < sizeof(*sa))
+       BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) !=
+                    offsetof(struct sockaddr_alg, salg_name));
+       BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa));
+
+       if (addr_len < sizeof(*sa) + 1)
                return -EINVAL;

        /* If caller uses non-allowed flag, return error. */

@@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
                return -EINVAL;

        sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
-       sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
+       sa->salg_name[addr_len - sizeof(*sa) - 1] = 0;

This looks like an out-of-bounds write in the case `addr_len ==
sizeof(struct sockaddr_storage)`.

Sorry, I've been unusually unconcentrated today. Sorry about the
noise, ignore what I said.