osmocom
/
wireshark
11
0
Fork 1
Code Issues Pull Requests Projects Releases Wiki Activity

eth: require padding to be zeros by default 

Ethernet frame padding for short frames _should_ be zeros. Replace
the assume_padding preference with the padding preference that by default
will only consider consecutive zeros long enough to reach the minimum
ethernet length to be padding.  The old behaviors are preserved.
Never (old FALSE) and Any (old TRUE - old default)

The old behavior broke some trailer dissectors when the trailer was
added before the determination of needing padding was made.  Thus the
ethernet dissector would consume some of the trailer as padding.

Bug: 16481
Change-Id: I6b9e1d26d07d84cb768eece5e44412e23dfe37ca
Reviewed-on: https://code.wireshark.org/review/36691
Reviewed-by: Jason Cohen <kryojenik2@gmail.com>
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
This commit is contained in:
Jason Cohen 2020-04-01 18:25:45 -05:00 committed by Anders Broman
parent 15b53b6892
commit 76b530eb6a
1 changed files with 62 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
epan/dissectors/packet-eth.c
View File

@ -39,8 +39,12 @@
void proto_register_eth(void);
void proto_reg_handoff_eth(void);
#define PADDING_NONE 0
#define PADDING_ZEROS 1
#define PADDING_ANY 2
/* Assume all packets have an FCS */
static gboolean eth_assume_padding = TRUE;
static gint eth_padding = PADDING_ZEROS;
static guint eth_trailer_length = 0;
static gboolean eth_assume_fcs = FALSE;
static gboolean eth_check_fcs = FALSE;
@ -91,6 +95,7 @@ static expert_field ei_eth_invalid_lentype = EI_INIT;
static expert_field ei_eth_src_not_group = EI_INIT;
static expert_field ei_eth_fcs_bad = EI_INIT;
static expert_field ei_eth_len = EI_INIT;
static expert_field ei_eth_padding_bad = EI_INIT;
static dissector_handle_t fw1_handle;
static dissector_handle_t ethertype_handle;
@ -116,6 +121,12 @@ static const true_false_string lg_tfs = {
"Globally unique address (factory default)"
};
static const enum_val_t eth_padding_vals[] = {
{"never", "Never", PADDING_NONE},
{"zeros", "Zeros", PADDING_ZEROS},
{"any", "Any", PADDING_ANY},
{NULL, NULL, 0}
};
static const char* eth_conv_get_filter_type(conv_item_t* conv, conv_filter_type_e filter)
{
@ -668,28 +679,35 @@ add_ethernet_trailer(packet_info *pinfo, proto_tree *tree, proto_tree *fh_tree,
guint trailer_length, trailer_reported_length;
guint padding_length = 0;
gboolean has_fcs = FALSE;
gboolean maybe_padded = FALSE;
tvbuff_t *real_trailer_tvb;
trailer_length = tvb_captured_length(trailer_tvb);
trailer_reported_length = tvb_reported_length(trailer_tvb);
/* There can not have been padding when the length of the frame (including the
trailer) is less than 60 bytes. */
if (eth_assume_padding && pinfo->fd->pkt_len>=60) {
/* Calculate the amount of padding needed for a minimum sized frame */
if ( (pinfo->fd->pkt_len - trailer_reported_length) < 60 )
padding_length = 60 - (pinfo->fd->pkt_len - trailer_reported_length);
/* There couldn't be a padding if the length of the frame (including the trailer) is still
less than 60 bytes. */
maybe_padded = (pinfo->fd->pkt_len >= 60 && (pinfo->fd->pkt_len - trailer_reported_length) < 60);
/* Add the padding to the tree, unless it should be treated as
part of the trailer and therefor be handed over to (one of)
the ethernet-trailer dissectors */
if (padding_length > 0) {
tvb_ensure_bytes_exist(tvb, 0, padding_length);
proto_tree_add_item(fh_tree, hf_eth_padding, trailer_tvb, 0,
padding_length, ENC_NA);
trailer_length -= padding_length;
trailer_reported_length -= padding_length;
if (eth_padding != PADDING_NONE && maybe_padded) {
padding_length = 60 - (pinfo->fd->pkt_len - trailer_reported_length);
/* Require padding to be zeros */
if (eth_padding == PADDING_ZEROS) {
for (guint i = 0; i < padding_length; i++) {
if (tvb_get_gint8(trailer_tvb, i) != 0) {
padding_length = 0;
break;
}
}
}
/* If it was determined that we have padding, add it to the tree. */
if (padding_length > 0) {
tvb_ensure_bytes_exist(tvb, 0, padding_length);
proto_tree_add_item(fh_tree, hf_eth_padding, trailer_tvb, 0,
padding_length, ENC_NA);
trailer_length -= padding_length;
trailer_reported_length -= padding_length;
}
}
if (fcs_len != 0) {
@ -752,8 +770,18 @@ add_ethernet_trailer(packet_info *pinfo, proto_tree *tree, proto_tree *fh_tree,
extra bytes as general trailer */
if (trailer_length != 0) {
tvb_ensure_bytes_exist(tvb, 0, trailer_length);
proto_tree_add_item(fh_tree, trailer_id, real_trailer_tvb, 0,
proto_item *pi = proto_tree_add_item(fh_tree, trailer_id, real_trailer_tvb, 0,
trailer_length, ENC_NA);
if (maybe_padded) {
if (eth_padding == PADDING_ANY && padding_length > 0) {
expert_add_info_format(pinfo, pi, &ei_eth_padding_bad,
"Padding was assumed, and an undecoded trailer exists. Some of the trailer may have been consumed by padding.");
}
else if (eth_padding == PADDING_ZEROS && padding_length == 0) {
expert_add_info_format(pinfo, pi, &ei_eth_padding_bad,
"Didn't find padding of zeros, and an undecoded trailer exists. There may be padding of non-zeros.");
}
}
}
}
}
@ -985,6 +1013,7 @@ proto_register_eth(void)
{ &ei_eth_src_not_group, { "eth.src_not_group", PI_PROTOCOL, PI_WARN, "Source MAC must not be a group address: IEEE 802.3-2002, Section 3.2.3(b)", EXPFILL }},
{ &ei_eth_fcs_bad, { "eth.fcs_bad", PI_CHECKSUM, PI_ERROR, "Bad checksum", EXPFILL }},
{ &ei_eth_len, { "eth.len.past_end", PI_MALFORMED, PI_ERROR, "Length field value goes past the end of the payload", EXPFILL }},
{ &ei_eth_padding_bad, {"eth.padding_bad", PI_PROTOCOL, PI_NOTE, "Padding identification may be inaccurate and impact trailer dissector", EXPFILL }},
};
module_t *eth_module;
@ -1003,13 +1032,22 @@ proto_register_eth(void)
/* Register configuration preferences */
eth_module = prefs_register_protocol(proto_eth, NULL);
prefs_register_bool_preference(eth_module, "assume_padding",
"Assume short frames which include a trailer contain padding",
"Some devices add trailing data to frames. When this setting is checked "
"the Ethernet dissector will assume there has been added padding to the "
"frame before the trailer was added. Uncheck if a device added a trailer "
"before the frame was padded.",
&eth_assume_padding);
prefs_register_obsolete_preference(eth_module, "assume_padding");
prefs_register_enum_preference(eth_module, "padding",
"Assume padding for short frames with trailer",
"Some devices add trailing data to frames. Depending on where this "
"device exists in the network, padding could be added to short "
"frames before the additional trailer. This option determines how "
"that padding will be detected.\n\n"
"Never - Don't detect any padding. Any bytes after the ethernet "
"payload will be considered trailer.\n"
"Zeros (default) - Consecutive bytes of zeros up to the minimum "
"ethernet frame size will be treated as padding. Additional bytes will "
"be considered trailer.\n"
"Any - Any bytes after the payload up to the minimum ethernet frame "
"size will be treated as padding. Additional bytes will be considered "
"trailer.",
&eth_padding, eth_padding_vals, FALSE);
prefs_register_uint_preference(eth_module, "trailer_length",
"Fixed ethernet trailer length",