From cddb8112b80fa9867156c637d63e6e79eeac67bb Mon Sep 17 00:00:00 2001 From: Chris Hall Date: Mon, 9 Aug 2010 22:31:37 +0400 Subject: bgpd: fix handling of AS path data * bgpd/bgp_aspath.c * assegments_parse(): add handling of AS4_PATH input, update bounds checks, add check for AS segment type * aspath_parse(): add handling of AS4_PATH input, expect assegments_parse() to do length checking * aspath_empty(): update for the new function prototype * bgpd/bgp_aspath.h: ditto * tests/aspath_test.c: ditto * bgpd/bgp_attr.c * bgp_attr_aspath(): add handling of AS4_PATH input, update flags checks, change returned type * bgp_attr_as4_path(): discard, superseded by bgp_attr_aspath() * bgp_attr_parse(): update respectively --- bgpd/bgp_aspath.c | 115 ++++++++++++++++++++++++++++++++-------------------- bgpd/bgp_aspath.h | 2 +- bgpd/bgp_attr.c | 103 +++++++++++++++++++++++++--------------------- tests/aspath_test.c | 2 +- 4 files changed, 131 insertions(+), 91 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index a9602d90..5a73eeff 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -671,58 +671,79 @@ aspath_hash_alloc (void *arg) return aspath; } -/* parse as-segment byte stream in struct assegment */ +/* parse as-segment byte stream in struct assegment + * + * Returns NULL if the AS_PATH or AS4_PATH is not valid. + */ static struct assegment * -assegments_parse (struct stream *s, size_t length, int use32bit) +assegments_parse (struct stream *s, size_t length, int use32bit, int as4_path) { struct assegment_header segh; struct assegment *seg, *prev = NULL, *head = NULL; - size_t bytes = 0; - /* empty aspath (ie iBGP or somesuch) */ - if (length == 0) - return NULL; + assert (length > 0); /* does not expect empty AS_PATH or AS4_PATH */ if (BGP_DEBUG (as4, AS4_SEGMENT)) zlog_debug ("[AS4SEG] Parse aspath segment: got total byte length %lu", (unsigned long) length); - /* basic checks */ - if ( (STREAM_READABLE(s) < length) - || (STREAM_READABLE(s) < AS_HEADER_SIZE) - || (length % AS16_VALUE_SIZE )) + + /* double check that length does not exceed stream */ + if (STREAM_READABLE(s) < length) return NULL; - while ( (STREAM_READABLE(s) > AS_HEADER_SIZE) - && (bytes < length)) + /* deal with each segment in turn */ + while (length > 0) { int i; - int seg_size; + size_t seg_size; /* softly softly, get the header first on its own */ + if (length >= AS_HEADER_SIZE) + { segh.type = stream_getc (s); segh.length = stream_getc (s); seg_size = ASSEGMENT_SIZE(segh.length, use32bit); + /* includes the header bytes */ if (BGP_DEBUG (as4, AS4_SEGMENT)) zlog_debug ("[AS4SEG] Parse aspath segment: got type %d, length %d", segh.type, segh.length); - /* check it.. */ - if ( ((bytes + seg_size) > length) - /* 1771bis 4.3b: seg length contains one or more */ - || (segh.length == 0) - /* Paranoia in case someone changes type of segment length. - * Shift both values by 0x10 to make the comparison operate - * on more, than 8 bits (otherwise it's a warning, bug #564). + switch (segh.type) + { + case AS_SEQUENCE: + case AS_SET: + break ; + + case AS_CONFED_SEQUENCE: + case AS_CONFED_SET: + if (!as4_path) + break ; + /* RFC4893 3: "invalid for the AS4_PATH attribute" */ + /* fall through */ + + default: /* reject unknown or invalid AS_PATH segment types */ + seg_size = 0 ; + } ; + } + else + seg_size = 0 ; + + /* Stop now if segment is not valid (discarding anything collected to date) + * + * RFC4271 4.3, Path Attributes, b) AS_PATH: + * + * "path segment value field contains one or more AS numbers" */ - || ((sizeof segh.length > 1) && (0x10 + segh.length > 0x10 + AS_SEGMENT_MAX)) ) + if ((seg_size == 0) || (seg_size > length) || (segh.length == 0)) { - if (head) assegment_free_all (head); return NULL; } + length -= seg_size ; + /* now its safe to trust lengths */ seg = assegment_new (segh.type, segh.length); @@ -734,11 +755,9 @@ assegments_parse (struct stream *s, size_t length, int use32bit) for (i = 0; i < segh.length; i++) seg->as[i] = (use32bit) ? stream_getl (s) : stream_getw (s); - bytes += seg_size; - if (BGP_DEBUG (as4, AS4_SEGMENT)) - zlog_debug ("[AS4SEG] Parse aspath segment: Bytes now: %lu", - (unsigned long) bytes); + zlog_debug ("[AS4SEG] Parse aspath segment: length left: %lu", + (unsigned long) length); prev = seg; } @@ -746,30 +765,42 @@ assegments_parse (struct stream *s, size_t length, int use32bit) return assegment_normalise (head); } -/* AS path parse function. pnt is a pointer to byte stream and length - is length of byte stream. If there is same AS path in the the AS - path hash then return it else make new AS path structure. */ +/* AS path parse function -- parses AS_PATH and AS4_PATH attributes + * + * Requires: s -- stream, currently positioned before first segment + * of AS_PATH or AS4_PATH (ie after attribute header) + * length -- length of the value of the AS_PATH or AS4_PATH + * use32bit -- true <=> 4Byte ASN, otherwise 2Byte ASN + * as4_path -- true <=> AS4_PATH, otherwise AS_PATH + * + * Returns: if valid: address of struct aspath in the hash of known aspaths, + * with reference count incremented. + * else: NULL + * + * NB: empty AS path (length == 0) is valid. The returned struct aspath will + * have segments == NULL and str == zero length string (unique). + */ struct aspath * -aspath_parse (struct stream *s, size_t length, int use32bit) +aspath_parse (struct stream *s, size_t length, int use32bit, int as4_path) { struct aspath as; struct aspath *find; - /* If length is odd it's malformed AS path. */ - /* Nit-picking: if (use32bit == 0) it is malformed if odd, - * otherwise its malformed when length is larger than 2 and (length-2) - * is not dividable by 4. - * But... this time we're lazy - */ - if (length % AS16_VALUE_SIZE ) - return NULL; - + /* Parse each segment and construct normalised list of struct assegment */ memset (&as, 0, sizeof (struct aspath)); - as.segments = assegments_parse (s, length, use32bit); + if (length != 0) + { + as.segments = assegments_parse (s, length, use32bit, as4_path); + + if (as.segments == NULL) + return NULL ; /* Invalid AS_PATH or AS4_PATH */ + } ; /* If already same aspath exist then return it. */ find = hash_get (ashash, &as, aspath_hash_alloc); + assert(find) ; /* valid aspath, so must find or create */ + /* aspath_hash_alloc dupes segments too. that probably could be * optimised out. */ @@ -777,8 +808,6 @@ aspath_parse (struct stream *s, size_t length, int use32bit) if (as.str) XFREE (MTYPE_AS_STR, as.str); - if (! find) - return NULL; find->refcnt++; return find; @@ -1602,7 +1631,7 @@ aspath_segment_add (struct aspath *as, int type) struct aspath * aspath_empty (void) { - return aspath_parse (NULL, 0, 1); /* 32Bit ;-) */ + return aspath_parse (NULL, 0, 1, 0); /* 32Bit ;-) not AS4_PATH */ } struct aspath * diff --git a/bgpd/bgp_aspath.h b/bgpd/bgp_aspath.h index b8a5dfab..d63b914c 100644 --- a/bgpd/bgp_aspath.h +++ b/bgpd/bgp_aspath.h @@ -65,7 +65,7 @@ struct aspath /* Prototypes. */ extern void aspath_init (void); extern void aspath_finish (void); -extern struct aspath *aspath_parse (struct stream *, size_t, int); +extern struct aspath *aspath_parse (struct stream *, size_t, int, int); extern struct aspath *aspath_dup (struct aspath *); extern struct aspath *aspath_aggregate (struct aspath *, struct aspath *); extern struct aspath *aspath_prepend (struct aspath *, struct aspath *); diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 5e7536ae..3996afd3 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -807,54 +807,78 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, return 0; } - -/* Parse AS path information. This function is wrapper of - aspath_parse. */ -static int +/* Parse AS path information. This function is wrapper of aspath_parse. + * + * Parses AS_PATH or AS4_PATH. + * + * Returns: if valid: address of struct aspath in the hash of known aspaths, + * with reference count incremented. + * else: NULL + * + * NB: empty AS path (length == 0) is valid. The returned struct aspath will + * have segments == NULL and str == zero length string (unique). + */ +static struct aspath * bgp_attr_aspath (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) + struct attr *attr, u_char flag, u_char *startp, int as4_path) { - bgp_size_t total; + u_char require ; + struct aspath *asp ; - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Check the attribute flags */ + require = as4_path ? BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS + : BGP_ATTR_FLAG_TRANS ; - /* Flag check. */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) - || ! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + if ((flag & (BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS)) != require) { + const char* path_type ; + bgp_size_t total; + + path_type = as4_path ? "AS4_PATH" : "AS_PATH" ; + + if (!CHECK_FLAG(flag, BGP_ATTR_FLAG_TRANS)) zlog (peer->log, LOG_ERR, - "As-Path attribute flag isn't transitive %d", flag); + "%s attribute flag isn't transitive %d", path_type, flag) ; + + if ((flag & BGP_ATTR_FLAG_OPTIONAL) != (require & BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, + "%s attribute flag must %sbe optional %d", path_type, + (flag & BGP_ATTR_FLAG_OPTIONAL) ? "not " : "", flag) ; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; - } - /* - * peer with AS4 => will get 4Byte ASnums - * otherwise, will get 16 Bit + return NULL ; + } ; + + /* Parse the AS_PATH/AS4_PATH body. + * + * For AS_PATH peer with AS4 => 4Byte ASN otherwise 2Byte ASN + * AS4_PATH 4Byte ASN */ - attr->aspath = aspath_parse (peer->ibuf, length, - CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)); + asp = aspath_parse (peer->ibuf, length, + as4_path || CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV), as4_path) ; - /* In case of IBGP, length will be zero. */ - if (! attr->aspath) + if (asp != NULL) + { + attr->flag |= ATTR_FLAG_BIT (as4_path ? BGP_ATTR_AS4_PATH + : BGP_ATTR_AS_PATH) ; + } + else { zlog (peer->log, LOG_ERR, "Malformed AS path length is %d", length); + + /* TODO: should BGP_NOTIFY_UPDATE_MAL_AS_PATH be sent for AS4_PATH ?? */ bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_AS_PATH); - return -1; - } - - /* Forward pointer. */ -/* stream_forward_getp (peer->ibuf, length);*/ - - /* Set aspath attribute flag. */ - attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS_PATH); + } ; - return 0; + return asp ; } static int bgp_attr_aspath_check( struct peer *peer, @@ -912,21 +936,6 @@ static int bgp_attr_aspath_check( struct peer *peer, } -/* Parse AS4 path information. This function is another wrapper of - aspath_parse. */ -static int -bgp_attr_as4_path (struct peer *peer, bgp_size_t length, - struct attr *attr, struct aspath **as4_path) -{ - *as4_path = aspath_parse (peer->ibuf, length, 1); - - /* Set aspath attribute flag. */ - if (as4_path) - attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH); - - return 0; -} - /* Nexthop attribute. */ static int bgp_attr_nexthop (struct peer *peer, bgp_size_t length, @@ -1657,10 +1666,12 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_origin (peer, length, attr, flag, startp); break; case BGP_ATTR_AS_PATH: - ret = bgp_attr_aspath (peer, length, attr, flag, startp); + attr->aspath = bgp_attr_aspath (peer, length, attr, flag, startp, 0); + ret = attr->aspath ? 0 : -1 ; break; case BGP_ATTR_AS4_PATH: - ret = bgp_attr_as4_path (peer, length, attr, &as4_path ); + as4_path = bgp_attr_aspath (peer, length, attr, flag, startp, 1); + ret = as4_path ? 0 : -1 ; break; case BGP_ATTR_NEXT_HOP: ret = bgp_attr_nexthop (peer, length, attr, flag, startp); diff --git a/tests/aspath_test.c b/tests/aspath_test.c index fb504f31..9e51e8dd 100644 --- a/tests/aspath_test.c +++ b/tests/aspath_test.c @@ -649,7 +649,7 @@ make_aspath (const u_char *data, size_t len, int use32bit) s = stream_new (len); stream_put (s, data, len); } - as = aspath_parse (s, len, use32bit); + as = aspath_parse (s, len, use32bit, 0); if (s) stream_free (s); -- cgit v1.2.1