diff options
authorDarius Davis <darius@vmware.com>2018-05-12 17:30:48 +1000
committerAnders Broman <a.broman58@gmail.com>2018-05-14 08:17:09 +0000
commit6e88943d0eabc8c8bc11334ba4213ec64129575c (patch)
parentd80dbe533cb927ca16912c6ca9dead2d165362e9 (diff)
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,
+ 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;