[v2] ROSE: prevent heap corruption with bad facilities

On Sun, Mar 20, 2011 at 04:48:05PM +0000, Ben Hutchings wrote:
> @@ -365,49 +393,44 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
>  	return n;
>  }
>  
> -int rose_parse_facilities(unsigned char *p,
> +int rose_parse_facilities(unsigned char *p, unsigned packet_len,
>  	struct rose_facilities_struct *facilities)
>  {
>  	int facilities_len, len;
>  
>  	facilities_len = *p++;
>  
> -	if (facilities_len == 0)
> +	if (facilities_len == 0 || (unsigned)facilities_len > packet_len)
>  		return 0;
>  
> -	while (facilities_len > 0) {
> -		if (*p == 0x00) {
> -			facilities_len--;
> -			p++;
> -
> -			switch (*p) {
> -			case FAC_NATIONAL:		/* National */
> -				len = rose_parse_national(p + 1, facilities, facilities_len - 1);
> -				if (len < 0)
> -					return 0;
> -				facilities_len -= len + 1;
> -				p += len + 1;
> -				break;
> -
> -			case FAC_CCITT:		/* CCITT */
> -				len = rose_parse_ccitt(p + 1, facilities, facilities_len - 1);
> -				if (len < 0)
> -					return 0;
> -				facilities_len -= len + 1;
> -				p += len + 1;
> -				break;
> -
> -			default:
> -				printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
> -				facilities_len--;
> -				p++;
> -				break;
> -			}
> -		} else
> -			break;	/* Error in facilities format */
> +	while (facilities_len >= 3 && *p == 0x00) {
> +		facilities_len--;
> +		p++;
> +
> +		switch (*p) {
> +		case FAC_NATIONAL:		/* National */
> +			len = rose_parse_national(p + 1, facilities, facilities_len - 1);
> +			break;
> +
> +		case FAC_CCITT:		/* CCITT */
> +			len = rose_parse_ccitt(p + 1, facilities, facilities_len - 1);
> +			break;
> +
> +		default:
> +			printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
> +			len = 1;
> +			break;
> +		}
> +
> +		if (len < 0)
> +			return 0;
> +		if (WARN_ON(len >= facilities_len))
> +			return 0;
> +		facilities_len -= len + 1;
> +		p += len + 1;
>  	}
>  
> -	return 1;
> +	return facilities_len == 0;
>  }


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:

On Thu, Mar 31, 2011 at 08:02:25PM +0200, Jiri Bohac wrote:
> 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

oops, typo:
s/16 bytes a new/16 bits as 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.

From: Jiri Bohac <jbohac@suse.cz>
Date: Thu, 31 Mar 2011 20:02:25 +0200

> So, to get the old behaviour back:

Jiri, please do not submit two patches in one email, it's beyond
confusing.

Instead, please submit a proper two-patch series.

Thanks.
--
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

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.

On Fri, Apr 01, 2011 at 09:41:48PM -0700, David Miller wrote:
> From: Jiri Bohac <jbohac@suse.cz>
> Date: Thu, 31 Mar 2011 20:02:25 +0200
> > So, to get the old behaviour back:
> 
> Jiri, please do not submit two patches in one email, it's beyond
> confusing.
> 
> Instead, please submit a proper two-patch series.

The two one line patches were two suggestions what to do. It's
either one or the other...

Sorry for the confusion.