From 7dfaec969e67e3aa14b9763d804802ef614c9ddd Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Fri, 21 Oct 2016 00:50:03 +0200 Subject: [PATCH] alljoyn: fix signature length adjustments Ensure that the signature pointer and length always matches, otherwise a buffer overrun (read) is possible. Tested with the original captures from bug 12953, the PDML output is still the same while the fuzzed capture does not crash anymore. Bug: 12953 Change-Id: I8843a5daf98a79fb19906e824326cdf619164484 Reviewed-on: https://code.wireshark.org/review/18347 Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Martin Kaiser --- epan/dissectors/packet-alljoyn.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/epan/dissectors/packet-alljoyn.c b/epan/dissectors/packet-alljoyn.c index fd9b1dd93e..76dccb9ce1 100644 --- a/epan/dissectors/packet-alljoyn.c +++ b/epan/dissectors/packet-alljoyn.c @@ -759,8 +759,9 @@ advance_to_end_of_signature(const guint8 **signature, gint8 current_type; gint8 end_type = ARG_INVALID; - while(*(++(*signature)) && --(*signature_length) > 0 && !done) { - current_type = **signature; + while (*signature_length > 0 && **signature && !done) { + current_type = *(++(*signature)); + --*signature_length; /* Were we looking for the end of a structure or dictionary? If so, did we find it? */ if(end_type != ARG_INVALID) { @@ -892,7 +893,6 @@ parse_arg(tvbuff_t *tvb, const guint8 *sig_saved; gint starting_offset; gint number_of_items = 0; - guint8 remaining_sig_length = *signature_length; gint packet_length = (gint)tvb_reported_length(tvb); header_type_name = "array"; @@ -928,14 +928,17 @@ parse_arg(tvbuff_t *tvb, add_padding_item(padding_start, offset, tvb, tree); if(0 == length) { - advance_to_end_of_signature(signature, &remaining_sig_length); + advance_to_end_of_signature(signature, signature_length); } else { + guint8 sig_length_saved = *signature_length - 1; + while((offset - starting_offset) < length) { const guint8 *sig_pointer; + guint8 remaining_sig_length; number_of_items++; sig_pointer = sig_saved; - remaining_sig_length = *signature_length - 1; + remaining_sig_length = sig_length_saved; offset = parse_arg(tvb, pinfo, @@ -952,11 +955,10 @@ parse_arg(tvbuff_t *tvb, /* Set the signature pointer to be just past the type just handled. */ *signature = sig_pointer; + *signature_length = remaining_sig_length; } } - *signature_length = remaining_sig_length; - if(item) { proto_item_append_text(item, " of %d '%c' elements", number_of_items, *sig_saved); } @@ -985,23 +987,26 @@ parse_arg(tvbuff_t *tvb, case ARG_SIGNATURE: /* AllJoyn signature basic type */ header_type_name = "signature"; - *signature_length = tvb_get_guint8(tvb, offset); + length = tvb_get_guint8(tvb, offset); - if(*signature_length + 2 > tvb_reported_length_remaining(tvb, offset)) { + if (length + 2 > tvb_reported_length_remaining(tvb, offset)) { gint bytes_left = tvb_reported_length_remaining(tvb, offset); col_add_fstr(pinfo->cinfo, COL_INFO, "BAD DATA: Signature length is %d. Only %d bytes left in packet.", - (gint)(*signature_length), bytes_left); + length, bytes_left); return tvb_reported_length(tvb); } /* Include the terminating '/0'. */ - length = *signature_length + 1; + length++; proto_tree_add_item(field_tree, hf_alljoyn_mess_body_signature_length, tvb, offset, 1, encoding); offset += 1; + /* Extract signature from tvb and return to caller. */ + /* XXX should this extract "length - 1" since we always expect /0? */ proto_tree_add_item_ret_string(field_tree, hf_alljoyn_mess_body_signature, tvb, offset, length, ENC_ASCII|ENC_NA, wmem_packet_scope(), signature); + *signature_length = length; if(HDR_SIGNATURE == field_code) { col_append_fstr(pinfo->cinfo, COL_INFO, " (%s)", *signature); @@ -1268,7 +1273,7 @@ parse_arg(tvbuff_t *tvb, break; } - if(*signature && ARG_ARRAY != type_id && HDR_INVALID == field_code) { + if (*signature && *signature_length > 0 && ARG_ARRAY != type_id && HDR_INVALID == field_code) { (*signature)++; (*signature_length)--; } @@ -1453,7 +1458,7 @@ handle_message_body_parameters(tvbuff_t *tvb, end_of_body = packet_length; } - while(offset < end_of_body && signature && *signature) { + while(offset < end_of_body && signature_length > 0 && signature && *signature) { offset = parse_arg(tvb, pinfo, NULL,