From 809acb69e5928c0e678ad25b43e53b567cb23a3b Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 11 Jun 2019 14:37:10 +0200 Subject: [PATCH 1/3] stability: avoid race condition - no unban if the bans occur continuously (e. g. banning action too slow, so new bans found each time during the default sleeptime); now unban will happen not later than 10 tickets get banned regardless there are still active bans available (precedence of ban is 10 now); closes gh-2410 --- fail2ban/server/actions.py | 15 ++++++++++++--- fail2ban/tests/actionstestcase.py | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index 2253ee0b..40fb487f 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -81,6 +81,8 @@ class Actions(JailThread, Mapping): self._actions = OrderedDict() ## The ban manager. self.__banManager = BanManager() + ## precedence of ban (over unban), so max number of tickets banned (to call an unban check): + self.banPrecedence = 10 @staticmethod def _load_python_module(pythonModule): @@ -296,6 +298,7 @@ class Actions(JailThread, Mapping): bool True when the thread exits nicely. """ + cnt = 0 for name, action in self._actions.iteritems(): try: action.start() @@ -310,8 +313,14 @@ class Actions(JailThread, Mapping): lambda: False, self.sleeptime) logSys.debug("Actions: leave idle mode") continue - if not Utils.wait_for(lambda: not self.active or self.__checkBan(), self.sleeptime): - self.__checkUnBan() + # wait for ban (stop if gets inactive): + bancnt = Utils.wait_for(lambda: not self.active or self.__checkBan(), self.sleeptime) + cnt += bancnt + # unban if nothing is banned not later than banned tickets >= banPrecedence + if not bancnt or cnt >= self.banPrecedence: + if self.active: + self.__checkUnBan() + cnt = 0 self.__flushBan() self.stopActions() @@ -425,7 +434,7 @@ class Actions(JailThread, Mapping): """ cnt = 0 if not tickets: - tickets = self.__getFailTickets() + tickets = self.__getFailTickets(self.banPrecedence) for ticket in tickets: bTicket = BanManager.createBanTicket(ticket) ip = bTicket.getIP() diff --git a/fail2ban/tests/actionstestcase.py b/fail2ban/tests/actionstestcase.py index 2fd39108..09879795 100644 --- a/fail2ban/tests/actionstestcase.py +++ b/fail2ban/tests/actionstestcase.py @@ -173,3 +173,27 @@ class ExecuteActions(LogCaptureTestCase): self.assertNotLogged("Failed to execute unban") self.assertLogged("action1 unban deleted aInfo IP") self.assertLogged("action2 unban deleted aInfo IP") + + def testUnbanOnBusyBanBombing(self): + # check unban happens in-between of "ban bombing" despite lower precedence, + # if it is not work, we'll see "Unbanned 25" earliest at flushing (after stop) + + # each 3rd ban we should see an unban check (and tickets gets unbanned): + self.__actions.banPrecedence = 3 + + self.__actions.start() + + i = 0 + while i < 25: + ip = "192.0.2.%d" % i + self.__jail.putFailTicket(FailTicket(ip, 0)) + if not i: # wait for first to start "ban bombing": + self.assertLogged('Ban %s' % ip, wait=True) + i += 1 + + # wait for last ban (all 25 tickets gets banned): + self.assertLogged(' / 25,', wait=True) + self.__actions.stop() + self.__actions.join() + + self.assertNotLogged('Unbanned 25, 0 ticket(s)', wait=False) From 93727abeb8e3d79729cf27e144dd1b32a1480a14 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 11 Jun 2019 15:49:04 +0200 Subject: [PATCH 2/3] cherry-pick with_alt_time helper decorator from 0.11 --- fail2ban/tests/utils.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/fail2ban/tests/utils.py b/fail2ban/tests/utils.py index 65e09005..d4129b2a 100644 --- a/fail2ban/tests/utils.py +++ b/fail2ban/tests/utils.py @@ -245,6 +245,17 @@ def with_tmpdir(f): shutil.rmtree(tmp) return wrapper +def with_alt_time(f): + """Helper decorator to execute test in alternate (fixed) test time.""" + @wraps(f) + def wrapper(self, *args, **kwargs): + setUpMyTime() + try: + return f(self, *args, **kwargs) + finally: + tearDownMyTime() + return wrapper + # backwards compatibility to python 2.6: if not hasattr(unittest, 'SkipTest'): # pragma: no cover From 3326ec95ce0dde509891b845187d1f510b73f7b2 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 11 Jun 2019 15:46:06 +0200 Subject: [PATCH 3/3] small amend (preparing to merge in 0.11): more precise test and avoid "expired bantime" (in 0.11) --- fail2ban/tests/actionstestcase.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/fail2ban/tests/actionstestcase.py b/fail2ban/tests/actionstestcase.py index 09879795..d68990f4 100644 --- a/fail2ban/tests/actionstestcase.py +++ b/fail2ban/tests/actionstestcase.py @@ -32,7 +32,7 @@ from ..server.actions import Actions from ..server.ticket import FailTicket from ..server.utils import Utils from .dummyjail import DummyJail -from .utils import LogCaptureTestCase +from .utils import LogCaptureTestCase, with_alt_time, MyTime TEST_FILES_DIR = os.path.join(os.path.dirname(__file__), "files") @@ -174,26 +174,38 @@ class ExecuteActions(LogCaptureTestCase): self.assertLogged("action1 unban deleted aInfo IP") self.assertLogged("action2 unban deleted aInfo IP") + @with_alt_time def testUnbanOnBusyBanBombing(self): # check unban happens in-between of "ban bombing" despite lower precedence, # if it is not work, we'll see "Unbanned 25" earliest at flushing (after stop) # each 3rd ban we should see an unban check (and tickets gets unbanned): self.__actions.banPrecedence = 3 + self.__actions.setBanTime(100) self.__actions.start() + MyTime.setTime(0); # avoid "expired bantime" (in 0.11) i = 0 while i < 25: ip = "192.0.2.%d" % i self.__jail.putFailTicket(FailTicket(ip, 0)) - if not i: # wait for first to start "ban bombing": - self.assertLogged('Ban %s' % ip, wait=True) i += 1 # wait for last ban (all 25 tickets gets banned): self.assertLogged(' / 25,', wait=True) + + MyTime.setTime(200); # unban time for 25 tickets reached + + while i < 50: + ip = "192.0.2.%d" % i + self.__jail.putFailTicket(FailTicket(ip, 200)) + i += 1 + + # wait for last ban (all 50 tickets gets banned): + self.assertLogged(' / 50,', wait=True) self.__actions.stop() self.__actions.join() - self.assertNotLogged('Unbanned 25, 0 ticket(s)', wait=False) + self.assertLogged('Unbanned 25, 0 ticket(s)') + self.assertNotLogged('Unbanned 50, 0 ticket(s)')