* [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
@ 2017-10-31 16:14 Kees Cook
2017-10-31 17:31 ` Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Kees Cook @ 2017-10-31 16:14 UTC (permalink / raw)
To: David S. Miller
Cc: Alexander Potapenko, Kostya Serebryany, Andrey Konovalov,
Eric Dumazet, netdev, linux-kernel, security
Some protocols do not correctly wipe the contents of the on-stack
struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
kernel stack contents to userspace. This wipes it unconditionally before
per-protocol handlers run.
Note that leaks like this are mitigated by building with
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
Reported-by: Alexander Potapenko <glider@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
net/socket.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/socket.c b/net/socket.c
index c729625eb5d3..34183f4fbdf8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
struct sockaddr __user *uaddr;
int __user *uaddr_len = COMPAT_NAMELEN(msg);
+ memset(&addr, 0, sizeof(addr));
msg_sys->msg_name = &addr;
if (MSG_CMSG_COMPAT & flags)
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
@ 2017-10-31 17:31 ` Eric Dumazet
2017-11-01 12:48 ` Eric W. Biederman
2017-10-31 17:35 ` Ben Hutchings
2017-11-01 6:49 ` Willy Tarreau
2 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2017-10-31 17:31 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Alexander Potapenko, Kostya Serebryany,
Andrey Konovalov, Eric Dumazet, netdev, linux-kernel, security
On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
> Some protocols do not correctly wipe the contents of the on-stack
> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
> kernel stack contents to userspace. This wipes it unconditionally before
> per-protocol handlers run.
>
> Note that leaks like this are mitigated by building with
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>
> Reported-by: Alexander Potapenko <glider@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> net/socket.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/socket.c b/net/socket.c
> index c729625eb5d3..34183f4fbdf8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
> struct sockaddr __user *uaddr;
> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>
> + memset(&addr, 0, sizeof(addr));
> msg_sys->msg_name = &addr;
>
This kind of patch comes every year.
Standard answer is : We fix the buggy protocol, we do not make
everything slower just because we are lazy.
struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
Also memset() is using long word stores, so next 4-byte or 2-byte stores
on same location hit a performance problem on x86.
By adding all these defensive programming, we would give strong
incentives to bypass the kernel for networking. That would be bad.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
2017-10-31 17:31 ` Eric Dumazet
@ 2017-10-31 17:35 ` Ben Hutchings
2017-11-01 6:49 ` Willy Tarreau
2 siblings, 0 replies; 13+ messages in thread
From: Ben Hutchings @ 2017-10-31 17:35 UTC (permalink / raw)
To: Kees Cook, David S. Miller
Cc: Alexander Potapenko, Kostya Serebryany, Andrey Konovalov,
Eric Dumazet, netdev, linux-kernel, security
[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]
On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
> Some protocols do not correctly wipe the contents of the on-stack
> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
> kernel stack contents to userspace. This wipes it unconditionally before
> per-protocol handlers run.
>
> Note that leaks like this are mitigated by building with
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>
> Reported-by: Alexander Potapenko <glider@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> net/socket.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/socket.c b/net/socket.c
> index c729625eb5d3..34183f4fbdf8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
> struct sockaddr __user *uaddr;
> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>
> + memset(&addr, 0, sizeof(addr));
That's a fairly large structure (128 bytes), most of which won't
normally be used. We already initialise msg_namelen to 0 before
calling the per-protocol handler, which means by default nothing leaks.
Only cases where msg_namelen is set but msg_name[] is not initialised
up to that length are a problem. I would have thought they were not
too hard to find and fix.
Ben.
> msg_sys->msg_name = &addr;
>
> if (MSG_CMSG_COMPAT & flags)
> --
> 2.7.4
>
>
--
Ben Hutchings
It is a miracle that curiosity survives formal education. - Albert
Einstein
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
2017-10-31 17:31 ` Eric Dumazet
2017-10-31 17:35 ` Ben Hutchings
@ 2017-11-01 6:49 ` Willy Tarreau
2 siblings, 0 replies; 13+ messages in thread
From: Willy Tarreau @ 2017-11-01 6:49 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Alexander Potapenko, Kostya Serebryany,
Andrey Konovalov, Eric Dumazet, netdev, linux-kernel, security
On Tue, Oct 31, 2017 at 09:14:45AM -0700, Kees Cook wrote:
> diff --git a/net/socket.c b/net/socket.c
> index c729625eb5d3..34183f4fbdf8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
> struct sockaddr __user *uaddr;
> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>
> + memset(&addr, 0, sizeof(addr));
> msg_sys->msg_name = &addr;
Isn't this going to cause a performance hit in the fast path ? Just
checking, I have not read the whole code with the patch in its context.
Willy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-10-31 17:31 ` Eric Dumazet
@ 2017-11-01 12:48 ` Eric W. Biederman
2017-11-01 18:23 ` Kees Cook
2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
0 siblings, 2 replies; 13+ messages in thread
From: Eric W. Biederman @ 2017-11-01 12:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: Kees Cook, David S. Miller, Alexander Potapenko,
Kostya Serebryany, Andrey Konovalov, Eric Dumazet, netdev,
linux-kernel, security
Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>> Some protocols do not correctly wipe the contents of the on-stack
>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>> kernel stack contents to userspace. This wipes it unconditionally before
>> per-protocol handlers run.
>>
>> Note that leaks like this are mitigated by building with
>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>
>> Reported-by: Alexander Potapenko <glider@google.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> net/socket.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index c729625eb5d3..34183f4fbdf8 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>> struct sockaddr __user *uaddr;
>> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>
>> + memset(&addr, 0, sizeof(addr));
>> msg_sys->msg_name = &addr;
>>
>
> This kind of patch comes every year.
>
> Standard answer is : We fix the buggy protocol, we do not make
> everything slower just because we are lazy.
>
> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>
> Also memset() is using long word stores, so next 4-byte or 2-byte stores
> on same location hit a performance problem on x86.
>
> By adding all these defensive programming, we would give strong
> incentives to bypass the kernel for networking. That would be bad.
In this case it looks like the root cause is something in sctp
not filling in the ipv6 sin6_scope_id.
Which is not only a leak but a correctness bug.
I ran the reproducer test program and while none of the leak checkers
are telling me anything I have gotten as far as seeing that the returned
length is correct and sometimes nonsense.
Hmm.
At a quick look it looks like all that is necessary is to do this:
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 51c488769590..6301913d0516 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
addr->v6.sin6_flowinfo = 0;
addr->v6.sin6_port = sh->source;
addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
- if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+ if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
- }
+ else
+ addr->v6.sin6_scope_id = 0;
}
*addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
Eric
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-11-01 12:48 ` Eric W. Biederman
@ 2017-11-01 18:23 ` Kees Cook
2017-11-15 8:22 ` Alexander Potapenko
2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
1 sibling, 1 reply; 13+ messages in thread
From: Kees Cook @ 2017-11-01 18:23 UTC (permalink / raw)
To: Eric W. Biederman, Alexander Potapenko
Cc: Eric Dumazet, David S. Miller, Kostya Serebryany,
Andrey Konovalov, Eric Dumazet, Network Development, LKML,
security
On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>> Some protocols do not correctly wipe the contents of the on-stack
>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>> kernel stack contents to userspace. This wipes it unconditionally before
>>> per-protocol handlers run.
>>>
>>> Note that leaks like this are mitigated by building with
>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>
>>> Reported-by: Alexander Potapenko <glider@google.com>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> net/socket.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index c729625eb5d3..34183f4fbdf8 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>> struct sockaddr __user *uaddr;
>>> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>
>>> + memset(&addr, 0, sizeof(addr));
>>> msg_sys->msg_name = &addr;
>>>
>>
>> This kind of patch comes every year.
>>
>> Standard answer is : We fix the buggy protocol, we do not make
>> everything slower just because we are lazy.
>>
>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>
>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>> on same location hit a performance problem on x86.
>>
>> By adding all these defensive programming, we would give strong
>> incentives to bypass the kernel for networking. That would be bad.
>
> In this case it looks like the root cause is something in sctp
> not filling in the ipv6 sin6_scope_id.
>
> Which is not only a leak but a correctness bug.
>
> I ran the reproducer test program and while none of the leak checkers
> are telling me anything I have gotten as far as seeing that the returned
> length is correct and sometimes nonsense.
>
> Hmm.
>
> At a quick look it looks like all that is necessary is to do this:
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 51c488769590..6301913d0516 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
> addr->v6.sin6_flowinfo = 0;
> addr->v6.sin6_port = sh->source;
> addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
> - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
> + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
> addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
> - }
> + else
> + addr->v6.sin6_scope_id = 0;
> }
>
> *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>
> Eric
>
Thanks for digging into this Eric! Alexander, can you confirm this
fixes it for you when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not
enabled?
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-11-01 12:48 ` Eric W. Biederman
2017-11-01 18:23 ` Kees Cook
@ 2017-11-15 2:13 ` Kees Cook
2017-11-15 18:37 ` Eric W. Biederman
1 sibling, 1 reply; 13+ messages in thread
From: Kees Cook @ 2017-11-15 2:13 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Eric Dumazet, David S. Miller, Alexander Potapenko,
Kostya Serebryany, Andrey Konovalov, Eric Dumazet,
Network Development, LKML, security
On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>> Some protocols do not correctly wipe the contents of the on-stack
>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>> kernel stack contents to userspace. This wipes it unconditionally before
>>> per-protocol handlers run.
>>>
>>> Note that leaks like this are mitigated by building with
>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>
>>> Reported-by: Alexander Potapenko <glider@google.com>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> net/socket.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index c729625eb5d3..34183f4fbdf8 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>> struct sockaddr __user *uaddr;
>>> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>
>>> + memset(&addr, 0, sizeof(addr));
>>> msg_sys->msg_name = &addr;
>>>
>>
>> This kind of patch comes every year.
>>
>> Standard answer is : We fix the buggy protocol, we do not make
>> everything slower just because we are lazy.
>>
>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>
>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>> on same location hit a performance problem on x86.
>>
>> By adding all these defensive programming, we would give strong
>> incentives to bypass the kernel for networking. That would be bad.
>
> In this case it looks like the root cause is something in sctp
> not filling in the ipv6 sin6_scope_id.
>
> Which is not only a leak but a correctness bug.
>
> I ran the reproducer test program and while none of the leak checkers
> are telling me anything I have gotten as far as seeing that the returned
> length is correct and sometimes nonsense.
>
> Hmm.
>
> At a quick look it looks like all that is necessary is to do this:
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 51c488769590..6301913d0516 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
> addr->v6.sin6_flowinfo = 0;
> addr->v6.sin6_port = sh->source;
> addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
> - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
> + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
> addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
> - }
> + else
> + addr->v6.sin6_scope_id = 0;
> }
>
> *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>
It looks like this never landed anywhere? Eric, are you able to resend
this as a full patch?
Thanks!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-11-01 18:23 ` Kees Cook
@ 2017-11-15 8:22 ` Alexander Potapenko
2017-11-16 4:17 ` Eric W. Biederman
0 siblings, 1 reply; 13+ messages in thread
From: Alexander Potapenko @ 2017-11-15 8:22 UTC (permalink / raw)
To: Kees Cook
Cc: Eric W. Biederman, Eric Dumazet, David S. Miller,
Kostya Serebryany, Andrey Konovalov, Eric Dumazet,
Network Development, LKML, security
On Wed, Nov 1, 2017 at 7:23 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>
>>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>>> Some protocols do not correctly wipe the contents of the on-stack
>>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>>> kernel stack contents to userspace. This wipes it unconditionally before
>>>> per-protocol handlers run.
>>>>
>>>> Note that leaks like this are mitigated by building with
>>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>>
>>>> Reported-by: Alexander Potapenko <glider@google.com>
>>>> Cc: "David S. Miller" <davem@davemloft.net>
>>>> Cc: netdev@vger.kernel.org
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>> net/socket.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index c729625eb5d3..34183f4fbdf8 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>>> struct sockaddr __user *uaddr;
>>>> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>>
>>>> + memset(&addr, 0, sizeof(addr));
>>>> msg_sys->msg_name = &addr;
>>>>
>>>
>>> This kind of patch comes every year.
>>>
>>> Standard answer is : We fix the buggy protocol, we do not make
>>> everything slower just because we are lazy.
>>>
>>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>>
>>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>>> on same location hit a performance problem on x86.
>>>
>>> By adding all these defensive programming, we would give strong
>>> incentives to bypass the kernel for networking. That would be bad.
>>
>> In this case it looks like the root cause is something in sctp
>> not filling in the ipv6 sin6_scope_id.
>>
>> Which is not only a leak but a correctness bug.
>>
>> I ran the reproducer test program and while none of the leak checkers
>> are telling me anything I have gotten as far as seeing that the returned
>> length is correct and sometimes nonsense.
>>
>> Hmm.
>>
>> At a quick look it looks like all that is necessary is to do this:
>>
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index 51c488769590..6301913d0516 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
>> addr->v6.sin6_flowinfo = 0;
>> addr->v6.sin6_port = sh->source;
>> addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
>> - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
>> + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
>> addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
>> - }
>> + else
>> + addr->v6.sin6_scope_id = 0;
>> }
>>
>> *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>>
>> Eric
>>
>
> Thanks for digging into this Eric! Alexander, can you confirm this
> fixes it for you when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not
> enabled?
Sorry, I accidentally missed this patch.
Yes, I confirm it fixes the problem with
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL disabled.
> -Kees
>
> --
> Kees Cook
> Pixel Security
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
@ 2017-11-15 18:37 ` Eric W. Biederman
0 siblings, 0 replies; 13+ messages in thread
From: Eric W. Biederman @ 2017-11-15 18:37 UTC (permalink / raw)
To: Kees Cook
Cc: Eric Dumazet, David S. Miller, Alexander Potapenko,
Kostya Serebryany, Andrey Konovalov, Eric Dumazet,
Network Development, LKML, security
Kees Cook <keescook@chromium.org> writes:
> On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>
>>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>>> Some protocols do not correctly wipe the contents of the on-stack
>>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>>> kernel stack contents to userspace. This wipes it unconditionally before
>>>> per-protocol handlers run.
>>>>
>>>> Note that leaks like this are mitigated by building with
>>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>>
>>>> Reported-by: Alexander Potapenko <glider@google.com>
>>>> Cc: "David S. Miller" <davem@davemloft.net>
>>>> Cc: netdev@vger.kernel.org
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>> net/socket.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index c729625eb5d3..34183f4fbdf8 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>>> struct sockaddr __user *uaddr;
>>>> int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>>
>>>> + memset(&addr, 0, sizeof(addr));
>>>> msg_sys->msg_name = &addr;
>>>>
>>>
>>> This kind of patch comes every year.
>>>
>>> Standard answer is : We fix the buggy protocol, we do not make
>>> everything slower just because we are lazy.
>>>
>>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>>
>>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>>> on same location hit a performance problem on x86.
>>>
>>> By adding all these defensive programming, we would give strong
>>> incentives to bypass the kernel for networking. That would be bad.
>>
>> In this case it looks like the root cause is something in sctp
>> not filling in the ipv6 sin6_scope_id.
>>
>> Which is not only a leak but a correctness bug.
>>
>> I ran the reproducer test program and while none of the leak checkers
>> are telling me anything I have gotten as far as seeing that the returned
>> length is correct and sometimes nonsense.
>>
>> Hmm.
>>
>> At a quick look it looks like all that is necessary is to do this:
>>
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index 51c488769590..6301913d0516 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
>> addr->v6.sin6_flowinfo = 0;
>> addr->v6.sin6_port = sh->source;
>> addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
>> - if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
>> + if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
>> addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
>> - }
>> + else
>> + addr->v6.sin6_scope_id = 0;
>> }
>>
>> *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>>
>
> It looks like this never landed anywhere? Eric, are you able to resend
> this as a full patch?
I will take a look. I have not conducted a thorough review to make
certain that is everything. I was hoping someone else would pick that
change up and run with it. However the change seems obviously correct
as is, so I don't have any problem sending just this bit.
Eric
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname
2017-11-15 8:22 ` Alexander Potapenko
@ 2017-11-16 4:17 ` Eric W. Biederman
0 siblings, 0 replies; 13+ messages in thread
From: Eric W. Biederman @ 2017-11-16 4:17 UTC (permalink / raw)
To: David S. Miller
Cc: Kees Cook, Eric Dumazet, Kostya Serebryany, Andrey Konovalov,
Eric Dumazet, Network Development, LKML, security,
Alexander Potapenko, linux-sctp, Neil Horman, Vlad Yasevich
Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
discovered that in some configurations sctp would leak 4 bytes of
kernel stack.
Working with his reproducer I discovered that those 4 bytes that
are leaked is the scope id of an ipv6 address returned by recvmsg.
With a little code inspection and a shrewd guess I discovered that
sctp_inet6_skb_msgname only initializes the scope_id field for link
local ipv6 addresses to the interface index the link local address
pertains to instead of initializing the scope_id field for all ipv6
addresses.
That is almost reasonable as scope_id's are meaniningful only for link
local addresses. Set the scope_id in all other cases to 0 which is
not a valid interface index to make it clear there is nothing useful
in the scope_id field.
There should be no danger of breaking userspace as the stack leak
guaranteed that previously meaningless random data was being returned.
Cc: stable@vger.kernel.org
Fixes: 372f525b495c ("SCTP: Resync with LKSCTP tree.")
History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Reported-by: Alexander Potapenko <glider@google.com>
Tested-by: Alexander Potapenko <glider@google.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
net/sctp/ipv6.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index a6dfa86c0201..3b18085e3b10 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
addr->v6.sin6_flowinfo = 0;
addr->v6.sin6_port = sh->source;
addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
- if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+ if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
- }
+ else
+ addr->v6.sin6_scope_id = 0;
}
*addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
--
2.14.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname
@ 2017-11-16 4:17 ` Eric W. Biederman
0 siblings, 0 replies; 13+ messages in thread
From: Eric W. Biederman @ 2017-11-16 4:17 UTC (permalink / raw)
To: David S. Miller
Cc: Kees Cook, Eric Dumazet, Kostya Serebryany, Andrey Konovalov,
Eric Dumazet, Network Development, LKML, security,
Alexander Potapenko, linux-sctp, Neil Horman, Vlad Yasevich
Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
discovered that in some configurations sctp would leak 4 bytes of
kernel stack.
Working with his reproducer I discovered that those 4 bytes that
are leaked is the scope id of an ipv6 address returned by recvmsg.
With a little code inspection and a shrewd guess I discovered that
sctp_inet6_skb_msgname only initializes the scope_id field for link
local ipv6 addresses to the interface index the link local address
pertains to instead of initializing the scope_id field for all ipv6
addresses.
That is almost reasonable as scope_id's are meaniningful only for link
local addresses. Set the scope_id in all other cases to 0 which is
not a valid interface index to make it clear there is nothing useful
in the scope_id field.
There should be no danger of breaking userspace as the stack leak
guaranteed that previously meaningless random data was being returned.
Cc: stable@vger.kernel.org
Fixes: 372f525b495c ("SCTP: Resync with LKSCTP tree.")
History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Reported-by: Alexander Potapenko <glider@google.com>
Tested-by: Alexander Potapenko <glider@google.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
net/sctp/ipv6.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index a6dfa86c0201..3b18085e3b10 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
addr->v6.sin6_flowinfo = 0;
addr->v6.sin6_port = sh->source;
addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
- if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+ if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
- }
+ else
+ addr->v6.sin6_scope_id = 0;
}
*addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
--
2.14.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname
2017-11-16 4:17 ` Eric W. Biederman
@ 2017-11-16 14:00 ` David Miller
-1 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2017-11-16 14:00 UTC (permalink / raw)
To: ebiederm
Cc: keescook, eric.dumazet, kcc, andreyknvl, edumazet, netdev,
linux-kernel, security, glider, linux-sctp, nhorman, vyasevich
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 15 Nov 2017 22:17:48 -0600
>
> Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
> discovered that in some configurations sctp would leak 4 bytes of
> kernel stack.
>
> Working with his reproducer I discovered that those 4 bytes that
> are leaked is the scope id of an ipv6 address returned by recvmsg.
>
> With a little code inspection and a shrewd guess I discovered that
> sctp_inet6_skb_msgname only initializes the scope_id field for link
> local ipv6 addresses to the interface index the link local address
> pertains to instead of initializing the scope_id field for all ipv6
> addresses.
>
> That is almost reasonable as scope_id's are meaniningful only for link
> local addresses. Set the scope_id in all other cases to 0 which is
> not a valid interface index to make it clear there is nothing useful
> in the scope_id field.
>
> There should be no danger of breaking userspace as the stack leak
> guaranteed that previously meaningless random data was being returned.
>
> Fixes: 372f525b495c ("SCTP: Resync with LKSCTP tree.")
> History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> Reported-by: Alexander Potapenko <glider@google.com>
> Tested-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname
@ 2017-11-16 14:00 ` David Miller
0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2017-11-16 14:00 UTC (permalink / raw)
To: ebiederm
Cc: keescook, eric.dumazet, kcc, andreyknvl, edumazet, netdev,
linux-kernel, security, glider, linux-sctp, nhorman, vyasevich
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 15 Nov 2017 22:17:48 -0600
>
> Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
> discovered that in some configurations sctp would leak 4 bytes of
> kernel stack.
>
> Working with his reproducer I discovered that those 4 bytes that
> are leaked is the scope id of an ipv6 address returned by recvmsg.
>
> With a little code inspection and a shrewd guess I discovered that
> sctp_inet6_skb_msgname only initializes the scope_id field for link
> local ipv6 addresses to the interface index the link local address
> pertains to instead of initializing the scope_id field for all ipv6
> addresses.
>
> That is almost reasonable as scope_id's are meaniningful only for link
> local addresses. Set the scope_id in all other cases to 0 which is
> not a valid interface index to make it clear there is nothing useful
> in the scope_id field.
>
> There should be no danger of breaking userspace as the stack leak
> guaranteed that previously meaningless random data was being returned.
>
> Fixes: 372f525b495c ("SCTP: Resync with LKSCTP tree.")
> History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> Reported-by: Alexander Potapenko <glider@google.com>
> Tested-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-11-16 14:01 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-31 16:14 [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
2017-10-31 17:31 ` Eric Dumazet
2017-11-01 12:48 ` Eric W. Biederman
2017-11-01 18:23 ` Kees Cook
2017-11-15 8:22 ` Alexander Potapenko
2017-11-16 4:17 ` [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname Eric W. Biederman
2017-11-16 4:17 ` Eric W. Biederman
2017-11-16 14:00 ` David Miller
2017-11-16 14:00 ` David Miller
2017-11-15 2:13 ` [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage Kees Cook
2017-11-15 18:37 ` Eric W. Biederman
2017-10-31 17:35 ` Ben Hutchings
2017-11-01 6:49 ` Willy Tarreau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.