Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Anders Broman]

Hi,
Yes the method is fragile. At the time of development I think it was
proposed to pass a struct containing a string and the void pointer where
the string could be used as a identifier. But that was voted down.
Regards
Anders

Den lör 29 maj 2021 09:26Guy Harris  skrev:

> On May 29, 2021, at 12:12 AM, Anders Broman  wrote:
>
> > Shouldn't the caller be calling with the right data type or NULL? So a
> bug in the MQTT disector?
>
> How can the MQTT dissector determine what the right data type *is* -
> especially given that the dissectors aren't wired in, there's a UAT
> preference that lets the user configure it.
>
> This is where the current mechanism for passing data between dissectors
> goes crashing to the ground.
>
> MQTT passes a topic string, which is just a string, to the dissectors it
> calls.
>
> JSON expects to be passed a pointer to an http_message_info_t.
>
> JSON registers its non-heuristic dissector by name, and allows it to be
> used with Decode As... for UDP ports.
>
> It might *look* safe if you check the UDP dissector and the dissectors
> that use "media_type" and "grpc_message_type", but the "registers its
> non-heuristic dissector by name" mean there are no guarantees, given that
> another dissector that passes a pointer to something *other* than an
> http_message_info_t to dissectors that are specified by name in a UAT.
> ___
> Sent via:Wireshark-dev mailing list 
> Archives:https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>  mailto:wireshark-dev-requ...@wireshark.org
> ?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Guy Harris]

On May 29, 2021, at 12:12 AM, Anders Broman  wrote:

> Shouldn't the caller be calling with the right data type or NULL? So a bug in 
> the MQTT disector?

How can the MQTT dissector determine what the right data type *is* - especially 
given that the dissectors aren't wired in, there's a UAT preference that lets 
the user configure it.

This is where the current mechanism for passing data between dissectors goes 
crashing to the ground.

MQTT passes a topic string, which is just a string, to the dissectors it calls.

JSON expects to be passed a pointer to an http_message_info_t.

JSON registers its non-heuristic dissector by name, and allows it to be used 
with Decode As... for UDP ports.

It might *look* safe if you check the UDP dissector and the dissectors that use 
"media_type" and "grpc_message_type", but the "registers its non-heuristic 
dissector by name" mean there are no guarantees, given that another dissector 
that passes a pointer to something *other* than an http_message_info_t to 
dissectors that are specified by name in a UAT.
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Uli Heilmeier]

Makes sense for me. I will change the MQTT dissector to call 
call_dissector_with_data() only for sparkplug payload.

> Am 29.05.2021 um 09:12 schrieb Anders Broman  >:
> 
> Hi,
> Shouldn't the caller be calling with the right data type or NULL? So a bug in 
> the MQTT disector?
> Regards
> Anders
> 
> 
> Den lör 29 maj 2021 09:07Uli Heilmeier  > skrev:
> With MR 2706 [1] the MQTT dissector calls now subdissectors with 
> call_dissector_with_data() [2]. Previously this was call_dissector().
> 
> With this change the JSON dissector crashes WS with a memory access segfault 
> (while using MQTT decode topic as option).
> This is because for JSON we expect http_message_info_t for data pointer [3]. 
> MQTT hands over a pointer to a char array (topic_str) for data.
> 
> How do we handle data type for the data parameter pointer? Would it make 
> sense to compare at least the size of data with the size of 
> http_message_info_t?
> 
> I don’t know any way to check the data type of a pointer address location.
> 
> Cheers
> Uli
> 
> [1]: https://gitlab.com/wireshark/wireshark/-/merge_requests/2706/ 
> 
> [2]: 
> https://gitlab.com/wireshark/wireshark/-/merge_requests/2706//diffs#07171e000b3caaabee32ab5dc04a5d28efbaaae3_755_755
>  
> 
> [3]: 
> https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-json.c#L221
>  
> 
> ___
> Sent via:Wireshark-dev mailing list  >
> Archives:https://www.wireshark.org/lists/wireshark-dev 
> 
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev 
> 
>  mailto:wireshark-dev-requ...@wireshark.org 
> ?subject=unsubscribe
> ___
> Sent via:Wireshark-dev mailing list  >
> Archives:https://www.wireshark.org/lists/wireshark-dev 
> 
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev 
> 
> mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe 
> 

___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Calling a dissector: Type for data parameter

[Anders Broman]

Hi,
Shouldn't the caller be calling with the right data type or NULL? So a bug
in the MQTT disector?
Regards
Anders


Den lör 29 maj 2021 09:07Uli Heilmeier  skrev:

> With MR 2706 [1] the MQTT dissector calls now subdissectors with
> call_dissector_with_data() [2]. Previously this was call_dissector().
>
> With this change the JSON dissector crashes WS with a memory access
> segfault (while using MQTT decode topic as option).
> This is because for JSON we expect http_message_info_t for data pointer
> [3]. MQTT hands over a pointer to a char array (topic_str) for data.
>
> How do we handle data type for the data parameter pointer? Would it make
> sense to compare at least the size of data with the size of
> http_message_info_t?
>
> I don’t know any way to check the data type of a pointer address location.
>
> Cheers
> Uli
>
> [1]: https://gitlab.com/wireshark/wireshark/-/merge_requests/2706/
> [2]:
> https://gitlab.com/wireshark/wireshark/-/merge_requests/2706//diffs#07171e000b3caaabee32ab5dc04a5d28efbaaae3_755_755
> [3]:
> https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-json.c#L221
> ___
> Sent via:Wireshark-dev mailing list 
> Archives:https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>  mailto:wireshark-dev-requ...@wireshark.org
> ?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

[Wireshark-dev] Calling a dissector: Type for data parameter

[Uli Heilmeier]

With MR 2706 [1] the MQTT dissector calls now subdissectors with 
call_dissector_with_data() [2]. Previously this was call_dissector().

With this change the JSON dissector crashes WS with a memory access segfault 
(while using MQTT decode topic as option).
This is because for JSON we expect http_message_info_t for data pointer [3]. 
MQTT hands over a pointer to a char array (topic_str) for data.

How do we handle data type for the data parameter pointer? Would it make sense 
to compare at least the size of data with the size of http_message_info_t?

I don’t know any way to check the data type of a pointer address location.

Cheers
Uli

[1]: https://gitlab.com/wireshark/wireshark/-/merge_requests/2706/
[2]: 
https://gitlab.com/wireshark/wireshark/-/merge_requests/2706//diffs#07171e000b3caaabee32ab5dc04a5d28efbaaae3_755_755
[3]: 
https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-json.c#L221
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe