On 15/08/19 23:48, Peter Wu wrote:
The problem was introduced with v3.1.1rc0-144-gede7be3440 ("TLS: allow
dissectors to set the appdata protocol via the data param"). Since that
commit, the "data" parameter of TCP is interpreted as a string.

The problem is that the SCTP dissector can also call the TLS dissector
with a non-NULL data parameter:

     dissector_try_uint_new(sctp_port_dissector_table, high_port, payload_tvb, 
pinfo, tree, TRUE, GUINT_TO_POINTER(ppi)))

This dissector table registration happened in ssl_association_add:

     dissector_add_uint("sctp.port", port, main_handle);

The data parameter is badly overloaded, all I wanted to is to directly
pass data from the EAP dissector to the TLS dissector via
call_dissector_with_data(tls_handle, ...);
Instead we have several things that can go wrong:

  - sctp.port - sctp dissector passes an integer
  - tcp.port - TCP passes a "struct tcpinfo" structure
  - udp.port - UDP passes NULL (ok).

So far I only considered the case where the Lua dissector passes NULL, I
did not think about the above dissector table cases... Meh.

There are at least two ways to fix this:

  - Add an explicit check to ignore the data parameter when invoked
    through the TCP or SCTP dissectors. Disadvantage: any other user that
    adds TLS to their dissector table with non-NULL data will have
    exactly the same issue.
  - Apply my initial approach: do not use the data parameter and instead
    introduce a new function similar to ssl_starttls
    (tls_set_appdata_dissector). That does not reuse existing dissector
    APIs however and is indirect which is why I considered the data
    parameter instead.

João's proposed patch to allow sub-dissectors to pass data via a
hashtable[1] would have a similar affect to the second option, except
that it would require additional code in the TLS dissector to actually
look up the data. Such approaches also do not work if you have nested
TLS traffic for some reason (maybe a VPN tunnel in TLS?).

I would like to understand your concern with encapsulation/nested traffic and [1]. I think the point your are missing, correct me if I'm wrong, is that encapsulation already does not work (for your definition of "not working") with the void data pointer dissector argument and my patch is orthogonal to that issue.

Using a hash table is an indirect method of passing data. A void pointer function argument is a direct method of passing data. So why would the former present problems with nested TLS traffic and the latter not? Any limitations present in one would be present in the other and vice-versa. What am I missing?

What I would like to do is implement a consumer pull model of data passing between dissectors instead of a producer push model. And one that would be Lua friendly too. This seems like a difficult and very time consuming task. One that would require breaking all sorts of compatibility, optimizations and assumptions I suspect.

  [1]: https://code.wireshark.org/review/34049

For now I will consider the first option, but I am open to other

Kind regards,

Sent via:    Wireshark-dev mailing list <wireshark-dev@wireshark.org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev

Reply via email to