From c1da6611ecfdc4cf86597f614b7b5f0d780b4ab1 Mon Sep 17 00:00:00 2001 From: sebres Date: Mon, 13 Mar 2017 18:47:26 +0100 Subject: [PATCH 1/3] [BF] prevents always converting of calling map items in replaceTag (without direct access of item): substituteRecursiveTags: ignore replacing callable items from calling map - should be converted on demand only (by get) --- fail2ban/helpers.py | 3 +++ fail2ban/server/action.py | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/fail2ban/helpers.py b/fail2ban/helpers.py index 2407df1f..726631a6 100644 --- a/fail2ban/helpers.py +++ b/fail2ban/helpers.py @@ -240,6 +240,7 @@ def substituteRecursiveTags(inptags, conditional='', # init: ignore = set(ignore) done = set() + calmap = hasattr(tags, "getRawItem") # repeat substitution while embedded-recursive (repFlag is True) while True: repFlag = False @@ -247,6 +248,8 @@ def substituteRecursiveTags(inptags, conditional='', for tag in tags.iterkeys(): # 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 value = orgval = str(tags[tag]) # search and replace all tags within value, that can be interpolated using other tags: m = tre_search(value) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index e7a98dbb..06a499df 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -98,6 +98,13 @@ class CallingMap(MutableMapping, object): except: return dict(self.data, **self.storage) + def getRawItem(self, key): + try: + value = self.storage[key] + except KeyError: + value = self.data[key] + return value + def __getitem__(self, key): try: value = self.storage[key] From 5030e3a1228265638460fcf7ffe2385dacfd5d30 Mon Sep 17 00:00:00 2001 From: sebres Date: Mon, 13 Mar 2017 20:45:35 +0100 Subject: [PATCH 2/3] [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", From ccfd1ccb2d80087841c6e9242bf913f0ff7ee41c Mon Sep 17 00:00:00 2001 From: sebres Date: Mon, 13 Mar 2017 21:56:06 +0100 Subject: [PATCH 3/3] code review, increase coverage, etc. --- fail2ban/client/configreader.py | 14 +------------- fail2ban/helpers.py | 30 ++++++++++++++++++++++++++++++ fail2ban/server/action.py | 4 ++-- fail2ban/tests/actiontestcase.py | 29 +++++++++++++++++++++++------ 4 files changed, 56 insertions(+), 21 deletions(-) diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index b7da271b..04502504 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -29,24 +29,12 @@ import os from ConfigParser import NoOptionError, NoSectionError from .configparserinc import sys, SafeConfigParserWithIncludes, logLevel -from ..helpers import getLogger, substituteRecursiveTags +from ..helpers import getLogger, _merge_dicts, substituteRecursiveTags # Gets the instance of the logger. logSys = getLogger(__name__) -# if sys.version_info >= (3,5): -# def _merge_dicts(x, y): -# return {**x, **y} -# else: -def _merge_dicts(x, y): - r = x - if y: - r = x.copy() - r.update(y) - return r - - class ConfigReader(): """Generic config reader class. diff --git a/fail2ban/helpers.py b/fail2ban/helpers.py index de26dbcd..556ca173 100644 --- a/fail2ban/helpers.py +++ b/fail2ban/helpers.py @@ -169,6 +169,36 @@ def splitwords(s): return [] return filter(bool, map(str.strip, re.split('[ ,\n]+', s))) +if sys.version_info >= (3,5): + eval(compile(r'''if 1: + def _merge_dicts(x, y): + """Helper to merge dicts. + """ + if y: + return {**x, **y} + return x + + def _merge_copy_dicts(x, y): + """Helper to merge dicts to guarantee a copy result (r is never x). + """ + return {**x, **y} + ''', __file__, 'exec')) +else: + def _merge_dicts(x, y): + """Helper to merge dicts. + """ + r = x + if y: + r = x.copy() + r.update(y) + return r + def _merge_copy_dicts(x, y): + """Helper to merge dicts to guarantee a copy result (r is never x). + """ + r = x.copy() + if y: + r.update(y) + return r # # Following "uni_decode" function unified python independent any to string converting diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index 31a1a3dc..2a773638 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -36,7 +36,7 @@ from .failregex import mapTag2Opt from .ipdns import asip from .mytime import MyTime from .utils import Utils -from ..helpers import getLogger, substituteRecursiveTags, TAG_CRE, MAX_TAG_REPLACE_COUNT +from ..helpers import getLogger, _merge_copy_dicts, substituteRecursiveTags, TAG_CRE, MAX_TAG_REPLACE_COUNT # Gets the instance of the logger. logSys = getLogger(__name__) @@ -148,7 +148,7 @@ class CallingMap(MutableMapping, object): return len(self.data) def copy(self): # pargma: no cover - return self.__class__(self.data.copy()) + return self.__class__(_merge_copy_dicts(self.data, self.storage)) class ActionBase(object): diff --git a/fail2ban/tests/actiontestcase.py b/fail2ban/tests/actiontestcase.py index 610857d6..70562baf 100644 --- a/fail2ban/tests/actiontestcase.py +++ b/fail2ban/tests/actiontestcase.py @@ -158,18 +158,35 @@ class CommandActionTest(LogCaptureTestCase): {'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 - ) + cm = CallingMap({ + 'A':0, + 'B':lambda self: '', + 'C':'', + 'D':'' + }) + # + # should raise no exceptions: + substituteRecursiveTags(cm) + # add exception tag: + cm['C'] = lambda self,i=0: 5 // int(self['A']) # raise error by access + # test direct get of callable (should raise an error): + self.assertRaises(ZeroDivisionError, lambda: cm['C']) + # should raise no exceptions (tag "C" still unused): + substituteRecursiveTags(cm) + # add reference to "broken" tag: + cm['D'] = 'test=' + # should raise an exception (BOOM by replacement of tag "D" recursive): + self.assertRaises(ZeroDivisionError, lambda: substituteRecursiveTags(cm)) + # # 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: + # should raise an exception (BOOM by replacement of tag "C"): self.assertRaises(ZeroDivisionError, lambda: self.__action.replaceTag('test=', cm)) + # should raise no exceptions (replaces tag "D" only): + self.assertEqual(self.__action.replaceTag('', cm), "test=") def testReplaceTag(self): aInfo = {