On Sun, Oct 25, 2015 at 09:36:20AM +0100, Claudio Jeker wrote: > 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.
Thanks for having a look. A 4-byte first pass made sense to me; it does seem probable that it would increase the chance of triggering an overflow if the ASNs aren't the assumed length. I quickly implemented the change to default to 4-bytes and it seems to work well but it did break some of my testing pcaps. A few test cases (with 2-byte ASNs) had segments that landed on the exact attribute length with both 2-byte and 4-bytes. (For example: a 2 ASN segment followed by a 1 ASN segment - a 2-byte reading of the entire AS PATH and a 4-byte reading of the first segment are both exactly 10 bytes long - the same as the attribute's length. So the function incorrectly returned a 4-byte size). The best reason I can think of to stick with the 2-byte default is this: with it, you're more likely to land in the middle of a 4-byte ASN and catch the invalid ASN length with the sanity checks on the segment's first two bytes (at the start of the AS path). The opposite case is less likely since with 4-bytes you'd be jumping across two ASNs at a time and are more likely to land on a valid next segment. 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 can't think of a single authoritative reason to stay with the 2-byte default. Only that it seems to increase the chances of the checks catching invalid data, and that Wireshark and tcpdump.org both went that way (not a particularly strong argument). I have diffs for either option. I'm interested in your (and anyone else's) opinions.
