* [PATCH 3/3] net: tipc: fix information leak to userland
@ 2010-10-31 17:10 ` Vasiliy Kulikov
0 siblings, 0 replies; 11+ messages in thread
From: Vasiliy Kulikov @ 2010-10-31 17:10 UTC (permalink / raw)
To: kernel-janitors
Cc: Jon Maloy, Allan Stephens, David S. Miller, tipc-discussion,
netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] net: tipc: fix information leak to userland
@ 2010-10-31 17:10 ` Vasiliy Kulikov
0 siblings, 0 replies; 11+ messages in thread
From: Vasiliy Kulikov @ 2010-10-31 17:10 UTC (permalink / raw)
To: kernel-janitors
Cc: Jon Maloy, Allan Stephens, David S. Miller, tipc-discussion,
netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
2010-10-31 17:10 ` Vasiliy Kulikov
(?)
@ 2010-11-09 17:26 ` David Miller
-1 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2010-11-09 17:26 UTC (permalink / raw)
To: segooon
Cc: kernel-janitors, jon.maloy, allan.stephens, tipc-discussion,
netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
@ 2010-11-09 17:26 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2010-11-09 17:26 UTC (permalink / raw)
To: segooon
Cc: jon.maloy, netdev, kernel-janitors, linux-kernel,
tipc-discussion, allan.stephens
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
@ 2010-11-09 17:26 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2010-11-09 17:26 UTC (permalink / raw)
To: segooon
Cc: kernel-janitors, jon.maloy, allan.stephens, tipc-discussion,
netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
2010-11-09 17:26 ` David Miller
@ 2010-11-09 20:33 ` Vasiliy Kulikov
-1 siblings, 0 replies; 11+ messages in thread
From: Vasiliy Kulikov @ 2010-11-09 20:33 UTC (permalink / raw)
To: David Miller
Cc: kernel-janitors, jon.maloy, allan.stephens, tipc-discussion,
netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
@ 2010-11-09 20:33 ` Vasiliy Kulikov
0 siblings, 0 replies; 11+ messages in thread
From: Vasiliy Kulikov @ 2010-11-09 20:33 UTC (permalink / raw)
To: David Miller
Cc: kernel-janitors, jon.maloy, allan.stephens, tipc-discussion,
netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
2010-11-09 20:33 ` Vasiliy Kulikov
@ 2010-11-10 11:58 ` walter harms
-1 siblings, 0 replies; 11+ messages in thread
From: walter harms @ 2010-11-10 11:58 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: David Miller, kernel-janitors, jon.maloy, allan.stephens,
tipc-discussion, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
@ 2010-11-10 11:58 ` walter harms
0 siblings, 0 replies; 11+ messages in thread
From: walter harms @ 2010-11-10 11:58 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: David Miller, kernel-janitors, jon.maloy, allan.stephens,
tipc-discussion, netdev, linux-kernel
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\x15.
re,
wh
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
2010-11-10 11:58 ` walter harms
@ 2010-11-10 15:54 ` Vasiliy Kulikov
-1 siblings, 0 replies; 11+ messages in thread
From: Vasiliy Kulikov @ 2010-11-10 15:54 UTC (permalink / raw)
To: walter harms
Cc: David Miller, kernel-janitors, jon.maloy, allan.stephens,
tipc-discussion, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] net: tipc: fix information leak to userland
@ 2010-11-10 15:54 ` Vasiliy Kulikov
0 siblings, 0 replies; 11+ messages in thread
From: Vasiliy Kulikov @ 2010-11-10 15:54 UTC (permalink / raw)
To: walter harms
Cc: David Miller, kernel-janitors, jon.maloy, allan.stephens,
tipc-discussion, netdev, linux-kernel
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\x15.
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-11-10 15:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-31 17:10 [PATCH 3/3] net: tipc: fix information leak to userland Vasiliy Kulikov
2010-10-31 17:10 ` Vasiliy Kulikov
2010-11-09 17:26 ` David Miller
2010-11-09 17:26 ` David Miller
2010-11-09 17:26 ` David Miller
2010-11-09 20:33 ` Vasiliy Kulikov
2010-11-09 20:33 ` Vasiliy Kulikov
2010-11-10 11:58 ` walter harms
2010-11-10 11:58 ` walter harms
2010-11-10 15:54 ` Vasiliy Kulikov
2010-11-10 15:54 ` Vasiliy Kulikov
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.