Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Sun, 7 Nov 2010 15:06:36 +0300 | From | Vasiliy Kulikov <> | Subject | Re: [PATCH 2/3] net: packet: fix information leak to userland |
| |
On Sun, Nov 07, 2010 at 12:37 +0100, walter harms wrote: > Am 06.11.2010 15:39, schrieb Vasiliy Kulikov: > > On Mon, Nov 01, 2010 at 10:14 +0100, walter harms wrote: > >> Vasiliy Kulikov schrieb: > >>> @@ -1719,7 +1719,7 @@ static int packet_getname_spkt(struct socket *sock, struct sockaddr *uaddr, > >>> rcu_read_lock(); > >>> dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex); > >>> if (dev) > >>> - strlcpy(uaddr->sa_data, dev->name, 15); > >>> + strncpy(uaddr->sa_data, dev->name, 14); > >>> else > >>> memset(uaddr->sa_data, 0, 14); > >> > >> if i understand the code correcly the max size for dev->name is IFNAMSIZ. > > > > For dev->name - IFNAMSIZ, for uaddr->sa_data - 14. > > > > > did not notice, since uaddr->sa_data should take dev->name this does no look very > clever. How is the size of sa_data defined ?
Magic size...
~/linux/include/linux/socket.h:
struct sockaddr { sa_family_t sa_family; /* address family, AF_xxx */ char sa_data[14]; /* 14 bytes of protocol address */ };
> Would it hurt when some uses IFNAMSIZ here ?
If copy _to_ sa_data string of maximum IFNAMSIZ bytes - yes.
In packet_getname_spkt() the output buffer is 128 bytes, so it doesn't really overflows anything. I don't think that *_getname() implementations don't know this.
> Perhaps someone who know more about the network stack can figure out what is actualy done > with uaddr->sa_data.
Yeah, also I wonder whether it is always NULL-terminated string or not.
> looks like a can of worms. > > > In packet_bind_spkt() they will copy a char[15], obviously it is a real problem.
No, packet_bind_spkt() copies 14-byte string into array of 15 bytes. The vice versa would be a bug.
> re, > wh > > > >> You can simply that part: > >> > >> memset(uaddr->sa_data, 0, IFNAMSIZ); > >> dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex); > >> if (dev) > >> strlcpy(uaddr->sa_data, dev->name, IFNAMSIZ); > > > > This will overflow uaddr->sa_data. Also I don't see any difficulty to > > fill the array only once. > > > >> you should send that as separate patch. > >> re, > >> wh > >> > >> > >>> rcu_read_unlock(); > >>> @@ -1742,6 +1742,7 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr, > >>> sll->sll_family = AF_PACKET; > >>> sll->sll_ifindex = po->ifindex; > >>> sll->sll_protocol = po->num; > >>> + sll->sll_pkttype = 0; > >>> rcu_read_lock(); > >>> dev = dev_get_by_index_rcu(sock_net(sk), po->ifindex); > >>> if (dev) { > > > > Thanks, > >
-- Vasiliy
| ![\](/images/icornerr.gif) |