Wireshark · Wireshark-dev: Re: [Wireshark-dev] payload_proto_id in SCPT dissector

[Peter Wu <peter@xxxxxxxxxxxxx>]

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

 [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

On Thu, Aug 15, 2019 at 12:09:33PM +0000, Anders Broman wrote:
> Hi,
> 
> It is the SCTP PPI which is defined by IANA
> 
> Payload protocol identifier: M3UA (3)
> 
>     { &hf_data_chunk_payload_proto_id,              { "Payload protocol 
> identifier",                    "sctp.data_payload_proto_id", 
> FT_UINT32,  BASE_DEC,  VALS(sctp_payload_proto_id_values),             0x0, 
> NULL, HFILL } },
> 
> 
> 
> You find it in sctpppids.h and
> 
> static const value_string sctp_payload_proto_id_values[] = {
> 
>   { NOT_SPECIFIED_PROTOCOL_ID,                      "not specified" },
> 
>   { IUA_PAYLOAD_PROTOCOL_ID,                        "IUA" },
> 
>   { M2UA_PAYLOAD_PROTOCOL_ID,                       "M2UA" },
> 
>   { M3UA_PAYLOAD_PROTOCOL_ID,                       "M3UA" },
> 
>   { SUA_PAYLOAD_PROTOCOL_ID,                        "SUA" },
> 
>   { M2PA_PAYLOAD_PROTOCOL_ID,                       "M2PA" },
> 
>   { V5UA_PAYLOAD_PROTOCOL_ID,                       "V5UA" },
> 
>   { H248_PAYLOAD_PROTOCOL_ID,                       "H.248/MEGACO" },
> 
>   { BICC_PAYLOAD_PROTOCOL_ID,                       "BICC/Q.2150.3" },
> 
>   { TALI_PAYLOAD_PROTOCOL_ID,                       "TALI" },
> 
>   { DUA_PAYLOAD_PROTOCOL_ID,                        "DUA" },
> 
>   { ASAP_PAYLOAD_PROTOCOL_ID,                       "ASAP" },
> 
>   { ENRP_PAYLOAD_PROTOCOL_ID,                       "ENRP" },
> 
>   { H323_PAYLOAD_PROTOCOL_ID,                       "H.323" },
> 
>   { QIPC_PAYLOAD_PROTOCOL_ID,                       "Q.IPC/Q.2150.3" },
> 
>   { SIMCO_PAYLOAD_PROTOCOL_ID,                      "SIMCO" },
> 
>   { DDP_SEG_CHUNK_PROTOCOL_ID,                      "DDP Segment Chunk" },
> 
>   { DDP_STREAM_SES_CTRL_PROTOCOL_ID,                "DDP Stream Session 
> Control" },
> 
>   { S1AP_PAYLOAD_PROTOCOL_ID,                       "S1 Application Protocol 
> (S1AP)" },
> 
>   { RUA_PAYLOAD_PROTOCOL_ID,                        "RUA" },
> 
>   { HNBAP_PAYLOAD_PROTOCOL_ID,                      "HNBAP" },
> 
>   { FORCES_HP_PAYLOAD_PROTOCOL_ID,                  "ForCES-HP" },
> 
>   { FORCES_MP_PAYLOAD_PROTOCOL_ID,                  "ForCES-MP" },
> 
>   { FORCES_LP_PAYLOAD_PROTOCOL_ID,                  "ForCES-LP" },
> 
>   { SBC_AP_PAYLOAD_PROTOCOL_ID,                     "SBc-AP" },
> 
>   { NBAP_PAYLOAD_PROTOCOL_ID,                       "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 <wireshark-dev-bounces@xxxxxxxxxxxxx> On Behalf Of Dario 
> Lombardo
> Sent: den 15 augusti 2019 13:55
> To: Developer support list for Wireshark <wireshark-dev@xxxxxxxxxxxxx>
> 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?