[PATCH 3/3] net: tipc: fix information leak to userland

[PATCH 3/3] net: tipc: fix information leak to userland

[Vasiliy Kulikov]

Structure sockaddr_tipc is copied to userland with padding bytes after
"id" field in union field "name" unitialized.  It leads to leaking of
contents of kernel stack memory.  We have to initialize them to zero.

Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
 net/tipc/socket.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 33217fc..e9f0d50 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -396,6 +396,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
 	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
 	struct tipc_sock *tsock = tipc_sk(sock->sk);
 
+	memset(addr, 0, sizeof(*addr));
 	if (peer) {
 		if ((sock->state != SS_CONNECTED) &&
 			((peer != 2) || (sock->state != SS_DISCONNECTING)))
-- 
1.7.0.4

Re: [PATCH 3/3] net: tipc: fix information leak to userland

[David Miller]

From: Vasiliy Kulikov <segooon@gmail.com>
Date: Sun, 31 Oct 2010 20:10:32 +0300

> Structure sockaddr_tipc is copied to userland with padding bytes after
> "id" field in union field "name" unitialized.  It leads to leaking of
> contents of kernel stack memory.  We have to initialize them to zero.
> 
> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>

Applied.

Patches #1 and #2 were given feedback which I need you to integrate
and submit new patches based upon, thanks.

------------------------------------------------------------------------------
The Next 800 Companies to Lead America's Growth: New Video Whitepaper
David G. Thomson, author of the best-selling book "Blueprint to a 
Billion" shares his insights and actions to help propel your 
business during the next growth cycle. Listen Now!
http://p.sf.net/sfu/SAP-dev2dev

Re: [PATCH 3/3] net: tipc: fix information leak to userland

[Vasiliy Kulikov]

On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote:
> From: Vasiliy Kulikov <segooon@gmail.com>
> Date: Sun, 31 Oct 2010 20:10:32 +0300
> 
> > Structure sockaddr_tipc is copied to userland with padding bytes after
> > "id" field in union field "name" unitialized.  It leads to leaking of
> > contents of kernel stack memory.  We have to initialize them to zero.
> > 
> > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
> 
> Applied.
> 
> Patches #1 and #2 were given feedback which I need you to integrate
> and submit new patches based upon, thanks.

About #2:

I still think that this:

    if (dev)
        strncpy(uaddr->sa_data, dev->name, 14);
    else
        memset(uaddr->sa_data, 0, 14);

is better than this:

    memset(uaddr->sa_data, 0, 14);
    dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex);
    if (dev)
        strlcpy(uaddr->sa_data, dev->name, 15);

Doesn't it?  Explicitly filling with zero on the same "if" level is
slightly easier to read and understand.

-- 
Vasiliy

Re: [PATCH 3/3] net: tipc: fix information leak to userland

[walter harms]

Am 09.11.2010 21:33, schrieb Vasiliy Kulikov:
> On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote:
>> From: Vasiliy Kulikov <segooon@gmail.com>
>> Date: Sun, 31 Oct 2010 20:10:32 +0300
>>
>>> Structure sockaddr_tipc is copied to userland with padding bytes after
>>> "id" field in union field "name" unitialized.  It leads to leaking of
>>> contents of kernel stack memory.  We have to initialize them to zero.
>>>
>>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
>>
>> Applied.
>>
>> Patches #1 and #2 were given feedback which I need you to integrate
>> and submit new patches based upon, thanks.
> 
> About #2:
> 
> I still think that this:
> 
>     if (dev)
>         strncpy(uaddr->sa_data, dev->name, 14);
>     else
>         memset(uaddr->sa_data, 0, 14);
> 
> is better than this:
> 
>     memset(uaddr->sa_data, 0, 14);
>     dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex);
>     if (dev)
>         strlcpy(uaddr->sa_data, dev->name, 15);
> 
> Doesn't it?  Explicitly filling with zero on the same "if" level is
> slightly easier to read and understand.
> 

no problem with me, since i came up with the idea a simple explanation:
IMHO the pattern clear/if/copy is more robust

NTL the core problem was that sizeof sa_data is 14 while dev->name is IFNAMESZ=15.


re,
 wh

Re: [PATCH 3/3] net: tipc: fix information leak to userland

[Vasiliy Kulikov]

On Wed, Nov 10, 2010 at 12:58 +0100, walter harms wrote:
> NTL the core problem was that sizeof sa_data is 14 while dev->name is IFNAMESZ=15.

With this code it is NOT a bug because the output buffer is much bigger
than 14 (128 bytes).  I think it was just designed to overflow 14 bytes,
assign sa_data[14] = 0 and ignore it (lack of snprintf() those days?).

Anywhere else sa_data[14] = ... is a bug.

-- 
Vasiliy