Add an heuristic to detect/ignore some anomalous TCP ACK packets

[IvanNardi]

In some networks, there are some anomalous TCP flows where the smallest
ACK packets have some kind of zero padding.
It looks like the IP and TCP headers in those frames wrongly consider the
0x00 Ethernet padding bytes as part of the TCP payload.
While this kind of packets is perfectly valid per-se, in some conditions
they might be treated by the TCP reassembler logic as (partial) overlaps,
deceiving the classification engine.
Add an heuristic to detect these packets and to ignore them, allowing
correct detection/classification.

This heuristic is configurable. Default value:

• in the library, it is disabled
• in ndpiReader and in the fuzzers, it is enabled (to ease testing)

Credit to @vel21ripn for the initial patch.

Close #1946

[IvanNardi]

Still a proof-of-concept. Some notes:

• it works for all kinds of TCP traffic
• the check is strict enough that it is never triggered with the test traces

Missing:

• configuration to enable/disable
• example

[vel21ripn]

I selected 3 good traffic examples for testing #1498.
Look in the "padding" branch of my repository in the utils/tcp_check_seq/samples_for_tests directory.

[IvanNardi]

I selected 3 good traffic examples for testing #1498. Look in the "padding" branch of my repository in the utils/tcp_check_seq/samples_for_tests directory.

I quickly tested the traces in your repository and it seems this patch works, IMO.
Do you agree?

[vel21ripn]

Yes! Your version works better.
You need to rename the traffic sample files for the tests and add them to the rest of the tests.

[IvanNardi]

@vel21ripn , thanks for the confirmation
I'll push a complete version

[vel21ripn]

I found another option for incorrect packet processing.

12:41:28.015038 IP 194.226.199.103.62580 > 217.69.139.59.443: Flags [S], seq 4211199516, win 8192, options [mss 1460,nop,wscale 8,nop,nop,sackOK], length 0
12:41:28.025503 IP 217.69.139.59.443 > 194.226.199.103.62580: Flags [S.], seq 1907400742:1907400744, ack 4211199517, win 14100, options [mss 1410], length 2
12:41:29.574311 IP 217.69.139.59.443 > 194.226.199.103.62580: Flags [S.], seq 1907400742:1907400744, ack 4211199517, win 14100, options [mss 1410], length 2
12:41:29.574986 IP 194.226.199.103.62580 > 217.69.139.59.443: Flags [.], seq 1:3, ack 1, win 64860, length 2

SYN+ACK with length 2

The second packet (syn+ack) gets into the non-empty packet handler.
This patch fixes the error, but perhaps there is a more accurate version.

diff --git a/src/lib/ndpi_main.c b/src/lib/ndpi_main.c
index 12efcee..0837b83 100644
--- a/src/lib/ndpi_main.c
+++ b/src/lib/ndpi_main.c
@@ -5617,6 +5617,10 @@ void ndpi_connection_tracking(struct ndpi_detection_module_struct *ndpi_str,
          if(flow->num_processed_pkts > 1)
            flow->next_tcp_seq_nr[1 - packet->packet_direction] = ntohl(tcph->ack_seq);
        }
+        if(tcph->syn) {
+           NDPI_LOG_DBG2(ndpi_str, "TCP SYN with zero padding. Ignored\n");
+          packet->tcp_retransmission = 1;
+        }
       } else if(packet->payload_packet_len > 0) {
        /* check tcp sequence counters */
        if(tcp_ack_padding(packet)) {

[IvanNardi]

I found another option for incorrect packet processing.

12:41:28.015038 IP 194.226.199.103.62580 > 217.69.139.59.443: Flags [S], seq 4211199516, win 8192, options [mss 1460,nop,wscale 8,nop,nop,sackOK], length 0
12:41:28.025503 IP 217.69.139.59.443 > 194.226.199.103.62580: Flags [S.], seq 1907400742:1907400744, ack 4211199517, win 14100, options [mss 1410], length 2
12:41:29.574311 IP 217.69.139.59.443 > 194.226.199.103.62580: Flags [S.], seq 1907400742:1907400744, ack 4211199517, win 14100, options [mss 1410], length 2
12:41:29.574986 IP 194.226.199.103.62580 > 217.69.139.59.443: Flags [.], seq 1:3, ack 1, win 64860, length 2

SYN+ACK with length 2

The second packet (syn+ack) gets into the non-empty packet handler. This patch fixes the error, but perhaps there is a more accurate version.

diff --git a/src/lib/ndpi_main.c b/src/lib/ndpi_main.c
index 12efcee..0837b83 100644
--- a/src/lib/ndpi_main.c
+++ b/src/lib/ndpi_main.c
@@ -5617,6 +5617,10 @@ void ndpi_connection_tracking(struct ndpi_detection_module_struct *ndpi_str,
          if(flow->num_processed_pkts > 1)
            flow->next_tcp_seq_nr[1 - packet->packet_direction] = ntohl(tcph->ack_seq);
        }
+        if(tcph->syn) {
+           NDPI_LOG_DBG2(ndpi_str, "TCP SYN with zero padding. Ignored\n");
+          packet->tcp_retransmission = 1;
+        }
       } else if(packet->payload_packet_len > 0) {
        /* check tcp sequence counters */
        if(tcp_ack_padding(packet)) {

@vel21ripn , are you able to share a pcap with this new pattern?

[vel21ripn]

I found an even more interesting example of traffic.
Look in the "padding" branch of my repository in the utils/tcp_check_seq/samples_syn_retrans directory.

[IvanNardi]

I found an even more interesting example of traffic. Look in the "padding" branch of my repository in the utils/tcp_check_seq/samples_syn_retrans directory.

In vel21ripn@6f0c659 there are not SYN-ACK packets with len != 0...
(yes, these 2 traces are not properly classified because of issues with the reassembler code, but it seems another, unrelated issue compared to #1946)

[vel21ripn]

I added a sample traffic with syn+ack+padding

[IvanNardi]

I added a sample traffic with syn+ack+padding

Pushed a new version handling the SYN/ACK case

[IvanNardi]

Even the other 2 traces seems working now

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[utoni]

Good catch! :)

[IvanNardi]

@vel21ripn , what do you think? Can we merge this PR?

[vel21ripn]

Yes.