Hello list!
I've got several questions after some attribute code digging:
1) We're calling custom formatting function bgp_get_attr() with wrong
lentgh (attribute name is not taken into account).
This probably never will cause buffer overflow, but still: if we're
passing buffer length it should be real length.
Possible simple fix is in bgp_get_attr.diff
2) We know that either no extended attributes (route withdraw) or a pack
(ORIGIN, AS_PATH, NEXTHOP) of mandatory attributes
(and possibly some other) are passed within BGP UPDATE message. However
we're allocating ea_list
with single extended attribute on every BGP attribute we parse in
bgp_decode_attr(). Maybe we can pre-allocate more than one
attribute in ea_list ? Example implementation can be found in attached
bgp_attr_ea.diff
--
Alexander V. Chernikov
Yandex NOC
Index: proto/bgp/attrs.c
===================================================================
--- proto/bgp/attrs.c (revision 5017)
+++ proto/bgp/attrs.c (working copy)
@@ -1518,18 +1518,19 @@ err:
int
bgp_get_attr(eattr *a, byte *buf, int buflen)
{
- unsigned int i = EA_ID(a->id);
+ unsigned int len, i = EA_ID(a->id);
struct attr_desc *d;
if (ATTR_KNOWN(i))
{
d = &bgp_attr_table[i];
- buf += bsprintf(buf, "%s", d->name);
- if (d->format)
+ len = bsprintf(buf, "%s", d->name);
+ buf += len;
+ if (d->format && (len > 2))
{
*buf++ = ':';
*buf++ = ' ';
- d->format(a, buf, buflen);
+ d->format(a, buf, buflen - len - 2);
return GA_FULL;
}
return GA_NAME;
Index: proto/bgp/attrs.c
===================================================================
--- proto/bgp/attrs.c (revision 5033)
+++ proto/bgp/attrs.c (working copy)
@@ -1410,9 +1410,9 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr
int errcode;
byte *z, *attr_start;
byte seen[256/8];
- ea_list *ea;
+ ea_list *ea = NULL;
struct adata *ad;
- int withdraw = 0;
+ int withdraw = 0, ea_count = 0;
bzero(a, sizeof(rta));
a->proto = &bgp->p;
@@ -1499,20 +1499,25 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr
// { errcode = 4; goto err; }
seen[code/8] |= (1 << (code%8));
- ea = lp_alloc(pool, sizeof(ea_list) + sizeof(eattr));
- ea->next = a->eattrs;
- a->eattrs = ea;
- ea->flags = 0;
- ea->count = 1;
- ea->attrs[0].id = EA_CODE(EAP_BGP, code);
- ea->attrs[0].flags = flags;
- ea->attrs[0].type = type;
+ if (!ea || (ea_count >= (5 - 1)))
+ {
+ ea = lp_alloc(pool, sizeof(ea_list) + sizeof(eattr) * 5);
+ ea->next = a->eattrs;
+ a->eattrs = ea;
+ ea->flags = 0;
+ ea->count = 0;
+ ea_count = 0;
+ }
+
+ ea->attrs[ea_count].id = EA_CODE(EAP_BGP, code);
+ ea->attrs[ea_count].flags = flags;
+ ea->attrs[ea_count].type = type;
if (type & EAF_EMBEDDED)
ad = NULL;
else
{
ad = lp_alloc(pool, sizeof(struct adata) + l);
- ea->attrs[0].u.ptr = ad;
+ ea->attrs[ea_count].u.ptr = ad;
ad->length = l;
memcpy(ad->data, z, l);
}
@@ -1521,9 +1526,9 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr
case EAF_TYPE_ROUTER_ID:
case EAF_TYPE_INT:
if (l == 1)
- ea->attrs[0].u.data = *z;
+ ea->attrs[ea_count].u.data = *z;
else
- ea->attrs[0].u.data = get_u32(z);
+ ea->attrs[ea_count].u.data = get_u32(z);
break;
case EAF_TYPE_IP_ADDRESS:
ipa_ntoh(*(ip_addr *)ad->data);
@@ -1537,6 +1542,9 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr
break;
}
}
+
+ ea_count++;
+ ea->count++;
}
if (withdraw)
