Question about using "guessed_protocol_id"

[vel21ripn]

Consider a fairly typical option: a tcp connection to port 443.
(This is a very frequent situation for the Telegram protocol)

In this case "guessed_protocol" would be "TLS". Based on the ip address was determined "guessed_protocol_id_by_ip=Telegram".

On the first packet with payload, we determine that it is not "TLS" and it is excluded from further checks.
If the parsing of the tcp stream data failed, then the final result will be "confidence: match by ip" and "proto: TLS.Telegram", which is actually not correct, since "TLS" is missing in this case.

As a solution, you can offer this option:
do not result in "guessed_protocol" if it was excluded (flow->excluded_protocol_bitmask bitmap).

And another question: is the ndpi_detection_giveup() function in the ndpi_main.c file

  if((flow->guessed_protocol_id_by_ip != NDPI_PROTOCOL_UNKNOWN)
     && ((ret.app_protocol == NDPI_PROTOCOL_UNKNOWN) || (ret.master_protocol == NDPI_PROTOCOL_UNKNOWN))) {

    if(ret.app_protocol == NDPI_PROTOCOL_UNKNOWN)
      ndpi_int_change_protocol(ndpi_str, flow,
                               flow->guessed_protocol_id_by_ip, ret.master_protocol,
                               NDPI_CONFIDENCE_MATCH_BY_IP);
    else
      /* master_protocol == NDPI_PROTOCOL_UNKNOWN) */
      ndpi_int_change_protocol(ndpi_str, flow,
                               flow->guessed_protocol_id_by_ip, ret.app_protocol,
                               NDPI_CONFIDENCE_DPI_PARTIAL);

Why is NDPI_CONFIDENCE_DPI_PARTIAL used instead of NDPI_CONFIDENCE_MATCH_BY_IP?

[vel21ripn]

@IvanNardi Please look at the traffic examples from the directory "utils/tcp_check_seq/samples_guessed_protocol" in the repository "vel21ripn/nDPI" branch "tcp_ack_padding". There are full debug output dumps (-V 4).

Note that the TLS protocol is excluded after the first packet containing data has been processed.

It is possible that there is a typo in ndpi_detection_giveup(). NDPI_CONFIDENCE_DPI_PARTIAL is specified instead of NDPI_CONFIDENCE_MATCH_BY_IP.

[IvanNardi]

@vel21ripn , I will try to take a look next week

[IvanNardi]

@vel21ripn, sorry for the delay.
I am going to push a PR that will change that code. In particular, the part about NDPI_CONFIDENCE_MATCH_BY_IP issue will be completely removed; that value of confidence was wrong anyway, just as you noticed.

About the first issue, let me use the same example that you reported: a Telegram connection using TCP over 443. If we are not able to properly classify it, the result TLS with confidence NDPI_CONFIDENCE_MATCH_BY_PORT seems right (with TELEGRAM in the guessed_protocol_id_by_ip field) [after my PR has been merged]

The logic that you are suggesting make sense only when nDPI is always able to classify a protocol, even if only mid-flow is provided: that is possible only for some UDP protocols (SNMP, NETFLOW, maybe DNS...). Please note that this logic is already implemented: see is_udp_not_guessable_protocol().
But we can not safely extend it to TCP/TLS: even if we excluded it, the session might still be TLS, because the dissector recognize it only with the handshake or with packets starting at the TLS Application Block boundary, and that is not always the case.

Please, take a look at #1981

[vel21ripn]

Thanks!