Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-11-15 Thread Eric W. Biederman 
Kees Cook  writes:

> On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman  
> wrote:
>> Eric Dumazet  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 
 Cc: "David S. Miller" 
 Cc: netdev@vger.kernel.org
 Signed-off-by: Kees Cook 
 ---
  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(, 0, sizeof(addr));
  msg_sys->msg_name = 

>>>
>>> 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(>v6.sin6_addr) & 
>> IPV6_ADDR_LINKLOCAL) {
>> +   if (ipv6_addr_type(>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

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-11-15 Thread Alexander Potapenko 
On Wed, Nov 1, 2017 at 7:23 PM, Kees Cook  wrote:
> On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman  
> wrote:
>> Eric Dumazet  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 
 Cc: "David S. Miller" 
 Cc: netdev@vger.kernel.org
 Signed-off-by: Kees Cook 
 ---
  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(, 0, sizeof(addr));
  msg_sys->msg_name = 

>>>
>>> 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(>v6.sin6_addr) & 
>> IPV6_ADDR_LINKLOCAL) {
>> +   if (ipv6_addr_type(>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

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-11-14 Thread Kees Cook 
On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman  wrote:
> Eric Dumazet  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 
>>> Cc: "David S. Miller" 
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Kees Cook 
>>> ---
>>>  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(, 0, sizeof(addr));
>>>  msg_sys->msg_name = 
>>>
>>
>> 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(>v6.sin6_addr) & 
> IPV6_ADDR_LINKLOCAL) {
> +   if (ipv6_addr_type(>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

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-11-01 Thread Kees Cook 
On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman  wrote:
> Eric Dumazet  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 
>>> Cc: "David S. Miller" 
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Kees Cook 
>>> ---
>>>  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(, 0, sizeof(addr));
>>>  msg_sys->msg_name = 
>>>
>>
>> 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(>v6.sin6_addr) & 
> IPV6_ADDR_LINKLOCAL) {
> +   if (ipv6_addr_type(>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

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-11-01 Thread Eric W. Biederman 
Eric Dumazet  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 
>> Cc: "David S. Miller" 
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Kees Cook 
>> ---
>>  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(, 0, sizeof(addr));
>>  msg_sys->msg_name = 
>>  
>
> 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(>v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+   if (ipv6_addr_type(>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

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-11-01 Thread Willy Tarreau 
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(, 0, sizeof(addr));
>   msg_sys->msg_name = 

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

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-10-31 Thread Ben Hutchings 
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 
> Cc: "David S. Miller" 
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook 
> ---
>  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(, 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 = 
>  
>   if (MSG_CMSG_COMPAT & flags)
> -- 
> 2.7.4
> 
> 
-- 
Ben Hutchings
It is a miracle that curiosity survives formal education. - Albert
Einstein



signature.asc
Description: This is a digitally signed message part

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-10-31 Thread Eric Dumazet 
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 
> Cc: "David S. Miller" 
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook 
> ---
>  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(, 0, sizeof(addr));
>   msg_sys->msg_name = 
>  

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.

[PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

2017-10-31 Thread Kees Cook 
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 
Cc: "David S. Miller" 
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook 
---
 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(, 0, sizeof(addr));
msg_sys->msg_name = 
 
if (MSG_CMSG_COMPAT & flags)
-- 
2.7.4


-- 
Kees Cook
Pixel Security