osmith
/
wireshark
forked from osmocom/wireshark
1
0
Fork 0
Code Pull Requests Releases Wiki Activity

exceptions: set FragmentBoundsError priority above ContainedBoundsError 

All fragment errors are bounds errors that go past the contained length,
but they do not necessarily involve going past the reported length,
so the checks for FragmentBoundsError should reflect that.

With some forms of reassembly, like IP fragmentation, we don't know how
big the PDU/reassembled packet is until reassembly is complete, so we
probably use tvb_new_subset_remaining() to create fragments and the tvb's
reported length is equal to its contained length. In these cases
ReportedBoundsError would be otherwise thrown, except when the existing
checks for FragmentBoundsError intervene.

However, with other forms of reassembly, like various PDUs carried over TCP,
we know the total PDU length, so we use tvb_new_subset_length[_caplen](),
setting the proper reported length, but not changing the contained
length when reassembly is not performed. In those cases, a bounds error
that occurs due to lack of reassembly is otherwise a ContainedBoundsError,
not a ReportedBoundsError.

In both cases, a bounds error caused by an unreassembled fragment should
be a FragmentBoundsError for the existing reasons. It is not necessarily
a malformed packet (to the extent reassembly is not performed because of a
malformed error elsewhere, that should be reported separately) and can
likely be avoided by changing preferences (e.g., turning reassembly
preferences on, turning off checksum verification, etc.) Otherwise it
is probably a dissector bug.
This commit is contained in:
John Thacker 2021-10-15 17:53:23 -04:00 committed by Wireshark GitLab Utility
parent 33708af75f
commit fad8346282
4 changed files with 46 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
epan/exceptions.h
View File

@ -49,8 +49,12 @@
#define ReportedBoundsError 3
/**
Index is beyond the fragment length but not the reported length.
This means that the packet wasn't reassembled.
Index is beyond the contained length, and possibly the reported length,
of the tvbuff, but we believe it is an unreassembled fragment, either
because the "this is an unreassembled fragment" flag or pinfo->fragmented
is set. This means that the packet wasn't reassembled, but could possibly
be correctly dissected if reassembly preferences were changed. It is
therefore not reported as a "Malformed packet".
**/
#define FragmentBoundsError 4

7
epan/show_exception.c
View File

@ -68,7 +68,7 @@ show_exception(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree,
"Dissector writer didn't bother saying what the error was";
proto_item *item;
if (exception == ReportedBoundsError && pinfo->fragmented)
if ((exception == ReportedBoundsError || exception == ContainedBoundsError) && pinfo->fragmented)
exception = FragmentBoundsError;
switch (exception) {
@ -114,7 +114,10 @@ show_exception(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree,
pinfo->noreassembly_reason, pinfo->current_proto);
/* Don't record FragmentBoundsError exceptions as expert events - they merely
* reflect dissection done with reassembly turned off
* (any case where it's caused by something else is a bug). */
* (any case where it's caused by something else is a bug).
* XXX: It's not malformed, but perhaps an expert info at a lower
* severity like PI_NOTE would be useful to suggest trying to
* change reassembly preferences. */
break;
case ContainedBoundsError:

66
epan/tvbuff.c
View File

@ -191,29 +191,14 @@ validate_offset(const tvbuff_t *tvb, const guint abs_offset)
* artificial boundary imposed by packet slicing, that
* we're past.
*/
if (abs_offset <= tvb->reported_length) {
/*
* We're within the bounds of what this tvbuff
* purportedly contains, based on some length
* value, but we're not within the bounds of
* something from which this tvbuff was
* extracted, so that length value ran past
* the end of some parent tvbuff.
*/
return ContainedBoundsError;
}
/*
* OK, we're past the bounds of what this tvbuff
* purportedly contains.
*/
if (tvb->flags & TVBUFF_FRAGMENT) {
/*
* This tvbuff is the first fragment of a larger
* packet that hasn't been reassembled, so we
* assume that's the source of the prblem - if
* we'd reassembled the packet, we wouldn't
* have gone past the end.
* assume that's the source of the problem - if
* we'd reassembled the packet, we wouldn't have
* gone past the end.
*
* That might not be true, but for at least
* some forms of reassembly, such as IP
@ -226,6 +211,19 @@ validate_offset(const tvbuff_t *tvb, const guint abs_offset)
return FragmentBoundsError;
}
/* OK, we're not an unreassembled fragment (that we know of). */
if (abs_offset <= tvb->reported_length) {
/*
* We're within the bounds of what this tvbuff
* purportedly contains, based on some length
* value, but we're not within the bounds of
* something from which this tvbuff was
* extracted, so that length value ran past
* the end of some parent tvbuff.
*/
return ContainedBoundsError;
}
/*
* OK, it looks as if we ran past the claimed length
* of data.
@ -242,10 +240,10 @@ compute_offset(const tvbuff_t *tvb, const gint offset, guint *offset_ptr)
*offset_ptr = offset;
} else if ((guint) offset <= tvb->contained_length) {
return BoundsError;
} else if ((guint) offset <= tvb->reported_length) {
return ContainedBoundsError;
} else if (tvb->flags & TVBUFF_FRAGMENT) {
return FragmentBoundsError;
} else if ((guint) offset <= tvb->reported_length) {
return ContainedBoundsError;
} else {
return ReportedBoundsError;
}
@ -256,10 +254,10 @@ compute_offset(const tvbuff_t *tvb, const gint offset, guint *offset_ptr)
*offset_ptr = tvb->length + offset;
} else if ((guint) -offset <= tvb->contained_length) {
return BoundsError;
} else if ((guint) -offset <= tvb->reported_length) {
return ContainedBoundsError;
} else if (tvb->flags & TVBUFF_FRAGMENT) {
return FragmentBoundsError;
} else if ((guint) -offset <= tvb->reported_length) {
return ContainedBoundsError;
} else {
return ReportedBoundsError;
}
@ -598,10 +596,10 @@ tvb_ensure_captured_length_remaining(const tvbuff_t *tvb, const gint offset)
*/
if (abs_offset < tvb->contained_length) {
THROW(BoundsError);
} else if (abs_offset < tvb->reported_length) {
THROW(ContainedBoundsError);
} else if (tvb->flags & TVBUFF_FRAGMENT) {
THROW(FragmentBoundsError);
} else if (abs_offset < tvb->reported_length) {
THROW(ContainedBoundsError);
} else {
THROW(ReportedBoundsError);
}
@ -685,10 +683,10 @@ tvb_ensure_bytes_exist(const tvbuff_t *tvb, const gint offset, const gint length
real_offset = offset;
} else if ((guint) offset <= tvb->contained_length) {
THROW(BoundsError);
} else if ((guint) offset <= tvb->reported_length) {
THROW(ContainedBoundsError);
} else if (tvb->flags & TVBUFF_FRAGMENT) {
THROW(FragmentBoundsError);
} else if ((guint) offset <= tvb->reported_length) {
THROW(ContainedBoundsError);
} else {
THROW(ReportedBoundsError);
}
@ -699,10 +697,10 @@ tvb_ensure_bytes_exist(const tvbuff_t *tvb, const gint offset, const gint length
real_offset = tvb->length + offset;
} else if ((guint) -offset <= tvb->contained_length) {
THROW(BoundsError);
} else if ((guint) -offset <= tvb->reported_length) {
THROW(ContainedBoundsError);
} else if (tvb->flags & TVBUFF_FRAGMENT) {
THROW(FragmentBoundsError);
} else if ((guint) -offset <= tvb->reported_length) {
THROW(ContainedBoundsError);
} else {
THROW(ReportedBoundsError);
}
@ -723,10 +721,10 @@ tvb_ensure_bytes_exist(const tvbuff_t *tvb, const gint offset, const gint length
return;
else if (end_offset <= tvb->contained_length)
THROW(BoundsError);
else if (end_offset <= tvb->reported_length)
THROW(ContainedBoundsError);
else if (tvb->flags & TVBUFF_FRAGMENT)
THROW(FragmentBoundsError);
else if (end_offset <= tvb->reported_length)
THROW(ContainedBoundsError);
else
THROW(ReportedBoundsError);
}
@ -904,10 +902,10 @@ fast_ensure_contiguous(tvbuff_t *tvb, const gint offset, const guint length)
return tvb->real_data + u_offset;
} else if (end_offset <= tvb->contained_length) {
THROW(BoundsError);
} else if (end_offset <= tvb->reported_length) {
THROW(ContainedBoundsError);
} else if (tvb->flags & TVBUFF_FRAGMENT) {
THROW(FragmentBoundsError);
} else if (end_offset <= tvb->reported_length) {
THROW(ContainedBoundsError);
} else {
THROW(ReportedBoundsError);
}
@ -2473,10 +2471,10 @@ tvb_strsize(tvbuff_t *tvb, const gint offset)
*/
if (tvb->length < tvb->contained_length) {
THROW(BoundsError);
} else if (tvb->length < tvb->reported_length) {
THROW(ContainedBoundsError);
} else if (tvb->flags & TVBUFF_FRAGMENT) {
THROW(FragmentBoundsError);
} else if (tvb->length < tvb->reported_length) {
THROW(ContainedBoundsError);
} else {
THROW(ReportedBoundsError);
}

4
epan/tvbuff.h
View File

@ -276,7 +276,9 @@ WS_DLL_PUBLIC guint tvb_offset_from_real_beginning(const tvbuff_t *tvb);
/* Returns the offset from the first byte of real data. */
WS_DLL_PUBLIC gint tvb_raw_offset(tvbuff_t *tvb);
/** Set the "this is a fragment" flag. */
/** Set the "this is a fragment" flag. This affects whether
* FragmentBoundsError is thrown instead of ContainedBoundsError
* or ReportedBoundsError. */
WS_DLL_PUBLIC void tvb_set_fragment(tvbuff_t *tvb);
WS_DLL_PUBLIC struct tvbuff *tvb_get_ds_tvb(tvbuff_t *tvb);