Re: [patch] tcpdump - better BGP UPDATE AS_PATH size calculations
On Tue, Oct 27, 2015 at 12:59:20AM -0600, Kevin Reay wrote:
> I did add an additional check for "zero" ASNs to the 2-byte default,
> inspired by a quick glance at Wireshark's heuristics. I now flip
> through each segment's ASNs inside of bgp_attr_get_as_size(), looking
> for any zero-value 16bit ASNs that would indicate the set isn't valid
> for the assumed byte-length (ie. the first two bytes of a 4-byte ASN).
> I'd need to read through the RFCs in detail to ensure this is witout
> issue though.
I've attached the updated version of the previous tcpdump bgp AS_PATH
size discovery function.
It uses the original 2-byte size assumption with the addition of a
check for any zero-value ASNs. After some research, it appears this is
a valid approach according to RFC7607 (they aren't valid in an
AS_PATH).
The additional check appears to further improve the success in my test
cases as well as obviously correcting the original in-tree behaviour.
Any feedback would be great.
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 - 1.18
+++ print-bgp.c 29 Oct 2015 20:48:06 -
@@ -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_MINBGP_AS_SET
+#define BGP_AS_SEG_TYPE_MAXBGP_CONFED_AS_SET
+
static struct tok bgp_as_path_segment_open_values[] = {
{ BGP_AS_SET, " {" },
{ BGP_AS_SEQUENCE, " " },
@@ -400,6 +403,64 @@ 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;
+ int i;
+
+ 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.
+*/
+ 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;
+
+ TCHECK2(p[1], 1 + p[1] * 2);
+
+ /* check segment length: invalid indicates wrong size */
+ if (p[1] == 0)
+ goto trunc;
+
+ /*
+* Look for zero-value ASs which are likely the first 2 bytes of
+* a 4-byte AS. RFC7607 asserts a zero in an AS_PATH is invalid.
+*/
+ for (i = 0; i < p[1]; i++) {
+ if (EXTRACT_16BITS([2 + i * 2]) == 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 +486,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 +496,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");
-
Re: [patch] tcpdump - better BGP UPDATE AS_PATH size calculations
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 - 1.18
> +++ print-bgp.c 24 Oct 2015 18:21:55 -
> @@ -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_MINBGP_AS_SET
> +#define BGP_AS_SEG_TYPE_MAXBGP_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)
[patch] tcpdump - better BGP UPDATE AS_PATH size calculations
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 - 1.18
+++ print-bgp.c 24 Oct 2015 18:21:55 -
@@ -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_MINBGP_AS_SET
+#define BGP_AS_SEG_TYPE_MAXBGP_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.
+*/
+ 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
