From patchwork Mon Jan 31 17:01:16 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Dapper CVE-2010-3873, memory corruption in X.25 facilities parsing From: Tim Gardner X-Patchwork-Id: 81183 X-Patchwork-Delegate: stefan.bader@canonical.com Message-Id: <4D46EADC.7050002@canonical.com> To: Andy Whitcroft Cc: kernel-team@lists.ubuntu.com Date: Mon, 31 Jan 2011 10:01:16 -0700 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 >> 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 >> >> Signed-of-by: Andrew Hendry >> >> Signed-off-by: David S. Miller >> Signed-off-by: Tim Gardner >> --- >> 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 Acked-by: Andy Whitcroft >From 6ca583d619cacd4b0fe9cd183de2608ca8780b9d Mon Sep 17 00:00:00 2001 From: Tim Gardner 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 Signed-of-by: Andrew Hendry Signed-off-by: David S. Miller Signed-off-by: Tim Gardner --- 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