Add auxiliary stuff for the COTP protocol

[0xA50C1A1]

Is your feature request related to a problem? Please describe.

I don't think this is a problem now, just wanna suggest a small improvement that will make the code of dissectors of COTP-encapsulated protocols cleaner and easier to understand.

Describe the solution you'd like

Idk, maybe it is worth to add a COTP protocol dissector to replace copypasted TPKT and COTP header checks in the new and existing dissectors with something like this:

if (flow->detected_protocol_stack[0] == NDPI_PROTOCOL_COTP) { do_something(); }

and add to ndpi_packet_struct the most frequently checked COTP header fields like these:

struct ndpi_packet_struct {
  const struct ndpi_iphdr *iph;
  const struct ndpi_ipv6hdr *iphv6;
  const struct ndpi_tcphdr *tcp;
  const struct ndpi_udphdr *udp;
  const u_int8_t *generic_l4_ptr;	/* is set only for non tcp-udp traffic */
  const u_int8_t *payload;

  ....

  u_int8_t cotp_pdu_type;
  u_int8_t cotp_length;
  u_int8_t cotp_is_last_du  : 1;

Additional context

Well, if it's possible to implement, I could give it a try.

[0xA50C1A1]

@IvanNardi @utoni What do you think about it?

[utoni]

If I understood that correctly, you want to call a certain dissector after another protocol dissector at a lower layer successfully classified the flow, right?

That was the initial idea of the "subprotocol" thingy, that I've added a few months (or years? O.o) ago.

[0xA50C1A1]

If I understood that correctly, you want to call a certain dissector after another protocol dissector at a lower layer successfully classified the flow, right?

Yea.

[0xA50C1A1]

I dunno if it would be correct to call encapsulated stuff a sub-protocols (for me it's kind of like HTTP Live Streaming) in that case, since COTP is a transport protocol, even though in today's world it runs on top of the another transport protocol - TCP. 😄

[IvanNardi]

Just to understand the scope of this issue...
If I checked the code correctly, right now we have COTP stuff only for 3 protocols in 2 different dissectors:

• COTP/S7COMM
• COTP/S7COMM_PLUS
• COTP/RDP

In all these cases the identification of the COTP "sub-protocol" is very trivial, basically checking only a few bytes

How many others cases do you think we will have?

[0xA50C1A1]

How many others cases do you think we will have?

Atm I'm working on IEC 61850 MMS protocol dissector, which like some other ICS protocols runs on top of the ISO stack. There's also an IEC 60870-6 TASE.2, but this is more of a special case of MMS.

Well, I'm sure there'll be something else.

[IvanNardi]

@utoni, @0xA50C1A1

In think that we have a few options:

1. change nothing and keep doing as before
2. keep creating a new dissector for each protocol but use a generic helper is_cotp() to avoid code duplication
3. create a new cotp dissector, and add all the sub-classification logic there
4. sub-protocols logic (not sure if it works, since the sub-protocols callbacks are called only once, i.e. only for one packet)

I don't have strong opinions and I am fine with all these options.
If I did the work, I would probably do 3) and switch to 4) in the future if necessary, but, as I said, I am fine in any case

[0xA50C1A1]

@utoni, @0xA50C1A1

In think that we have a few options:

1. change nothing and keep doing as before

2. keep creating a new dissector for each protocol but use a generic helper `is_cotp()` to avoid code duplication

3. create a new cotp dissector, and add all the sub-classification logic there

4. sub-protocols logic (not sure if it works, since the sub-protocols callbacks are called only once, i.e. only for one packet)

I don't have strong opinions and I am fine with all these options. If I did the work, I would probably do 3) and switch to 4) in the future if necessary, but, as I said, I am fine in any case

Hmm, option 2 seems the most optimal to me.

[utoni]

I am also fine with option 2.
One question: Do we want to keep the subprotocol feature as it is or remove it?

[0xA50C1A1]

One question: Do we want to keep the subprotocol feature as it is or remove it?

Yeah, that's definitely worth keeping. There are many protocols working on top of HTTP (IPP for example), so I think the subprotocols feature is great for such cases.

[IvanNardi]

One question: Do we want to keep the subprotocol feature as it is or remove it?

Yeah, that's definitely worth keeping. There are many protocols working on top of HTTP (IPP for example), so I think the subprotocols feature is great for such cases.

+1