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

Ben, thanks for your reply, I fixed the indentation and I addressed
the possible leak in nr_listen() too. Here is the patch.

Signed-off-by: Clement Lecigne <redacted>

--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c

@@ -408,7 +408,7 @@ static int nr_listen(struct socket *sock, int backlog)

        lock_sock(sk);
        if (sk->sk_state != TCP_LISTEN) {
-               memset(&nr_sk(sk)->user_addr, 0, AX25_ADDR_LEN);
+               memset(&nr_sk(sk)->user_addr, 0, sizeof(nr_sk(sk)->user_addr));
                sk->sk_max_ack_backlog = backlog;
                sk->sk_state           = TCP_LISTEN;
                release_sock(sk);

@@ -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));
+
        lock_sock(sk);
        if (peer != 0) {
                if (sk->sk_state != TCP_ESTABLISHED) {

2012/10/15 Ben Hutchings [off-list ref]:

On Sun, 2012-10-14 at 20:23 +0200, Clément Lecigne wrote:

quoted

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

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

quoted

        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.