From 5b96d57b18d2d184e503a517b8cbc63ad721ca8e Mon Sep 17 00:00:00 2001 From: John Thacker Date: Thu, 15 Dec 2022 20:36:17 -0500 Subject: [PATCH] protobuf: Do not crash on zero length bytes element If a field name has been written to the json dumper for a bytes element (Base64), then a Base64 value must be written later, even if the value is zero length. Move the JSON_DUMPER_FLAGS_NO_DEBUG flag to the json_dumper header, and use it in the protobuf dissector, so that errors in the JSON dumper state transitions do not abort the application through a ws_error() call. Use DISSECTOR_ASSERT in that case, since it should happen only with a dissector bug (as with the zero bytes elements issue fixed here), not with malformed packets. Only instantiate the json_dumper and create its output string if we intend on displaying its output, instead of doing so whenever we have a message type name. Fix #18730. --- epan/dissectors/packet-protobuf.c | 26 +++++++++++++++++--------- wsutil/json_dumper.c | 1 - wsutil/json_dumper.h | 1 + 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/epan/dissectors/packet-protobuf.c b/epan/dissectors/packet-protobuf.c index 9a8ca22115..3aef28dcc5 100644 --- a/epan/dissectors/packet-protobuf.c +++ b/epan/dissectors/packet-protobuf.c @@ -698,13 +698,13 @@ protobuf_dissect_field_value(proto_tree *value_tree, tvbuff_t *tvb, guint offset case PROTOBUF_TYPE_BYTES: if (field_desc && dumper) { + json_dumper_begin_base64(dumper); buf = (char*) tvb_memdup(wmem_file_scope(), tvb, offset, length); if (buf) { - json_dumper_begin_base64(dumper); json_dumper_write_base64(dumper, buf, length); - json_dumper_end_base64(dumper); wmem_free(wmem_file_scope(), buf); } + json_dumper_end_base64(dumper); } if (field_dissector) { if (!show_details) { /* don't show Value node if there is a subdissector for this field */ @@ -1528,10 +1528,6 @@ dissect_protobuf(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void *data const PbwDescriptor* message_desc = NULL; const gchar* data_str = NULL; gchar *json_str, *p, *q; - json_dumper dumper = { - .output_string = (display_json_mapping ? g_string_new(NULL) : NULL), - .flags = JSON_DUMPER_FLAGS_PRETTY_PRINT, - }; /* initialize only the first time the protobuf dissector is called */ if (!protobuf_dissector_called) { @@ -1620,10 +1616,19 @@ dissect_protobuf(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void *data message_desc = find_message_type_by_udp_port(pinfo); } - dissect_protobuf_message(tvb, offset, tvb_reported_length_remaining(tvb, offset), pinfo, - protobuf_tree, message_desc, -1, pinfo->ptype == PT_UDP, (message_desc ? &dumper : NULL), NULL, NULL); + if (display_json_mapping && message_desc) { + json_dumper dumper = { + .output_string = g_string_new(NULL), + .flags = JSON_DUMPER_FLAGS_PRETTY_PRINT | JSON_DUMPER_FLAGS_NO_DEBUG, + }; - if (display_json_mapping && message_desc && json_dumper_finish(&dumper)) { + /* Dissecting can throw an exception, ideally CLEANUP_PUSH and _POP + * should be used to free the GString to avoid a leak. + */ + dissect_protobuf_message(tvb, offset, tvb_reported_length_remaining(tvb, offset), pinfo, + protobuf_tree, message_desc, -1, pinfo->ptype == PT_UDP, &dumper, NULL, NULL); + + DISSECTOR_ASSERT_HINT(json_dumper_finish(&dumper), "Bad json_dumper state"); ti = proto_tree_add_item(tree, proto_protobuf_json_mapping, tvb, 0, -1, ENC_NA); protobuf_json_tree = proto_item_add_subtree(ti, ett_protobuf_json); @@ -1644,6 +1649,9 @@ dissect_protobuf(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void *data g_free(json_str); } + } else { + dissect_protobuf_message(tvb, offset, tvb_reported_length_remaining(tvb, offset), pinfo, + protobuf_tree, message_desc, -1, pinfo->ptype == PT_UDP, NULL, NULL, NULL); } return tvb_captured_length(tvb); diff --git a/wsutil/json_dumper.c b/wsutil/json_dumper.c index 0a61593389..731594a6d6 100644 --- a/wsutil/json_dumper.c +++ b/wsutil/json_dumper.c @@ -34,7 +34,6 @@ enum json_dumper_element_type { #define JSON_DUMPER_HAS_NAME (1 << 3) #define JSON_DUMPER_FLAGS_ERROR (1 << 16) /* Output flag: an error occurred. */ -#define JSON_DUMPER_FLAGS_NO_DEBUG (1 << 17) /* Input flag: disable debug prints (intended for speeding up fuzzing). */ enum json_dumper_change { JSON_DUMPER_BEGIN, diff --git a/wsutil/json_dumper.h b/wsutil/json_dumper.h index 84a2c0df97..966c1afa76 100644 --- a/wsutil/json_dumper.h +++ b/wsutil/json_dumper.h @@ -55,6 +55,7 @@ typedef struct json_dumper { GString *output_string; /**< Output GLib strings. If it is not NULL, JSON will be dumped in the string. */ #define JSON_DUMPER_FLAGS_PRETTY_PRINT (1 << 0) /* Enable pretty printing. */ #define JSON_DUMPER_DOT_TO_UNDERSCORE (1 << 1) /* Convert dots to underscores in keys */ +#define JSON_DUMPER_FLAGS_NO_DEBUG (1 << 17) /* Disable fatal ws_error messsges on error(intended for speeding up fuzzing). */ int flags; /* for internal use, initialize with zeroes. */ int current_depth;