From: Paul Jakma Date: Tue, 23 Sep 2014 14:23:01 +0000 (+0100) Subject: bgpd: well-known attr check only run for v4/uni, which could cause a crash. X-Git-Tag: frr-2.0-rc1~676 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=bb7bef1490138c464ffe5f065a5f3755fc6e1b6f;p=matthieu%2Ffrr.git bgpd: well-known attr check only run for v4/uni, which could cause a crash. * ANVL testing by Martin Winter threw up a crash in bgpd in aspath_dup called from bgp_packet_attribute, if attr->aspath was NULL, on an IPv6 UPDATE. This root cause is that the checks for well-known, mandatory attributes were being applied only if an UPDATE contained the IPv4 NLRI and the peer was configured for v4/unicast (i.e. not deconfigured). This is something inherited from GNU Zebra, and never noticed before. * bgp_attr.c: (bgp_attr_parse) Move the well-known mandatory attribute check to here, so that it can be run immediately after all attributes are parsed, and before any further processing of attributes that might assume the existence of WK/M attributes (e.g. AS4-Path). (bgp_attr_munge_as4_attrs) Missing AS_PATH shouldn't happen here anymore, but retain a check anyway for robustness - it's definitely a hard error though. * bgp_attr.h: (bgp_attr_check) No longer needs to be exported, make static. * bgp_packet.c: (bgp_update_receive) Responsibility for well-known check now in bgp_attr_parse. (cherry picked from commit 055086f70febc30fdfd94bb4406e9075d6934cd8) Conflicts: bgpd/bgp_attr.c bgpd/bgp_attr.h bgpd/bgp_packet.c --- diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index c2c86e51a2..646d16200f 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1359,7 +1359,17 @@ bgp_attr_munge_as4_attrs (struct peer *const peer, int ignore_as4_path = 0; struct aspath *newpath; struct attr_extra *attre = attr->extra; - + + if (!attr->aspath) + { + /* NULL aspath shouldn't be possible as bgp_attr_parse should have + * checked that all well-known, mandatory attributes were present. + * + * Can only be a problem with peer itself - hard error + */ + return BGP_ATTR_PARSE_ERROR; + } + if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) { /* peer can do AS4, so we ignore AS4_PATH and AS4_AGGREGATOR @@ -1439,12 +1449,9 @@ bgp_attr_munge_as4_attrs (struct peer *const peer, /* need to reconcile NEW_AS_PATH and AS_PATH */ if (!ignore_as4_path && (attr->flag & (ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH)))) { - if (!attr->aspath) - return BGP_ATTR_PARSE_PROCEED; - - newpath = aspath_reconcile_as4 (attr->aspath, as4_path); - aspath_unintern (&attr->aspath); - attr->aspath = aspath_intern (newpath); + newpath = aspath_reconcile_as4 (attr->aspath, as4_path); + aspath_unintern (&attr->aspath); + attr->aspath = aspath_intern (newpath); } return BGP_ATTR_PARSE_PROCEED; } @@ -1808,11 +1815,48 @@ bgp_attr_unknown (struct bgp_attr_parser_args *args) return BGP_ATTR_PARSE_PROCEED; } +/* Well-known attribute check. */ +static int +bgp_attr_check (struct peer *peer, struct attr *attr, bgp_size_t nlri_len) +{ + u_char type = 0; + + if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_ORIGIN))) + type = BGP_ATTR_ORIGIN; + + if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_AS_PATH))) + type = BGP_ATTR_AS_PATH; + + /* As per RFC 2858, NEXT_HOP is needed only if the update contains NLRI + * other than in MP_REACH. In fact, if only MP_REACH_NLRI is present, the + * update should not contain NEXT_HOP but it should be ignored, if recvd. + */ + if (nlri_len) + if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP))) + type = BGP_ATTR_NEXT_HOP; + + if (peer->sort == BGP_PEER_IBGP + && ! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF))) + type = BGP_ATTR_LOCAL_PREF; + + if (type) + { + zlog_warn ("%s Missing well-known attribute %d.", peer->host, type); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_MISS_ATTR, + &type, 1); + return BGP_ATTR_PARSE_ERROR; + } + return BGP_ATTR_PARSE_PROCEED; +} + /* Read attribute of update packet. This function is called from bgp_update_receive() in bgp_packet.c. */ bgp_attr_parse_ret_t bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, - struct bgp_nlri *mp_update, struct bgp_nlri *mp_withdraw) + struct bgp_nlri *mp_update, struct bgp_nlri *mp_withdraw, + bgp_size_t nlri_len) { int ret; u_char flag = 0; @@ -2048,6 +2092,17 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, return BGP_ATTR_PARSE_ERROR; } + /* Check all mandatory well-known attributes are present */ + { + bgp_attr_parse_ret_t ret; + if ((ret = bgp_attr_check (peer, attr, nlri_len)) < 0) + { + if (as4_path) + aspath_unintern (&as4_path); + return ret; + } + } + /* * At this place we can see whether we got AS4_PATH and/or * AS4_AGGREGATOR from a 16Bit peer and act accordingly. @@ -2108,42 +2163,6 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, return BGP_ATTR_PARSE_PROCEED; } -/* Well-known attribute check. */ -int -bgp_attr_check (struct peer *peer, struct attr *attr, bgp_size_t nlri_len) -{ - u_char type = 0; - - if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_ORIGIN))) - type = BGP_ATTR_ORIGIN; - - if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_AS_PATH))) - type = BGP_ATTR_AS_PATH; - - /* As per RFC 2858, NEXT_HOP is needed only if the update contains NLRI - * other than in MP_REACH. In fact, if only MP_REACH_NLRI is present, the - * update should not contain NEXT_HOP but it should be ignored, if recvd. - */ - if (nlri_len) - if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP))) - type = BGP_ATTR_NEXT_HOP; - - if (peer->sort == BGP_PEER_IBGP - && ! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF))) - type = BGP_ATTR_LOCAL_PREF; - - if (type) - { - zlog_warn ("%s Missing well-known attribute %d.", peer->host, type); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MISS_ATTR, - &type, 1); - return BGP_ATTR_PARSE_ERROR; - } - return BGP_ATTR_PARSE_PROCEED; -} - size_t bgp_packet_mpattr_start (struct stream *s, afi_t afi, safi_t safi, afi_t nh_afi, struct bpacket_attr_vec_arr *vecarr, diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index 5a5492aad4..a9e8b4f5d6 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -171,8 +171,7 @@ extern void bgp_attr_init (void); extern void bgp_attr_finish (void); extern bgp_attr_parse_ret_t bgp_attr_parse (struct peer *, struct attr *, bgp_size_t, struct bgp_nlri *, - struct bgp_nlri *); -extern int bgp_attr_check (struct peer *, struct attr *, bgp_size_t); + struct bgp_nlri *, bgp_size_t); extern struct attr_extra *bgp_attr_extra_get (struct attr *); extern void bgp_attr_extra_free (struct attr *); extern void bgp_attr_dup (struct attr *, struct attr *); diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 025cfbaeeb..e2b8a3346a 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1430,7 +1430,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (attribute_len) { attr_parse_ret = bgp_attr_parse (peer, &attr, attribute_len, - &mp_update, &mp_withdraw); + &mp_update, &mp_withdraw, update.length); if (attr_parse_ret == BGP_ATTR_PARSE_ERROR) { bgp_attr_unintern_sub (&attr); @@ -1483,22 +1483,6 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) peer->host, withdraw_len, num_pfx_wd, attribute_len, update_len, num_pfx_adv); - /* - * Validate for well-known mandatory attributes. We only do this if - * we intend to process the update further - i.e., the AFI/SAFI is - * enabled. - */ - if ((update.length && peer->afc[AFI_IP][SAFI_UNICAST]) || - (mp_update.length && peer->afc[mp_update.afi][mp_update.safi])) - { - ret = bgp_attr_check (peer, &attr, update.length); - if (ret < 0) - { - bgp_attr_unintern_sub (&attr); - return -1; - } - } - /* NLRI is processed only when the the corresponding address-family * has been negotiated with the peer. */