diff mbox

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

Message ID 4D46ECED.1020601@canonical.com
State Accepted
Delegated to: Stefan Bader
Headers show

Commit Message

Tim Gardner Jan. 31, 2011, 5:10 p.m. UTC 
Return 0 instead of -1 in order to avoid SKB leak.

rtg

Comments

Stefan Bader Feb. 1, 2011, 2:19 p.m. UTC | #1 
On 01/31/2011 06:10 PM, Tim Gardner wrote:
> Return 0 instead of -1 in order to avoid SKB leak.
> 
> rtg
> 

ACK
Andy Whitcroft Feb. 1, 2011, 2:41 p.m. UTC | #2 
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@canonical.com

> From 1005daf240df317ad02adda7dd05fbd58930bc1e Mon Sep 17 00:00:00 2001
> From: andrew hendry <andrew.hendry@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@canonical.com>
> 
> Signed-of-by: Andrew Hendry <andrew.hendry@gmail.com>
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Tim Gardner <tim.gardner@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@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@vsecurity.com>
  Date:   Fri Nov 12 12:44:42 2010 -0800

    x25: Prevent crashing when parsing bad X.25 facilities

-apw
Kees Cook Feb. 1, 2011, 5:06 p.m. UTC | #3 
Hi Andy,

On Tue, Feb 01, 2011 at 02:41:26PM +0000, Andy Whitcroft wrote:
> 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@vsecurity.com>
>   Date:   Fri Nov 12 12:44:42 2010 -0800
> 
>     x25: Prevent crashing when parsing bad X.25 facilities

Yes, please. :)

-Kees
Tim Gardner Feb. 1, 2011, 5:18 p.m. UTC | #4 
On 02/01/2011 10:06 AM, Kees Cook wrote:
> Hi Andy,
>
> On Tue, Feb 01, 2011 at 02:41:26PM +0000, Andy Whitcroft wrote:
>> 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@vsecurity.com>
>>    Date:   Fri Nov 12 12:44:42 2010 -0800
>>
>>      x25: Prevent crashing when parsing bad X.25 facilities
>
> Yes, please. :)
>
> -Kees
>

Under the auspices of CVE-2010-3873 ? Or a new CVE?

I'm not really interested in putting too much work into X.25 'cause I 
don't think anyone is even using it these days. I haven't encountered a 
phy over which X.25 would have run in nearly a decade.

rtg
Kees Cook Feb. 1, 2011, 6 p.m. UTC | #5 
Hi Andy,

On Tue, Feb 01, 2011 at 09:06:43AM -0800, Kees Cook wrote:
> On Tue, Feb 01, 2011 at 02:41:26PM +0000, Andy Whitcroft wrote:
> > 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@vsecurity.com>
> >   Date:   Fri Nov 12 12:44:42 2010 -0800
> > 
> >     x25: Prevent crashing when parsing bad X.25 facilities
> 
> Yes, please. :)

Actually, the above patch is for CVE-2010-4164

http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-4164

-Kees
Tim Gardner Feb. 1, 2011, 6:16 p.m. UTC | #6 
On 02/01/2011 11:00 AM, Kees Cook wrote:
> Hi Andy,
>
> On Tue, Feb 01, 2011 at 09:06:43AM -0800, Kees Cook wrote:
>> On Tue, Feb 01, 2011 at 02:41:26PM +0000, Andy Whitcroft wrote:
>>> 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@vsecurity.com>
>>>    Date:   Fri Nov 12 12:44:42 2010 -0800
>>>
>>>      x25: Prevent crashing when parsing bad X.25 facilities
>>
>> Yes, please. :)
>
> Actually, the above patch is for CVE-2010-4164
>
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-4164
>
> -Kees
>

OK, then we'll get to it as we grind down the list.
diff mbox

Patch

From 1005daf240df317ad02adda7dd05fbd58930bc1e Mon Sep 17 00:00:00 2001
From: andrew hendry <andrew.hendry@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@canonical.com>

Signed-of-by: Andrew Hendry <andrew.hendry@gmail.com>

Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Tim Gardner <tim.gardner@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.
 			 */
-- 
1.7.0.4