From 8268c1641f968d553141236803f2b2467a477071 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Tue, 17 Jun 2014 23:24:23 +0100 Subject: [PATCH 1/3] BF: aInfo could be modified by actions, causing unexpected behaviour A separate copy of aInfo is passed to each action --- config/action.d/smtp.py | 2 +- fail2ban/server/action.py | 6 ++++++ fail2ban/server/actions.py | 14 ++++++++------ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/config/action.d/smtp.py b/config/action.d/smtp.py index 2d0add8e..86857616 100644 --- a/config/action.d/smtp.py +++ b/config/action.d/smtp.py @@ -45,7 +45,7 @@ messages['ban'] = {} messages['ban']['head'] = \ """Hi, -The IP %(ip)s has just been banned for %(bantime)s seconds +The IP %(ip)s has just been banned for %(bantime)i seconds by Fail2Ban after %(failures)i attempts against %(jailname)s. """ messages['ban']['tail'] = \ diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index fefe2c2c..7464e008 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -68,6 +68,9 @@ class CallingMap(MutableMapping): def __init__(self, *args, **kwargs): self.data = dict(*args, **kwargs) + def __repr__(self): + return "%s(%r)" % (self.__class__.__name__, self.data) + def __getitem__(self, key): value = self.data[key] if callable(value): @@ -87,6 +90,9 @@ class CallingMap(MutableMapping): def __len__(self): return len(self.data) + def copy(self): + return self.__class__(self.data.copy()) + class ActionBase(object): """An abstract base class for actions in Fail2Ban. diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index dd68ac13..77cc208d 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -273,11 +273,12 @@ class Actions(JailThread, Mapping): logSys.notice("[%s] Ban %s" % (self._jail.name, aInfo["ip"])) for name, action in self._actions.iteritems(): try: - action.ban(aInfo) + action.ban(aInfo.copy()) except Exception as e: logSys.error( - "Failed to execute ban jail '%s' action '%s': %s", - self._jail.name, name, e, + "Failed to execute ban jail '%s' action '%s' " + "info '%r': %s", + self._jail.name, name, aInfo, e, exc_info=logSys.getEffectiveLevel()<=logging.DEBUG) return True else: @@ -321,11 +322,12 @@ class Actions(JailThread, Mapping): logSys.notice("[%s] Unban %s" % (self._jail.name, aInfo["ip"])) for name, action in self._actions.iteritems(): try: - action.unban(aInfo) + action.unban(aInfo.copy()) except Exception as e: logSys.error( - "Failed to execute unban jail '%s' action '%s': %s", - self._jail.name, name, e, + "Failed to execute unban jail '%s' action '%s' " + "info '%r': %s", + self._jail.name, name, aInfo, e, exc_info=logSys.getEffectiveLevel()<=logging.DEBUG) @property From 7640aa091825baf715fe7dfe5fd0210fa2d40537 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 22 Jun 2014 13:46:25 +0100 Subject: [PATCH 2/3] TST: Test for actions modifying (un)ban aInfo --- fail2ban/tests/actionstestcase.py | 22 +++++++++++++++++++ .../files/action.d/action_modifyainfo.py | 11 ++++++++++ 2 files changed, 33 insertions(+) create mode 100644 fail2ban/tests/files/action.d/action_modifyainfo.py diff --git a/fail2ban/tests/actionstestcase.py b/fail2ban/tests/actionstestcase.py index ed0fb619..3f968e46 100644 --- a/fail2ban/tests/actionstestcase.py +++ b/fail2ban/tests/actionstestcase.py @@ -29,6 +29,7 @@ import os import tempfile from ..server.actions import Actions +from ..server.ticket import FailTicket from .dummyjail import DummyJail from .utils import LogCaptureTestCase @@ -140,3 +141,24 @@ class ExecuteActions(LogCaptureTestCase): self.__actions.stop() self.__actions.join() self.assertTrue(self._is_logged("Failed to stop")) + + def testBanActionsAInfo(self): + # Action which deletes IP address from aInfo + self.__actions.add( + "action1", + os.path.join(TEST_FILES_DIR, "action.d/action_modifyainfo.py"), + {}) + self.__actions.add( + "action2", + os.path.join(TEST_FILES_DIR, "action.d/action_modifyainfo.py"), + {}) + self.__jail.putFailTicket(FailTicket("1.2.3.4", 0)) + self.__actions._Actions__checkBan() + # Will fail if modification of aInfo from first action propagates + # to second action, as both delete same key + self.assertFalse(self._is_logged("Failed to execute ban")) + + self.__actions._Actions__flushBan() + # Will fail if modification of aInfo from first action propagates + # to second action, as both delete same key + self.assertFalse(self._is_logged("Failed to execute unban")) diff --git a/fail2ban/tests/files/action.d/action_modifyainfo.py b/fail2ban/tests/files/action.d/action_modifyainfo.py new file mode 100644 index 00000000..0a1d944a --- /dev/null +++ b/fail2ban/tests/files/action.d/action_modifyainfo.py @@ -0,0 +1,11 @@ + +from fail2ban.server.action import ActionBase + +class TestAction(ActionBase): + + def ban(self, aInfo): + del aInfo['ip'] + + unban = ban + +Action = TestAction From dd3ab858dd30d4ebcab09f6d88e03726a9fdad60 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 22 Jun 2014 13:56:32 +0100 Subject: [PATCH 3/3] TST: actions modifying aInfo test more robust --- fail2ban/tests/actionstestcase.py | 4 ++++ fail2ban/tests/files/action.d/action_modifyainfo.py | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/fail2ban/tests/actionstestcase.py b/fail2ban/tests/actionstestcase.py index 3f968e46..d66930ef 100644 --- a/fail2ban/tests/actionstestcase.py +++ b/fail2ban/tests/actionstestcase.py @@ -157,8 +157,12 @@ class ExecuteActions(LogCaptureTestCase): # Will fail if modification of aInfo from first action propagates # to second action, as both delete same key self.assertFalse(self._is_logged("Failed to execute ban")) + self.assertTrue(self._is_logged("action1 ban deleted aInfo IP")) + self.assertTrue(self._is_logged("action2 ban deleted aInfo IP")) self.__actions._Actions__flushBan() # Will fail if modification of aInfo from first action propagates # to second action, as both delete same key self.assertFalse(self._is_logged("Failed to execute unban")) + self.assertTrue(self._is_logged("action1 unban deleted aInfo IP")) + self.assertTrue(self._is_logged("action2 unban deleted aInfo IP")) diff --git a/fail2ban/tests/files/action.d/action_modifyainfo.py b/fail2ban/tests/files/action.d/action_modifyainfo.py index 0a1d944a..9fbe1e0b 100644 --- a/fail2ban/tests/files/action.d/action_modifyainfo.py +++ b/fail2ban/tests/files/action.d/action_modifyainfo.py @@ -5,7 +5,10 @@ class TestAction(ActionBase): def ban(self, aInfo): del aInfo['ip'] + self._logSys.info("%s ban deleted aInfo IP", self._name) - unban = ban + def unban(self, aInfo): + del aInfo['ip'] + self._logSys.info("%s unban deleted aInfo IP", self._name) Action = TestAction