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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1301804590.4157.53.camel@localhost>
Date:	Sun, 03 Apr 2011 05:23:10 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Jiri Bohac <jbohac@...e.cz>
Cc:	Dan Rosenberg <drosenberg@...curity.com>, ralf@...ux-mips.org,
	davem@...emloft.net, netdev@...r.kernel.org, security@...nel.org
Subject: Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

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.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)