From 5701d6b8f3159e466d46ecee5fb37a8a2d48c5f0 Mon Sep 17 00:00:00 2001 From: alya Date: Mon, 7 Oct 2024 19:37:31 +0300 Subject: [PATCH 1/2] horizontal_portscan.py: optimize get_uids() by reducing the overhead of list concatenation --- modules/network_discovery/horizontal_portscan.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/modules/network_discovery/horizontal_portscan.py b/modules/network_discovery/horizontal_portscan.py index a0b2675e6..95034e4da 100644 --- a/modules/network_discovery/horizontal_portscan.py +++ b/modules/network_discovery/horizontal_portscan.py @@ -48,9 +48,6 @@ def get_not_estab_dst_ports( """ Get the list of dstports that we tried to connect to (not established flows) - these unknowns are the info this function retrieves - profileid -> unknown_dstip:unknown_dstports - here, the profileid given is the client. :return: the following dict #TODO this is wrong, fix it @@ -165,11 +162,7 @@ def get_uids(self, dstips: dict): returns all the uids of flows sent on a sigle port to different destination IPs """ - uids = [] - for dstip in dstips: - for uid in dstips[dstip]["uid"]: - uids.append(uid) - return uids + return [uid for dstip in dstips for uid in dstips[dstip]["uid"]] def set_evidence_horizontal_portscan(self, evidence: dict): threat_level = ThreatLevel.HIGH From ef5ae476c654cbabafc4293d6fdf1777de966048 Mon Sep 17 00:00:00 2001 From: alya Date: Mon, 7 Oct 2024 20:55:18 +0300 Subject: [PATCH 2/2] horizontal_portscan.py: speedup filtering of dstips by filtering before storing the ips in the db in 'DstPortsClientTCPNot Established' --- .../network_discovery/horizontal_portscan.py | 34 +-------- .../core/database/redis_db/profile_handler.py | 76 ++++++++++++++----- 2 files changed, 56 insertions(+), 54 deletions(-) diff --git a/modules/network_discovery/horizontal_portscan.py b/modules/network_discovery/horizontal_portscan.py index 95034e4da..fb1b30242 100644 --- a/modules/network_discovery/horizontal_portscan.py +++ b/modules/network_discovery/horizontal_portscan.py @@ -157,7 +157,7 @@ def check_if_enough_dstips_to_trigger_an_evidence( return True return False - def get_uids(self, dstips: dict): + def get_uids(self, dstips: dict) -> List[str]: """ returns all the uids of flows sent on a sigle port to different destination IPs @@ -213,40 +213,10 @@ def is_valid_saddr(profileid: str): return False - @staticmethod - def is_valid_daddr(daddr: str): - """ - to avoid reporting port scans on the - broadcast or multicast addresses or invalid values - """ - if validators.ipv4(daddr) or validators.ipv6(daddr): - daddr_obj = ipaddress.ip_address(daddr) - return not daddr_obj.is_multicast and daddr != "255.255.255.255" - - return False - @staticmethod def is_valid_twid(twid: str) -> bool: return not (twid in ("", None) or "timewindow" not in twid) - def filter_dstips(self, dstips: dict) -> dict: - """ - returns the given dict of dstips without resolved IPs, broadcast, - and multicast addrs - """ - resolved_ips: List[str] = self.get_resolved_ips(dstips) - dstips_to_discard = [] - for ip in dstips: - if not self.is_valid_daddr(ip): - dstips_to_discard.append(ip) - if ip in resolved_ips: - dstips_to_discard.append(ip) - - for ip in dstips_to_discard: - dstips.pop(ip) - - return dstips - def check(self, profileid: str, twid: str): if not self.is_valid_saddr(profileid) or not self.is_valid_twid(twid): return False @@ -268,8 +238,6 @@ def check(self, profileid: str, twid: str): # PortScan Type 2. Direction OUT dstips: dict = dports[dport]["dstips"] - dstips: dict = self.filter_dstips(dstips) - twid_identifier: str = self.get_twid_identifier( profileid, twid, dport ) diff --git a/slips_files/core/database/redis_db/profile_handler.py b/slips_files/core/database/redis_db/profile_handler.py index 0ea53bf08..ccaa4b8fb 100644 --- a/slips_files/core/database/redis_db/profile_handler.py +++ b/slips_files/core/database/redis_db/profile_handler.py @@ -1,3 +1,4 @@ +import ipaddress import json import sys import time @@ -223,13 +224,42 @@ def add_out_dns(self, profileid, twid, flow): extra_info=extra_info, ) + def _was_flow_flipped(self, flow) -> bool: + """ + The majority of the FP with horizontal port scan detection + happen because a benign computer changes wifi, and many not + established conns are redone, which look like a port scan to + 10 webpages. To avoid this, we IGNORE all the flows that have + in the history of flags (field history in zeek), the ^, + that means that the flow was swapped/flipped. + The below key_name is only used by the portscan module to check + for horizontal portscan, which means we can safely ignore it + here and it won't affect the rest of slips + """ + state_hist = flow.state_hist if hasattr(flow, "state_hist") else "" + return "^" in state_hist + + @staticmethod + def _is_multicast_or_broadcast(daddr: str) -> bool: + """ + to avoid reporting port scans on the + broadcast or multicast addresses or invalid values + """ + if daddr == "255.255.255.255": + return True + + daddr_obj = ipaddress.ip_address(daddr) + return daddr_obj.is_multicast + def add_port( self, profileid: str, twid: str, flow: dict, role: str, port_type: str ): """ Store info learned from ports for this flow - The flow can go out of the IP (we are acting as Client) or into the IP (we are acting as Server) - role: 'Client' or 'Server'. Client also defines that the flow is going out, Server that is going in + The flow can go out of the IP (we are acting as Client) or into the IP + (we are acting as Server) + role: 'Client' or 'Server'. Client also defines that the flow is going + out, Server that is going in port_type: 'Dst' or 'Src'. Depending if this port was a destination port or a source port """ @@ -244,17 +274,8 @@ def add_port( uid = flow.uid ip = str(flow.daddr) spkts = flow.spkts - state_hist = flow.state_hist if hasattr(flow, "state_hist") else "" - if "^" in state_hist: - # The majority of the FP with horizontal port scan detection happen because a - # benign computer changes wifi, and many not established conns are redone, - # which look like a port scan to 10 webpages. To avoid this, we IGNORE all - # the flows that have in the history of flags (field history in zeek), the ^, - # that means that the flow was swapped/flipped. - # The below key_name is only used by the portscan module to check for horizontal - # portscan, which means we can safely ignore it here and it won't affect the rest - # of slips + if self._was_flow_flipped(flow): return False # Choose which port to use based if we were asked Dst or Src @@ -265,10 +286,10 @@ def add_port( ip_key = "srcips" if role == "Server" else "dstips" # Get the state. Established, NotEstablished - summaryState = self.get_final_state_from_flags(state, pkts) + summary_state = self.get_final_state_from_flags(state, pkts) old_profileid_twid_data = self.get_data_from_profile_tw( - profileid, twid, port_type, summaryState, proto, role, "Ports" + profileid, twid, port_type, summary_state, proto, role, "Ports" ) try: @@ -278,7 +299,8 @@ def add_port( port_data["totalpkt"] += pkts port_data["totalbytes"] += totbytes - # if there's a conn from this ip on this port, update the pkts of this conn + # if there's a conn from this ip on this port, update the pkts + # of this conn if ip in port_data[ip_key]: port_data[ip_key][ip]["pkts"] += pkts port_data[ip_key][ip]["spkts"] += spkts @@ -309,13 +331,25 @@ def add_port( old_profileid_twid_data[port] = port_data data = json.dumps(old_profileid_twid_data) hash_key = f"{profileid}{self.separator}{twid}" - key_name = f"{port_type}Ports{role}{proto}{summaryState}" - self.r.hset(hash_key, key_name, str(data)) + key_name = f"{port_type}Ports{role}{proto}{summary_state}" self.mark_profile_tw_as_modified(profileid, twid, starttime) + if key_name == "DstPortsClientTCPNot Established": + # this key is used in horizontal ps module only + # to avoid unnecessary storing and filtering of data, we store + # only unresolved non multicast non broadcast ips. + # if this key is ever needed for another module, we'll need to + # workaround this + ip_resolved = self.get_dns_resolution(ip) + if ip_resolved or self._is_multicast_or_broadcast(ip): + return + + self.r.hset(hash_key, key_name, str(data)) + def get_final_state_from_flags(self, state, pkts): """ - Analyze the flags given and return a summary of the state. Should work with Argus and Bro flags + Analyze the flags given and return a summary of the state. Should work + with Argus and Bro flags We receive the pakets to distinguish some Reset connections """ try: @@ -641,14 +675,14 @@ def add_ips(self, profileid, twid, flow, role): self.update_times_contacted(ip, direction, profileid, twid) # Get the state. Established, NotEstablished - summaryState = self.get_final_state_from_flags(flow.state, flow.pkts) - key_name = f"{direction}IPs{role}{flow.proto.upper()}{summaryState}" + summary_state = self.get_final_state_from_flags(flow.state, flow.pkts) + key_name = f"{direction}IPs{role}{flow.proto.upper()}{summary_state}" # Get the previous data about this key old_profileid_twid_data = self.get_data_from_profile_tw( profileid, twid, direction, - summaryState, + summary_state, flow.proto, role, "IPs",