whatsapp bug

[vel21ripn]

We have a very strange code in whatsapp.c

 29   u_int8_t whatsapp_sequence[] = {
 30     0x45, 0x44, 0x0, 0x01, 0x0, 0x0, 0x02, 0x08,
 31     0x0, 0x57, 0x41, 0x02, 0x0, 0x0, 0x0
 32   };
 33 
 34   NDPI_LOG_DBG(ndpi_struct, "search WhatsApp\n");
 35 
 36   if(flow->l4.tcp.wa_matched_so_far == 0) {
 37     if(memcmp(packet->payload, whatsapp_sequence, packet->payload_packet_len)) {
 38       NDPI_EXCLUDE_PROTO(ndpi_struct, flow);
 39     } else
 40       flow->l4.tcp.wa_matched_so_far = packet->payload_packet_len;
 41 
 42     return;
 43   } else {
 44     if(memcmp(packet->payload, &whatsapp_sequence[flow->l4.tcp.wa_matched_so_far],
 45               sizeof(whatsapp_sequence)-flow->l4.tcp.wa_matched_so_far))

1. The whatsapp_sequence array must be described as static, otherwise gcc generates array initialization code separately for each byte.
2. Line 37 has two errors: the minimum/maximum packet size is not checked, and the length of the compared region is equal to the packet length, not whatsapp_sequence. What is if payload_packet_len 1500 ?
3. Lines 45 and 46 do something completely strange.
   Are we trying to find a signature, parts of which are in different packets?

 53   if((packet->payload_packet_len > 240)
 54      && (memcmp(packet->payload, whatsapp_sequence, sizeof(whatsapp_sequence)) == 0)) {
 55     NDPI_LOG_INFO(ndpi_struct, "found WhatsApp\n");
 56     ndpi_set_detected_protocol(ndpi_struct, flow, NDPI_PROTOCOL_WHATSAPP, NDPI_PROTOCOL_UNKNOWN);
 57   }

After line 56 must be "return".

[k0ste]

Related vel21ripn#45.

[vel21ripn]

Look at the possible fix in PR #665

[mmanoj]

@vel21ripn

I found below error while running detection for whatapp. Code from git on 5th Feb 2019.

AddressSanitizer: stack-buffer-underflow ??:0 memcmp

#0 0x7f86cf15271f in memcmp (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x7771f)
#1 0x7f86ce818f58 in ndpi_search_whatsapp protocols/whatsapp.c:37
#2 0x7f86ce81455d in check_ndpi_tcp_flow_func /opt/deps/nDPI-HyperScan/nDPI/src/lib/ndpi_main.c:3957
#3 0x7f86ce814ddf in ndpi_detection_process_packet /opt/deps/nDPI-HyperScan/nDPI/src/lib/ndpi_main.c:4573
#4 0x7f86ce805d4e in packet_processing

Thread T2 created by T0 here:
#0 0x7f86cf111253 in pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x36253)

SUMMARY: AddressSanitizer: stack-buffer-underflow ??:0 memcmp
Shadow bytes around the buggy address:
0x0ff15916b640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff15916b650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff15916b660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff15916b670: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff15916b680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ff15916b690: 00 00 00 00[f1]f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2
0x0ff15916b6a0: 04 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2
0x0ff15916b6b0: 04 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2
0x0ff15916b6c0: 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
0x0ff15916b6d0: 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
0x0ff15916b6e0: 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f3 f3 f3 f3
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Heap right redzone: fb
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
==22678==ABORTING

[vel21ripn]

@lucaderi
Please, see PR #665 .
The original code contains duplicate actions (searching for the signature in whole and in parts), a dangerous code (line 37), no return after line 56, after ndpi_set_detected_protocol().

[mmanoj]

@vel21ripn

I'm going to test your code privately local merge to current source and update you the result. As it's not merge to current Git source tree.

[vel21ripn]

I will be very happy if you check PR #665 and in case of successful testing, merge this code with the 'dev' branch.

[mmanoj]

@vel21ripn
Initial test was success, more detail testing in progress. I will update soon possible. For code merge may be Mr.Luca's team can help.

[mmanoj]

@lucaderi

Any plan to merge the code to production? I was tested successfully.