From afc2b641eb9cf5035b5147e78030bafe0b40dd87 Mon Sep 17 00:00:00 2001 From: Ivan Nardi <12729895+IvanNardi@users.noreply.github.com> Date: Mon, 15 Nov 2021 16:20:57 +0100 Subject: Fix writes to `flow->protos` union fields (#1354) We can write to `flow->protos` only after a proper classification. This issue has been found in Kerberos, DHCP, HTTP, STUN, IMO, FTP, SMTP, IMAP and POP code. There are two kinds of fixes: * write to `flow->protos` only if a final protocol has been detected * move protocol state out of `flow->protos` The hard part is to find, for each protocol, the right tradeoff between memory usage and code complexity. Handle Kerberos like DNS: if we find a request, we set the protocol and an extra callback to further parsing the reply. For all the other protocols, move the state out of `flow->protos`. This is an issue only for the FTP/MAIL stuff. Add DHCP Class Identification value to the output of ndpiReader and to the Jason serialization. Extend code coverage of fuzz tests. Close #1343 Close #1342 --- src/lib/protocols/quic.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) (limited to 'src/lib/protocols/quic.c') diff --git a/src/lib/protocols/quic.c b/src/lib/protocols/quic.c index 206c3b951..f908dc58c 100644 --- a/src/lib/protocols/quic.c +++ b/src/lib/protocols/quic.c @@ -1297,7 +1297,7 @@ static void process_tls(struct ndpi_detection_module_struct *ndpi_struct, packet->payload_packet_len = crypto_data_len; processClientServerHello(ndpi_struct, flow, version); - flow->protos.tls_quic_stun.tls_quic.hello_processed = 1; /* Allow matching of custom categories */ + flow->protos.tls_quic.hello_processed = 1; /* Allow matching of custom categories */ /* Restore */ packet->payload = p; @@ -1307,12 +1307,12 @@ static void process_tls(struct ndpi_detection_module_struct *ndpi_struct, this way we lose JA3S and negotiated ciphers... Negotiated version is only present in the ServerHello message too, but fortunately, QUIC always uses TLS version 1.3 */ - flow->protos.tls_quic_stun.tls_quic.ssl_version = 0x0304; + flow->protos.tls_quic.ssl_version = 0x0304; /* DNS-over-QUIC: ALPN is "doq" or "doq-XXX" (for drafts versions) */ - if(flow->protos.tls_quic_stun.tls_quic.alpn && - strncmp(flow->protos.tls_quic_stun.tls_quic.alpn, "doq", 3) == 0) { - NDPI_LOG_DBG(ndpi_struct, "Found DOQ (ALPN: [%s])\n", flow->protos.tls_quic_stun.tls_quic.alpn); + if(flow->protos.tls_quic.alpn && + strncmp(flow->protos.tls_quic.alpn, "doq", 3) == 0) { + NDPI_LOG_DBG(ndpi_struct, "Found DOQ (ALPN: [%s])\n", flow->protos.tls_quic.alpn); ndpi_int_change_protocol(ndpi_struct, flow, NDPI_PROTOCOL_DOH_DOT, NDPI_PROTOCOL_QUIC); } } @@ -1356,22 +1356,22 @@ static void process_chlo(struct ndpi_detection_module_struct *ndpi_struct, crypto_data_len, tag_offset_start, prev_offset, offset, len); #endif if(memcmp(tag, "SNI\0", 4) == 0) { - sni_len = MIN(len, sizeof(flow->protos.tls_quic_stun.tls_quic.client_requested_server_name) - 1); - memcpy(flow->protos.tls_quic_stun.tls_quic.client_requested_server_name, + sni_len = MIN(len, sizeof(flow->protos.tls_quic.client_requested_server_name) - 1); + memcpy(flow->protos.tls_quic.client_requested_server_name, &crypto_data[tag_offset_start + prev_offset], sni_len); - flow->protos.tls_quic_stun.tls_quic.client_requested_server_name[sni_len] = '\0'; + flow->protos.tls_quic.client_requested_server_name[sni_len] = '\0'; NDPI_LOG_DBG2(ndpi_struct, "SNI: [%s]\n", - flow->protos.tls_quic_stun.tls_quic.client_requested_server_name); + flow->protos.tls_quic.client_requested_server_name); ndpi_match_host_subprotocol(ndpi_struct, flow, - (char *)flow->protos.tls_quic_stun.tls_quic.client_requested_server_name, - strlen((const char*)flow->protos.tls_quic_stun.tls_quic.client_requested_server_name), + (char *)flow->protos.tls_quic.client_requested_server_name, + strlen((const char*)flow->protos.tls_quic.client_requested_server_name), &ret_match, NDPI_PROTOCOL_QUIC); - flow->protos.tls_quic_stun.tls_quic.hello_processed = 1; /* Allow matching of custom categories */ + flow->protos.tls_quic.hello_processed = 1; /* Allow matching of custom categories */ ndpi_check_dga_name(ndpi_struct, flow, - flow->protos.tls_quic_stun.tls_quic.client_requested_server_name, 1); + flow->protos.tls_quic.client_requested_server_name, 1); sni_found = 1; if (ua_found) @@ -1396,7 +1396,7 @@ static void process_chlo(struct ndpi_detection_module_struct *ndpi_struct, NDPI_LOG_DBG(ndpi_struct, "Something went wrong in tags iteration\n"); /* Add check for missing SNI */ - if(flow->protos.tls_quic_stun.tls_quic.client_requested_server_name[0] == '\0') { + if(flow->protos.tls_quic.client_requested_server_name[0] == '\0') { /* This is a bit suspicious */ ndpi_set_risk(ndpi_struct, flow, NDPI_TLS_MISSING_SNI); } @@ -1508,7 +1508,7 @@ static int eval_extra_processing(struct ndpi_detection_module_struct *ndpi_struc */ if((version == V_Q046 && - flow->protos.tls_quic_stun.tls_quic.client_requested_server_name[0] == '\0') || + flow->protos.tls_quic.client_requested_server_name[0] == '\0') || is_ch_reassembler_pending(flow)) { NDPI_LOG_DBG2(ndpi_struct, "We have further work to do\n"); return 1; -- cgit v1.2.3