From 4f1473724bdb074f7f271a0dcc18ff8a928aaf2f Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 15 Mar 2017 20:58:01 +0100 Subject: [PATCH] fixed grave vulnerability by wrong escape of tags by executing of shell actions --- fail2ban/server/action.py | 44 +++++++++++++++++++++++++++++--- fail2ban/server/utils.py | 5 ++-- fail2ban/tests/actiontestcase.py | 30 +++++++++++++++++++++- fail2ban/tests/servertestcase.py | 6 ++--- 4 files changed, 75 insertions(+), 10 deletions(-) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index bdb25d27..1c5eb0c9 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -539,6 +539,9 @@ class CommandAction(ActionBase): # return value + ESCAPE_CRE = re.compile(r"""[\\#&;`|*?~<>\^\(\)\[\]{}$'"\n\r]""") + ESCAPE_VN_CRE = re.compile(r"\W") + @classmethod def replaceDynamicTags(cls, realCmd, aInfo): """Replaces dynamical tags in `query` with property values. @@ -546,7 +549,7 @@ class CommandAction(ActionBase): **Important** ------------- Because this tags are dynamic resp. foreign (user) input: - - values should be escaped + - values should be escaped (using "escape" as shell variable) - no recursive substitution (no interpolation for >) - don't use cache @@ -562,19 +565,52 @@ class CommandAction(ActionBase): str shell script as string or array with tags replaced (direct or as variables). """ - realCmd = cls.replaceTag(realCmd, aInfo, conditional=False) + # array for escaped vars: + varsDict = dict() + + def escapeVal(tag, value): + # if the value should be escaped: + if cls.ESCAPE_CRE.search(value): + # That one needs to be escaped since its content is + # out of our control + tag = 'f2bV_%s' % cls.ESCAPE_VN_CRE.sub('_', tag) + varsDict[tag] = value # add variable + value = '$'+tag # replacement as variable + # replacement for tag: + return value + + # substitution callable, used by interpolation of each tag + def substVal(m): + tag = m.group(1) # tagname from match + try: + value = aInfo[tag] + except KeyError: + # fallback (no or default replacement) + return ADD_REPL_TAGS.get(tag, m.group()) + value = str(value) # assure string + # replacement for tag: + return escapeVal(tag, value) + + # Replace normally properties of aInfo non-recursive: + realCmd = TAG_CRE.sub(substVal, realCmd) + # 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]) + tag = mapTag2Opt(m.groups()[0]) try: - return str(tickData[tn]) + value = str(tickData[tag]) except KeyError: return "" + return escapeVal("F_"+tag, value) realCmd = FCUSTAG_CRE.sub(substTag, realCmd) + + # build command corresponding "escaped" variables: + if varsDict: + realCmd = Utils.buildShellCmd(realCmd, varsDict) return realCmd def _processCmd(self, cmd, aInfo=None, conditional=''): diff --git a/fail2ban/server/utils.py b/fail2ban/server/utils.py index 56428294..a11759b1 100644 --- a/fail2ban/server/utils.py +++ b/fail2ban/server/utils.py @@ -60,6 +60,7 @@ class Utils(): DEFAULT_SLEEP_TIME = 2 DEFAULT_SLEEP_INTERVAL = 0.2 DEFAULT_SHORT_INTERVAL = 0.001 + DEFAULT_SHORTEST_INTERVAL = DEFAULT_SHORT_INTERVAL / 100 class Cache(object): @@ -183,8 +184,8 @@ class Utils(): def _popen_wait_end(): retcode = popen.poll() return (True, retcode) if retcode is not None else None - # 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) + # popen.poll is fast operation so we can use the shortest sleep interval: + retcode = Utils.wait_for(_popen_wait_end, timeout, Utils.DEFAULT_SHORTEST_INTERVAL) if retcode: retcode = retcode[1] # if timeout: diff --git a/fail2ban/tests/actiontestcase.py b/fail2ban/tests/actiontestcase.py index dc847290..cbd0aaca 100644 --- a/fail2ban/tests/actiontestcase.py +++ b/fail2ban/tests/actiontestcase.py @@ -394,7 +394,7 @@ class CommandActionTest(LogCaptureTestCase): 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''' -- $(echo -n $f2bV_C) --''' # echo just replaces \n to test it as single line r'''"''', varsDict={ 'f2bV_A': 'I\'m a hacker; && $(echo $f2bV_B)', @@ -406,6 +406,34 @@ class CommandActionTest(LogCaptureTestCase): ' -- I"m very bad hacker --', ' -- `Very | very $(bad & worst hacker)` --', all=True) + def testExecuteReplaceEscapeWithVars(self): + self.__action.actionban = 'echo "** ban , reason: ...\\n"' + self.__action.actionunban = 'echo "** unban "' + self.__action.actionstop = 'echo "** stop monitoring"' + matches = [ + '', + '" Hooray! #', + '`I\'m cool script kiddy', + '`I`m very cool > /here-is-the-path/to/bin/.x-attempt.sh', + '', + ] + aInfo = { + 'ip': '192.0.2.1', + 'reason': 'hacking attempt ( he thought he knows how f2b internally works ;)', + 'matches': '\n'.join(matches) + } + self.pruneLog() + self.__action.ban(aInfo) + self.assertLogged( + '** ban %s' % aInfo['ip'], aInfo['reason'], *matches, all=True) + self.assertNotLogged( + '** unban %s' % aInfo['ip'], '** stop monitoring', all=True) + self.pruneLog() + self.__action.unban(aInfo) + self.__action.stop() + self.assertLogged( + '** unban %s' % aInfo['ip'], '** stop monitoring', all=True) + def testExecuteIncorrectCmd(self): CommandAction.executeCmd('/bin/ls >/dev/null\nbogusXXX now 2>/dev/null') self.assertLogged('HINT on 127: "Command not found"') diff --git a/fail2ban/tests/servertestcase.py b/fail2ban/tests/servertestcase.py index 7a79b6a5..8d1cfa62 100644 --- a/fail2ban/tests/servertestcase.py +++ b/fail2ban/tests/servertestcase.py @@ -1679,7 +1679,7 @@ class ServerConfigReaderTests(LogCaptureTestCase): # complain -- ('j-complain-abuse', 'complain[' - 'name=%(__name__)s, grepopts="-m 1", grepmax=2, mailcmd="mail -s Hostname: - ",' + + 'name=%(__name__)s, grepopts="-m 1", grepmax=2, mailcmd="mail -s \'Hostname: , family: \' - ",' + # test reverse ip: 'debug=1,' + # 2 logs to test grep from multiple logs: @@ -1694,14 +1694,14 @@ class ServerConfigReaderTests(LogCaptureTestCase): 'testcase01.log:Dec 31 11:59:59 [sshd] error: PAM: Authentication failure for kevin from 87.142.124.10', 'testcase01a.log:Dec 31 11:55:01 [sshd] error: PAM: Authentication failure for test from 87.142.124.10', # both abuse mails should be separated with space: - 'mail -s Hostname: test-host - Abuse from 87.142.124.10 abuse-1@abuse-test-server abuse-2@abuse-test-server', + 'mail -s Hostname: test-host, family: inet4 - Abuse from 87.142.124.10 abuse-1@abuse-test-server abuse-2@abuse-test-server', ), 'ip6-ban': ( # test reverse ip: 'try to resolve 1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.abuse-contacts.abusix.org', 'Lines containing failures of 2001:db8::1 (max 2)', # both abuse mails should be separated with space: - 'mail -s Hostname: test-host - Abuse from 2001:db8::1 abuse-1@abuse-test-server abuse-2@abuse-test-server', + 'mail -s Hostname: test-host, family: inet6 - Abuse from 2001:db8::1 abuse-1@abuse-test-server abuse-2@abuse-test-server', ), }), )