From 5541c28ae66f2febd02244d5f7ff6dfcead17641 Mon Sep 17 00:00:00 2001 From: Jeff Morriss Date: Mon, 8 Jul 2013 21:12:06 +0000 Subject: Fix the very long loop reported in https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8911 : In parseFields() ensure that we have at least one byte so that callers don't have to protect against it returning an offset which hasn't incremented. Remove a couple of now-unnecessary length-remaining (aka "will the offset move?") checks. In some other checks, use tvb_ensure_length_remaining() rather than calling tvb_length_remaining() and (potentially) THROWing an exception. I'm not sure if these are really necessary now or not... svn path=/trunk/; revision=50450 --- epan/dissectors/packet-dis-pdus.c | 79 +++++++++++++++------------------------ 1 file changed, 31 insertions(+), 48 deletions(-) diff --git a/epan/dissectors/packet-dis-pdus.c b/epan/dissectors/packet-dis-pdus.c index 583179e7d4..241de23ef2 100644 --- a/epan/dissectors/packet-dis-pdus.c +++ b/epan/dissectors/packet-dis-pdus.c @@ -746,7 +746,10 @@ gint parseFields(tvbuff_t *tvb, proto_tree *tree, gint offset, DIS_ParserNode pa guint16 spread_spectrum = 0; - length = tvb_length_remaining(tvb, offset); + /* Get the length while ensuring there's at least one byte for us to + * decode (if not, throw an exception so as to prevent very long loops). + */ + length = tvb_ensure_length_remaining(tvb, offset+1); while ((parserNodes[fieldIndex].fieldType != DIS_FIELDTYPE_END) && (length > 0 ) ) @@ -1239,23 +1242,18 @@ gint parseFields(tvbuff_t *tvb, proto_tree *tree, gint offset, DIS_ParserNode pa for (i = 0; i < numFixed; ++i) { + proto_item *newSubtree; + /* is remaining length large enough for another fixed datum (ID & value) */ - length = tvb_length_remaining(tvb, offset); - if ( length >= 8 ) - { - proto_item *newSubtree; - newField = proto_tree_add_text(tree, tvb, offset, -1, "%s", - parserNodes[fieldIndex].fieldLabel); - newSubtree = proto_item_add_subtree(newField, ettFixedData); - offset = parseFields - (tvb, newSubtree, offset, - parserNodes[fieldIndex].children); - proto_item_set_end(newField, tvb, offset); - } - else { - THROW(ReportedBoundsError); - break; - } + /* XXX is this really necessary? */ + tvb_ensure_length_remaining(tvb, offset+8); + + newField = proto_tree_add_text(tree, tvb, offset, -1, "%s", + parserNodes[fieldIndex].fieldLabel); + newSubtree = proto_item_add_subtree(newField, ettFixedData); + offset = parseFields (tvb, newSubtree, offset, + parserNodes[fieldIndex].children); + proto_item_set_end(newField, tvb, offset); } } break; @@ -1277,16 +1275,10 @@ gint parseFields(tvbuff_t *tvb, proto_tree *tree, gint offset, DIS_ParserNode pa for (i = 0; i < numFixed; ++i) { /* is remaining length large enough for another fixed datum ID (32 bit int) */ - if (tvb_length_remaining(tvb, offset) >= 4 ) - { - offset = parseFields - (tvb, newSubtree, offset, - parserNodes[fieldIndex].children); - } - else { - THROW(ReportedBoundsError); - break; - } + /* XXX is this really necessary? */ + tvb_ensure_length_remaining(tvb, offset+4); + offset = parseFields (tvb, newSubtree, offset, + parserNodes[fieldIndex].children); } proto_item_set_end(newField, tvb, offset); } @@ -1374,26 +1366,17 @@ gint parseFields(tvbuff_t *tvb, proto_tree *tree, gint offset, DIS_ParserNode pa for (i = 0; i < numVariable; ++i) { - /* simple check to detect malformed, field parsers will detect specifics */ - length = tvb_length_remaining(tvb, offset); - if ( length > 0 ) - { - proto_item *newSubtree; - newField = proto_tree_add_text(tree, tvb, offset, -1, "%s", - parserNodes[fieldIndex].fieldLabel); - newSubtree = proto_item_add_subtree(newField, - ettVariableRecords[i]); - offset = parseFields - (tvb, newSubtree, offset, - parserNodes[fieldIndex].children); - offset = parseField_VariableRecord - (tvb, newSubtree, offset); - proto_item_set_end(newField, tvb, offset); - } - else { - THROW(ReportedBoundsError); - break; - } + proto_item *newSubtree; + newField = proto_tree_add_text(tree, tvb, offset, -1, "%s", + parserNodes[fieldIndex].fieldLabel); + newSubtree = proto_item_add_subtree(newField, + ettVariableRecords[i]); + offset = parseFields + (tvb, newSubtree, offset, + parserNodes[fieldIndex].children); + offset = parseField_VariableRecord + (tvb, newSubtree, offset); + proto_item_set_end(newField, tvb, offset); } } break; @@ -1552,7 +1535,7 @@ gint parseFields(tvbuff_t *tvb, proto_tree *tree, gint offset, DIS_ParserNode pa case DIS_FIELDTYPE_UA_EMITTER_SYSTEMS: { guint i; - + if (numUAEmitter > DIS_PDU_MAX_UA_EMITTER_SYSTEMS) { numUAEmitter = DIS_PDU_MAX_UA_EMITTER_SYSTEMS; -- cgit v1.2.1