ROSE: prevent heap corruption with bad facilities

When parsing the FAC_NATIONAL_DIGIS facilities field, it's possible for a remote host to provide more digipeaters than expected, resulting in heap corruption. Check against ROSE_MAX_DIGIS to prevent overflows, and abort facilities parsing on failure.

Additionally, when parsing the FAC_CCITT_DEST_NSAP and FAC_CCITT_SRC_NSAP facilities fields, a remote host can provide a length of less than 10, resulting in an underflow in a memcpy size, causing a kernel panic due to massive heap corruption. A length of greater than 20 results in a stack overflow of the callsign array. Abort facilities parsing on these invalid length values.

Subject: [PATCH] rose: Add length checks to CALL_REQUEST parsing

Define some constant offsets for CALL_REQUEST based on the description at <http://www.techfest.com/networking/wan/x25plp.htm> and the definition of ROSE as using 10-digit (5-byte) addresses. Use them consistently. Validate all implicit and explicit facilities lengths. Validate the address length byte rather than either trusting or assuming its value.

Signed-off-by Ben Hutchings <[email protected]>

From: Ben Hutchings <[email protected]>
Date: Sun, 20 Mar 2011 16:48:05 +0000

Quoted 9 lines

Subject: [PATCH] rose: Add length checks to CALL_REQUEST parsing

Define some constant offsets for CALL_REQUEST based on the description at <http://www.techfest.com/networking/wan/x25plp.htm> and the definition of ROSE as using 10-digit (5-byte) addresses. Use them consistently. Validate all implicit and explicit facilities lengths. Validate the address length byte rather than either trusting or assuming its value.

Quoted 1 line

Signed-off-by Ben Hutchings <[email protected]>

Applied.

Quoted 12 lines · David Miller

From: Ben Hutchings <[email protected]>
Date: Sun, 20 Mar 2011 16:48:05 +0000

Quoted 9 lines

Subject: [PATCH] rose: Add length checks to CALL_REQUEST parsing

Define some constant offsets for CALL_REQUEST based on the description at <http://www.techfest.com/networking/wan/x25plp.htm> and the definition of ROSE as using 10-digit (5-byte) addresses. Use them consistently. Validate all implicit and explicit facilities lengths. Validate the address length byte rather than either trusting or assuming its value.

Quoted 3 lines

Quoted 1 line

Signed-off-by Ben Hutchings <[email protected]>

Applied.

Ralf, I would really appreciate it if you could test this soon...

(quoted diff) +22 -32

@@ -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)

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.

So, to get the old behaviour back:

From: Jiri Bohac <[email protected]>
Date: Thu, 31 Mar 2011 20:02:25 +0200

Quoted 1 line

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.

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:

net/rose/rose_subr.c +0 -0

index f6c71ca..9777700 100644

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.

When parsing the FAC_NATIONAL_DIGIS facilities field, it's possible for a remote host to provide more digipeaters than expected, resulting in heap corruption. Check against ROSE_MAX_DIGIS to prevent overflows, and abort facilities parsing on failure.

Additionally, when parsing the FAC_CCITT_DEST_NSAP and FAC_CCITT_SRC_NSAP facilities fields, a remote host can provide a length of less than 10, resulting in an underflow in a memcpy size, causing a kernel panic due to massive heap corruption. A length of greater than 20 results in a stack overflow of the callsign array. Abort facilities parsing on these invalid length values.

Signed-off-by Dan Rosenberg <[email protected]>

Cc [email protected]