From ee0a5273fb304c594a13b3b7edf38271bbe6a8c2 Mon Sep 17 00:00:00 2001 From: Laurent Desausoi Date: Thu, 22 Jun 2023 12:10:27 +0200 Subject: [PATCH 1/5] Allow unbanip of tuple IDs As specified [here], IDs in Fail2ban can be tuples. However, when a tuple ID is banned, there is no way to remove it via the `unbanip` command line. If tried, the tuple will be considered as a list with each element being an ID. Instead, we should try to parse the string to see if it represents an object. [here]: https://github.com/fail2ban/fail2ban/blob/226a59445a8046b7f86c3bc072be1d78555ccdb0/fail2ban/server/failregex.py#L313 --- fail2ban/server/actions.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index b052d342..74e2a615 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -274,6 +274,13 @@ class Actions(JailThread, Mapping): if missed: raise ValueError("not banned: %r" % missed) return cnt + # IPs can be represented in string format (e.g.: tuples) + is_ip_parsed = True + if isinstance(ip, str): + try: + ip = eval(ip) + except Exception: + is_ip_parsed = False # Single IP: # Always delete ip from database (also if currently not banned) if db and self._jail.database is not None: @@ -285,14 +292,14 @@ class Actions(JailThread, Mapping): self.__unBan(ticket) else: # Multiple IPs by subnet or dns: - if not isinstance(ip, IPAddr): + if not is_ip_parsed and not isinstance(ip, IPAddr): ipa = IPAddr(ip) if not ipa.isSingle: # subnet (mask/cidr) or raw (may be dns/hostname): ips = list(filter(ipa.contains, self.banManager.getBanList())) if ips: return self.removeBannedIP(ips, db, ifexists) # not found: - msg = "%s is not banned" % ip + msg = "%s is not banned" % str(ip) logSys.log(logging.MSG, msg) if ifexists: return 0 From e03e9bde0b04f4248a08cd4b8af4a660536b2fad Mon Sep 17 00:00:00 2001 From: Laurent Desausoi Date: Thu, 22 Jun 2023 16:14:41 +0200 Subject: [PATCH 2/5] fixup! Allow unbanip of tuple IDs --- fail2ban/protocol.py | 2 +- fail2ban/server/actions.py | 55 +++++++++++++++++++--------------- fail2ban/server/server.py | 15 ++++++++-- fail2ban/server/transmitter.py | 14 +++++---- 4 files changed, 53 insertions(+), 33 deletions(-) diff --git a/fail2ban/protocol.py b/fail2ban/protocol.py index a81c6657..9e5e4245 100644 --- a/fail2ban/protocol.py +++ b/fail2ban/protocol.py @@ -105,7 +105,7 @@ protocol = [ ["set usedns ", "sets the usedns mode for "], ["set attempt [ ... ]", "manually notify about failure"], ["set banip ... ", "manually Ban for "], -["set unbanip [--report-absent] ... ", "manually Unban in "], +["set unbanip [--report-absent] [--expr] ... ", "manually Unban in "], ["set maxretry ", "sets the number of failures before banning the host for "], ["set maxmatches ", "sets the max number of matches stored in memory per ticket in "], ["set maxlines ", "sets the number of to buffer for regex search for "], diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index 74e2a615..3a0a8e3f 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -242,6 +242,33 @@ class Actions(JailThread, Mapping): return self.__checkBan(tickets) + def removeMultiBannedIP(self, ips=None, db=True, ifexists=False): + """Removes multiple banned IPs calling actions' unban method + + Parameters + ---------- + ip : list, tuple or None + The IPs as list to unban or all IPs if None + + Raises + ------ + ValueError + If at least one `ip` is not banned + """ + if ips is None: + return self.__flushBan(db) + missed = [] + cnt = 0 + for ip in ips: + try: + cnt += self.removeBannedIP(ip, db, ifexists) + except ValueError: + if not ifexists: + missed.append(ip) + if missed: + raise ValueError("not banned: %r" % missed) + return cnt + def removeBannedIP(self, ip=None, db=True, ifexists=False): """Removes banned IP calling actions' unban method @@ -250,8 +277,8 @@ class Actions(JailThread, Mapping): Parameters ---------- - ip : list, str, IPAddr or None - The IP address (or multiple IPs as list) to unban or all IPs if None + ip : str, IPAddr, TUPLE_ID representation (tuple/list) or None + The ID to unban or all IPs if None Raises ------ @@ -261,26 +288,6 @@ class Actions(JailThread, Mapping): # Unban all? if ip is None: return self.__flushBan(db) - # Multiple IPs: - if isinstance(ip, (list, tuple)): - missed = [] - cnt = 0 - for i in ip: - try: - cnt += self.removeBannedIP(i, db, ifexists) - except ValueError: - if not ifexists: - missed.append(i) - if missed: - raise ValueError("not banned: %r" % missed) - return cnt - # IPs can be represented in string format (e.g.: tuples) - is_ip_parsed = True - if isinstance(ip, str): - try: - ip = eval(ip) - except Exception: - is_ip_parsed = False # Single IP: # Always delete ip from database (also if currently not banned) if db and self._jail.database is not None: @@ -292,12 +299,12 @@ class Actions(JailThread, Mapping): self.__unBan(ticket) else: # Multiple IPs by subnet or dns: - if not is_ip_parsed and not isinstance(ip, IPAddr): + if not isinstance(ip, (IPAddr, tuple, list)): ipa = IPAddr(ip) if not ipa.isSingle: # subnet (mask/cidr) or raw (may be dns/hostname): ips = list(filter(ipa.contains, self.banManager.getBanList())) if ips: - return self.removeBannedIP(ips, db, ifexists) + return self.removeMultiBannedIP(ips, db, ifexists) # not found: msg = "%s is not banned" % str(ip) logSys.log(logging.MSG, msg) diff --git a/fail2ban/server/server.py b/fail2ban/server/server.py index bdbe88ca..beafe8d9 100644 --- a/fail2ban/server/server.py +++ b/fail2ban/server/server.py @@ -31,6 +31,7 @@ import os import signal import stat import sys +from ast import literal_eval from .observer import Observers, ObserverThread from .jails import Jails @@ -528,18 +529,26 @@ class Server: def setBanIP(self, name, value): return self.__jails[name].actions.addBannedIP(value) - def setUnbanIP(self, name=None, value=None, ifexists=True): + def setUnbanIP(self, name=None, values=None, ifexists=True, ifexpr=False): + def parseExpr(v): + try: + return literal_eval(v) + except SyntaxError: + return v if name is not None: # single jail: jails = [self.__jails[name]] else: # in all jails: jails = list(self.__jails.values()) - # unban given or all (if value is None): + # parse values if it contains an expression + if values and ifexpr: + values = map(parseExpr, values) + # unban given or all (if values is None): cnt = 0 ifexists |= (name is None) for jail in jails: - cnt += jail.actions.removeBannedIP(value, ifexists=ifexists) + cnt += jail.actions.removeMultiBannedIP(values, ifexists=ifexists) return cnt def banned(self, name=None, ids=None): diff --git a/fail2ban/server/transmitter.py b/fail2ban/server/transmitter.py index 28931344..cee0b7a5 100644 --- a/fail2ban/server/transmitter.py +++ b/fail2ban/server/transmitter.py @@ -363,13 +363,17 @@ class Transmitter: value = command[2:] return self.__server.setBanIP(name,value) elif command[1] == "unbanip": + ifexpr = False ifexists = True - if command[2] != "--report-absent": - value = command[2:] - else: + offset = 2 + if "--report-absent" in command: ifexists = False - value = command[3:] - return self.__server.setUnbanIP(name, value, ifexists=ifexists) + offset += 1 + if "--expr" in command: + ifexpr = True + offset += 1 + value = command[offset:] + return self.__server.setUnbanIP(name, value, ifexists=ifexists, ifexpr=ifexpr) elif command[1] == "addaction": args = [command[2]] if len(command) > 3: From 6afdf239c47b548f4ee6052770e424e138330a4a Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 22 Jun 2023 19:20:55 +0200 Subject: [PATCH 3/5] code review: parse options properly (order independently), same logic for `unban` command, etc --- fail2ban/protocol.py | 4 ++-- fail2ban/server/server.py | 10 +++++----- fail2ban/server/transmitter.py | 23 +++++++++-------------- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/fail2ban/protocol.py b/fail2ban/protocol.py index 9e5e4245..28c490fc 100644 --- a/fail2ban/protocol.py +++ b/fail2ban/protocol.py @@ -54,7 +54,7 @@ protocol = [ ["reload [--restart] [--unban] [--if-exists] ", "reloads the jail , or restarts it (if option '--restart' specified)"], ["stop", "stops all jails and terminate the server"], ["unban --all", "unbans all IP addresses (in all jails and database)"], -["unban ... ", "unbans (in all jails and database)"], +["unban [--expr] [--] ... ", "unbans (in all jails and database)"], ["banned", "return jails with banned IPs as dictionary"], ["banned ... ]", "return list(s) of jails where given IP(s) are banned"], ["status", "gets the current status of the server"], @@ -105,7 +105,7 @@ protocol = [ ["set usedns ", "sets the usedns mode for "], ["set attempt [ ... ]", "manually notify about failure"], ["set banip ... ", "manually Ban for "], -["set unbanip [--report-absent] [--expr] ... ", "manually Unban in "], +["set unbanip [--report-absent] [--expr] [--] ... ", "manually Unban in "], ["set maxretry ", "sets the number of failures before banning the host for "], ["set maxmatches ", "sets the max number of matches stored in memory per ticket in "], ["set maxlines ", "sets the number of to buffer for regex search for "], diff --git a/fail2ban/server/server.py b/fail2ban/server/server.py index beafe8d9..d46af872 100644 --- a/fail2ban/server/server.py +++ b/fail2ban/server/server.py @@ -24,14 +24,14 @@ __author__ = "Cyril Jaquier" __copyright__ = "Copyright (c) 2004 Cyril Jaquier" __license__ = "GPL" -import threading -from threading import Lock, RLock +from ast import literal_eval import logging import os import signal import stat import sys -from ast import literal_eval +import threading +from threading import Lock, RLock from .observer import Observers, ObserverThread from .jails import Jails @@ -529,7 +529,7 @@ class Server: def setBanIP(self, name, value): return self.__jails[name].actions.addBannedIP(value) - def setUnbanIP(self, name=None, values=None, ifexists=True, ifexpr=False): + def setUnbanIP(self, name=None, values=None, ifexists=True, isexpr=False): def parseExpr(v): try: return literal_eval(v) @@ -542,7 +542,7 @@ class Server: # in all jails: jails = list(self.__jails.values()) # parse values if it contains an expression - if values and ifexpr: + if values and isexpr: values = map(parseExpr, values) # unban given or all (if values is None): cnt = 0 diff --git a/fail2ban/server/transmitter.py b/fail2ban/server/transmitter.py index cee0b7a5..26a1b15f 100644 --- a/fail2ban/server/transmitter.py +++ b/fail2ban/server/transmitter.py @@ -24,6 +24,7 @@ __author__ = "Cyril Jaquier" __copyright__ = "Copyright (c) 2004 Cyril Jaquier" __license__ = "GPL" +import getopt import time import json @@ -113,11 +114,12 @@ class Transmitter: return 'OK' elif name == "unban" and len(command) >= 2: # unban in all jails: - value = command[1:] + opts, value = getopt.getopt(command[1:], "", ["expr", "all"]) + opts = dict(opts) # if all ips: - if len(value) == 1 and value[0] == "--all": + if "--all" in opts: return self.__server.setUnbanIP() - return self.__server.setUnbanIP(None, value) + return self.__server.setUnbanIP(None, value, isexpr=("--expr" in opts)) elif name == "banned": # check IP is banned in all jails: return self.__server.banned(None, command[1:]) @@ -363,17 +365,10 @@ class Transmitter: value = command[2:] return self.__server.setBanIP(name,value) elif command[1] == "unbanip": - ifexpr = False - ifexists = True - offset = 2 - if "--report-absent" in command: - ifexists = False - offset += 1 - if "--expr" in command: - ifexpr = True - offset += 1 - value = command[offset:] - return self.__server.setUnbanIP(name, value, ifexists=ifexists, ifexpr=ifexpr) + opts, value = getopt.getopt(command[2:], "", ["expr", "report-absent"]) + opts = dict(opts) + return self.__server.setUnbanIP(name, value, + ifexists=("--report-absent" not in opts), isexpr=("--expr" in opts)) elif command[1] == "addaction": args = [command[2]] if len(command) > 3: From 34390b3fe1e4e69df3ea3cc7f45f8a7a3a471ddd Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 22 Jun 2023 19:35:09 +0200 Subject: [PATCH 4/5] extended tests covering unban tuple IDs --- fail2ban/tests/fail2banclienttestcase.py | 8 ++++++-- fail2ban/tests/servertestcase.py | 7 ++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/fail2ban/tests/fail2banclienttestcase.py b/fail2ban/tests/fail2banclienttestcase.py index 9d3f382f..1ab6ac9e 100644 --- a/fail2ban/tests/fail2banclienttestcase.py +++ b/fail2ban/tests/fail2banclienttestcase.py @@ -1408,8 +1408,12 @@ class Fail2banServerTest(Fail2banClientServerBase): all=True, wait=MID_WAITTIME ) - # unban 1, 2 and 5: - self.execCmd(SUCCESS, startparams, 'unban', '125-000-001', '125-000-002', '125-000-005') + # unban 1, 2 and 5 (as expr): + self.execCmd(SUCCESS, startparams, 'unban', '125-000-001', '125-000-002') + self.execCmd(SUCCESS, startparams, 'unban', '--expr', '"125-000-005"') + # check tuple IDs don't consider as a list of IDs to unban: + self.execCmd(SUCCESS, startparams, 'unban', '--expr', '("125-000-003","125-000-004")') + self.assertLogged('%r is not banned' % (('125-000-003', '125-000-004'),)) _out_file(mpfn) # check really unbanned but other sessions are still present (blacklisted in map-file): mp = _read_file(mpfn) diff --git a/fail2ban/tests/servertestcase.py b/fail2ban/tests/servertestcase.py index 7cba9add..628b7253 100644 --- a/fail2ban/tests/servertestcase.py +++ b/fail2ban/tests/servertestcase.py @@ -372,8 +372,13 @@ class Transmitter(TransmitterBase): # ... (no error, IPs logged only): self.assertEqual( self.transm.proceed( - ["set", self.jailName, "unbanip", "192.0.2.255", "192.0.2.254"]),(0, 0)) + ["set", self.jailName, "unbanip", "--", "192.0.2.255", "192.0.2.254"]),(0, 0)) self.assertLogged("192.0.2.255 is not banned", "192.0.2.254 is not banned", all=True, wait=True) + # tuple ID: + self.assertEqual( + self.transm.proceed( + ["set", self.jailName, "unbanip", "--expr", "(1,2,3)"]),(0, 0)) + self.assertLogged("%r is not banned" % ((1,2,3),), wait=True) def testJailAttemptIP(self): self.server.startJail(self.jailName) # Jail must be started From 98a4cee1c0af79fababc2dd0f06ccd103c34f277 Mon Sep 17 00:00:00 2001 From: Laurent Desausoi Date: Fri, 23 Jun 2023 12:36:37 +0200 Subject: [PATCH 5/5] allow tuple IDs on all commands --- fail2ban/client/beautifier.py | 2 +- fail2ban/helpers.py | 11 +++++++++ fail2ban/protocol.py | 12 +++++----- fail2ban/server/filter.py | 2 +- fail2ban/server/server.py | 16 +++---------- fail2ban/server/transmitter.py | 43 ++++++++++++++++++++++++---------- 6 files changed, 52 insertions(+), 34 deletions(-) diff --git a/fail2ban/client/beautifier.py b/fail2ban/client/beautifier.py index 97cd38b2..1c59016f 100644 --- a/fail2ban/client/beautifier.py +++ b/fail2ban/client/beautifier.py @@ -185,7 +185,7 @@ class Beautifier: sep = " " if len(inC) <= 3 else inC[3] if sep == "--with-time": sep = "\n" - msg = sep.join(response) + msg = sep.join([str(res) for res in response]) except Exception: logSys.warning("Beautifier error. Please report the error") logSys.error("Beautify %r with %r failed", response, self.__inputCmd, diff --git a/fail2ban/helpers.py b/fail2ban/helpers.py index fa16693a..815ecf63 100644 --- a/fail2ban/helpers.py +++ b/fail2ban/helpers.py @@ -28,6 +28,7 @@ import re import sys import traceback +from ast import literal_eval from threading import Lock from .server.mytime import MyTime @@ -298,6 +299,16 @@ def _merge_copy_dicts(x, y): """ return {**x, **y} +def parseExpressions(value): + def parseExpr(v): + try: + return literal_eval(v) + except SyntaxError: + return v + if value: + return list(map(parseExpr, value)) if isinstance(value, (list, tuple)) else parseExpr(value) + return None + # # Following function used for parse options from parameter (e.g. `name[p1=0, p2="..."][p3='...']`). # diff --git a/fail2ban/protocol.py b/fail2ban/protocol.py index 28c490fc..4182d67c 100644 --- a/fail2ban/protocol.py +++ b/fail2ban/protocol.py @@ -56,7 +56,7 @@ protocol = [ ["unban --all", "unbans all IP addresses (in all jails and database)"], ["unban [--expr] [--] ... ", "unbans (in all jails and database)"], ["banned", "return jails with banned IPs as dictionary"], -["banned ... ]", "return list(s) of jails where given IP(s) are banned"], +["banned [--expr] [--] ... ]", "return list(s) of jails where given IP(s) are banned"], ["status", "gets the current status of the server"], ["ping", "tests if the server is alive"], ["echo", "for internal usage, returns back and outputs a given string"], @@ -86,8 +86,8 @@ protocol = [ ['', "JAIL CONFIGURATION", ""], ["set idle on|off", "sets the idle state of "], ["set ignoreself true|false", "allows the ignoring of own IP addresses"], -["set addignoreip ", "adds to the ignore list of "], -["set delignoreip ", "removes from the ignore list of "], +["set addignoreip [--expr] [--] ... ", "adds to the ignore list of "], +["set delignoreip [--expr] [--] ... ", "removes from the ignore list of "], ["set ignorecommand ", "sets ignorecommand of "], ["set ignorecache ", "sets ignorecache of "], ["set addlogpath ['tail']", "adds to the monitoring list of , optionally starting at the 'tail' of the file (default 'head')."], @@ -103,8 +103,8 @@ protocol = [ ["set bantime