Comment # 4
on bug 10840
from Ivan
(In reply to Pascal Quantin from comment #3)
> I agree with Alexis: there does not seem to have something to be fixed here.
Hi,
can you please explain me a bit?
Thanks in advance.
As I can see all validations occur in this order:
// \wireshark-1.12.2\epan\dissectors\packet-bjnp.c
static int dissect_bjnp (tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree,
void *data _U_)
{
proto_tree *bjnp_tree;
payload_len = tvb_get_ntohl (tvb, offset);
proto_tree_add_item (bjnp_tree, hf_payload_len, tvb, offset, 4,
ENC_BIG_ENDIAN);
offset += 4;
if (payload_len > 0) {
/* TBD: Dissect various commands */
proto_tree_add_item (bjnp_tree, hf_payload, tvb, offset, payload_len,
ENC_NA); // <-- we are here and trying to check "payload_len", let it be
0x7fffffff
offset += payload_len;
}
return offset;
}
Then we go down to:
// \wireshark-1.12.2\epan\proto.c
proto_item *
proto_tree_add_item(proto_tree *tree, int hfindex, tvbuff_t *tvb,
const gint start, gint length, const guint encoding)
{
register header_field_info *hfinfo;
PROTO_REGISTRAR_GET_NTH(hfindex, hfinfo);
return proto_tree_add_item_new(tree, hfinfo, tvb, start, length,
encoding);
}
And then:
proto_item *
proto_tree_add_item_new(proto_tree *tree, header_field_info *hfinfo, tvbuff_t
*tvb,
const gint start, gint length, const guint encoding)
{
field_info *new_fi;
gint item_length;
DISSECTOR_ASSERT_HINT(hfinfo != NULL, "Not passed hfi!");
get_hfi_length(hfinfo, tvb, start, &length, &item_length); //
out-of-bounds checking occurs is here
test_length(hfinfo, tvb, start, item_length);
TRY_TO_FAKE_THIS_ITEM(tree, hfinfo->id, hfinfo);
new_fi = new_field_info(tree, hfinfo, tvb, start, item_length);
if (new_fi == NULL)
return NULL;
return proto_tree_new_item(new_fi, tree, tvb, start, length, encoding);
}
And the last one:
static void
get_hfi_length(header_field_info *hfinfo, tvbuff_t *tvb, const gint start, gint
*length,
gint *item_length)
{
//...skipped
*item_length = *length;
if (hfinfo->type == FT_PROTOCOL || hfinfo->type == FT_NONE) {
if (tvb) {
length_remaining =
tvb_captured_length_remaining(tvb, start);
if (*item_length < 0 ||
(*item_length > 0 &&
(length_remaining < *item_length)))
*item_length = length_remaining;
}
}
if (*item_length < 0) {
THROW(ReportedBoundsError);
}
}
}
We did all checks and even got THROW(ReportedBoundsError) if payload_len was <
0.
But what about the pass corrected value to the original dissect_bjnp()
function?
For accurate math(for return) at least in case of big positive
numbers(payload_len = 0x7fffffff) which were corrected by
tvb_captured_length_remaining().
static int dissect_bjnp (tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree,
void *data _U_)
{
//...
offset += payload_len; //still has random garbage
}
return offset; //same
}