Dapper CVE-2010-3873, memory corruption in X.25 facilities parsing

Andy Whitcroft apw at canonical.com
Mon Jan 31 10:25:19 UTC 2011


On Fri, Jan 28, 2011 at 01:35:26PM -0700, Tim Gardner wrote:
> The following changes since commit 9f646039dcba8632e8d68f73652ce6949d6b20ba:
>   David S. Miller (1):
>         net: Limit socket I/O iovec total length to INT_MAX., CVE-2010-3859
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3873
> 
> Tim Gardner (1):
>       memory corruption in X.25 facilities parsing, CVE-2010-3873
> 
>  net/x25/x25_in.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> From 92700cf43dcb6ffdd998550a2ecadb7a2343e222 Mon Sep 17 00:00:00 2001
> From: Tim Gardner <tim.gardner at canonical.com>
> Date: Fri, 28 Jan 2011 11:17:01 -0700
> Subject: [PATCH] memory corruption in X.25 facilities parsing, CVE-2010-3873
> 
> BugLink: http://bugs.launchpad.net/bugs/709372
> 
> CVE-2010-3873
> 
> Partial backport from a6331d6f9a4298173b413cf99a40cc86a9d92c37
> by Tim Gardner <tim.gardner at canonical.com>
> 
> Signed-of-by: Andrew Hendry <andrew.hendry at gmail.com>
> 
> Signed-off-by: David S. Miller <davem at davemloft.net>
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
> ---
>  net/x25/x25_in.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
> index 2614687..659252b 100644
> --- a/net/x25/x25_in.c
> +++ b/net/x25/x25_in.c
> @@ -90,6 +90,7 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp
>  	switch (frametype) {
>  		case X25_CALL_ACCEPTED: {
>  			struct x25_sock *x25 = x25_sk(sk);
> +			int len;
>  
>  			x25_stop_timer(sk);
>  			x25->condition = 0x00;
> @@ -104,9 +105,12 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp
>  			 */
>  			skb_pull(skb, X25_STD_MIN_LEN);
>  			skb_pull(skb, x25_addr_ntoa(skb->data, &source_addr, &dest_addr));
> -			skb_pull(skb,
> -				 x25_parse_facilities(skb, &x25->facilities,
> -						      &x25->vc_facil_mask));
> +			len = x25_parse_facilities(skb, &x25->facilities,
> +							&x25->vc_facil_mask);
> +			if (len <= 0)
> +				return -1;
> +			skb_pull(skb, len);
> +
>  			/*
>  			 *	Copy any Call User Data.
>  			 */

As we do not support CLASS_D at all in Dapper it is not clear that we
can ever return a negative or zero length from x25_parse_facilities().
This is however safe and matches upstream.

I do have one concern, and this applies to the change as applied to
upstream and not a specific concern with any of the backports.  I think
that returning -1 here will trigger the calling backlog handler to think
that we have queued the packet for further processing (which we have not)
and thus we will leak the skb in the code below:

    int x25_backlog_rcv(struct sock *sk, struct sk_buff *skb)
    {
        int queued = x25_process_rx_frame(sk, skb);

        if (!queued)
                kfree_skb(skb);

        return 0;
    }

I will separatly ask about this upstream, as this may mean we are
replacing a remote corrupter with a remote DOS.

-apw




More information about the kernel-team mailing list