On Mon, Oct 26, 2020 at 10:23:35PM +0100, 'Jann Horn' via syzkaller-bugs wrote:
On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers ebiggers@kernel.org wrote:
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...
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).
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...
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().
[...]
@@ -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)`.
I think you mean addr_len == sizeof(*sa)? That's what the 'if (addr_len < sizeof(*sa) + 1) return -EINVAL' above is for.
- Eric