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

[Bernard Pidoux <bernard.pidoux@xxxxxxx>]

Hi,

I applied the AX25 patch and did not observe any problem with AX25 nor NetROM
or ROSE traffic.

I observed that a similar structure initialization existed in rose_getname() but
not in netrom nr_getname().

A similar patch for af_netrom could be :


--- a/net/netrom/af_netrom.c	2010-08-27 01:47:12.000000000 +0200
+++ b/net/netrom/af_netrom.c	2010-11-01 14:38:34.580000003 +0100
@@ -840,6 +840,7 @@ static int nr_getname(struct socket *soc
 	struct sock *sk = sock->sk;
 	struct nr_sock *nr = nr_sk(sk);

+	memset(sax, 0, sizeof(*sax));
 	lock_sock(sk);
 	if (peer != 0) {
 		if (sk->sk_state != TCP_ESTABLISHED) {
@@ -854,7 +855,6 @@ static int nr_getname(struct socket *soc
 		*uaddr_len = sizeof(struct full_sockaddr_ax25);
 	} else {
 		sax->fsa_ax25.sax25_family = AF_NETROM;
-		sax->fsa_ax25.sax25_ndigis = 0;
 		sax->fsa_ax25.sax25_call   = nr->source_addr;
 		*uaddr_len = sizeof(struct sockaddr_ax25);
 	}


Signed-off-by: Bernard Pidoux<bernard.pidoux@xxxxxxx>



Le dimanche 31 octobre 2010 à 20:10 +0300, Vasiliy Kulikov a écrit :
>  Sometimes ax25_getname() doesn't initialize all members of fsa_digipeater
>  field of fsa struct.  This structure is then copied to userland.  It leads to
>  leaking of contents of kernel stack memory.  We have to initialize them to zero.
>
>  Signed-off-by: Vasiliy Kulikov<segooon@xxxxxxxxx>
>  ---
>   net/ax25/af_ax25.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
>  diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
>  index 26eaebf..a324d83 100644
>  --- a/net/ax25/af_ax25.c
>  +++ b/net/ax25/af_ax25.c
>  @@ -1392,6 +1392,7 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
>   	ax25_cb *ax25;
>   	int err = 0;
>
>  +	memset(&fsa->fsa_digipeater, 0, sizeof(fsa->fsa_digipeater));
>   	lock_sock(sk);
>   	ax25 = ax25_sk(sk);

If you really want to fix this for good, please do it completely ?

sa_family_t is a short

ax25_address is 7 bytes.

Therefore, there is a hole before sax25_ndigis.

struct sockaddr_ax25 {
        sa_family_t     sax25_family;
        ax25_address    sax25_call;
<hole>
        int             sax25_ndigis;
        /* Digipeater ax25_address sets follow */
};
struct full_sockaddr_ax25 {
        struct sockaddr_ax25 fsa_ax25;
        ax25_address    fsa_digipeater[AX25_MAX_DIGIS];
};


So a correct patch is the following one. Note AX25 is probably used by
nobody at all, so a full memset() is not performance critical in this
path.


diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 26eaebf..6da5dae 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1392,6 +1392,7 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,
 	ax25_cb *ax25;
 	int err = 0;

+	memset(fsa, 0, sizeof(*fsa));
 	lock_sock(sk);
 	ax25 = ax25_sk(sk);

@@ -1403,7 +1404,6 @@ static int ax25_getname(struct socket *sock, struct sockaddr *uaddr,

 		fsa->fsa_ax25.sax25_family = AF_AX25;
 		fsa->fsa_ax25.sax25_call   = ax25->dest_addr;
-		fsa->fsa_ax25.sax25_ndigis = 0;

 		if (ax25->digipeat != NULL) {
 			ndigi = ax25->digipeat->ndigi;



--
To unsubscribe from this list: send the line "unsubscribe linux-hams" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html