Patchwork [SECURITY] Prevent crashing when parsing bad X.25 facilities

login
register
mail settings
Submitter Dan Rosenberg
Date Nov. 11, 2010, 10:43 p.m.
Message ID <1289515414.5167.15.camel@dan>
Download mbox | patch
Permalink /patch/70885/
State Superseded
Delegated to: David Miller
Headers show

Comments

Dan Rosenberg - Nov. 11, 2010, 10:43 p.m.
Sorry I didn't catch this at the same time as my previous report so it
could be included in one patch.

On parsing malformed X.25 facilities, decrementing the remaining length
may cause it to underflow.  Since the length is an unsigned integer,
this will result in the loop continuing until the kernel crashes.

This patch adds checks to ensure decrementing the remaining length does
not cause it to wrap around.

Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
CC: stable <stable@kernel.org>
---
 net/x25/x25_facilities.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
andrew hendry - Nov. 11, 2010, 11:24 p.m.
Tested ok, although I noticed the CLASS_D default: message tries to
print the facility type, length and 4 values, while it may only have 2
values. Just needs p[4] and p[5] taken out. Do you want me to spin a
secondary patch or do you want to put them together?

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

On Fri, Nov 12, 2010 at 9:43 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> Sorry I didn't catch this at the same time as my previous report so it
> could be included in one patch.
>
> On parsing malformed X.25 facilities, decrementing the remaining length
> may cause it to underflow.  Since the length is an unsigned integer,
> this will result in the loop continuing until the kernel crashes.
>
> This patch adds checks to ensure decrementing the remaining length does
> not cause it to wrap around.
>
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> CC: stable <stable@kernel.org>
> ---
>  net/x25/x25_facilities.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
> index 3a8c4c4..bef8330 100644
> --- a/net/x25/x25_facilities.c
> +++ b/net/x25/x25_facilities.c
> @@ -61,6 +61,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>        while (len > 0) {
>                switch (*p & X25_FAC_CLASS_MASK) {
>                case X25_FAC_CLASS_A:
> +                       if (len < 2)
> +                               return 0;
>                        switch (*p) {
>                        case X25_FAC_REVERSE:
>                                if((p[1] & 0x81) == 0x81) {
> @@ -104,6 +106,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>                        len -= 2;
>                        break;
>                case X25_FAC_CLASS_B:
> +                       if (len < 3)
> +                               return 0;
>                        switch (*p) {
>                        case X25_FAC_PACKET_SIZE:
>                                facilities->pacsize_in  = p[1];
> @@ -125,6 +129,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>                        len -= 3;
>                        break;
>                case X25_FAC_CLASS_C:
> +                       if (len < 4)
> +                               return 0;
>                        printk(KERN_DEBUG "X.25: unknown facility %02X, "
>                               "values %02X, %02X, %02X\n",
>                               p[0], p[1], p[2], p[3]);
> @@ -132,6 +138,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>                        len -= 4;
>                        break;
>                case X25_FAC_CLASS_D:
> +                       if (len < p[1] + 2)
> +                               return 0;
>                        switch (*p) {
>                        case X25_FAC_CALLING_AE:
>                                if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Rosenberg - Nov. 11, 2010, 11:49 p.m.
I can take care of it.  Do you want me to just remove those items from
the printk statement?

-Dan

On Fri, 2010-11-12 at 10:24 +1100, Andrew Hendry wrote:
> Tested ok, although I noticed the CLASS_D default: message tries to
> print the facility type, length and 4 values, while it may only have 2
> values. Just needs p[4] and p[5] taken out. Do you want me to spin a
> secondary patch or do you want to put them together?
> 
> Acked-by: Andrew Hendry <andrew.hendry@gmail.com>
> 
> On Fri, Nov 12, 2010 at 9:43 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> > Sorry I didn't catch this at the same time as my previous report so it
> > could be included in one patch.
> >
> > On parsing malformed X.25 facilities, decrementing the remaining length
> > may cause it to underflow.  Since the length is an unsigned integer,
> > this will result in the loop continuing until the kernel crashes.
> >
> > This patch adds checks to ensure decrementing the remaining length does
> > not cause it to wrap around.
> >
> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> > CC: stable <stable@kernel.org>
> > ---
> >  net/x25/x25_facilities.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
> > index 3a8c4c4..bef8330 100644
> > --- a/net/x25/x25_facilities.c
> > +++ b/net/x25/x25_facilities.c
> > @@ -61,6 +61,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
> >        while (len > 0) {
> >                switch (*p & X25_FAC_CLASS_MASK) {
> >                case X25_FAC_CLASS_A:
> > +                       if (len < 2)
> > +                               return 0;
> >                        switch (*p) {
> >                        case X25_FAC_REVERSE:
> >                                if((p[1] & 0x81) == 0x81) {
> > @@ -104,6 +106,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
> >                        len -= 2;
> >                        break;
> >                case X25_FAC_CLASS_B:
> > +                       if (len < 3)
> > +                               return 0;
> >                        switch (*p) {
> >                        case X25_FAC_PACKET_SIZE:
> >                                facilities->pacsize_in  = p[1];
> > @@ -125,6 +129,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
> >                        len -= 3;
> >                        break;
> >                case X25_FAC_CLASS_C:
> > +                       if (len < 4)
> > +                               return 0;
> >                        printk(KERN_DEBUG "X.25: unknown facility %02X, "
> >                               "values %02X, %02X, %02X\n",
> >                               p[0], p[1], p[2], p[3]);
> > @@ -132,6 +138,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
> >                        len -= 4;
> >                        break;
> >                case X25_FAC_CLASS_D:
> > +                       if (len < p[1] + 2)
> > +                               return 0;
> >                        switch (*p) {
> >                        case X25_FAC_CALLING_AE:
> >                                if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
> >
> >
> >


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
andrew hendry - Nov. 11, 2010, 11:53 p.m.
Thanks, yes just remove them from the printk.

On Fri, Nov 12, 2010 at 10:49 AM, Dan Rosenberg
<drosenberg@vsecurity.com> wrote:
> I can take care of it.  Do you want me to just remove those items from
> the printk statement?
>
> -Dan
>
> On Fri, 2010-11-12 at 10:24 +1100, Andrew Hendry wrote:
>> Tested ok, although I noticed the CLASS_D default: message tries to
>> print the facility type, length and 4 values, while it may only have 2
>> values. Just needs p[4] and p[5] taken out. Do you want me to spin a
>> secondary patch or do you want to put them together?
>>
>> Acked-by: Andrew Hendry <andrew.hendry@gmail.com>
>>
>> On Fri, Nov 12, 2010 at 9:43 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
>> > Sorry I didn't catch this at the same time as my previous report so it
>> > could be included in one patch.
>> >
>> > On parsing malformed X.25 facilities, decrementing the remaining length
>> > may cause it to underflow.  Since the length is an unsigned integer,
>> > this will result in the loop continuing until the kernel crashes.
>> >
>> > This patch adds checks to ensure decrementing the remaining length does
>> > not cause it to wrap around.
>> >
>> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
>> > CC: stable <stable@kernel.org>
>> > ---
>> >  net/x25/x25_facilities.c |    8 ++++++++
>> >  1 files changed, 8 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
>> > index 3a8c4c4..bef8330 100644
>> > --- a/net/x25/x25_facilities.c
>> > +++ b/net/x25/x25_facilities.c
>> > @@ -61,6 +61,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>> >        while (len > 0) {
>> >                switch (*p & X25_FAC_CLASS_MASK) {
>> >                case X25_FAC_CLASS_A:
>> > +                       if (len < 2)
>> > +                               return 0;
>> >                        switch (*p) {
>> >                        case X25_FAC_REVERSE:
>> >                                if((p[1] & 0x81) == 0x81) {
>> > @@ -104,6 +106,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>> >                        len -= 2;
>> >                        break;
>> >                case X25_FAC_CLASS_B:
>> > +                       if (len < 3)
>> > +                               return 0;
>> >                        switch (*p) {
>> >                        case X25_FAC_PACKET_SIZE:
>> >                                facilities->pacsize_in  = p[1];
>> > @@ -125,6 +129,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>> >                        len -= 3;
>> >                        break;
>> >                case X25_FAC_CLASS_C:
>> > +                       if (len < 4)
>> > +                               return 0;
>> >                        printk(KERN_DEBUG "X.25: unknown facility %02X, "
>> >                               "values %02X, %02X, %02X\n",
>> >                               p[0], p[1], p[2], p[3]);
>> > @@ -132,6 +138,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>> >                        len -= 4;
>> >                        break;
>> >                case X25_FAC_CLASS_D:
>> > +                       if (len < p[1] + 2)
>> > +                               return 0;
>> >                        switch (*p) {
>> >                        case X25_FAC_CALLING_AE:
>> >                                if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
>> >
>> >
>> >
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
index 3a8c4c4..bef8330 100644
--- a/net/x25/x25_facilities.c
+++ b/net/x25/x25_facilities.c
@@ -61,6 +61,8 @@  int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 	while (len > 0) {
 		switch (*p & X25_FAC_CLASS_MASK) {
 		case X25_FAC_CLASS_A:
+			if (len < 2)
+				return 0;
 			switch (*p) {
 			case X25_FAC_REVERSE:
 				if((p[1] & 0x81) == 0x81) {
@@ -104,6 +106,8 @@  int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 			len -= 2;
 			break;
 		case X25_FAC_CLASS_B:
+			if (len < 3)
+				return 0;
 			switch (*p) {
 			case X25_FAC_PACKET_SIZE:
 				facilities->pacsize_in  = p[1];
@@ -125,6 +129,8 @@  int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 			len -= 3;
 			break;
 		case X25_FAC_CLASS_C:
+			if (len < 4)
+				return 0;
 			printk(KERN_DEBUG "X.25: unknown facility %02X, "
 			       "values %02X, %02X, %02X\n",
 			       p[0], p[1], p[2], p[3]);
@@ -132,6 +138,8 @@  int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 			len -= 4;
 			break;
 		case X25_FAC_CLASS_D:
+			if (len < p[1] + 2)
+				return 0;
 			switch (*p) {
 			case X25_FAC_CALLING_AE:
 				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)