From e5c9f9ec1cfa27180ce1f0f62036fe4fd42316b3 Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 15 Mar 2017 16:35:40 +0100 Subject: [PATCH] [interim commit] try to fix possible escape vulnerability in actions --- fail2ban/server/action.py | 67 +++++++++++++++++++++----------- fail2ban/server/actions.py | 1 + fail2ban/server/ipdns.py | 5 +++ fail2ban/server/utils.py | 64 +++++++++++++++++++++--------- fail2ban/tests/actiontestcase.py | 28 ++++++++++--- 5 files changed, 119 insertions(+), 46 deletions(-) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index 2a773638..bdb25d27 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, substRec=True): + def replaceTag(cls, query, aInfo, conditional='', cache=None): """Replaces tags in `query` with property values. Parameters @@ -481,9 +481,8 @@ class CommandAction(ActionBase): # **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: + subInfo = aInfo + if not noRecRepl: # substitute tags recursive (and cache if possible), # first try get cached tags dictionary: subInfo = csubkey = None @@ -534,13 +533,50 @@ class CommandAction(ActionBase): "unexpected too long replacement interpolation, " "possible self referencing definitions in query: %s" % (query,)) - # cache if possible: if cache is not None: cache[ckey] = value # return value + @classmethod + def replaceDynamicTags(cls, realCmd, aInfo): + """Replaces dynamical tags in `query` with property values. + + **Important** + ------------- + Because this tags are dynamic resp. foreign (user) input: + - values should be escaped + - no recursive substitution (no interpolation for >) + - don't use cache + + Parameters + ---------- + query : str + String with tags. + aInfo : dict + Tags(keys) and associated values for substitution in query. + + Returns + ------- + str + shell script as string or array with tags replaced (direct or as variables). + """ + realCmd = cls.replaceTag(realCmd, aInfo, conditional=False) + # Replace ticket options (filter capture groups) non-recursive: + if '<' in realCmd: + tickData = aInfo.get("F-*") + if not tickData: tickData = {} + def substTag(m): + tn = mapTag2Opt(m.groups()[0]) + try: + return str(tickData[tn]) + except KeyError: + return "" + + realCmd = FCUSTAG_CRE.sub(substTag, realCmd) + return realCmd + def _processCmd(self, cmd, aInfo=None, conditional=''): """Executes a command with preliminary checks and substitutions. @@ -605,21 +641,9 @@ class CommandAction(ActionBase): realCmd = self.replaceTag(cmd, self._properties, conditional=conditional, cache=self.__substCache) - # Replace dynamical tags (don't use cache here) + # Replace dynamical tags, important - don't cache, no recursion and auto-escape here if aInfo is not None: - realCmd = self.replaceTag(realCmd, aInfo, conditional=conditional) - # Replace ticket options (filter capture groups) non-recursive: - if '<' in realCmd: - tickData = aInfo.get("F-*") - if not tickData: tickData = {} - def substTag(m): - tn = mapTag2Opt(m.groups()[0]) - try: - return str(tickData[tn]) - except KeyError: - return "" - - realCmd = FCUSTAG_CRE.sub(substTag, realCmd) + realCmd = self.replaceDynamicTags(realCmd, aInfo) else: realCmd = cmd @@ -653,8 +677,5 @@ class CommandAction(ActionBase): logSys.debug("Nothing to do") return True - _cmd_lock.acquire() - try: + with _cmd_lock: return Utils.executeCmd(realCmd, timeout, shell=True, output=False, **kwargs) - finally: - _cmd_lock.release() diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index e0719cde..3a85e569 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -290,6 +290,7 @@ class Actions(JailThread, Mapping): AI_DICT = { "ip": lambda self: self.__ticket.getIP(), + "family": lambda self: self['ip'].familyStr, "ip-rev": lambda self: self['ip'].getPTR(''), "ip-host": lambda self: self['ip'].getHost(), "fid": lambda self: self.__ticket.getID(), diff --git a/fail2ban/server/ipdns.py b/fail2ban/server/ipdns.py index 8990618a..bd3b812d 100644 --- a/fail2ban/server/ipdns.py +++ b/fail2ban/server/ipdns.py @@ -261,6 +261,11 @@ class IPAddr(object): def family(self): return self._family + FAM2STR = {socket.AF_INET: 'inet4', socket.AF_INET6: 'inet6'} + @property + def familyStr(self): + return IPAddr.FAM2STR.get(self._family) + @property def plen(self): return self._plen diff --git a/fail2ban/server/utils.py b/fail2ban/server/utils.py index b258ae77..56428294 100644 --- a/fail2ban/server/utils.py +++ b/fail2ban/server/utils.py @@ -28,7 +28,7 @@ import signal import subprocess import sys import time -from ..helpers import getLogger, uni_decode +from ..helpers import getLogger, _merge_dicts, uni_decode if sys.version_info >= (3, 3): import importlib.machinery @@ -116,7 +116,22 @@ class Utils(): return flags @staticmethod - def executeCmd(realCmd, timeout=60, shell=True, output=False, tout_kill_tree=True, success_codes=(0,)): + def buildShellCmd(realCmd, varsDict): + # build map as array of vars and command line array: + varsStat = "" + if not isinstance(realCmd, list): + realCmd = [realCmd] + i = len(realCmd)-1 + for k, v in varsDict.iteritems(): + varsStat += "%s=$%s " % (k, i) + realCmd.append(v) + i += 1 + realCmd[0] = varsStat + "\n" + realCmd[0] + return realCmd + + @staticmethod + def executeCmd(realCmd, timeout=60, shell=True, output=False, tout_kill_tree=True, + success_codes=(0,), varsDict=None): """Executes a command. Parameters @@ -131,6 +146,8 @@ class Utils(): output : bool If output is True, the function returns tuple (success, stdoutdata, stderrdata, returncode). If False, just indication of success is returned + varsDict: dict + variables supplied to the command (or to the shell script) Returns ------- @@ -146,10 +163,18 @@ class Utils(): """ stdout = stderr = None retcode = None - popen = None + popen = env = None + if varsDict: + if shell: + # build map as array of vars and command line array: + realCmd = Utils.buildShellCmd(realCmd, varsDict) + else: # pragma: no cover - currently unused + env = _merge_dicts(os.environ, varsDict) + realCmdId = id(realCmd) + outCmd = lambda level: logSys.log(level, "%x -- exec: %s", realCmdId, realCmd) try: popen = subprocess.Popen( - realCmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell, + realCmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell, env=env, preexec_fn=os.setsid # so that killpg does not kill our process ) # wait with timeout for process has terminated: @@ -158,13 +183,15 @@ class Utils(): def _popen_wait_end(): retcode = popen.poll() return (True, retcode) if retcode is not None else None - retcode = Utils.wait_for(_popen_wait_end, timeout, Utils.DEFAULT_SHORT_INTERVAL) + # popen.poll is fast operation so we can put down the sleep interval: + retcode = Utils.wait_for(_popen_wait_end, timeout, Utils.DEFAULT_SHORT_INTERVAL / 100) if retcode: retcode = retcode[1] # if timeout: if retcode is None: - logSys.error("%s -- timed out after %s seconds." % - (realCmd, timeout)) + if outCmd: outCmd(logging.ERROR); outCmd = None + logSys.error("%x -- timed out after %s seconds." % + (realCmdId, timeout)) pgid = os.getpgid(popen.pid) # if not tree - first try to terminate and then kill, otherwise - kill (-9) only: os.killpg(pgid, signal.SIGTERM) # Terminate the process @@ -185,48 +212,49 @@ class Utils(): return False if not output else (False, stdout, stderr, retcode) std_level = logging.DEBUG if retcode in success_codes else logging.ERROR + if std_level > logSys.getEffectiveLevel(): + if outCmd: outCmd(std_level-1); outCmd = None # if we need output (to return or to log it): if output or std_level >= logSys.getEffectiveLevel(): + # if was timeouted (killed/terminated) - to prevent waiting, set std handles to non-blocking mode. if popen.stdout: try: if retcode is None or retcode < 0: Utils.setFBlockMode(popen.stdout, False) stdout = popen.stdout.read() - except IOError as e: + except IOError as e: # pragma: no cover logSys.error(" ... -- failed to read stdout %s", e) if stdout is not None and stdout != '' and std_level >= logSys.getEffectiveLevel(): - logSys.log(std_level, "%s -- stdout:", realCmd) for l in stdout.splitlines(): - logSys.log(std_level, " -- stdout: %r", uni_decode(l)) + logSys.log(std_level, "%x -- stdout: %r", realCmdId, uni_decode(l)) popen.stdout.close() if popen.stderr: try: if retcode is None or retcode < 0: Utils.setFBlockMode(popen.stderr, False) stderr = popen.stderr.read() - except IOError as e: + except IOError as e: # pragma: no cover logSys.error(" ... -- failed to read stderr %s", e) if stderr is not None and stderr != '' and std_level >= logSys.getEffectiveLevel(): - logSys.log(std_level, "%s -- stderr:", realCmd) for l in stderr.splitlines(): - logSys.log(std_level, " -- stderr: %r", uni_decode(l)) + logSys.log(std_level, "%x -- stderr: %r", realCmdId, uni_decode(l)) popen.stderr.close() success = False if retcode in success_codes: - logSys.debug("%-.40s -- returned successfully %i", realCmd, retcode) + logSys.debug("%x -- returned successfully %i", realCmdId, retcode) success = True elif retcode is None: - logSys.error("%-.40s -- unable to kill PID %i", realCmd, popen.pid) + logSys.error("%x -- unable to kill PID %i", realCmdId, popen.pid) elif retcode < 0 or retcode > 128: # dash would return negative while bash 128 + n sigcode = -retcode if retcode < 0 else retcode - 128 - logSys.error("%-.40s -- killed with %s (return code: %s)", - realCmd, signame.get(sigcode, "signal %i" % sigcode), retcode) + logSys.error("%x -- killed with %s (return code: %s)", + realCmdId, signame.get(sigcode, "signal %i" % sigcode), retcode) else: msg = _RETCODE_HINTS.get(retcode, None) - logSys.error("%-.40s -- returned %i", realCmd, retcode) + logSys.error("%x -- returned %i", realCmdId, retcode) if msg: logSys.info("HINT on %i: %s", retcode, msg % locals()) if output: diff --git a/fail2ban/tests/actiontestcase.py b/fail2ban/tests/actiontestcase.py index 70562baf..dc847290 100644 --- a/fail2ban/tests/actiontestcase.py +++ b/fail2ban/tests/actiontestcase.py @@ -389,6 +389,23 @@ class CommandActionTest(LogCaptureTestCase): self.assertLogged('Nothing to do') self.pruneLog() + def testExecuteWithVars(self): + self.assertTrue(self.__action.executeCmd( + r'''printf %b "foreign input:\n''' + r''' -- $f2bV_A --\n''' + r''' -- $f2bV_B --\n''' + r''' -- $(echo $f2bV_C) --''' # echo just replaces \n to test it as single line + r'''"''', + varsDict={ + 'f2bV_A': 'I\'m a hacker; && $(echo $f2bV_B)', + 'f2bV_B': 'I"m very bad hacker', + 'f2bV_C': '`Very | very\n$(bad & worst hacker)`' + })) + self.assertLogged(r"""foreign input:""", + ' -- I\'m a hacker; && $(echo $f2bV_B) --', + ' -- I"m very bad hacker --', + ' -- `Very | very $(bad & worst hacker)` --', all=True) + def testExecuteIncorrectCmd(self): CommandAction.executeCmd('/bin/ls >/dev/null\nbogusXXX now 2>/dev/null') self.assertLogged('HINT on 127: "Command not found"') @@ -400,8 +417,9 @@ class CommandActionTest(LogCaptureTestCase): self.assertFalse(CommandAction.executeCmd('sleep 30', timeout=timeout)) # give a test still 1 second, because system could be too busy self.assertTrue(time.time() >= stime + timeout and time.time() <= stime + timeout + 1) - self.assertLogged('sleep 30 -- timed out after') - self.assertLogged('sleep 30 -- killed with SIGTERM') + self.assertLogged('sleep 30', ' -- timed out after', all=True) + self.assertLogged(' -- killed with SIGTERM', + ' -- killed with SIGKILL') def testExecuteTimeoutWithNastyChildren(self): # temporary file for a nasty kid shell script @@ -457,9 +475,9 @@ class CommandActionTest(LogCaptureTestCase): # Verify that the process itself got killed self.assertTrue(Utils.wait_for(lambda: not pid_exists(cpid), 3)) self.assertLogged('my pid ', 'Resource temporarily unavailable') - self.assertLogged('timed out') - self.assertLogged('killed with SIGTERM', - 'killed with SIGKILL') + self.assertLogged(' -- timed out') + self.assertLogged(' -- killed with SIGTERM', + ' -- killed with SIGKILL') os.unlink(tmpFilename) os.unlink(tmpFilename + '.pid')