On 2015/10/13 18:21, Kevin Reay wrote: > Fix a segfault when printing a malformed BGP AS_PATH update due to ASN > extraction.
There are line-wrapping and whitespace issues in this patch. > Better AS size extraction from AS paths: better heuristics (see > bgp_attr_get_as_size). That makes sense though the English in the comments relating to this change is very awkward. > Also fixes output support for 4-byte ASNs. For example; > (AS_PATH[T] {500.500 513.65211}) > becomes: > (AS_PATH[T] {500 500} 65211) This example seems very wrong. I think what the diff is doing in this area is switching from ASDOT to ASPLAIN which is alright (though it should be a separate diff - one change, one diff - and I don't regard it as "fix" but "change to the more common format") ... but that doesn't match this example at all, which appears to be doing something strange with as-sets. > Collapses bgp_attr_print switch 2-byte and 4-byte path cases. > > bgp_attr_get_as_size function copied from tcpdump Git repo master. The formatting of this function doesn't match style(9) or the code in the rest of this file. > This brings behavior in-line with newer versions on Linux. > > Finally, minor output tweak: add a space after BGP update attribute > ORIGINATOR_ID's flags to make consistent with other attributes. > > > Index: print-bgp.c > =================================================================== > RCS file: /cvs/src/usr.sbin/tcpdump/print-bgp.c,v > retrieving revision 1.17 > diff -u -p -r1.17 print-bgp.c > --- print-bgp.c 16 Jan 2015 06:40:21 -0000 1.17 > +++ print-bgp.c 14 Oct 2015 00:15:13 -0000 > @@ -140,6 +140,9 @@ struct bgp_attr { > #define BGP_CONFED_AS_SEQUENCE 3 /* draft-ietf-idr-rfc3065bis-01 */ > #define BGP_CONFED_AS_SET 4 /* draft-ietf-idr-rfc3065bis-01 */ > > +#define BGP_AS_SEG_TYPE_MIN BGP_AS_SET > +#define BGP_AS_SEG_TYPE_MAX BGP_CONFED_AS_SET > + > static struct tok bgp_as_path_segment_open_values[] = { > { BGP_AS_SET, " {" }, > { BGP_AS_SEQUENCE, " " }, > @@ -400,6 +403,63 @@ trunc: > } > #endif > > +/* > + * bgp_attr_get_as_size > + * > + * Try to find the size of the ASs encoded in an as-path. It is not obvious, > as > + * both Old speakers that do not support 4 byte AS, and the new speakers > that do > + * support, exchange AS-Path with the same path-attribute type value 0x02. > + */ > +static int > +bgp_attr_get_as_size (u_int8_t bgpa_type, const u_char *pptr, int len) > +{ > + const u_char *tptr = pptr; > + > + /* > + * If the path attribute is the optional AS4 path type, then we already > + * know, that ASs must be encoded in 4 byte format. > + */ > + if (bgpa_type == BGPTYPE_AS4_PATH) { > + return 4; > + } > + > + /* > + * Let us assume that ASs are of 2 bytes in size, and check if the > AS-Path > + * TLV is good. If not, ask the caller to try with AS encoded as 4 bytes > + * each. > + */ > + while (tptr < pptr + len) { > + TCHECK(tptr[0]); > + > + /* > + * If we do not find a valid segment type, our guess might be wrong. > + */ > + if (tptr[0] < BGP_AS_SEG_TYPE_MIN || tptr[0] > BGP_AS_SEG_TYPE_MAX) { > + goto trunc; > + } > + TCHECK(tptr[1]); > + tptr += 2 + tptr[1] * 2; > + } > + > + /* > + * If we correctly reached end of the AS path attribute data content, > + * then most likely ASs were indeed encoded as 2 bytes. > + */ > + if (tptr == pptr + len) { > + return 2; > + } > + > +trunc: > + > + /* > + * We can come here, either we did not have enough data, or if we > + * try to decode 4 byte ASs in 2 byte format. Either way, return 4, > + * so that calller can try to decode each AS as of 4 bytes. If indeed > + * there was not enough data, it will crib and end the parse anyways. > + */ > + return 4; > +} > + > static int > bgp_attr_print(const struct bgp_attr *attr, const u_char *dat, int len) > { > @@ -413,7 +473,6 @@ bgp_attr_print(const struct bgp_attr *at > > p = dat; > tlen = len; > - asn_bytes = 0; > > switch (attr->bgpa_type) { > case BGPTYPE_ORIGIN: > @@ -425,17 +484,8 @@ bgp_attr_print(const struct bgp_attr *at > } > break; > case BGPTYPE_AS4_PATH: > - asn_bytes = 4; > /* FALLTHROUGH */ > case BGPTYPE_AS_PATH: > - /* > - * 2-byte speakers will receive AS4_PATH as well AS_PATH (2-byte). > - * 4-byte speakers will only receive AS_PATH but it will be 4-byte. > - * To identify which is the case, compare the length of the path > - * segment value in bytes, with the path segment length from the > - * message (counted in # of AS) > - */ > - > if (len % 2) { > printf(" invalid len"); > break; > @@ -444,11 +494,19 @@ bgp_attr_print(const struct bgp_attr *at > printf(" empty"); > break; > } > + > + /* > + * BGP updates exchanged between New speakers that support 4 > + * byte AS, ASs are always encoded in 4 bytes. There is no > + * definitive way to find this, just by the packet's > + * contents. So, check for packet's TLV's sanity assuming > + * 2 bytes first, and it does not pass, assume that ASs are > + * encoded in 4 bytes format and move on. > + */ > + asn_bytes = bgp_attr_get_as_size(attr->bgpa_type, dat, len); > + > while (p < dat + len) { > TCHECK(p[0]); > - if (asn_bytes == 0) { > - asn_bytes = (len-2)/p[1]; > - } > printf("%s", > tok2str(bgp_as_path_segment_open_values, > "?", p[0])); > @@ -456,13 +514,10 @@ bgp_attr_print(const struct bgp_attr *at > for (i = 0; i < p[1] * asn_bytes; i += asn_bytes) { > TCHECK2(p[2 + i], asn_bytes); > printf("%s", i == 0 ? "" : " "); > - if (asn_bytes == 2 || EXTRACT_16BITS(&p[2 + i])) > - printf("%u%s", > - EXTRACT_16BITS(&p[2 + i]), > - asn_bytes == 4 ? "." : ""); > - if (asn_bytes == 4) > - printf("%u", > - EXTRACT_16BITS(&p[2 + i + 2])); > + printf("%u", > + asn_bytes == 2 ? > + EXTRACT_16BITS(&p[2 + i]) : > + EXTRACT_32BITS(&p[2 + i])); > } > TCHECK(p[0]); > printf("%s", > @@ -549,7 +604,7 @@ bgp_attr_print(const struct bgp_attr *at > break; > } > TCHECK2(p[0], 4); > - printf("%s",getname(p)); > + printf(" %s", getname(p)); > break; > case BGPTYPE_CLUSTER_LIST: > if (len % 4) { >