From f6f434b2822c453f898552537180a812538bd19e Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Tue, 23 Nov 2010 21:28:03 +0000 Subject: bgpd: Try fix extcommunity resource allocation probs, particularly with 'set extcom..' * Extended communities has some kind of resource allocation problem which causes a double-free if the 'set extcommunity ...' command is used. Try fix by properly interning extcommunities. Also, more generally, make unintern functions take a double pointer so they can NULL out callers references - a usefully defensive programming pattern for functions which make refs invalid. Sadly, this patch doesn't fix the problem entirely - crashes still occur on session clear. * bgp_ecommunity.h: (ecommunity_{free,unintern}) take double pointer args. * bgp_community.h: (community_unintern) ditto * bgp_attr.h: (bgp_attr_intern) ditto * bgp_aspath.h: (bgp_aspath.h) ditto * (general) update all callers of above * bgp_routemap.c: (route_set_ecommunity_{rt,soo}) intern the new extcom added to the attr, and unintern any old one. (route_set_ecommunity_{rt,soo}_compile) intern the extcom to be used for the route-map set. (route_set_ecommunity_*_free) unintern to match, instead of free (route_set_ecommunity_soo) Do as _rt does and don't just leak any pre-existing community, add to it (is additive right though?) --- bgpd/bgp_ecommunity.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) (limited to 'bgpd/bgp_ecommunity.c') diff --git a/bgpd/bgp_ecommunity.c b/bgpd/bgp_ecommunity.c index 8d5fa741..8d91c746 100644 --- a/bgpd/bgp_ecommunity.c +++ b/bgpd/bgp_ecommunity.c @@ -42,13 +42,14 @@ ecommunity_new (void) /* Allocate ecommunities. */ void -ecommunity_free (struct ecommunity *ecom) +ecommunity_free (struct ecommunity **ecom) { - if (ecom->val) - XFREE (MTYPE_ECOMMUNITY_VAL, ecom->val); - if (ecom->str) - XFREE (MTYPE_ECOMMUNITY_STR, ecom->str); - XFREE (MTYPE_ECOMMUNITY, ecom); + if ((*ecom)->val) + XFREE (MTYPE_ECOMMUNITY_VAL, (*ecom)->val); + if ((*ecom)->str) + XFREE (MTYPE_ECOMMUNITY_STR, (*ecom)->str); + XFREE (MTYPE_ECOMMUNITY, *ecom); + ecom = NULL; } /* Add a new Extended Communities value to Extended Communities @@ -197,7 +198,7 @@ ecommunity_intern (struct ecommunity *ecom) find = (struct ecommunity *) hash_get (ecomhash, ecom, hash_alloc_intern); if (find != ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); find->refcnt++; @@ -209,18 +210,18 @@ ecommunity_intern (struct ecommunity *ecom) /* Unintern Extended Communities Attribute. */ void -ecommunity_unintern (struct ecommunity *ecom) +ecommunity_unintern (struct ecommunity **ecom) { struct ecommunity *ret; - if (ecom->refcnt) - ecom->refcnt--; - + if ((*ecom)->refcnt) + (*ecom)->refcnt--; + /* Pull off from hash. */ - if (ecom->refcnt == 0) + if ((*ecom)->refcnt == 0) { /* Extended community must be in the hash. */ - ret = (struct ecommunity *) hash_release (ecomhash, ecom); + ret = (struct ecommunity *) hash_release (ecomhash, *ecom); assert (ret != NULL); ecommunity_free (ecom); @@ -516,7 +517,7 @@ ecommunity_str2com (const char *str, int type, int keyword_included) if (! keyword_included || keyword) { if (ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); return NULL; } keyword = 1; @@ -536,7 +537,7 @@ ecommunity_str2com (const char *str, int type, int keyword_included) if (! keyword) { if (ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); return NULL; } keyword = 0; @@ -549,7 +550,7 @@ ecommunity_str2com (const char *str, int type, int keyword_included) case ecommunity_token_unknown: default: if (ecom) - ecommunity_free (ecom); + ecommunity_free (&ecom); return NULL; } } -- cgit v1.2.1 From 94431dbc753171b48b5c6806af97fd690813b00a Mon Sep 17 00:00:00 2001 From: CROSS Date: Mon, 26 Sep 2011 13:17:05 +0400 Subject: bgpd: CVE-2011-3327 (ext. comm. buffer overflow) This vulnerability (CERT-FI #513254) was reported by CROSS project. They have also suggested a fix to the problem, which was found acceptable. The problem occurs when bgpd receives an UPDATE message containing 255 unknown AS_PATH attributes in Path Attribute Extended Communities. This causes a buffer overlow in bgpd. * bgp_ecommunity.c * ecommunity_ecom2str(): perform size check earlier --- bgpd/bgp_ecommunity.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'bgpd/bgp_ecommunity.c') diff --git a/bgpd/bgp_ecommunity.c b/bgpd/bgp_ecommunity.c index 8d91c746..440c15a4 100644 --- a/bgpd/bgp_ecommunity.c +++ b/bgpd/bgp_ecommunity.c @@ -620,6 +620,13 @@ ecommunity_ecom2str (struct ecommunity *ecom, int format) for (i = 0; i < ecom->size; i++) { + /* Make it sure size is enough. */ + while (str_pnt + ECOMMUNITY_STR_DEFAULT_LEN >= str_size) + { + str_size *= 2; + str_buf = XREALLOC (MTYPE_ECOMMUNITY_STR, str_buf, str_size); + } + /* Space between each value. */ if (! first) str_buf[str_pnt++] = ' '; @@ -663,13 +670,6 @@ ecommunity_ecom2str (struct ecommunity *ecom, int format) break; } - /* Make it sure size is enough. */ - while (str_pnt + ECOMMUNITY_STR_DEFAULT_LEN >= str_size) - { - str_size *= 2; - str_buf = XREALLOC (MTYPE_ECOMMUNITY_STR, str_buf, str_size); - } - /* Put string into buffer. */ if (encode == ECOMMUNITY_ENCODE_AS4) { -- cgit v1.2.1