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 <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Martin Kaiser <wireshark@kaiser.cx>
This commit is contained in:
Peter Wu 2016-10-21 00:50:03 +02:00 committed by Martin Kaiser
parent e80a8acbe3
commit 7dfaec969e
1 changed files with 18 additions and 13 deletions

View File

@ -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,