Hardy CVE-2010-3873, memory corruption in X.25 facilities parsing (V2)

[Andy Whitcroft]

On Mon, Jan 31, 2011 at 10:10:05AM -0700, Tim Gardner wrote:
> Return 0 instead of -1 in order to avoid SKB leak.
> 
> rtg
> 
> -- 
> Tim Gardner tim.gardner at canonical.com

> From 1005daf240df317ad02adda7dd05fbd58930bc1e Mon Sep 17 00:00:00 2001
> From: andrew hendry <andrew.hendry at gmail.com>
> Date: Wed, 3 Nov 2010 12:54:53 +0000
> Subject: [PATCH] memory corruption in X.25 facilities parsing, CVE-2010-3873
> 
> BugLink: http://bugs.launchpad.net/bugs/709372
> 
> CVE-2010-3873
> 
> Backported 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_facilities.c |    8 ++++----
>  net/x25/x25_in.c         |   11 +++++++----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
> index dec404a..fa5eb98 100644
> --- a/net/x25/x25_facilities.c
> +++ b/net/x25/x25_facilities.c
> @@ -126,15 +126,15 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>  		case X25_FAC_CLASS_D:
>  			switch (*p) {
>  			case X25_FAC_CALLING_AE:
> -				if (p[1] > X25_MAX_DTE_FACIL_LEN)
> -					break;
> +				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
> +					return 0;
>  				dte_facs->calling_len = p[2];
>  				memcpy(dte_facs->calling_ae, &p[3], p[1] - 1);
>  				*vc_fac_mask |= X25_MASK_CALLING_AE;
>  				break;
>  			case X25_FAC_CALLED_AE:
> -				if (p[1] > X25_MAX_DTE_FACIL_LEN)
> -					break;
> +				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
> +					return 0;
>  				dte_facs->called_len = p[2];
>  				memcpy(dte_facs->called_ae, &p[3], p[1] - 1);
>  				*vc_fac_mask |= X25_MASK_CALLED_AE;
> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
> index 1c88762..5677d0e 100644
> --- a/net/x25/x25_in.c
> +++ b/net/x25/x25_in.c
> @@ -93,6 +93,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;
> @@ -107,10 +108,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->dte_facilities,
> -						&x25->vc_facil_mask));
> +			len = x25_parse_facilities(skb, &x25->facilities,
> +				&x25->dte_facilities, &x25->vc_facil_mask);
> +			if (len <= 0)
> +				return 0;
> +			skb_pull(skb, len);
> +
>  			/*
>  			 *	Copy any Call User Data.
>  			 */


Acked-by: Andy Whitcroft <apw at canonical.com>

Kees, I note that in v2.6.37 and later there is also this commit below,
you might want to review for relevance here.  It seems to prevent bad
packets triggering panics.

  commit 5ef41308f94dcbb3b7afc56cdef1c2ba53fa5d2f
  Author: Dan Rosenberg <drosenberg at vsecurity.com>
  Date:   Fri Nov 12 12:44:42 2010 -0800

    x25: Prevent crashing when parsing bad X.25 facilities

-apw