From 6edb7bedd7b5c1eb0060f5d503f9e37a6d58c086 Mon Sep 17 00:00:00 2001
From: Ivan Nardi <12729895+IvanNardi@users.noreply.github.com>
Date: Wed, 27 Oct 2021 09:23:07 +0200
Subject: Avoid overwriting valid protocol in `ndpi_detection_giveup` (#1360)
We should avoid updating any valid protocol in `ndpi_detection_giveup`; we
should try to find a proper classification only if the flow is still
completely unclassified.
For example in the attached pcap there is a valid TLS session, recognized
as such by TLS dissector. However, the `ndpi_detection_giveup`function
updates it to "HTTP/TLS" (!?) simply because the server port is 80.
Note that the real issue is not the wrong classification, but the
wrong access to `flow->protos` union. If we already set some fields of
`flow->protos` and we change the protocol in `ndpi_detection_giveup`, we
might end up freeing some invalid pointers in `ndpi_free_flow_data`
(no wonder this issue has been found while fuzzing #1354)
Fix GIT and TLS dissectors (issues found by CI fuzzer)
---
src/lib/ndpi_main.c | 112 ++++++++++++++++++++++++------------------------
src/lib/protocols/git.c | 7 ++-
src/lib/protocols/tls.c | 2 +-
3 files changed, 61 insertions(+), 60 deletions(-)
(limited to 'src')
diff --git a/src/lib/ndpi_main.c b/src/lib/ndpi_main.c
index 1fa2445de..7baa96be2 100644
--- a/src/lib/ndpi_main.c
+++ b/src/lib/ndpi_main.c
@@ -4886,6 +4886,7 @@ static void ndpi_reconcile_protocols(struct ndpi_detection_module_struct *ndpi_s
ndpi_protocol ndpi_detection_giveup(struct ndpi_detection_module_struct *ndpi_str, struct ndpi_flow_struct *flow,
u_int8_t enable_guess, u_int8_t *protocol_was_guessed) {
ndpi_protocol ret = {NDPI_PROTOCOL_UNKNOWN, NDPI_PROTOCOL_UNKNOWN, NDPI_PROTOCOL_CATEGORY_UNSPECIFIED};
+ u_int16_t guessed_protocol_id = NDPI_PROTOCOL_UNKNOWN, guessed_host_protocol_id = NDPI_PROTOCOL_UNKNOWN;
/*
*** We can't access ndpi_str->packet from this function!! ***
@@ -4901,7 +4902,7 @@ ndpi_protocol ndpi_detection_giveup(struct ndpi_detection_module_struct *ndpi_st
ret.category = flow->category;
/* Ensure that we don't change our mind if detection is already complete */
- if((ret.master_protocol != NDPI_PROTOCOL_UNKNOWN) && (ret.app_protocol != NDPI_PROTOCOL_UNKNOWN))
+ if(ret.app_protocol != NDPI_PROTOCOL_UNKNOWN)
return(ret);
/* TODO: this lookup seems in the wrong place here...
@@ -4917,66 +4918,63 @@ ndpi_protocol ndpi_detection_giveup(struct ndpi_detection_module_struct *ndpi_st
}
}
- /* TODO: add the remaining stage_XXXX protocols */
- if(flow->detected_protocol_stack[0] == NDPI_PROTOCOL_UNKNOWN) {
- u_int16_t guessed_protocol_id = NDPI_PROTOCOL_UNKNOWN, guessed_host_protocol_id = NDPI_PROTOCOL_UNKNOWN;
+ if(flow->guessed_protocol_id == NDPI_PROTOCOL_STUN)
+ goto check_stun_export;
+ else if((flow->guessed_protocol_id == NDPI_PROTOCOL_HANGOUT_DUO) ||
+ (flow->guessed_protocol_id == NDPI_PROTOCOL_MESSENGER) ||
+ (flow->guessed_protocol_id == NDPI_PROTOCOL_WHATSAPP_CALL)) {
+ *protocol_was_guessed = 1;
+ ndpi_set_detected_protocol(ndpi_str, flow, flow->guessed_protocol_id, NDPI_PROTOCOL_UNKNOWN);
+ }
+ else if((flow->protos.tls_quic_stun.tls_quic.hello_processed == 1) &&
+ (flow->protos.tls_quic_stun.tls_quic.client_requested_server_name[0] != '\0')) {
+ *protocol_was_guessed = 1;
+ ndpi_set_detected_protocol(ndpi_str, flow, NDPI_PROTOCOL_TLS, NDPI_PROTOCOL_UNKNOWN);
+ } else if(enable_guess) {
+ if((flow->guessed_protocol_id == NDPI_PROTOCOL_UNKNOWN) && (flow->l4_proto == IPPROTO_TCP) &&
+ flow->protos.tls_quic_stun.tls_quic.hello_processed)
+ flow->guessed_protocol_id = NDPI_PROTOCOL_TLS;
+
+ guessed_protocol_id = flow->guessed_protocol_id, guessed_host_protocol_id = flow->guessed_host_protocol_id;
+
+ if((guessed_host_protocol_id != NDPI_PROTOCOL_UNKNOWN) &&
+ ((flow->l4_proto == IPPROTO_UDP) &&
+ NDPI_ISSET(&flow->excluded_protocol_bitmask, guessed_host_protocol_id) &&
+ is_udp_guessable_protocol(guessed_host_protocol_id)))
+ flow->guessed_host_protocol_id = guessed_host_protocol_id = NDPI_PROTOCOL_UNKNOWN;
+
+ /* Ignore guessed protocol if they have been discarded */
+ if((guessed_protocol_id != NDPI_PROTOCOL_UNKNOWN)
+ // && (guessed_host_protocol_id == NDPI_PROTOCOL_UNKNOWN)
+ && (flow->l4_proto == IPPROTO_UDP) &&
+ NDPI_ISSET(&flow->excluded_protocol_bitmask, guessed_protocol_id) &&
+ is_udp_guessable_protocol(guessed_protocol_id))
+ flow->guessed_protocol_id = guessed_protocol_id = NDPI_PROTOCOL_UNKNOWN;
+
+ if((guessed_protocol_id != NDPI_PROTOCOL_UNKNOWN) || (guessed_host_protocol_id != NDPI_PROTOCOL_UNKNOWN)) {
+ if((guessed_protocol_id == 0) && (flow->protos.tls_quic_stun.stun.num_binding_requests > 0) &&
+ (flow->protos.tls_quic_stun.stun.num_processed_pkts > 0))
+ guessed_protocol_id = NDPI_PROTOCOL_STUN;
+
+ if(flow->host_server_name[0] != '\0') {
+ ndpi_protocol_match_result ret_match;
+
+ memset(&ret_match, 0, sizeof(ret_match));
+
+ ndpi_match_host_subprotocol(ndpi_str, flow, (char *) flow->host_server_name,
+ strlen((const char *) flow->host_server_name), &ret_match,
+ NDPI_PROTOCOL_DNS);
+
+ if(ret_match.protocol_id != NDPI_PROTOCOL_UNKNOWN)
+ guessed_host_protocol_id = ret_match.protocol_id;
+ }
- if(flow->guessed_protocol_id == NDPI_PROTOCOL_STUN)
- goto check_stun_export;
- else if((flow->guessed_protocol_id == NDPI_PROTOCOL_HANGOUT_DUO) ||
- (flow->guessed_protocol_id == NDPI_PROTOCOL_MESSENGER) ||
- (flow->guessed_protocol_id == NDPI_PROTOCOL_WHATSAPP_CALL)) {
*protocol_was_guessed = 1;
- ndpi_set_detected_protocol(ndpi_str, flow, flow->guessed_protocol_id, NDPI_PROTOCOL_UNKNOWN);
+ ndpi_int_change_protocol(ndpi_str, flow, guessed_host_protocol_id, guessed_protocol_id);
}
- else if((flow->protos.tls_quic_stun.tls_quic.hello_processed == 1) &&
- (flow->protos.tls_quic_stun.tls_quic.client_requested_server_name[0] != '\0')) {
- *protocol_was_guessed = 1;
- ndpi_set_detected_protocol(ndpi_str, flow, NDPI_PROTOCOL_TLS, NDPI_PROTOCOL_UNKNOWN);
- } else if(enable_guess) {
- if((flow->guessed_protocol_id == NDPI_PROTOCOL_UNKNOWN) && (flow->l4_proto == IPPROTO_TCP) &&
- flow->protos.tls_quic_stun.tls_quic.hello_processed)
- flow->guessed_protocol_id = NDPI_PROTOCOL_TLS;
-
- guessed_protocol_id = flow->guessed_protocol_id, guessed_host_protocol_id = flow->guessed_host_protocol_id;
-
- if((guessed_host_protocol_id != NDPI_PROTOCOL_UNKNOWN) &&
- ((flow->l4_proto == IPPROTO_UDP) &&
- NDPI_ISSET(&flow->excluded_protocol_bitmask, guessed_host_protocol_id) &&
- is_udp_guessable_protocol(guessed_host_protocol_id)))
- flow->guessed_host_protocol_id = guessed_host_protocol_id = NDPI_PROTOCOL_UNKNOWN;
-
- /* Ignore guessed protocol if they have been discarded */
- if((guessed_protocol_id != NDPI_PROTOCOL_UNKNOWN)
- // && (guessed_host_protocol_id == NDPI_PROTOCOL_UNKNOWN)
- && (flow->l4_proto == IPPROTO_UDP) &&
- NDPI_ISSET(&flow->excluded_protocol_bitmask, guessed_protocol_id) &&
- is_udp_guessable_protocol(guessed_protocol_id))
- flow->guessed_protocol_id = guessed_protocol_id = NDPI_PROTOCOL_UNKNOWN;
-
- if((guessed_protocol_id != NDPI_PROTOCOL_UNKNOWN) || (guessed_host_protocol_id != NDPI_PROTOCOL_UNKNOWN)) {
- if((guessed_protocol_id == 0) && (flow->protos.tls_quic_stun.stun.num_binding_requests > 0) &&
- (flow->protos.tls_quic_stun.stun.num_processed_pkts > 0))
- guessed_protocol_id = NDPI_PROTOCOL_STUN;
-
- if(flow->host_server_name[0] != '\0') {
- ndpi_protocol_match_result ret_match;
-
- memset(&ret_match, 0, sizeof(ret_match));
-
- ndpi_match_host_subprotocol(ndpi_str, flow, (char *) flow->host_server_name,
- strlen((const char *) flow->host_server_name), &ret_match,
- NDPI_PROTOCOL_DNS);
-
- if(ret_match.protocol_id != NDPI_PROTOCOL_UNKNOWN)
- guessed_host_protocol_id = ret_match.protocol_id;
- }
+ }
- *protocol_was_guessed = 1;
- ndpi_int_change_protocol(ndpi_str, flow, guessed_host_protocol_id, guessed_protocol_id);
- }
- }
- } else if(enable_guess) {
+ if(flow->detected_protocol_stack[0] == NDPI_PROTOCOL_UNKNOWN && enable_guess) {
if(flow->guessed_protocol_id != NDPI_PROTOCOL_UNKNOWN) {
*protocol_was_guessed = 1;
flow->detected_protocol_stack[1] = flow->guessed_protocol_id;
diff --git a/src/lib/protocols/git.c b/src/lib/protocols/git.c
index 22fc6f76e..2d194be81 100644
--- a/src/lib/protocols/git.c
+++ b/src/lib/protocols/git.c
@@ -47,8 +47,11 @@ void ndpi_search_git(struct ndpi_detection_module_struct *ndpi_struct,
u_int32_t git_pkt_len;
memcpy(&len, &pp[offset], 4), len[4] = 0;
- sscanf(len, "%x", &git_pkt_len);
-
+ if(sscanf(len, "%x", &git_pkt_len) != 1) {
+ found_git = 0;
+ break;
+ }
+
if((payload_len < git_pkt_len) || (git_pkt_len == 0 /* Bad */)) {
found_git = 0;
break;
diff --git a/src/lib/protocols/tls.c b/src/lib/protocols/tls.c
index aafa89048..6be5740e0 100644
--- a/src/lib/protocols/tls.c
+++ b/src/lib/protocols/tls.c
@@ -1951,7 +1951,7 @@ int processClientServerHello(struct ndpi_detection_module_struct *ndpi_struct,
printf("Client TLS [SIGNATURE_ALGORITHMS: %s]\n", ja3.client.signature_algorithms);
#endif
} else if(extension_id == 16 /* application_layer_protocol_negotiation */ &&
- offset+extension_offset < total_len) {
+ offset+extension_offset+1 < total_len) {
u_int16_t s_offset = offset+extension_offset;
u_int16_t tot_alpn_len = ntohs(*((u_int16_t*)&packet->payload[s_offset]));
char alpn_str[256];
--
cgit v1.2.3