From 8f6ba1532563dbc8f41743817691be03ddc90881 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 20 Sep 2019 16:26:15 +0200 Subject: [PATCH] avoid unhandled exception during flush, better invariant check (and repair), avoid repair by unban/stop etc... --- config/jail.conf | 3 +- fail2ban/client/actionreader.py | 3 +- fail2ban/client/jailreader.py | 12 +- fail2ban/helpers.py | 12 +- fail2ban/server/action.py | 127 +++++++++++------- fail2ban/server/actions.py | 33 +++-- fail2ban/tests/actionstestcase.py | 52 +++++-- fail2ban/tests/clientreadertestcase.py | 4 +- .../files/action.d/action_modifyainfo.py | 5 + 9 files changed, 168 insertions(+), 83 deletions(-) diff --git a/config/jail.conf b/config/jail.conf index 41495a09..a77a47ba 100644 --- a/config/jail.conf +++ b/config/jail.conf @@ -861,7 +861,8 @@ filter = apache-pass[knocking_url="%(knocking_url)s"] logpath = %(apache_access_log)s blocktype = RETURN returntype = DROP -action = %(action_)s[blocktype=%(blocktype)s, returntype=%(returntype)s] +action = %(action_)s[blocktype=%(blocktype)s, returntype=%(returntype)s, + actionstart_on_demand=True, actionrepair_on_unban=True] bantime = 1h maxretry = 1 findtime = 1 diff --git a/fail2ban/client/actionreader.py b/fail2ban/client/actionreader.py index d7bba61f..0840fe68 100644 --- a/fail2ban/client/actionreader.py +++ b/fail2ban/client/actionreader.py @@ -44,6 +44,7 @@ class ActionReader(DefinitionInitConfigReader): "actionreload": ["string", None], "actioncheck": ["string", None], "actionrepair": ["string", None], + "actionrepair_on_unban": ["string", None], "actionban": ["string", None], "actionunban": ["string", None], "norestored": ["string", None], @@ -78,7 +79,7 @@ class ActionReader(DefinitionInitConfigReader): opts = self.getCombined( ignore=CommandAction._escapedTags | set(('timeout', 'bantime'))) # type-convert only after combined (otherwise boolean converting prevents substitution): - for o in ('norestored', 'actionstart_on_demand'): + for o in ('norestored', 'actionstart_on_demand', 'actionrepair_on_unban'): if opts.get(o): opts[o] = self._convert_to_boolean(opts[o]) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index 02368a8b..917a562c 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -33,7 +33,7 @@ from .configreader import ConfigReaderUnshared, ConfigReader from .filterreader import FilterReader from .actionreader import ActionReader from ..version import version -from ..helpers import getLogger, extractOptions, splitwords +from ..helpers import getLogger, extractOptions, splitWithOptions, splitwords # Gets the instance of the logger. logSys = getLogger(__name__) @@ -164,21 +164,15 @@ class JailReader(ConfigReader): self.__filter.getOptions(self.__opts) # Read action - prevln = '' - actlst = self.__opts["action"].split('\n') - for n, act in enumerate(actlst): + for act in splitWithOptions(self.__opts["action"]): try: + act = act.strip() if not act: # skip empty actions continue # join with previous line if needed (consider possible new-line): - if prevln: act = prevln + '\n' + act actName, actOpt = extractOptions(act) prevln = '' if not actName: - # consider possible new-line, so repeat with joined next line's: - if n < len(actlst) - 1: - prevln = act - continue raise JailDefError("Invalid action definition %r" % act) if actName.endswith(".py"): self.__actions.append([ diff --git a/fail2ban/helpers.py b/fail2ban/helpers.py index 44574ff8..213a405f 100644 --- a/fail2ban/helpers.py +++ b/fail2ban/helpers.py @@ -336,6 +336,9 @@ OPTION_CRE = re.compile(r"^([^\[]+)(?:\[(.*)\])?\s*$", re.DOTALL) # `action = act[p1=...][p2=...]` OPTION_EXTRACT_CRE = re.compile( r'([\w\-_\.]+)=(?:"([^"]*)"|\'([^\']*)\'|([^,\]]*))(?:,|\]\s*\[|$)', re.DOTALL) +# split by new-line considering possible new-lines within options [...]: +OPTION_SPLIT_CRE = re.compile( + r'(?:[^\[\n]+(?:\s*\[\s*(?:[\w\-_\.]+=(?:"[^"]*"|\'[^\']*\'|[^,\]]*)\s*(?:,|\]\s*\[)?\s*)*\])?\s*|[^\n]+)(?=\n\s*|$)', re.DOTALL) def extractOptions(option): match = OPTION_CRE.match(option) @@ -352,6 +355,9 @@ def extractOptions(option): option_opts[opt.strip()] = value.strip() return option_name, option_opts +def splitWithOptions(option): + return OPTION_SPLIT_CRE.findall(option) + # # Following facilities used for safe recursive interpolation of # tags () in tagged options. @@ -386,8 +392,7 @@ def substituteRecursiveTags(inptags, conditional='', """ #logSys = getLogger("fail2ban") tre_search = TAG_CRE.search - # copy return tags dict to prevent modifying of inptags: - tags = inptags.copy() + tags = inptags # init: ignore = set(ignore) done = set() @@ -449,6 +454,9 @@ def substituteRecursiveTags(inptags, conditional='', # check still contains any tag - should be repeated (possible embedded-recursive substitution): if tre_search(value): repFlag = True + # copy return tags dict to prevent modifying of inptags: + if id(tags) == id(inptags): + tags = inptags.copy() tags[tag] = value # no more sub tags (and no possible composite), add this tag to done set (just to be faster): if '<' not in value: done.add(tag) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index e4c83409..0942a3e5 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -50,6 +50,7 @@ allowed_ipv6 = True # capture groups from filter for map to ticket data: FCUSTAG_CRE = re.compile(r''); # currently uppercase only +COND_FAMILIES = ('inet4', 'inet6') CONDITIONAL_FAM_RE = re.compile(r"^(\w+)\?(family)=") # Special tags: @@ -363,8 +364,8 @@ class CommandAction(ActionBase): self.__properties = dict( (key, getattr(self, key)) for key in dir(self) - if not key.startswith("_") and not callable(getattr(self, key))) - # + if not key.startswith("_") and not callable(getattr(self, key)) + ) return self.__properties @property @@ -372,8 +373,13 @@ class CommandAction(ActionBase): return self.__substCache def _getOperation(self, tag, family): + # be sure family is enclosed as conditional value (if not overwritten in action): + if family and self._hasCondSection: + if 'family' not in self._properties and 'family?family='+family not in self._properties: + self._properties['family?family='+family] = family + # replace operation tag (interpolate all values): return self.replaceTag(tag, self._properties, - conditional=('family=' + family), cache=self.__substCache) + conditional=('family='+family if family else ''), cache=self.__substCache) def _executeOperation(self, tag, operation, family=[]): """Executes the operation commands (like "actionstart", "actionstop", etc). @@ -400,7 +406,17 @@ class CommandAction(ActionBase): raise RuntimeError("Error %s action %s/%s: %r" % (operation, self._jail, self._name, e)) return res - COND_FAMILIES = ('inet4', 'inet6') + @property + def _hasCondSection(self): + v = self._properties.get('__hasCondSection') + if v is None: + v = False + for n in self._properties: + if CONDITIONAL_FAM_RE.match(n): + v = True + break + self._properties['__hasCondSection'] = v + return v @property def _startOnDemand(self): @@ -409,11 +425,7 @@ class CommandAction(ActionBase): if v is not None: return v # not set - auto-recognize (depending on conditional): - v = False - for n in self._properties: - if CONDITIONAL_FAM_RE.match(n): - v = True - break + v = self._hasCondSection self._properties['actionstart_on_demand'] = v return v @@ -514,6 +526,17 @@ class CommandAction(ActionBase): """ return self._executeOperation('', 'reloading') + def consistencyCheck(self, beforeRepair=None): + """Executes the invariant check with repair if expected (conditional). + """ + ret = True + # for each started family: + if self.actioncheck: + for (family, started) in self.__started.iteritems(): + if started and not self._invariantCheck(family, beforeRepair): + ret &= False + return ret + @staticmethod def escapeTag(value): """Escape characters which may be used for command injection. @@ -705,7 +728,40 @@ class CommandAction(ActionBase): realCmd = Utils.buildShellCmd(realCmd, varsDict) return realCmd - def _processCmd(self, cmd, aInfo=None, conditional=''): + def _invariantCheck(self, family='', beforeRepair=None): + """Executes a substituted `actioncheck` command. + """ + checkCmd = self._getOperation('', family) + if not checkCmd or self.executeCmd(checkCmd, self.timeout): + return True + # if don't need repair/restore - just return: + if beforeRepair and not beforeRepair(): + return False + self._logSys.error( + "Invariant check failed. Trying to restore a sane environment") + # try to find repair command, if exists - exec it: + repairCmd = self._getOperation('', family) + if repairCmd: + if not self.executeCmd(repairCmd, self.timeout): + self._logSys.critical("Unable to restore environment") + return False + else: + # no repair command, try to restart action... + # [WARNING] TODO: be sure all banactions get a repair command, because + # otherwise stop/start will theoretically remove all the bans, + # but the tickets are still in BanManager, so in case of new failures + # it will not be banned, because "already banned" will happen. + try: + self.stop() + except RuntimeError: # bypass error in stop (if start/check succeeded hereafter). + pass + self.start() + if not self.executeCmd(checkCmd, self.timeout): + self._logSys.critical("Unable to restore environment") + return False + return True + + def _processCmd(self, cmd, aInfo=None): """Executes a command with preliminary checks and substitutions. Before executing any commands, executes the "check" command first @@ -730,47 +786,26 @@ class CommandAction(ActionBase): return True # conditional corresponding family of the given ip: - if conditional == '': - conditional = 'family=inet4' - if allowed_ipv6: - try: - ip = aInfo["ip"] - if ip and asip(ip).isIPv6: - conditional = 'family=inet6' - except KeyError: - pass + try: + family = aInfo["family"] + except KeyError: + family = '' - checkCmd = self.replaceTag('', self._properties, - conditional=conditional, cache=self.__substCache) - if checkCmd: - if not self.executeCmd(checkCmd, self.timeout): - self._logSys.error( - "Invariant check failed. Trying to restore a sane environment") - # try to find repair command, if exists - exec it: - repairCmd = self.replaceTag('', self._properties, - conditional=conditional, cache=self.__substCache) - if repairCmd: - if not self.executeCmd(repairCmd, self.timeout): - self._logSys.critical("Unable to restore environment") - return False - else: - # no repair command, try to restart action... - # [WARNING] TODO: be sure all banactions get a repair command, because - # otherwise stop/start will theoretically remove all the bans, - # but the tickets are still in BanManager, so in case of new failures - # it will not be banned, because "already banned" will happen. - try: - self.stop() - except RuntimeError: # bypass error in stop (if start/check succeeded hereafter). - pass - self.start() - if not self.executeCmd(checkCmd, self.timeout): - self._logSys.critical("Unable to restore environment") + # invariant check: + if self.actioncheck: + # don't repair/restore if unban (no matter): + def _beforeRepair(): + if cmd == '' and not self._properties.get('actionrepair_on_unban'): + self._logSys.error("Invariant check failed. Unban is impossible.") return False + return True + ret = self._invariantCheck(family, _beforeRepair) + if not ret: + return False # Replace static fields realCmd = self.replaceTag(cmd, self._properties, - conditional=conditional, cache=self.__substCache) + conditional=('family='+family if family else ''), cache=self.__substCache) # Replace dynamical tags, important - don't cache, no recursion and auto-escape here if aInfo is not None: diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index 51904ecd..35f2306b 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -160,8 +160,8 @@ class Actions(JailThread, Mapping): delacts = OrderedDict((name, action) for name, action in self._actions.iteritems() if name not in self._reload_actions) if len(delacts): - # unban all tickets using remove action only: - self.__flushBan(db=False, actions=delacts) + # unban all tickets using removed actions only: + self.__flushBan(db=False, actions=delacts, stop=True) # stop and remove it: self.stopActions(actions=delacts) delattr(self, '_reload_actions') @@ -326,7 +326,7 @@ class Actions(JailThread, Mapping): self.__checkUnBan(bancnt if bancnt and bancnt < self.unbanMaxCount else self.unbanMaxCount) cnt = 0 - self.__flushBan() + self.__flushBan(stop=True) self.stopActions() return True @@ -494,7 +494,7 @@ class Actions(JailThread, Mapping): cnt, self.__banManager.size(), self._jail.name) return cnt - def __flushBan(self, db=False, actions=None): + def __flushBan(self, db=False, actions=None, stop=False): """Flush the ban list. Unban all IP address which are still in the banning list. @@ -513,11 +513,26 @@ class Actions(JailThread, Mapping): # first we'll execute flush for actions supporting this operation: unbactions = {} for name, action in (actions if actions is not None else self._actions).iteritems(): - if hasattr(action, 'flush') and action.actionflush: - logSys.notice("[%s] Flush ticket(s) with %s", self._jail.name, name) - action.flush() - else: - unbactions[name] = action + try: + if hasattr(action, 'flush') and (not isinstance(action, CommandAction) or action.actionflush): + logSys.notice("[%s] Flush ticket(s) with %s", self._jail.name, name) + action.flush() + else: + unbactions[name] = action + except Exception as e: + logSys.error("Failed to flush bans in jail '%s' action '%s': %s", + self._jail.name, name, e, + exc_info=logSys.getEffectiveLevel()<=logging.DEBUG) + logSys.info("No flush occured, do consistency check") + def _beforeRepair(): + if stop: + self._logSys.error("Invariant check failed. Flush is impossible.") + return False + return True + if not hasattr(action, 'consistencyCheck') or action.consistencyCheck(_beforeRepair): + # fallback to single unbans: + logSys.info("unban tickets each individualy") + unbactions[name] = action actions = unbactions # flush the database also: if db and self._jail.database is not None: diff --git a/fail2ban/tests/actionstestcase.py b/fail2ban/tests/actionstestcase.py index d2dcdadb..86b539f0 100644 --- a/fail2ban/tests/actionstestcase.py +++ b/fail2ban/tests/actionstestcase.py @@ -44,20 +44,20 @@ class ExecuteActions(LogCaptureTestCase): super(ExecuteActions, self).setUp() self.__jail = DummyJail() self.__actions = Actions(self.__jail) - self.__tmpfile, self.__tmpfilename = tempfile.mkstemp() def tearDown(self): super(ExecuteActions, self).tearDown() - os.remove(self.__tmpfilename) - def defaultActions(self): + def defaultAction(self): self.__actions.add('ip') - self.__ip = self.__actions['ip'] - self.__ip.actionstart = 'echo ip start 64 >> "%s"' % self.__tmpfilename - self.__ip.actionban = 'echo ip ban >> "%s"' % self.__tmpfilename - self.__ip.actionunban = 'echo ip unban >> "%s"' % self.__tmpfilename - self.__ip.actioncheck = 'echo ip check >> "%s"' % self.__tmpfilename - self.__ip.actionstop = 'echo ip stop >> "%s"' % self.__tmpfilename + act = self.__actions['ip'] + act.actionstart = 'echo ip start' + act.actionban = 'echo ip ban ' + act.actionunban = 'echo ip unban ' + act.actioncheck = 'echo ip check' + act.actionflush = 'echo ip flush ' + act.actionstop = 'echo ip stop' + return act def testActionsAddDuplicateName(self): self.__actions.add('test') @@ -89,13 +89,12 @@ class ExecuteActions(LogCaptureTestCase): self.assertLogged('Ban 192.0.2.3') def testActionsOutput(self): - self.defaultActions() + self.defaultAction() self.__actions.start() - with open(self.__tmpfilename) as f: - self.assertTrue( Utils.wait_for(lambda: (f.read() == "ip start 64\n"), 3) ) - + self.assertLogged("stdout: %r" % 'ip start', wait=True) self.__actions.stop() self.__actions.join() + self.assertLogged("stdout: %r" % 'ip flush', "stdout: %r" % 'ip stop') self.assertEqual(self.__actions.status(),[("Currently banned", 0 ), ("Total banned", 0 ), ("Banned IP list", [] )]) @@ -211,3 +210,30 @@ class ExecuteActions(LogCaptureTestCase): self.assertLogged('Unbanned 30, 0 ticket(s)') self.assertNotLogged('Unbanned 50, 0 ticket(s)') + + @with_alt_time + def testActionsConsistencyCheck(self): + # flush is broken - test no unhandled except and invariant check: + act = self.defaultAction() + setattr(act, 'actionflush?family=inet6', 'echo ip flush ; exit 1') + act.actionstart_on_demand = True + self.__actions.start() + self.assertNotLogged("stdout: %r" % 'ip start') + + self.assertEqual(self.__actions.addBannedIP('192.0.2.1'), 1) + self.assertEqual(self.__actions.addBannedIP('2001:db8::1'), 1) + self.assertLogged('Ban 192.0.2.1', 'Ban 2001:db8::1', + "stdout: %r" % 'ip start', + "stdout: %r" % 'ip ban 192.0.2.1', + "stdout: %r" % 'ip ban 2001:db8::1', + all=True, wait=True) + + self.__actions._Actions__flushBan() + self.assertLogged('Failed to flush bans', + 'No flush occured, do consistency check', + "stdout: %r" % 'ip ban 192.0.2.1', + all=True, wait=True) + + self.__actions.stop() + self.__actions.join() + diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 5746f3fb..d39860f4 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -31,7 +31,7 @@ import unittest from ..client.configreader import ConfigReader, ConfigReaderUnshared, \ DefinitionInitConfigReader, NoSectionError from ..client import configparserinc -from ..client.jailreader import JailReader, extractOptions +from ..client.jailreader import JailReader, extractOptions, splitWithOptions from ..client.filterreader import FilterReader from ..client.jailsreader import JailsReader from ..client.actionreader import ActionReader, CommandAction @@ -778,7 +778,7 @@ class JailsReaderTest(LogCaptureTestCase): # somewhat duplicating here what is done in JailsReader if # the jail is enabled - for act in actions.split('\n'): + for act in splitWithOptions(actions): actName, actOpt = extractOptions(act) self.assertTrue(len(actName)) self.assertTrue(isinstance(actOpt, dict)) diff --git a/fail2ban/tests/files/action.d/action_modifyainfo.py b/fail2ban/tests/files/action.d/action_modifyainfo.py index b003edef..d585a7ee 100644 --- a/fail2ban/tests/files/action.d/action_modifyainfo.py +++ b/fail2ban/tests/files/action.d/action_modifyainfo.py @@ -12,4 +12,9 @@ class TestAction(ActionBase): del aInfo['ip'] self._logSys.info("%s unban deleted aInfo IP", self._name) + def flush(self): + # intended error to cover no unhandled exception occurs in flash + # as well as unbans are done individually after errored flush. + raise ValueError("intended error") + Action = TestAction