From 809acb69e5928c0e678ad25b43e53b567cb23a3b Mon Sep 17 00:00:00 2001
From: sebres <serg.brester@sebres.de>
Date: Tue, 11 Jun 2019 14:37:10 +0200
Subject: [PATCH] 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)