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

login
register
mail settings
Submitter Tim Gardner
Date Jan. 28, 2011, 8:35 p.m.
Message ID <20110128203527.20780F89F8@sepang.rtg.net>
Download mbox
Permalink /patch/80890/
State Accepted
Delegated to: Stefan Bader
Headers show

Pull-request

git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3873

Comments

Tim Gardner - Jan. 28, 2011, 8:35 p.m.
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(-)
Stefan Bader - Jan. 31, 2011, 8:53 a.m.
The change to the state1_machine function (applies to all patches) could have
been slightly more like upstream which is

if (len > 0)
	skb_pull(skb, len);
else
	return -1;

Though in the end it is the same and hopefully we neither need to backport the
change that modified that code before.

On 01/28/2011 09:35 PM, 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>
Acked-by: Stefan Bader <stefan.bader@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.
>  			 */
Andy Whitcroft - Jan. 31, 2011, 10:25 a.m.
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
Tim Gardner - Feb. 1, 2011, 2:47 p.m.
applied and pushed