Re: payload_proto_id in SCPT dissector

[Peter Wu <peter () lekensteyn nl>]

On Fri, Aug 16, 2019 at 10:09:43AM +0100, João Valverde wrote:
> 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.

The concern was a situation where the TLS dissector is initially called
with an explicit dissector. Then when HTTPS is within this initial TLS
tunnel, it would still use the old explicit dissector because it has not
been overridden by the new call. This problem will likely exist in any
approach that relies on indirectly passing information (option two or
your patch).

This is a hypothetical case however, I have not run into such a
TLS-over-TLS situation.

> 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?

In a direct approach, the caller either passes data or it passes NULL.
With indirect methods, the caller may pass data, but if it does not,
then the setting from previous layers would be applied, unless every
caller is audited and modified to clear the data. This is the
"unexpected interference" problem I mentioned in the review comments.

Another potential solution is to let the consumer (TLS) clear the data.
That could work with the indirect hashtable approach.

> 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
> > suggestions.

The first option with a blocklist approach feels too fragile, I'll just
revert the patch and try an alternative approach.

Kind regards,
Peter
___________________________________________________________________________
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
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe