BGP: Validate length of Path Attribute records.

Bug 13741 showed a case where the BGP dissector's failure to validate the
length of the Path Attribute record allowed a pathological BGP UPDATE packet to
generate more than one million items in the protocol tree by repeatedly
dissecting certain segments of the packet.

It's easy enough to detect when the Path Attribute length cannot be valid, so
let's do so.  When the condition arises, let's raise an Expert Info error in
the same style and format as used elsewhere in the same routine, and abandon
dissection of the Path Attributes list.

With this check in place, an incorrect length computation is revealed at a
callsite.  This would only have prevented a small (less than 5 bytes) Path
Attribute from being dissected if it was at the very end of the Path Attributes
list, but the bounds checking added in this change makes this problem much more
apparent, so we fix the length computation while we're here.

Testing Done: Built wireshark on Linux amd64.  Using bgp.pcap from the Sample
   Captures page on the wiki, verified that the dissection of the UPDATE
   packets were unaltered by this fix.  Using the capture attached to bug 13741
   (clusterfuzz-testcase-minimized-6689222578667520.pcap), verified that the
   packet no longer triggers the "too many items" exception, instead we see
   an Expert Info for each oversized Path Attribute length, and eventually an
   exception for "length of contained item exceeds length of containing item".
   30,000 iterations of fuzz test with bgp.pcap as input, and many iterations
   of randpkt-test too.  Crafted a packet with a 3-byte ATOMIC_AGGREGATE Path
   Attribute at the end of the Path Attributes list; Before this change, an
   exception is raised during dissection, but after this change it is dissected
   correctly.

Bug: 13741
Change-Id: I80f506b114a61e5b060d93b59bed6b94fb188b3e
Reviewed-on: https://code.wireshark.org/review/27466
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>

1 files changed, 7 insertions, 1 deletions

diff --git a/epan/dissectors/packet-bgp.c b/epan/dissectors/packet-bgp.c
index 706e127f37..4d8cd22a8d 100644
--- a/epan/dissectors/packet-bgp.c
+++ b/epan/dissectors/packet-bgp.c
@@ -6917,6 +6917,12 @@ dissect_bgp_path_attr(proto_tree *subtree, tvbuff_t *tvb, guint16 path_attr_len,
 
         attr_len_item = proto_tree_add_item(subtree2, hf_bgp_update_path_attribute_length, tvb, o + i + BGP_SIZE_OF_PATH_ATTRIBUTE,
                                             aoff - BGP_SIZE_OF_PATH_ATTRIBUTE, ENC_BIG_ENDIAN);
+        if (aoff + tlen > path_attr_len - i) {
+            proto_tree_add_expert_format(subtree2, pinfo, &ei_bgp_length_invalid, tvb, o + i + aoff, tlen,
+                                         "Path attribute length is invalid: %u byte%s", tlen,
+                                         plurality(tlen, "", "s"));
+            return;
+        }
 
         /* Path Attribute Type */
         switch (bgpa_type) {
@@ -7706,7 +7712,7 @@ dissect_bgp_update(tvbuff_t *tvb, proto_tree *tree, packet_info *pinfo)
         ti =  proto_tree_add_item(tree, hf_bgp_update_path_attributes, tvb, o+2, len, ENC_NA);
         subtree = proto_item_add_subtree(ti, ett_bgp_attrs);
 
-        dissect_bgp_path_attr(subtree, tvb, len-4, o+2, pinfo);
+        dissect_bgp_path_attr(subtree, tvb, len, o+2, pinfo);
 
         o += 2 + len;