* [SECURITY] [PATCH] Prevent crashing when parsing bad X.25 facilities
@ 2010-11-11 22:43 Dan Rosenberg
2010-11-11 23:24 ` Andrew Hendry
0 siblings, 1 reply; 4+ messages in thread
From: Dan Rosenberg @ 2010-11-11 22:43 UTC (permalink / raw)
To: andrew.hendry; +Cc: netdev, security
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)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [SECURITY] [PATCH] Prevent crashing when parsing bad X.25 facilities
2010-11-11 22:43 [SECURITY] [PATCH] Prevent crashing when parsing bad X.25 facilities Dan Rosenberg
@ 2010-11-11 23:24 ` Andrew Hendry
2010-11-11 23:49 ` Dan Rosenberg
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Hendry @ 2010-11-11 23:24 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: netdev, security
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)
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [SECURITY] [PATCH] Prevent crashing when parsing bad X.25 facilities
2010-11-11 23:24 ` Andrew Hendry
@ 2010-11-11 23:49 ` Dan Rosenberg
2010-11-11 23:53 ` Andrew Hendry
0 siblings, 1 reply; 4+ messages in thread
From: Dan Rosenberg @ 2010-11-11 23:49 UTC (permalink / raw)
To: Andrew Hendry; +Cc: netdev, security
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)
> >
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [SECURITY] [PATCH] Prevent crashing when parsing bad X.25 facilities
2010-11-11 23:49 ` Dan Rosenberg
@ 2010-11-11 23:53 ` Andrew Hendry
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Hendry @ 2010-11-11 23:53 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: netdev, security
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)
>> >
>> >
>> >
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-11-11 23:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-11 22:43 [SECURITY] [PATCH] Prevent crashing when parsing bad X.25 facilities Dan Rosenberg
2010-11-11 23:24 ` Andrew Hendry
2010-11-11 23:49 ` Dan Rosenberg
2010-11-11 23:53 ` Andrew Hendry
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).