Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

On Thu, 2011-03-31 at 20:02 +0200, Jiri Bohac wrote:
[...]
> This last hunk does not look correct. In the default branch of
> the switch, you set len = 1, which means 
> 	p += 2; facilities_len -= 2.
> 
> The original code does 
> 	facilities_len--; p++;
> ... and it looks correct. So, to get the old behaviour back:
> 
> diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
> index f6c71ca..9777700 100644
> --- a/net/rose/rose_subr.c
> +++ b/net/rose/rose_subr.c
> @@ -418,7 +418,7 @@ int rose_parse_facilities(unsigned char *p, unsigned packet_len,
>  
>  		default:
>  			printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
> -			len = 1;
> +			len = 0;
>  			break;
>  		}

Yes, agreed.

> However, I wonder how much sense it makes to continue parsing the
> facilities if an unknown facility family appears. We don't know
> the length of its data, so we will interpret each 16 bytes a new
> facilities header, hopefully soon bailing out on *p != 0x00.
>
> In case of a long packet where every other byte is zero, the loop
> will spam the kernel log with the printk ... which could probably
> be classified as a security problem on its own. So how about the
> following instead? I have no idea if this breaks some rose
> specification, though. 
[...]

I don't know any more than you do; maybe Ralf knows or can find out.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]