[PATCH] af_unix: Add sockaddr length checks before accessing sa_family in bind and connect handlers

[PATCH] af_unix: Add sockaddr length checks before accessing sa_family in bind and connect handlers

[Mateusz Jurczyk]

Verify that the caller-provided sockaddr structure is large enough to
contain the sa_family field, before accessing it in bind() and connect()
handlers of the AF_UNIX socket. Since neither syscall enforces a minimum
size of the corresponding memory region, very short sockaddrs (zero or
one byte long) result in operating on uninitialized memory while
referencing .sa_family.

Signed-off-by: Mateusz Jurczyk <mjurczyk@google.com>
---
 net/unix/af_unix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 6a7fe7660551..1a0c961f4ffe 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -999,7 +999,8 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	struct path path = { };
 
 	err = -EINVAL;
-	if (sunaddr->sun_family != AF_UNIX)
+	if (addr_len < offsetofend(struct sockaddr_un, sun_family) ||
+	    sunaddr->sun_family != AF_UNIX)
 		goto out;
 
 	if (addr_len == sizeof(short)) {
@@ -1110,6 +1111,10 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr,
 	unsigned int hash;
 	int err;
 
+	err = -EINVAL;
+	if (alen < offsetofend(struct sockaddr, sa_family))
+		goto out;
+
 	if (addr->sa_family != AF_UNSPEC) {
 		err = unix_mkname(sunaddr, alen, &hash);
 		if (err < 0)
-- 
2.13.1.508.gb3defc5cc-goog

Re: [PATCH] af_unix: Add sockaddr length checks before accessing sa_family in bind and connect handlers

[David Miller]

From: Mateusz Jurczyk <mjurczyk@google.com>
Date: Thu,  8 Jun 2017 11:13:36 +0200

> Verify that the caller-provided sockaddr structure is large enough to
> contain the sa_family field, before accessing it in bind() and connect()
> handlers of the AF_UNIX socket. Since neither syscall enforces a minimum
> size of the corresponding memory region, very short sockaddrs (zero or
> one byte long) result in operating on uninitialized memory while
> referencing .sa_family.
> 
> Signed-off-by: Mateusz Jurczyk <mjurczyk@google.com>

The sockaddr comes from a structure on the caller's kernel stack, even
if the user gives a smaller length, it is legal to access that memory.

Re: [PATCH] af_unix: Add sockaddr length checks before accessing sa_family in bind and connect handlers

[Mateusz Jurczyk]

On Thu, Jun 8, 2017 at 10:04 PM, David Miller <davem@davemloft.net> wrote:
> From: Mateusz Jurczyk <mjurczyk@google.com>
> Date: Thu,  8 Jun 2017 11:13:36 +0200
>
>> Verify that the caller-provided sockaddr structure is large enough to
>> contain the sa_family field, before accessing it in bind() and connect()
>> handlers of the AF_UNIX socket. Since neither syscall enforces a minimum
>> size of the corresponding memory region, very short sockaddrs (zero or
>> one byte long) result in operating on uninitialized memory while
>> referencing .sa_family.
>>
>> Signed-off-by: Mateusz Jurczyk <mjurczyk@google.com>
>
> The sockaddr comes from a structure on the caller's kernel stack, even
> if the user gives a smaller length, it is legal to access that memory.

It is legal to access it, but since it's uninitialized kernel stack
memory, the results of comparisons against AF_UNIX or AF_UNSPEC are
indeterminate. In practice a user-mode program could likely use timing
measurement to infer the evaluation of these comparisons, and hence
determine if a garbage 16-bit variable on the kernel stack is equal to
0x0000 or 0x0001, or a garbage byte is equal to 0x00 (if the first
byte is provided).

This is of course not very bad. However, my project for finding use of
uninitialized memory flagged it, and I thought it was worth fixing, at
least to avoid having this construct detected in the future (e.g. by
KMSAN).

There are a few more instances of this behavior in other socket types,
which I was going to report with separate patches. If you decide this
kind of issues indeed deserves a fix, please let me know if further
separate patches are the right approach.

Thanks,
Mateusz

Re: [PATCH] af_unix: Add sockaddr length checks before accessing sa_family in bind and connect handlers

[David Miller]

From: Mateusz Jurczyk <mjurczyk@google.com>
Date: Fri, 9 Jun 2017 13:15:40 +0200

> On Thu, Jun 8, 2017 at 10:04 PM, David Miller <davem@davemloft.net> wrote:
>> From: Mateusz Jurczyk <mjurczyk@google.com>
>> Date: Thu,  8 Jun 2017 11:13:36 +0200
>>
>>> Verify that the caller-provided sockaddr structure is large enough to
>>> contain the sa_family field, before accessing it in bind() and connect()
>>> handlers of the AF_UNIX socket. Since neither syscall enforces a minimum
>>> size of the corresponding memory region, very short sockaddrs (zero or
>>> one byte long) result in operating on uninitialized memory while
>>> referencing .sa_family.
>>>
>>> Signed-off-by: Mateusz Jurczyk <mjurczyk@google.com>
>>
>> The sockaddr comes from a structure on the caller's kernel stack, even
>> if the user gives a smaller length, it is legal to access that memory.
> 
> It is legal to access it, but since it's uninitialized kernel stack
> memory, the results of comparisons against AF_UNIX or AF_UNSPEC are
> indeterminate. In practice a user-mode program could likely use timing
> measurement to infer the evaluation of these comparisons, and hence
> determine if a garbage 16-bit variable on the kernel stack is equal to
> 0x0000 or 0x0001, or a garbage byte is equal to 0x00 (if the first
> byte is provided).
> 
> This is of course not very bad. However, my project for finding use of
> uninitialized memory flagged it, and I thought it was worth fixing, at
> least to avoid having this construct detected in the future (e.g. by
> KMSAN).
> 
> There are a few more instances of this behavior in other socket types,
> which I was going to report with separate patches. If you decide this
> kind of issues indeed deserves a fix, please let me know if further
> separate patches are the right approach.

Oh that's right, we don't zero initialize the on-stack object before
copying from userspace.

I'm going to apply this patch and please submit further changes fixing
bugs like this one.

Thanks.