On Sat, Oct 24, 2015 at 12:37:44PM -0600, Kevin Reay wrote:
> Adopt an updated version of the tcpdump.org ASN size calculation for
> BGP UPDATE message AS_PATHs. This corrects some bad behaviour due to
> incorrect ASN size calculations.
>
> I believe that the current way of calculating the ASN size for an
> UPDATE AS_PATH attribute is flawed.
>
> Currently, the ASN length (2 or 4 bytes) is calculated by dividing the
> total length of the AS_PATH attribute by the number of ASNs (p[1]) in
> the *first set encountered*:
>
> asn_bytes = (len-2)/p[1];
>
> The assumption that this first segment length describes the entire
> attribute is incorrect; there could be multiple path segment lengths
> in the attribute. The current method only uses the first encountered
> segment length (while using the entire attribute's byte length).
>
> This very often works fine, and when the calculation is incorrect the
> printf code only prints the first 2 bytes due to the way the if
> statements are structured, so it usually appears to work.
>
> (Sometimes the calculated ASN length isn't even 2 or 4 bytes, often
> causing 2 separate ASNs to be printed together as one ASN in ASDOT
> notation.)
>
> Example: here's the original OpenBSD output for a 2-byte encoded
> AS_PATH:
> (AS_PATH[T] 30)
> here's tcpdump.org 4.5.1 on Linux:
> AS Path (2), length: 10, Flags [T]: 30 { 10 20 }
> and here's OpenBSD with the changes in this patch:
> (AS_PATH[T] 30 {10 20})
>
> A comment in the original code claims:
> ...
> * 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)
>
> This incorrectly describes the calculation being performed; the length
> being compared isn't just the length of the "path segment value" but of
> the entire attribute.
>
> I've attached an updated version of the upstream's heuristics function
> for calculating the ASN size: bgp_attr_get_as_size(). The updated
> version contains style(9) changes, simplifications, an additional
> validity check (non-zero segment lengths), and comment re-writes.
>
> I'm interested in hearing any feedback. I'm also interested if anyone
> with experience using tcpdump with BGP packets can report seeing the
> incorrect behaviour described here in the past.
>
> Index: print-bgp.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/tcpdump/print-bgp.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 print-bgp.c
> --- print-bgp.c 20 Oct 2015 11:29:07 -0000 1.18
> +++ print-bgp.c 24 Oct 2015 18:21:55 -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,55 @@ trunc:
> }
> #endif
>
> +/*
> + * Try to determine the size of the ASs encoded in an AS-path. It is
> + * not obvious as both speaker types exchange AS-paths with the same
> + * path-attribute type.
> + */
> +static int
> +bgp_attr_get_as_size(u_int8_t bgpa_type, const u_char *dat, int len)
> +{
> + const u_char *p;
> +
> + p = dat;
> +
> + /* AS4 path types are always encoded in 4-byte format */
> + if (bgpa_type == BGPTYPE_AS4_PATH) {
> + return 4;
> + }
> +
> + /*
> + * Start by assuming 2-byte ASs. Iterate through the path data using the
> + * segment length values. Switch to 4-bytes if we encounter an invalid
> + * field value/if the AS-Path length is invalid for the assumed size.
> + */
I wonder if it would not be smarter to check the other way around. As in
start with 4 byte and see if we overflow. I would guess that this would
happen in most cases on the first segment for 2 byte AS pathes.
> + while (p < dat + len) {
> + TCHECK(p[0]);
> +
> + /* check segment type: invalid value means wrong size */
> + if (p[0] < BGP_AS_SEG_TYPE_MIN || p[0] > BGP_AS_SEG_TYPE_MAX)
> + goto trunc;
> +
> + TCHECK(p[1]);
> +
> + /* check segment length: invalid indicates wrong size */
> + if (p[1] == 0)
> + goto trunc;
> +
> + p += 2 + p[1] * 2;
> + }
> +
> + /* matching length: it's very likely the ASs were encoded as 2-bytes */
> + if (p == dat + len)
> + return 2;
> +trunc:
> + /*
> + * Either there was not enough data or we tried to decode 4-byte ASs
> + * with an incorrect size of 2-bytes.
> + */
> + return 4;
> +}
> +
> static int
> bgp_attr_print(const struct bgp_attr *attr, const u_char *dat, int len)
> {
> @@ -425,17 +477,7 @@ 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;
> @@ -445,15 +487,13 @@ bgp_attr_print(const struct bgp_attr *at
> printf(" empty");
> break;
> }
> + asn_bytes = bgp_attr_get_as_size(attr->bgpa_type, dat, len);
> while (p < dat + len) {
> TCHECK2(p[0], 2);
> - if (asn_bytes == 0) {
> - if (p[1] == 0) {
> - /* invalid: segment contains one or more AS */
> - printf(" malformed");
> - break;
> - }
> - asn_bytes = (len-2)/p[1];
> + if (p[1] == 0) {
> + /* invalid: segment contains no AS */
> + printf(" malformed");
> + break;
> }
> printf("%s",
> tok2str(bgp_as_path_segment_open_values,
--
:wq Claudio
