Re: [Wireshark-dev] payload_proto_id in SCPT dissector
On 17/08/19 11:16, Peter Wu wrote: On Fri, Aug 16, 2019 at 10:09:43AM +0100, João Valverde wrote: 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. The indirect approach naturally assumes a dissector won't behave whimsically about argument passing when called multiple times for the same frame (tunneling, etc). ___ 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] payload_proto_id in SCPT dissector
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
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] payload_proto_id in SCPT dissector
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
suggestions.
Kind regards,
Peter
___
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] payload_proto_id in SCPT dissector
NBAP" },
>
> /* Unassigned 26 */
>
> { X2AP_PAYLOAD_PROTOCOL_ID, "X2AP" },
>
> { IRCP_PAYLOAD_PROTOCOL_ID, "IRCP" },
>
> { LCS_AP_PAYLOAD_PROTOCOL_ID, "LCS-AP" },
>
> { MPICH2_PAYLOAD_PROTOCOL_ID, "MPICH2" },
>
> { SABP_PAYLOAD_PROTOCOL_ID, "SABP" },
>
> { FGP_PAYLOAD_PROTOCOL_ID,"Fractal Generator
> Protocol" },
>
> { PPP_PAYLOAD_PROTOCOL_ID,"Ping Pong Protocol" },
>
> { CALCAPP_PAYLOAD_PROTOCOL_ID,"CalcApp Protocol" },
>
> { SSP_PAYLOAD_PROTOCOL_ID,"Scripting Service
> Protocol" },
>
> { NPMP_CTRL_PAYLOAD_PROTOCOL_ID, "NetPerfMeter Control" },
>
> { NPMP_DATA_PAYLOAD_PROTOCOL_ID, "NetPerfMeter Data" },
>
> { ECHO_PAYLOAD_PROTOCOL_ID, "Echo" },
>
> { DISCARD_PAYLOAD_PROTOCOL_ID,"Discard" },
>
> { DAYTIME_PAYLOAD_PROTOCOL_ID,"Daytime" },
>
> { CHARGEN_PAYLOAD_PROTOCOL_ID,"Character Generator" },
>
> { PROTO_3GPP_RNA_PROTOCOL_ID, "3GPP RNA" },
>
> { PROTO_3GPP_M2AP_PROTOCOL_ID,"3GPP M2AP" },
>
> { PROTO_3GPP_M3AP_PROTOCOL_ID,"3GPP M3AP" },
>
> { SSH_PAYLOAD_PROTOCOL_ID,"SSH" },
>
> { DIAMETER_PROTOCOL_ID, "DIAMETER" },
>
> { DIAMETER_DTLS_PROTOCOL_ID, "DIAMETER OVER DTLS" },
>
> { R14P_BER_PROTOCOL_ID, "R14P" },
>
> { WEBRTC_DCEP_PROTOCOL_ID,"WebRTC Control" },
>
> { WEBRTC_STRING_PAYLOAD_PROTOCOL_ID, "WebRTC String" },
>
> { WEBRTC_BINARY_PARTIAL_PAYLOAD_PROTOCOL_ID, "WebRTC Binary Partial
> (Deprecated)" },
>
> { WEBRTC_BINARY_PAYLOAD_PROTOCOL_ID, "WebRTC Binary" },
>
> { WEBRTC_STRING_PARTIAL_PAYLOAD_PROTOCOL_ID, "WebRTC String Partial
> (Deprecated)" },
>
> { PROTO_3GPP_PUA_PAYLOAD_PROTOCOL_ID, "3GPP PUA" },
>
> { WEBRTC_STRING_EMPTY_PAYLOAD_PROTOCOL_ID,"WebRTC String Empty" },
>
> { WEBRTC_BINARY_EMPTY_PAYLOAD_PROTOCOL_ID,"WebRTC Binary Empty" },
>
> { XWAP_PROTOCOL_ID, "XwAP" },
>
> { XW_CONTROL_PLANE_PROTOCOL_ID, "Xw - Control Plane" },
>
> { NGAP_PROTOCOL_ID, "NGAP" },
>
> { XNAP_PROTOCOL_ID, "XnAP" },
>
> { F1AP_PROTOCOL_ID, "F1 AP" },
>
> { ELE2_PROTOCOL_ID, "ELE2 Lawful
> Interception" },
>
>
>
> { 0, NULL } };
>
>
>
>
>
> Regards
>
> Anders
>
> From: Wireshark-dev On Behalf Of Dario
> Lombardo
> Sent: den 15 augusti 2019 13:55
> To: Developer support list for Wireshark
> Subject: [Wireshark-dev] payload_proto_id in SCPT dissector
>
>
>
> Hi,
>
> I'm working on fixing a bug reported by oss-fuzz, and I got stuck. The
> payload
> is TLS over SCPT.
>
> In the sctp dissector the variable payload_proto_id is read from the tlv in
> dissect_data_chunk. What is it supposed to contain? Which proto_id?
>
> Following the stack trace, this value is passed to dissect_tls as-is. That
> function handles the value as it was a char* containing the name of the
> dissector to find (indeed it is passed to find_dissector).
>
> I can't figure out how this is supposed to work in the first place, or I am
> missing some piece of the puzzle.
>
> Any help?
___
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] payload_proto_id in SCPT dissector
"XwAP" },
{ XW_CONTROL_PLANE_PROTOCOL_ID, "Xw - Control Plane" },
{ NGAP_PROTOCOL_ID, "NGAP" },
{ XNAP_PROTOCOL_ID, "XnAP" },
{ F1AP_PROTOCOL_ID, "F1 AP" },
{ ELE2_PROTOCOL_ID, "ELE2 Lawful
Interception" },
{ 0, NULL } };
Regards
Anders
From: Wireshark-dev On Behalf Of Dario
Lombardo
Sent: den 15 augusti 2019 13:55
To: Developer support list for Wireshark
Subject: [Wireshark-dev] payload_proto_id in SCPT dissector
Hi,
I'm working on fixing a bug reported by oss-fuzz, and I got stuck. The payload
is TLS over SCPT.
In the sctp dissector the variable payload_proto_id is read from the tlv in
dissect_data_chunk. What is it supposed to contain? Which proto_id?
Following the stack trace, this value is passed to dissect_tls as-is. That
function handles the value as it was a char* containing the name of the
dissector to find (indeed it is passed to find_dissector).
I can't figure out how this is supposed to work in the first place, or I am
missing some piece of the puzzle.
Any help?
--
Naima is online.
smime.p7s
Description: S/MIME cryptographic signature
___
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] payload_proto_id in SCPT dissector
Hi, I'm working on fixing a bug reported by oss-fuzz, and I got stuck. The payload is TLS over SCPT. In the sctp dissector the variable payload_proto_id is read from the tlv in dissect_data_chunk. What is it supposed to contain? Which proto_id? Following the stack trace, this value is passed to dissect_tls as-is. That function handles the value as it was a char* containing the name of the dissector to find (indeed it is passed to find_dissector). I can't figure out how this is supposed to work in the first place, or I am missing some piece of the puzzle. Any help? -- Naima is online. ___ 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
