From f504d02e8e1ca9dd991d40c814007b9955434ae7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dr=2E=20Lars=20V=C3=B6lker?= Date: Sat, 8 Oct 2022 22:48:02 +0200 Subject: [PATCH] MACsec: improve FCS and Padding detection for Ethernet This patch fixes some bugs that occur with padded Ethernet frames and frames, when a Ethernet FCS is present. In the past that lead to wrong detection of the ICV by the Ethernet dissector (trailer). Such errors did occur for example with frames that were padded and MACsec was added later; thus, being bigger than expected for the heuristics in the packet-eth.c: "pinfo->fd->pkt_len >= 60 ..." --- epan/dissectors/packet-macsec.c | 47 +++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/epan/dissectors/packet-macsec.c b/epan/dissectors/packet-macsec.c index b186cd39394..288e4f2f582 100644 --- a/epan/dissectors/packet-macsec.c +++ b/epan/dissectors/packet-macsec.c @@ -44,6 +44,7 @@ static int hf_macsec_PN = -1; static int hf_macsec_SCI_system_identifier = -1; static int hf_macsec_SCI_port_identifier = -1; static int hf_macsec_etype = -1; +static int hf_macsec_eth_padding = -1; static int hf_macsec_ICV = -1; /* Initialize the subtree pointers */ @@ -52,7 +53,8 @@ static gint ett_macsec_tci = -1; /* Code to actually dissect the packets */ static int dissect_macsec(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void *data _U_) { - unsigned sectag_length, data_length, icv_length; + unsigned sectag_length, data_length, short_length, icv_length; + unsigned fcs_length = 0; unsigned data_offset, icv_offset; guint8 tci_an_field; @@ -75,14 +77,27 @@ static int dissect_macsec(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, v sectag_length = 6; } - /* Check for short length */ + /* Check for length too short */ if (tvb_captured_length(tvb) <= (sectag_length + icv_length)) { return 0; } + /* short length field: 1..47 bytes, 0 means 48 bytes or more */ + short_length = (guint32)tvb_get_guint8(tvb, 1); + /* Get the payload section */ + if (short_length != 0) { + data_length = short_length; + fcs_length = tvb_captured_length(tvb) - sectag_length - icv_length - short_length; + } else { + /* + * This assumes that no FCS is present after the ICV, which might not be true! + * Workaround: turn Ethernet "Assume packets have FCS" = Always, when FCS present. + * TODO: Find better heuristic to detect presence of FCS. + */ + data_length = tvb_captured_length(tvb) - sectag_length - icv_length; + } data_offset = sectag_length; - data_length = tvb_captured_length(tvb) - sectag_length - icv_length; icv_offset = data_length + data_offset; col_set_str(pinfo->cinfo, COL_PROTOCOL, "MACSEC"); @@ -138,15 +153,27 @@ static int dissect_macsec(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, v ethertype_data_t ethertype_data; ethertype_data.etype = tvb_get_ntohs(tvb, data_offset); - ethertype_data.payload_offset = data_offset + 2; + ethertype_data.payload_offset = 0; ethertype_data.fh_tree = macsec_tree; - ethertype_data.trailer_id = hf_macsec_ICV; + /* fallback, if ethernet dissector does not identify padding correctly */ + ethertype_data.trailer_id = hf_macsec_eth_padding; ethertype_data.fcs_len = 0; - call_dissector_with_data(ethertype_handle, tvb, pinfo, tree, ðertype_data); - } + /* lets hand over a buffer without ICV to limit effect of wrong padding calculation */ + next_tvb = tvb_new_subset_length(tvb, data_offset + 2, data_length - 2); + + /* help eth padding calculation by substracting length of the sectag, ethertype, icv, and fcs */ + gint pkt_len_saved = pinfo->fd->pkt_len; + pinfo->fd->pkt_len -= (sectag_length + 2 + icv_length + fcs_length); - return tvb_captured_length(tvb); + call_dissector_with_data(ethertype_handle, next_tvb, pinfo, tree, ðertype_data); + + /* restore original value */ + pinfo->fd->pkt_len = pkt_len_saved; + + proto_tree_add_item(macsec_tree, hf_macsec_ICV, tvb, icv_offset, icv_length, ENC_NA); + } + return tvb_captured_length(tvb) - fcs_length; } void @@ -205,6 +232,10 @@ proto_register_macsec(void) { "Ethertype", "macsec.etype", FT_UINT16, BASE_HEX, NULL, 0, NULL, HFILL } }, + { &hf_macsec_eth_padding, + { "Padding", "macsec.eth_padding", FT_BYTES, BASE_NONE, + NULL, 0, NULL, HFILL } + }, { &hf_macsec_ICV, { "ICV", "macsec.ICV", FT_BYTES, BASE_NONE, NULL, 0, NULL, HFILL }