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

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 <redacted>
---
 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

From: Vasiliy Kulikov <redacted>
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 <redacted>

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

On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote:

From: Vasiliy Kulikov <redacted>
Date: Sun, 31 Oct 2010 20:10:32 +0300

quoted

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 <redacted>

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

Am 09.11.2010 21:33, schrieb Vasiliy Kulikov:

On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote:

quoted

From: Vasiliy Kulikov <redacted>
Date: Sun, 31 Oct 2010 20:10:32 +0300

quoted

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 <redacted>

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

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