Get rid of RDP false positives (#2161)

* Get rid of false positives in the RDP protocol dissector

* Remove kludge for RDP

* RDP: improve detection

---------

Co-authored-by: 0xA50C1A1 <mage.wizard88@gmail.com>
Co-authored-by: Nardi Ivan <nardi.ivan@gmail.com>

6 files changed, 81 insertions, 24 deletions

diff --git a/src/lib/protocols/h323.c b/src/lib/protocols/h323.c
index d7041dfc0..4cafd4392 100644
--- a/src/lib/protocols/h323.c
+++ b/src/lib/protocols/h323.c
@@ -54,21 +54,6 @@ static void ndpi_search_h323(struct ndpi_detection_module_struct *ndpi_struct, s
       u_int16_t len = ntohs(t->len);
 
       if(packet->payload_packet_len == len) {
-	/*
-	  We need to check if this packet is in reality
-	  a RDP (Remote Desktop) packet encapsulated on TPTK
-	*/
-
-	if(packet->payload[4] == (packet->payload_packet_len - sizeof(struct tpkt) - 1)) {
-	  /* ISO 8073/X.224 */
-	  if((packet->payload[5] == 0xE0 /* CC Connect Request */)
-	     || (packet->payload[5] == 0xD0 /* CC Connect Confirm */)) {
-	    NDPI_LOG_INFO(ndpi_struct, "found RDP\n");
-	    ndpi_set_detected_protocol(ndpi_struct, flow, NDPI_PROTOCOL_RDP, NDPI_PROTOCOL_UNKNOWN, NDPI_CONFIDENCE_DPI);
-	    return;
-	  }
-	}
-
 	flow->h323_valid_packets++;
 
 	if(flow->h323_valid_packets >= 2) {
diff --git a/src/lib/protocols/rdp.c b/src/lib/protocols/rdp.c
index 305c1c27f..e95c6e853 100644
--- a/src/lib/protocols/rdp.c
+++ b/src/lib/protocols/rdp.c
@@ -46,16 +46,32 @@ static void ndpi_search_rdp(struct ndpi_detection_module_struct *ndpi_struct,
   NDPI_LOG_DBG(ndpi_struct, "search RDP\n");
 
   if (packet->tcp != NULL) {
-    if (packet->payload_packet_len > 10
-	&& get_u_int8_t(packet->payload, 0) > 0
-	&& get_u_int8_t(packet->payload, 0) < 4 && get_u_int16_t(packet->payload, 2) == ntohs(packet->payload_packet_len)
-	&& get_u_int8_t(packet->payload, 4) == packet->payload_packet_len - 5
-	&& get_u_int8_t(packet->payload, 5) == 0xe0
-	&& get_u_int16_t(packet->payload, 6) == 0 && get_u_int16_t(packet->payload, 8) == 0 && get_u_int8_t(packet->payload, 10) == 0) {
-      ndpi_int_rdp_add_connection(ndpi_struct, flow);
-      return;
-    }
+    if(packet->payload_packet_len > 13 &&
+       /* TPKT */
+       packet->payload[0] == 0x03 && packet->payload[1] == 0x00 &&
+       ntohs(*(uint16_t *)&packet->payload[2]) == packet->payload_packet_len &&
+       /* COTP */
+       packet->payload[4] == packet->payload_packet_len - 5) {
 
+      if(current_pkt_from_client_to_server(ndpi_struct, flow)) {
+        if(packet->payload[5] == 0xE0 && /* COTP CR */
+	   ((packet->payload[11] == 0x01 && /* RDP Negotiation Request */
+             packet->payload[13] == 0x08 /* RDP Length */) ||
+	    (packet->payload_packet_len > 17 &&
+	     memcmp(&packet->payload[11], "Cookie:", 7) == 0))) /* RDP Cookie */ {
+          ndpi_int_rdp_add_connection(ndpi_struct, flow);
+          return;
+	}
+      } else {
+        /* Asymmetric detection via RDP Negotiation Response */
+        if(packet->payload[5] == 0xD0 && /* COTP CC */
+	   packet->payload[11] == 0x02 && /* RDP Negotiation Response */
+           packet->payload[13] == 0x08 /* RDP Length */) {
+          ndpi_int_rdp_add_connection(ndpi_struct, flow);
+          return;
+	}
+      }
+    }
     NDPI_EXCLUDE_PROTO(ndpi_struct, flow);
   } else if(packet->udp != NULL) {
     u_int16_t s_port = ntohs(packet->udp->source);
diff --git a/tests/cfgs/default/pcap/rdp3.pcap b/tests/cfgs/default/pcap/rdp3.pcap
new file mode 100644
index 000000000..9a1ed4cbf
--- /dev/null
+++ b/tests/cfgs/default/pcap/rdp3.pcap
diff --git a/tests/cfgs/default/pcap/s7comm-plus.pcap b/tests/cfgs/default/pcap/s7comm-plus.pcap
new file mode 100644
index 000000000..8dfb033ce
--- /dev/null
+++ b/tests/cfgs/default/pcap/s7comm-plus.pcap
diff --git a/tests/cfgs/default/result/rdp3.pcap.out b/tests/cfgs/default/result/rdp3.pcap.out
new file mode 100644
index 000000000..d7f61a572
--- /dev/null
+++ b/tests/cfgs/default/result/rdp3.pcap.out
@@ -0,0 +1,28 @@
+Guessed flow protos:	0
+
+DPI Packets (TCP):	4	(4.00 pkts/flow)
+Confidence DPI              : 1 (flows)
+Num dissector calls: 1 (1.00 diss/flow)
+LRU cache ookla:      0/0/0 (insert/search/found)
+LRU cache bittorrent: 0/0/0 (insert/search/found)
+LRU cache zoom:       0/0/0 (insert/search/found)
+LRU cache stun:       0/0/0 (insert/search/found)
+LRU cache tls_cert:   0/0/0 (insert/search/found)
+LRU cache mining:     0/0/0 (insert/search/found)
+LRU cache msteams:    0/0/0 (insert/search/found)
+LRU cache stun_zoom:  0/0/0 (insert/search/found)
+Automa host:          0/0 (search/found)
+Automa domain:        0/0 (search/found)
+Automa tls cert:      0/0 (search/found)
+Automa risk mask:     0/0 (search/found)
+Automa common alpns:  0/0 (search/found)
+Patricia risk mask:   0/0 (search/found)
+Patricia risk mask IPv6: 0/0 (search/found)
+Patricia risk:        0/0 (search/found)
+Patricia risk IPv6:   0/0 (search/found)
+Patricia protocols:   2/0 (search/found)
+Patricia protocols IPv6: 0/0 (search/found)
+
+RDP	30	4151	1
+
+	1	TCP 10.150.9.21:1685 <-> 10.157.4.161:3389 [proto: 88/RDP][IP: 0/Unknown][ClearText][Confidence: DPI][DPI packets: 4][cat: RemoteAccess/12][17 pkts/2567 bytes <-> 13 pkts/1584 bytes][Goodput ratio: 63/54][0.67 sec][bytes ratio: 0.237 (Upload)][IAT c2s/s2c min/avg/max/stddev: 0/1 44/54 234/331 66/93][Pkt Len c2s/s2c min/avg/max/stddev: 60/60 151/122 573/440 162/126][Risk: ** Desktop/File Sharing **][Risk Score: 10][Risk Info: Found RDP][PLAIN TEXT (Cookie)][Plen Bins: 59,16,4,0,0,0,0,0,0,0,4,0,12,0,0,0,4,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]
diff --git a/tests/cfgs/default/result/s7comm-plus.pcap.out b/tests/cfgs/default/result/s7comm-plus.pcap.out
new file mode 100644
index 000000000..4b34c344e
--- /dev/null
+++ b/tests/cfgs/default/result/s7comm-plus.pcap.out
@@ -0,0 +1,28 @@
+Guessed flow protos:	0
+
+DPI Packets (TCP):	6	(6.00 pkts/flow)
+Confidence DPI              : 1 (flows)
+Num dissector calls: 1 (1.00 diss/flow)
+LRU cache ookla:      0/0/0 (insert/search/found)
+LRU cache bittorrent: 0/0/0 (insert/search/found)
+LRU cache zoom:       0/0/0 (insert/search/found)
+LRU cache stun:       0/0/0 (insert/search/found)
+LRU cache tls_cert:   0/0/0 (insert/search/found)
+LRU cache mining:     0/0/0 (insert/search/found)
+LRU cache msteams:    0/0/0 (insert/search/found)
+LRU cache stun_zoom:  0/0/0 (insert/search/found)
+Automa host:          0/0 (search/found)
+Automa domain:        0/0 (search/found)
+Automa tls cert:      0/0 (search/found)
+Automa risk mask:     0/0 (search/found)
+Automa common alpns:  0/0 (search/found)
+Patricia risk mask:   0/0 (search/found)
+Patricia risk mask IPv6: 0/0 (search/found)
+Patricia risk:        0/0 (search/found)
+Patricia risk IPv6:   0/0 (search/found)
+Patricia protocols:   4/0 (search/found)
+Patricia protocols IPv6: 0/0 (search/found)
+
+s7comm	79	10271	1
+
+	1	TCP 192.168.25.177:53162 <-> 192.168.25.131:102 [proto: 249/s7comm][IP: 0/Unknown][ClearText][Confidence: DPI][DPI packets: 6][cat: Network/14][54 pkts/6194 bytes <-> 25 pkts/4077 bytes][Goodput ratio: 53/65][7.11 sec][bytes ratio: 0.206 (Upload)][IAT c2s/s2c min/avg/max/stddev: 0/0 122/276 995/964 315/396][Pkt Len c2s/s2c min/avg/max/stddev: 54/60 115/163 395/351 76/132][PLAIN TEXT (SIMATIC)][Plen Bins: 42,6,28,3,1,0,3,0,0,12,3,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]