fixed grave vulnerability by wrong escape of tags by executing of shell actions

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 <a<b>>)
 		  - 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=''):

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:
+				# popen.poll is fast operation so we can use the shortest sleep interval:
-				retcode = Utils.wait_for(_popen_wait_end, timeout, Utils.DEFAULT_SHORT_INTERVAL / 100)
+				retcode = Utils.wait_for(_popen_wait_end, timeout, Utils.DEFAULT_SHORTEST_INTERVAL)
 				if retcode:
 					retcode = retcode[1]
 			# if timeout:

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 <ip>, reason: <reason> ...\\n<matches>"'
+		self.__action.actionunban = 'echo "** unban <ip>"'
+		self.__action.actionstop = 'echo "** stop monitoring"'
+		matches = [
+			'<actionunban>',
+			'" Hooray! #',
+			'`I\'m cool script kiddy',
+			'`I`m very cool > /here-is-the-path/to/bin/.x-attempt.sh',
+			'<actionstop>',
+		]
+		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"')

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: <ip-host> - ",' +
+					  'name=%(__name__)s, grepopts="-m 1", grepmax=2, mailcmd="mail -s \'Hostname: <ip-host>, family: <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',
 					),
 				}),
 			)