* [PATCH v2] ROSE: prevent heap corruption with bad facilities @ 2011-03-20 6:43 Dan Rosenberg 2011-03-20 16:48 ` Ben Hutchings 2011-03-28 0:59 ` David Miller 0 siblings, 2 replies; 11+ messages in thread From: Dan Rosenberg @ 2011-03-20 6:43 UTC (permalink / raw) To: ralf, davem; +Cc: netdev, security 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 <drosenberg@vsecurity.com> Cc: stable@kernel.org --- net/rose/rose_subr.c | 18 ++++++++++++++++-- 1 files changed, 16 insertions(+), 2 deletions(-) diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c index 1734abb..6f5f0b5 100644 --- a/net/rose/rose_subr.c +++ b/net/rose/rose_subr.c @@ -290,10 +290,15 @@ static int rose_parse_national(unsigned char *p, struct rose_facilities_struct * facilities->source_ndigis = 0; facilities->dest_ndigis = 0; for (pt = p + 2, lg = 0 ; lg < l ; pt += AX25_ADDR_LEN, lg += AX25_ADDR_LEN) { - if (pt[6] & AX25_HBIT) + if (pt[6] & AX25_HBIT) { + if (facilities->dest_ndigis >= ROSE_MAX_DIGIS) + return -1; memcpy(&facilities->dest_digis[facilities->dest_ndigis++], pt, AX25_ADDR_LEN); - else + } else { + if (facilities->source_ndigis >= ROSE_MAX_DIGIS) + return -1; memcpy(&facilities->source_digis[facilities->source_ndigis++], pt, AX25_ADDR_LEN); + } } } p += l + 2; @@ -333,6 +338,11 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac case 0xC0: l = p[1]; + + /* Prevent overflows*/ + if (l < 10 || l > 20) + return -1; + if (*p == FAC_CCITT_DEST_NSAP) { memcpy(&facilities->source_addr, p + 7, ROSE_ADDR_LEN); memcpy(callsign, p + 12, l - 10); @@ -373,12 +383,16 @@ int rose_parse_facilities(unsigned char *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; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-20 6:43 [PATCH v2] ROSE: prevent heap corruption with bad facilities Dan Rosenberg @ 2011-03-20 16:48 ` Ben Hutchings 2011-03-28 0:59 ` David Miller 2011-03-31 18:02 ` Jiri Bohac 2011-03-28 0:59 ` David Miller 1 sibling, 2 replies; 11+ messages in thread From: Ben Hutchings @ 2011-03-20 16:48 UTC (permalink / raw) To: Dan Rosenberg; +Cc: ralf, davem, netdev, security On Sun, 2011-03-20 at 02:43 -0400, Dan Rosenberg wrote: > 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. [...] Look further and you'll see that the *entire* parsing code makes not the slightest attempt to validate lengths. This applies on top of your patch, and hopefully fixes the remaining bugs. It's compile-tested only. Ben. --- 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 <ben@decadent.org.uk> --- include/net/rose.h | 8 ++++- net/rose/af_rose.c | 8 ++-- net/rose/rose_loopback.c | 13 ++++++- net/rose/rose_route.c | 20 +++++++---- net/rose/rose_subr.c | 91 +++++++++++++++++++++++++++++----------------- 5 files changed, 93 insertions(+), 47 deletions(-) diff --git a/include/net/rose.h b/include/net/rose.h index 5ba9f02..555dd19 100644 --- a/include/net/rose.h +++ b/include/net/rose.h @@ -14,6 +14,12 @@ #define ROSE_MIN_LEN 3 +#define ROSE_CALL_REQ_ADDR_LEN_OFF 3 +#define ROSE_CALL_REQ_ADDR_LEN_VAL 0xAA /* each address is 10 digits */ +#define ROSE_CALL_REQ_DEST_ADDR_OFF 4 +#define ROSE_CALL_REQ_SRC_ADDR_OFF 9 +#define ROSE_CALL_REQ_FACILITIES_OFF 14 + #define ROSE_GFI 0x10 #define ROSE_Q_BIT 0x80 #define ROSE_D_BIT 0x40 @@ -214,7 +220,7 @@ extern void rose_requeue_frames(struct sock *); extern int rose_validate_nr(struct sock *, unsigned short); extern void rose_write_internal(struct sock *, int); extern int rose_decode(struct sk_buff *, int *, int *, int *, int *, int *); -extern int rose_parse_facilities(unsigned char *, struct rose_facilities_struct *); +extern int rose_parse_facilities(unsigned char *, unsigned int, struct rose_facilities_struct *); extern void rose_disconnect(struct sock *, int, int, int); /* rose_timer.c */ diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c index 5ee0c62..a80aef6 100644 --- a/net/rose/af_rose.c +++ b/net/rose/af_rose.c @@ -978,7 +978,7 @@ int rose_rx_call_request(struct sk_buff *skb, struct net_device *dev, struct ros struct sock *make; struct rose_sock *make_rose; struct rose_facilities_struct facilities; - int n, len; + int n; skb->sk = NULL; /* Initially we don't know who it's for */ @@ -987,9 +987,9 @@ int rose_rx_call_request(struct sk_buff *skb, struct net_device *dev, struct ros */ memset(&facilities, 0x00, sizeof(struct rose_facilities_struct)); - len = (((skb->data[3] >> 4) & 0x0F) + 1) >> 1; - len += (((skb->data[3] >> 0) & 0x0F) + 1) >> 1; - if (!rose_parse_facilities(skb->data + len + 4, &facilities)) { + if (!rose_parse_facilities(skb->data + ROSE_CALL_REQ_FACILITIES_OFF, + skb->len - ROSE_CALL_REQ_FACILITIES_OFF, + &facilities)) { rose_transmit_clear_request(neigh, lci, ROSE_INVALID_FACILITY, 76); return 0; } diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c index ae4a9d9..3444562 100644 --- a/net/rose/rose_loopback.c +++ b/net/rose/rose_loopback.c @@ -73,9 +73,20 @@ static void rose_loopback_timer(unsigned long param) unsigned int lci_i, lci_o; while ((skb = skb_dequeue(&loopback_queue)) != NULL) { + if (skb->len < ROSE_MIN_LEN) { + kfree_skb(skb); + continue; + } lci_i = ((skb->data[0] << 8) & 0xF00) + ((skb->data[1] << 0) & 0x0FF); frametype = skb->data[2]; - dest = (rose_address *)(skb->data + 4); + if (frametype == ROSE_CALL_REQUEST && + (skb->len <= ROSE_CALL_REQ_FACILITIES_OFF || + skb->data[ROSE_CALL_REQ_ADDR_LEN_OFF] != + ROSE_CALL_REQ_ADDR_LEN_VAL)) { + kfree_skb(skb); + continue; + } + dest = (rose_address *)(skb->data + ROSE_CALL_REQ_DEST_ADDR_OFF); lci_o = ROSE_DEFAULT_MAXVC + 1 - lci_i; skb_reset_transport_header(skb); diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c index 88a77e9..08dcd2f 100644 --- a/net/rose/rose_route.c +++ b/net/rose/rose_route.c @@ -861,7 +861,7 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25) unsigned int lci, new_lci; unsigned char cause, diagnostic; struct net_device *dev; - int len, res = 0; + int res = 0; char buf[11]; #if 0 @@ -869,10 +869,17 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25) return res; #endif + if (skb->len < ROSE_MIN_LEN) + return res; frametype = skb->data[2]; lci = ((skb->data[0] << 8) & 0xF00) + ((skb->data[1] << 0) & 0x0FF); - src_addr = (rose_address *)(skb->data + 9); - dest_addr = (rose_address *)(skb->data + 4); + if (frametype == ROSE_CALL_REQUEST && + (skb->len <= ROSE_CALL_REQ_FACILITIES_OFF || + skb->data[ROSE_CALL_REQ_ADDR_LEN_OFF] != + ROSE_CALL_REQ_ADDR_LEN_VAL)) + return res; + src_addr = (rose_address *)(skb->data + ROSE_CALL_REQ_SRC_ADDR_OFF); + dest_addr = (rose_address *)(skb->data + ROSE_CALL_REQ_DEST_ADDR_OFF); spin_lock_bh(&rose_neigh_list_lock); spin_lock_bh(&rose_route_list_lock); @@ -1010,12 +1017,11 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25) goto out; } - len = (((skb->data[3] >> 4) & 0x0F) + 1) >> 1; - len += (((skb->data[3] >> 0) & 0x0F) + 1) >> 1; - memset(&facilities, 0x00, sizeof(struct rose_facilities_struct)); - if (!rose_parse_facilities(skb->data + len + 4, &facilities)) { + if (!rose_parse_facilities(skb->data + ROSE_CALL_REQ_FACILITIES_OFF, + skb->len - ROSE_CALL_REQ_FACILITIES_OFF, + &facilities)) { rose_transmit_clear_request(rose_neigh, lci, ROSE_INVALID_FACILITY, 76); goto out; } diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c index 174d51c..f6c71ca 100644 --- a/net/rose/rose_subr.c +++ b/net/rose/rose_subr.c @@ -142,7 +142,7 @@ void rose_write_internal(struct sock *sk, int frametype) *dptr++ = ROSE_GFI | lci1; *dptr++ = lci2; *dptr++ = frametype; - *dptr++ = 0xAA; + *dptr++ = ROSE_CALL_REQ_ADDR_LEN_VAL; memcpy(dptr, &rose->dest_addr, ROSE_ADDR_LEN); dptr += ROSE_ADDR_LEN; memcpy(dptr, &rose->source_addr, ROSE_ADDR_LEN); @@ -246,12 +246,16 @@ static int rose_parse_national(unsigned char *p, struct rose_facilities_struct * do { switch (*p & 0xC0) { case 0x00: + if (len < 2) + return -1; p += 2; n += 2; len -= 2; break; case 0x40: + if (len < 3) + return -1; if (*p == FAC_NATIONAL_RAND) facilities->rand = ((p[1] << 8) & 0xFF00) + ((p[2] << 0) & 0x00FF); p += 3; @@ -260,32 +264,48 @@ static int rose_parse_national(unsigned char *p, struct rose_facilities_struct * break; case 0x80: + if (len < 4) + return -1; p += 4; n += 4; len -= 4; break; case 0xC0: + if (len < 2) + return -1; l = p[1]; + if (len < 2 + l) + return -1; if (*p == FAC_NATIONAL_DEST_DIGI) { if (!fac_national_digis_received) { + if (l < AX25_ADDR_LEN) + return -1; memcpy(&facilities->source_digis[0], p + 2, AX25_ADDR_LEN); facilities->source_ndigis = 1; } } else if (*p == FAC_NATIONAL_SRC_DIGI) { if (!fac_national_digis_received) { + if (l < AX25_ADDR_LEN) + return -1; memcpy(&facilities->dest_digis[0], p + 2, AX25_ADDR_LEN); facilities->dest_ndigis = 1; } } else if (*p == FAC_NATIONAL_FAIL_CALL) { + if (l < AX25_ADDR_LEN) + return -1; memcpy(&facilities->fail_call, p + 2, AX25_ADDR_LEN); } else if (*p == FAC_NATIONAL_FAIL_ADD) { + if (l < 1 + ROSE_ADDR_LEN) + return -1; memcpy(&facilities->fail_addr, p + 3, ROSE_ADDR_LEN); } else if (*p == FAC_NATIONAL_DIGIS) { + if (l % AX25_ADDR_LEN) + return -1; fac_national_digis_received = 1; facilities->source_ndigis = 0; facilities->dest_ndigis = 0; @@ -319,24 +339,32 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac do { switch (*p & 0xC0) { case 0x00: + if (len < 2) + return -1; p += 2; n += 2; len -= 2; break; case 0x40: + if (len < 3) + return -1; p += 3; n += 3; len -= 3; break; case 0x80: + if (len < 4) + return -1; p += 4; n += 4; len -= 4; break; case 0xC0: + if (len < 2) + return -1; l = p[1]; /* Prevent overflows*/ @@ -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; } static int rose_create_facilities(unsigned char *buffer, struct rose_sock *rose) -- 1.7.4.1 -- Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse. ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-20 16:48 ` Ben Hutchings @ 2011-03-28 0:59 ` David Miller 2011-03-29 1:16 ` Ben Hutchings 2011-03-31 18:02 ` Jiri Bohac 1 sibling, 1 reply; 11+ messages in thread From: David Miller @ 2011-03-28 0:59 UTC (permalink / raw) To: ben; +Cc: drosenberg, ralf, netdev, security From: Ben Hutchings <ben@decadent.org.uk> Date: Sun, 20 Mar 2011 16:48:05 +0000 > 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 <ben@decadent.org.uk> Applied. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-28 0:59 ` David Miller @ 2011-03-29 1:16 ` Ben Hutchings 2011-03-29 16:26 ` Ralf Baechle 0 siblings, 1 reply; 11+ messages in thread From: Ben Hutchings @ 2011-03-29 1:16 UTC (permalink / raw) To: ralf; +Cc: drosenberg, netdev, security, David Miller [-- Attachment #1: Type: text/plain, Size: 854 bytes --] On Sun, 2011-03-27 at 17:59 -0700, David Miller wrote: > From: Ben Hutchings <ben@decadent.org.uk> > Date: Sun, 20 Mar 2011 16:48:05 +0000 > > > 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 <ben@decadent.org.uk> > > Applied. Ralf, I would really appreciate it if you could test this soon... 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-29 1:16 ` Ben Hutchings @ 2011-03-29 16:26 ` Ralf Baechle 0 siblings, 0 replies; 11+ messages in thread From: Ralf Baechle @ 2011-03-29 16:26 UTC (permalink / raw) To: Ben Hutchings; +Cc: drosenberg, netdev, security, David Miller On Tue, Mar 29, 2011 at 02:16:19AM +0100, Ben Hutchings wrote: > On Sun, 2011-03-27 at 17:59 -0700, David Miller wrote: > > From: Ben Hutchings <ben@decadent.org.uk> > > Date: Sun, 20 Mar 2011 16:48:05 +0000 > > > > > 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 <ben@decadent.org.uk> > > > > Applied. > > Ralf, I would really appreciate it if you could test this soon... Actual testing is a problem atm. But I've reviewed the patche and it appears ok. Acked-by: Ralf Baechle <ralf@linux-mips.org> Ralf ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-20 16:48 ` Ben Hutchings 2011-03-28 0:59 ` David Miller @ 2011-03-31 18:02 ` Jiri Bohac 2011-04-01 12:29 ` Jiri Bohac ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Jiri Bohac @ 2011-03-31 18:02 UTC (permalink / raw) To: Ben Hutchings; +Cc: Dan Rosenberg, ralf, davem, netdev, security 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: 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; } 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. Signed-off-by: Jiri Bohac <jbohac@suse.cz> diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c index f6c71ca..e687c7f 100644 --- a/net/rose/rose_subr.c +++ b/net/rose/rose_subr.c @@ -418,8 +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; - break; + return 0; } if (len < 0) -- Jiri Bohac <jbohac@suse.cz> SUSE Labs, SUSE CZ ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-31 18:02 ` Jiri Bohac @ 2011-04-01 12:29 ` Jiri Bohac 2011-04-02 4:41 ` David Miller 2011-04-03 4:23 ` Ben Hutchings 2 siblings, 0 replies; 11+ messages in thread From: Jiri Bohac @ 2011-04-01 12:29 UTC (permalink / raw) To: Jiri Bohac; +Cc: Ben Hutchings, Dan Rosenberg, ralf, davem, netdev, security 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. -- Jiri Bohac <jbohac@suse.cz> SUSE Labs, SUSE CZ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-31 18:02 ` Jiri Bohac 2011-04-01 12:29 ` Jiri Bohac @ 2011-04-02 4:41 ` David Miller 2011-04-05 8:20 ` Jiri Bohac 2011-04-03 4:23 ` Ben Hutchings 2 siblings, 1 reply; 11+ messages in thread From: David Miller @ 2011-04-02 4:41 UTC (permalink / raw) To: jbohac; +Cc: ben, drosenberg, ralf, netdev, security 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-04-02 4:41 ` David Miller @ 2011-04-05 8:20 ` Jiri Bohac 0 siblings, 0 replies; 11+ messages in thread From: Jiri Bohac @ 2011-04-05 8:20 UTC (permalink / raw) To: David Miller; +Cc: jbohac, ben, drosenberg, ralf, netdev, security 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. -- Jiri Bohac <jbohac@suse.cz> SUSE Labs, SUSE CZ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-31 18:02 ` Jiri Bohac 2011-04-01 12:29 ` Jiri Bohac 2011-04-02 4:41 ` David Miller @ 2011-04-03 4:23 ` Ben Hutchings 2 siblings, 0 replies; 11+ messages in thread From: Ben Hutchings @ 2011-04-03 4:23 UTC (permalink / raw) To: Jiri Bohac; +Cc: Dan Rosenberg, ralf, davem, netdev, security [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities 2011-03-20 6:43 [PATCH v2] ROSE: prevent heap corruption with bad facilities Dan Rosenberg 2011-03-20 16:48 ` Ben Hutchings @ 2011-03-28 0:59 ` David Miller 1 sibling, 0 replies; 11+ messages in thread From: David Miller @ 2011-03-28 0:59 UTC (permalink / raw) To: drosenberg; +Cc: ralf, netdev, security From: Dan Rosenberg <drosenberg@vsecurity.com> Date: Sun, 20 Mar 2011 02:43:43 -0400 > 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 <drosenberg@vsecurity.com> > Cc: stable@kernel.org Applied. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-04-05 8:20 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-20 6:43 [PATCH v2] ROSE: prevent heap corruption with bad facilities Dan Rosenberg 2011-03-20 16:48 ` Ben Hutchings 2011-03-28 0:59 ` David Miller 2011-03-29 1:16 ` Ben Hutchings 2011-03-29 16:26 ` Ralf Baechle 2011-03-31 18:02 ` Jiri Bohac 2011-04-01 12:29 ` Jiri Bohac 2011-04-02 4:41 ` David Miller 2011-04-05 8:20 ` Jiri Bohac 2011-04-03 4:23 ` Ben Hutchings 2011-03-28 0:59 ` David Miller
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).