Re: [PATCH] netrom: fix info leak via getsockname()

[Ben Hutchings <bhutchings@solarflare.com>]

On Sun, 2012-10-14 at 20:23 +0200, Clément Lecigne wrote:
> The following patch fixes a 3 bytes info leak via getsockname() on
> AF_NETROM socket.
> 
> Details:
> 
> typedef struct {
> 	char		ax25_call[7];	/* 6 call + SSID (shifted ascii!) */
> } ax25_address;
> 
> struct sockaddr_ax25 {
> 	__kernel_sa_family_t sax25_family;
> 	ax25_address	sax25_call;
> 	int		sax25_ndigis;
> 	/* Digipeater ax25_address sets follow */
> };
> 
> After compilation, gcc will add 3 padding bytes after sax25_call to
> align sax25_ndigis and nr_getname does not clear these padding bytes
> before returning to userland, poc attached.
> 
> Signed-off-by: Clement Lecigne <clemun@gmail.com>
> 
> --- a/net/netrom/af_netrom.c
> +++ b/net/netrom/af_netrom.c
> @@ -838,6 +838,8 @@ static int nr_getname(struct socket *sock, struct
> sockaddr *uaddr,
>         struct sock *sk = sock->sk;
>         struct nr_sock *nr = nr_sk(sk);
> 
> +    memset(sax, 0, sizeof(struct sockaddr_ax25));

This is not indented correctly.

>         lock_sock(sk);
>         if (peer != 0) {
>                 if (sk->sk_state != TCP_ESTABLISHED) {

On some architectures, structures have a minimum alignment of 4, so
there are 2 bytes of padding before sax25_call and 1 byte within it.
nr_getname() copies complete ax25_address structures, but nr_listen()
does not initialise the 1 byte of padding in user_addr:

		memset(&nr_sk(sk)->user_addr, 0, AX25_ADDR_LEN);

So I think you should address that too.  It appears that other
ax25_address structures are originally copied from userland and
therefore don't leak information, but I'm not sure.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.