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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKSJdAD-hFwHOnpWkCaW5bz68p-iK3nsA8FZEVFu5a9tog4cQg@mail.gmail.com>
Date:	Mon, 15 Oct 2012 09:46:59 +0200
From:	Clément Lecigne <clemun@...il.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: 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 <clemun@...il.com>
--- 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 <bhutchings@...arflare.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@...il.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.
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html