[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

@@ -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 <STDIN>.
 					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:

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:

fail2ban/tests/actiontestcase.py

@@ -157,6 +157,20 @@ class CommandActionTest(LogCaptureTestCase):
 		self.assertEqual(substituteRecursiveTags({'A': 'A <IP<PREF>HOST> B IP<PREF> 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: '<A><A>',
+			C=lambda self,i=0: 5 // int(self['A']) # raise error by access
+		)
+		# should raise no exceptions:
+		self.assertEqual(self.__action.replaceTag('test=<A>', 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=<A>--<B>--<A>', cm), "test=0--<A><A>--0")
+		# should raise an exception:
+		self.assertRaises(ZeroDivisionError, lambda: self.__action.replaceTag('test=<C>', cm))
+
 	def testReplaceTag(self):
 		aInfo = {
 			'HOST': "192.0.2.0",