From 5030e3a1228265638460fcf7ffe2385dacfd5d30 Mon Sep 17 00:00:00 2001 From: sebres Date: Mon, 13 Mar 2017 20:45:35 +0100 Subject: [PATCH] [Important] Prohibit replacement of recursive "tags" in the action info resp. calling map (very bad idea to do this): - the calling map contains normally dynamic values only (no recursive tags); - recursive replacement can be vulnerable, because can contain foreign (user) input captured from log (will be replaced in the shell arguments); --- fail2ban/helpers.py | 6 ++-- fail2ban/server/action.py | 47 ++++++++++++++++++-------------- fail2ban/tests/actiontestcase.py | 14 ++++++++++ 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/fail2ban/helpers.py b/fail2ban/helpers.py index 726631a6..de26dbcd 100644 --- a/fail2ban/helpers.py +++ b/fail2ban/helpers.py @@ -240,7 +240,7 @@ def substituteRecursiveTags(inptags, conditional='', # init: ignore = set(ignore) done = set() - calmap = hasattr(tags, "getRawItem") + noRecRepl = hasattr(tags, "getRawItem") # repeat substitution while embedded-recursive (repFlag is True) while True: repFlag = False @@ -249,7 +249,7 @@ def substituteRecursiveTags(inptags, conditional='', # ignore escaped or already done (or in ignore list): if tag in ignore or tag in done: continue # ignore replacing callable items from calling map - should be converted on demand only (by get): - if calmap and callable(tags.getRawItem(tag)): continue + if noRecRepl and callable(tags.getRawItem(tag)): continue value = orgval = str(tags[tag]) # search and replace all tags within value, that can be interpolated using other tags: m = tre_search(value) @@ -284,6 +284,8 @@ def substituteRecursiveTags(inptags, conditional='', # constructs like . m = tre_search(value, m.end()) continue + # if calling map - be sure we've string: + if noRecRepl: repl = str(repl) value = value.replace('<%s>' % rtag, repl) #logSys.log(5, 'value now: %s' % value) # increment reference count: diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index 06a499df..31a1a3dc 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -453,7 +453,7 @@ class CommandAction(ActionBase): return value @classmethod - def replaceTag(cls, query, aInfo, conditional='', cache=None): + def replaceTag(cls, query, aInfo, conditional='', cache=None, substRec=True): """Replaces tags in `query` with property values. Parameters @@ -478,23 +478,29 @@ class CommandAction(ActionBase): except KeyError: pass - # first try get cached tags dictionary: - subInfo = csubkey = None - if cache is not None: - csubkey = ('subst-tags', id(aInfo), conditional) - try: - subInfo = cache[csubkey] - except KeyError: - pass - # interpolation of dictionary: - if subInfo is None: - subInfo = substituteRecursiveTags(aInfo, conditional, ignore=cls._escapedTags) - # cache if possible: - if csubkey is not None: - cache[csubkey] = subInfo + # **Important**: don't replace if calling map - contains dynamic values only, + # no recursive tags, otherwise may be vulnerable on foreign user-input: + noRecRepl = isinstance(aInfo, CallingMap) + if noRecRepl: + subInfo = aInfo + else: + # substitute tags recursive (and cache if possible), + # first try get cached tags dictionary: + subInfo = csubkey = None + if cache is not None: + csubkey = ('subst-tags', id(aInfo), conditional) + try: + subInfo = cache[csubkey] + except KeyError: + pass + # interpolation of dictionary: + if subInfo is None: + subInfo = substituteRecursiveTags(aInfo, conditional, ignore=cls._escapedTags) + # cache if possible: + if csubkey is not None: + cache[csubkey] = subInfo # substitution callable, used by interpolation of each tag - repeatSubst = {0: 0} def substVal(m): tag = m.group(1) # tagname from match value = None @@ -510,18 +516,17 @@ class CommandAction(ActionBase): # That one needs to be escaped since its content is # out of our control value = cls.escapeTag(value) - # possible contains tags: - if '<' in value: - repeatSubst[0] = 1 + # replacement for tag: return value # interpolation of query: count = MAX_TAG_REPLACE_COUNT + 1 while True: - repeatSubst[0] = 0 value = TAG_CRE.sub(substVal, query) + # **Important**: no recursive replacement for tags from calling map (properties only): + if noRecRepl: break # possible recursion ? - if not repeatSubst or value == query: break + if value == query or '<' not in value: break query = value count -= 1 if count <= 0: diff --git a/fail2ban/tests/actiontestcase.py b/fail2ban/tests/actiontestcase.py index 8834ab8d..610857d6 100644 --- a/fail2ban/tests/actiontestcase.py +++ b/fail2ban/tests/actiontestcase.py @@ -157,6 +157,20 @@ class CommandActionTest(LogCaptureTestCase): self.assertEqual(substituteRecursiveTags({'A': 'A HOST> B IP C', 'PREF': 'V4', 'IPV4HOST': '1.2.3.4'}), {'A': 'A 1.2.3.4 B IPV4 C', 'PREF': 'V4', 'IPV4HOST': '1.2.3.4'}) + def testSubstRec_DontTouchUnusedCallable(self): + cm = CallingMap( + A=0, + B=lambda self: '', + C=lambda self,i=0: 5 // int(self['A']) # raise error by access + ) + # should raise no exceptions: + self.assertEqual(self.__action.replaceTag('test=', cm), "test=0") + # **Important**: recursive replacement of dynamic data from calling map should be prohibited, + # otherwise may be vulnerable on foreign user-input: + self.assertEqual(self.__action.replaceTag('test=----', cm), "test=0----0") + # should raise an exception: + self.assertRaises(ZeroDivisionError, lambda: self.__action.replaceTag('test=', cm)) + def testReplaceTag(self): aInfo = { 'HOST': "192.0.2.0",