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

login
register
mail settings
Submitter Tim Gardner
Date Jan. 31, 2011, 5:01 p.m.
Message ID <4D46EADC.7050002@canonical.com>
Download mbox | patch
Permalink /patch/81183/
State Accepted
Delegated to: Stefan Bader
Headers show

Comments

Tim Gardner - Jan. 31, 2011, 5:01 p.m.
On 01/31/2011 03:25 AM, Andy Whitcroft wrote:
> 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@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@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_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
>

Your observation seems obviously correct to me. Without waiting for 
upstream, how about the attached ?

rtg
Andy Whitcroft - Feb. 1, 2011, 2:31 p.m.
On Mon, Jan 31, 2011 at 10:01:16AM -0700, Tim Gardner wrote:
> On 01/31/2011 03:25 AM, Andy Whitcroft wrote:
> >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@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@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_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
> >
> 
> Your observation seems obviously correct to me. Without waiting for
> upstream, how about the attached ?
> 
> rtg
> -- 
> Tim Gardner tim.gardner@canonical.com

> From 6ca583d619cacd4b0fe9cd183de2608ca8780b9d Mon Sep 17 00:00:00 2001
> From: Tim Gardner <tim.gardner@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@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_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..85eb3cb 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 0;
> +			skb_pull(skb, len);
> +
>  			/*
>  			 *	Copy any Call User Data.
>  			 */
> -- 
> 1.7.0.4
> 

Upstream seems to concur this will leak with -1, and that 0 is better
here.

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

-apw

Patch

From 6ca583d619cacd4b0fe9cd183de2608ca8780b9d Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@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@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_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..85eb3cb 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 0;
+			skb_pull(skb, len);
+
 			/*
 			 *	Copy any Call User Data.
 			 */
-- 
1.7.0.4