netdev - Re: [PATCH] x25: Validate incoming call user data lengths

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]

Date:	Thu, 6 Oct 2011 07:09:31 +1100
From:	Andrew Hendry <andrew.hendry@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Matthew Daley <mattjd@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] x25: Validate incoming call user data lengths

Ran this on a few systems and put a few GB through it, looks good.

Tested-by: Andrew Hendry <andrew.hendry@...il.com>

On Wed, Oct 5, 2011 at 1:33 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> Le mardi 04 octobre 2011 à 14:00 +0000, Matthew Daley a écrit :
>> X.25 call user data is being copied in its entirety from incoming messages
>> without consideration to the size of the destination buffers, leading to
>> possible buffer overflows. Validate incoming call user data lengths before
>> these copies are performed.
>>
>> It appears this issue was noticed some time ago, however nothing seemed to
>> come of it: see http://www.spinics.net/lists/linux-x25/msg00043.html and
>> commit 8db09f26f912f7c90c764806e804b558da520d4f.
>>
>> Signed-off-by: Matthew Daley <mattjd@...il.com>
>> Cc: Eric Dumazet <eric.dumazet@...il.com>
>> Cc: Andrew Hendry <andrew.hendry@...il.com>
>> Cc: stable <stable@...nel.org>
>> ---
>> Hopefully this is acceptable; I thought someone with greater knowledge of
>> X.25 might step in :) It should take into account Andrew's suggestion to
>> move af_x25.c's check to before the call acception.
>>
>>  net/x25/af_x25.c |    6 ++++++
>>  net/x25/x25_in.c |    3 +++
>>  2 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
>> index d306154..a4bd172 100644
>> --- a/net/x25/af_x25.c
>> +++ b/net/x25/af_x25.c
>> @@ -959,6 +959,12 @@ int x25_rx_call_request(struct sk_buff *skb,
>> struct x25_neigh *nb,
>>       skb_pull(skb,len);
>>
>>       /*
>> +      *      Ensure that the amount of call user data is valid.
>> +      */
>> +     if (skb->len > X25_MAX_CUD_LEN)
>> +             goto out_clear_request;
>> +
>> +     /*
>>        *      Find a listener for the particular address/cud pair.
>>        */
>>       sk = x25_find_listener(&source_addr,skb);
>> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
>> index 0b073b5..63488fd 100644
>> --- a/net/x25/x25_in.c
>> +++ b/net/x25/x25_in.c
>> @@ -127,6 +127,9 @@ static int x25_state1_machine(struct sock *sk,
>> struct sk_buff *skb, int frametyp
>>                *      Copy any Call User Data.
>>                */
>>               if (skb->len > 0) {
>> +                     if (skb->len > X25_MAX_CUD_LEN)
>> +                             goto out_clear;
>> +
>>                       skb_copy_from_linear_data(skb,
>>                                                 x25->calluserdata.cuddata,
>>                                                 skb->len);
>> --
>
> It seems fine to me.
>
> Acked-by: Eric Dumazet <eric.dumazet@...il.com>
>
> Now, you could also fix x25 for undersized/frags frames :
>
> Apparently, no check is done in this respect (missing pskb_may_pull()
> calls)...
>
>
>
>
--
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