From 63eeaffe21f7436018a2028a1dc2ee0cb1d13135 Mon Sep 17 00:00:00 2001 From: Ad Schellevis Date: Wed, 7 Dec 2022 09:40:48 +0100 Subject: [PATCH] Firewall: Diagnostics: States - Performance improvements and better address parsing in search. As the output of pfctl -vvss can grow quite rapidly, it seemed like a good idea to run this code through a profiler. Some of the hotspots (like parsing addresses) are now cached in memory to prevent over enthusiastic computation, which can save quite some processing time. Pushing down the string join on which the pattern search should match does help prevent to prevent compiling a search string which turns out to be irrelevant later (no filter or ip[+port] filter). The network (address) search handles (optional) ports as well now, which allows for patterns like `10.0.0.1:80` and `10.0.0.0/24:80`. --- .../Diagnostics/Api/FirewallController.php | 2 +- src/opnsense/scripts/filter/lib/states.py | 107 ++++++++++++------ 2 files changed, 72 insertions(+), 37 deletions(-) diff --git a/src/opnsense/mvc/app/controllers/OPNsense/Diagnostics/Api/FirewallController.php b/src/opnsense/mvc/app/controllers/OPNsense/Diagnostics/Api/FirewallController.php index ecaa8e991..011b9b6fd 100644 --- a/src/opnsense/mvc/app/controllers/OPNsense/Diagnostics/Api/FirewallController.php +++ b/src/opnsense/mvc/app/controllers/OPNsense/Diagnostics/Api/FirewallController.php @@ -161,7 +161,7 @@ class FirewallController extends ApiControllerBase $filter = new Filter([ 'query' => function ($value) { - return preg_replace("/[^0-9,a-z,A-Z, ,\/,*,\-,_,.,\#]/", "", $value); + return preg_replace("/[^0-9,a-z,A-Z,\: ,\/,*,\-,_,.,\#]/", "", $value); } ]); $searchPhrase = ''; diff --git a/src/opnsense/scripts/filter/lib/states.py b/src/opnsense/scripts/filter/lib/states.py index 3916a34f2..b23b8f10f 100755 --- a/src/opnsense/scripts/filter/lib/states.py +++ b/src/opnsense/scripts/filter/lib/states.py @@ -32,22 +32,40 @@ import sys import ujson -def parse_address(addr): - parse_result = {'port': '0'} - if addr.count(':') > 1: - # parse IPv6 address - parse_result['addr'] = addr.split('[')[0] - parse_result['ipproto'] = 'ipv6' - if addr.find('[') > -1: - parse_result['port'] = addr.split('[')[1].split(']')[0] - else: - # parse IPv4 address - parse_result['ipproto'] = 'ipv4' - parse_result['addr'] = addr.split(':')[0] - if addr.find(':') > -1: - parse_result['port'] = addr.split(':')[1] +class AddressParser: + def __init__(self): + self._addresses = {} + self._in_network = {} - return parse_result + def split_ip_port(self, addr): + if addr not in self._addresses: + self._addresses[addr] = { + 'port': '0' + } + if addr.count(':') > 1: + # parse IPv6 address + tmp = addr.split('[') + self._addresses[addr]['addr'] = tmp[0] + self._addresses[addr]['ipproto'] = 'ipv6' + if addr.find('[') > -1: + self._addresses[addr]['port'] = tmp[1].split(']')[0] + else: + # parse IPv4 address + tmp = addr.split(':') + self._addresses[addr]['ipproto'] = 'ipv4' + self._addresses[addr]['addr'] = tmp[0] + if len(tmp) > 1: + self._addresses[addr]['port'] = tmp[1] + + return self._addresses[addr] + + def overlaps(self, net, addr: str): + if net not in self._in_network: + self._in_network[net] = {} + if addr not in self._in_network[net]: + self._in_network[net][addr] = net.overlaps(ipaddress.ip_network(addr)) + + return self._in_network[net][addr] def fetch_rule_labels(): @@ -96,13 +114,24 @@ def fetch_rule_labels(): def query_states(rule_label, filter_str): + addr_parser = AddressParser() + result = list() try: - filter_network = ipaddress.ip_network(filter_str.strip()) + addr = filter_str.strip() + filter_port = None + if addr.startswith('[') and addr.count(']') == 1: + filter_port = addr.split(']')[1].split(':')[1] if addr.split(']')[1].count(':') == 1 else None + addr = addr.split(']')[0] + elif addr.count(':') == 1: + filter_port = addr.split(':')[1] + addr = addr.split(':')[0] + filter_network = ipaddress.ip_network(addr) except ValueError: filter_network = None + filter_port = None - rule_labels = fetch_rule_labels() + rule_labels = {} lines = subprocess.run(['/sbin/pfctl', '-vvs', 'state'], capture_output=True, text=True).stdout.strip().split('\n') record = None for line in lines: @@ -128,7 +157,6 @@ def query_states(rule_label, filter_str): # XXX: in order to kill a state, we need to pass both the id and the creator, so it seeems to make # sense to uniquely identify the state by the combined number record["id"] = "%s/%s" % (parts[1], parts[3]) - search_line = " ".join(str(item) for item in filter(None, record.values())) if rule_label != "" and record['label'].lower().find(rule_label) == -1: # label continue @@ -136,17 +164,20 @@ def query_states(rule_label, filter_str): try: match = False for field in ['src_addr', 'dst_addr', 'nat_addr']: - addr = ipaddress.ip_network(record[field]) - if field is not None and ipaddress.ip_network(filter_network).overlaps(addr): - match = True + port_field = "%s_port" % field[0:3] + if record[field] is not None and addr_parser.overlaps(filter_network, record[field]): + if filter_port is None or filter_port == record[port_field]: + match = True break if not match: continue except: continue - elif filter_str != "" and search_line.lower().find(filter_str.lower()) == -1: - # apply filter when provided - continue + elif filter_str != "": + search_line = " ".join(str(item) for item in filter(None, record.values())) + if search_line.lower().find(filter_str.lower()) == -1: + # apply filter when provided + continue if parts[0] == "id:": # append to response @@ -159,11 +190,11 @@ def query_states(rule_label, filter_str): 'nat_port': None, 'iface': parts[0], 'proto': parts[1], - 'ipproto': parse_address(parts[2])['ipproto'] + 'ipproto': addr_parser.split_ip_port(parts[2])['ipproto'] } if parts[3].find('(') > -1: # NAT enabled - nat_record = parse_address(parts[3][1:-1]) + nat_record = addr_parser.split_ip_port(parts[3][1:-1]) record['nat_addr'] = nat_record['addr'] if nat_record['port'] != '0': record['nat_port'] = nat_record['port'] @@ -173,17 +204,21 @@ def query_states(rule_label, filter_str): else: record['direction'] = 'in' - record['dst_addr'] = parse_address(parts[-2])['addr'] if record['direction'] == 'out' else parse_address(parts[2])['addr'] - record['dst_port'] = parse_address(parts[-2])['port'] if record['direction'] == 'out' else parse_address(parts[2])['port'] - record['src_addr'] = parse_address(parts[2])['addr'] if record['direction'] == 'out' else parse_address(parts[-2])['addr'] - record['src_port'] = parse_address(parts[2])['port'] if record['direction'] == 'out' else parse_address(parts[-2])['port'] + tmp_parts1 = addr_parser.split_ip_port(parts[2]) + tmp_parts2 = addr_parser.split_ip_port(parts[-2]) + record['dst_addr'] = tmp_parts2['addr'] if record['direction'] == 'out' else tmp_parts1['addr'] + record['dst_port'] = tmp_parts2['port'] if record['direction'] == 'out' else tmp_parts1['port'] + record['src_addr'] = tmp_parts1['addr'] if record['direction'] == 'out' else tmp_parts2['addr'] + record['src_port'] = tmp_parts1['port'] if record['direction'] == 'out' else tmp_parts2['port'] record['state'] = parts[-1] return result + def query_top(rule_label, filter_str): + addr_parser = AddressParser() result = list() rule_labels = fetch_rule_labels() sp = subprocess.run(['/usr/local/sbin/pftop', '-w', '1000', '-b','-v', 'long','9999999999999'], capture_output=True, text=True) @@ -199,16 +234,16 @@ def query_top(rule_label, filter_str): record = { 'proto': parts[0], 'dir': parts[1].lower(), - 'src_addr': parse_address(parts[2])['addr'], - 'src_port': parse_address(parts[2])['port'], - 'dst_addr': parse_address(parts[3])['addr'], - 'dst_port': parse_address(parts[3])['port'], + 'src_addr': addr_parser.split_ip_port(parts[2])['addr'], + 'src_port': addr_parser.split_ip_port(parts[2])['port'], + 'dst_addr': addr_parser.split_ip_port(parts[3])['addr'], + 'dst_port': addr_parser.split_ip_port(parts[3])['port'], 'gw_addr': None, 'gw_port': None, } if parts[4].count(':') > 2 or parts[4].count('.') > 2: - record['gw_addr'] = parse_address(parts[4])['addr'] - record['gw_port'] = parse_address(parts[4])['port'] + record['gw_addr'] = addr_parser.split_ip_port(parts[4])['addr'] + record['gw_port'] = addr_parser.split_ip_port(parts[4])['port'] idx = 5 else: idx = 4