From c9bc26df379755faa423d4bfd4462c043d186ddc Mon Sep 17 00:00:00 2001 From: John Thacker Date: Sat, 11 Apr 2026 21:07:42 -0400 Subject: [PATCH] Openflow v5: Prevent infinite loops When looping on an Openflow v5 action, if the length is invalid set the offset to the end of structure containing all the actions (array of action buckets, etc.) rather than the claimed end of the entire message. This prevents an infinite loop in the case that the contained structure length is larger than the containing message. Take tvbuffer subsets in more places instead of passing a length around. That is a better approach, particularly when there are various nested lengths; it ensures that ReportedBoundsErrors and ContainedBoundsErrors are produced, and reduces the chance of overflow, infinite loops, and other strange behavior. It could be done even more in this dissector. Fix #21182 AI-Assisted: no (cherry picked from commit 92fdf8e04fc471f052f185b59233dbc1f3b7b655) Co-authored-by: John Thacker --- epan/dissectors/packet-openflow_v5.c | 56 +++++++++++++++------------- 1 file changed, 31 insertions(+), 25 deletions(-) Index: wireshark-3.6.24/epan/dissectors/packet-openflow_v5.c =================================================================== --- wireshark-3.6.24.orig/epan/dissectors/packet-openflow_v5.c +++ wireshark-3.6.24/epan/dissectors/packet-openflow_v5.c @@ -1075,7 +1075,7 @@ static value_string_ext openflow_v5_oxm_ #define OXM_FIELD_OFFSET 1 #define OXM_HM_MASK 0x01 static int -dissect_openflow_oxm_header_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset, guint16 length _U_) +dissect_openflow_oxm_header_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset) { guint16 oxm_class; @@ -1105,7 +1105,7 @@ dissect_openflow_oxm_header_v5(tvbuff_t #define OFPVID_PRESENT 0x1000 static int -dissect_openflow_oxm_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset, guint16 length _U_) +dissect_openflow_oxm_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset) { proto_tree *oxm_tree; guint16 oxm_class; @@ -1127,7 +1127,7 @@ dissect_openflow_oxm_v5(tvbuff_t *tvb, p oxm_tree = proto_tree_add_subtree(tree, tvb, offset, oxm_length + 4, ett_openflow_v5_oxm, NULL, "OXM field"); - offset = dissect_openflow_oxm_header_v5(tvb, pinfo, oxm_tree, offset, length); + offset = dissect_openflow_oxm_header_v5(tvb, pinfo, oxm_tree, offset); if (oxm_class == OFPXMC_OPENFLOW_BASIC) { switch(oxm_field) { @@ -1303,7 +1303,7 @@ dissect_openflow_match_v5(tvbuff_t *tvb, case OFPMT_OXM: fields_end = offset + match_length - 4; while(offset < fields_end) { - offset = dissect_openflow_oxm_v5(tvb, pinfo, match_tree, offset, length); + offset = dissect_openflow_oxm_v5(tvb, pinfo, match_tree, offset); } break; @@ -2192,7 +2192,7 @@ static const value_string openflow_v5_ac static int -dissect_openflow_action_header_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset, guint16 length _U_) +dissect_openflow_action_header_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset) { guint16 act_type; @@ -2216,7 +2216,7 @@ dissect_openflow_action_header_v5(tvbuff static int -dissect_openflow_action_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset, guint16 length _U_) +dissect_openflow_action_v5(tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree, int offset) { proto_tree *act_tree; guint16 act_type; @@ -2229,7 +2229,7 @@ dissect_openflow_action_v5(tvbuff_t *tvb act_tree = proto_tree_add_subtree(tree, tvb, offset, act_length, ett_openflow_v5_action, NULL, "Action"); - offset = dissect_openflow_action_header_v5(tvb, pinfo, act_tree, offset, length); + offset = dissect_openflow_action_header_v5(tvb, pinfo, act_tree, offset); switch (act_type) { case OFPAT_OUTPUT: @@ -2340,7 +2340,7 @@ dissect_openflow_action_v5(tvbuff_t *tvb break; case OFPAT_SET_FIELD: - offset = dissect_openflow_oxm_v5(tvb, pinfo, act_tree, offset, length); + offset = dissect_openflow_oxm_v5(tvb, pinfo, act_tree, offset); /* padded to 64 bits */ if (offset < act_end) { @@ -2368,7 +2368,7 @@ dissect_openflow_action_v5(tvbuff_t *tvb case OFPAT_EXPERIMENTER: if (act_length <= 8) { expert_add_info(pinfo, act_tree, &ei_openflow_v5_length_too_short); - offset = length; + offset = tvb_reported_length(tvb); break; } proto_tree_add_expert_format(act_tree, pinfo, &ei_openflow_v5_action_undecoded, @@ -2379,7 +2379,7 @@ dissect_openflow_action_v5(tvbuff_t *tvb default: if (act_length <= 4) { expert_add_info(pinfo, act_tree, &ei_openflow_v5_length_too_short); - offset = length; + offset = tvb_reported_length(tvb); break; } proto_tree_add_expert_format(act_tree, pinfo, &ei_openflow_v5_action_undecoded, @@ -2761,7 +2761,8 @@ dissect_openflow_packet_out_v5(tvbuff_t { proto_tree *data_tree; guint16 acts_len; - gint32 acts_end; + gint32 acts_offset = 0; + tvbuff_t *acts_tvb; tvbuff_t *next_tvb; gboolean save_writable; gboolean save_in_error_pkt; @@ -2785,11 +2786,12 @@ dissect_openflow_packet_out_v5(tvbuff_t offset+=6; /* struct ofp_action_header actions[0]; */ - acts_end = offset + acts_len; + acts_tvb = tvb_new_subset_length(tvb, offset, acts_len); - while (offset < acts_end) { - offset = dissect_openflow_action_v5(tvb, pinfo, tree, offset, length); + while (tvb_reported_length_remaining(acts_tvb, acts_offset)) { + acts_offset = dissect_openflow_action_v5(acts_tvb, pinfo, tree, acts_offset); } + offset += acts_offset; /* uint8_t data[0]; */ if (offset < length) { @@ -2873,7 +2875,8 @@ dissect_openflow_instruction_v5(tvbuff_t proto_tree *inst_tree; guint16 inst_type; guint16 inst_length; - gint32 acts_end; + tvbuff_t *acts_tvb; + int acts_offset = 0; inst_type = tvb_get_ntohs(tvb, offset); inst_length = tvb_get_ntohs(tvb, offset + 2); @@ -2917,10 +2920,11 @@ dissect_openflow_instruction_v5(tvbuff_t proto_tree_add_item(inst_tree, hf_openflow_v5_instruction_actions_pad, tvb, offset, 4, ENC_NA); offset+=4; - acts_end = offset + inst_length - 8; - while (offset < acts_end) { - offset = dissect_openflow_action_v5(tvb, pinfo, inst_tree, offset, length); + acts_tvb = tvb_new_subset_length(tvb, offset, inst_length - 8); + while (tvb_reported_length_remaining(acts_tvb, acts_offset)) { + acts_offset = dissect_openflow_action_v5(acts_tvb, pinfo, inst_tree, acts_offset); } + offset += acts_offset; break; case OFPIT_METER: @@ -3047,7 +3051,8 @@ dissect_openflow_bucket_v5(tvbuff_t *tvb proto_item *ti; proto_tree *bucket_tree; guint16 bucket_length; - gint32 acts_end; + tvbuff_t *acts_tvb; + gint32 acts_offset = 0; bucket_tree = proto_tree_add_subtree(tree, tvb, offset, -1, ett_openflow_v5_bucket, &ti, "Bucket"); @@ -3078,10 +3083,11 @@ dissect_openflow_bucket_v5(tvbuff_t *tvb offset+=4; /*struct ofp_action_header actions[0]; */ - acts_end = offset + bucket_length - 16; - while (offset < acts_end) { - offset = dissect_openflow_action_v5(tvb, pinfo, bucket_tree, offset, length); + acts_tvb = tvb_new_subset_length(tvb, offset, bucket_length - 16); + while (tvb_reported_length_remaining(acts_tvb, acts_offset)) { + acts_offset = dissect_openflow_action_v5(acts_tvb, pinfo, bucket_tree, acts_offset); } + offset += acts_offset; return offset; } @@ -3631,7 +3637,7 @@ dissect_openflow_table_feature_prop_v5(t elem_begin = offset; elem_tree = proto_tree_add_subtree(prop_tree, tvb, offset, -1, ett_openflow_v5_table_feature_prop_action_id, &ti, "Action ID"); - offset = dissect_openflow_action_header_v5(tvb, pinfo, elem_tree, offset, length); + offset = dissect_openflow_action_header_v5(tvb, pinfo, elem_tree, offset); proto_item_set_len(ti, offset - elem_begin); } break; @@ -3646,7 +3652,7 @@ dissect_openflow_table_feature_prop_v5(t elem_begin = offset; elem_tree = proto_tree_add_subtree(prop_tree, tvb, offset, -1, ett_openflow_v5_table_feature_prop_oxm_id, &ti, "OXM ID"); - offset = dissect_openflow_oxm_header_v5(tvb, pinfo, elem_tree, offset, length); + offset = dissect_openflow_oxm_header_v5(tvb, pinfo, elem_tree, offset); proto_item_set_len(ti, offset - elem_begin); } break;