From 294f073741dbfa49f43a9114322bb113f433eb47 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 17 Feb 2013 22:42:24 +0000 Subject: [PATCH 1/9] Typo in default pidfile in fail2ban.conf --- config/fail2ban.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/fail2ban.conf b/config/fail2ban.conf index e759513b..1888eddb 100644 --- a/config/fail2ban.conf +++ b/config/fail2ban.conf @@ -43,7 +43,7 @@ socket = /var/run/fail2ban/fail2ban.sock # Option: pidfile # Notes.: Set the PID file. This is used to store the process ID of the # fail2ban server. -# Values: FILE Default: /var/run/fail2ban/fail2ban.sock +# Values: FILE Default: /var/run/fail2ban/fail2ban.pid # pidfile = /var/run/fail2ban/fail2ban.pid From 39667ff6f7f8a5803368c4b75ab2a11c8c8e81d3 Mon Sep 17 00:00:00 2001 From: Nicolas Collignon Date: Tue, 2 Apr 2013 19:11:59 +0200 Subject: [PATCH 2/9] FD_CLOEXEC support * 001-fail2ban-server-socket-close-on-exec-no-leak.diff Add code that marks server and client sockets with FD_CLOEXEC flags. Avoid leaking file descriptors to processes spawned when handling fail2ban actions (ex: iptables). Unix sockets managed by fail2ban-server don't need to be passed to any child process. Fail2ban already uses the FD_CLOEXEC flags in the filter code. This patch also avoids giving iptables access to fail2ban UNIX socket in a SELinux environment (A sane SELinux policy should trigger an audit event because "iptables" will be given read/write access to the fail2ban control socket). Some random references related to this bug: http://sourceforge.net/tracker/?func=detail&atid=689044&aid=2086568&group_id=121032 http://www.redhat.com/archives/fedora-selinux-list/2009-June/msg00124.html http://forums.fedoraforum.org/showthread.php?t=234230 * 002-fail2ban-filters-close-on-exec-typo-fix.diff There is a typo in the fail2ban server/filter.py source code. The FD_CLOEXEC is correctly set but additional *random* flags are also set. It has no side-effect as long as the fd doesn't match a valid flag :) "fcntl.fcntl(fd, fcntl.F_SETFD, fd | fcntl.FD_CLOEXEC)" <== the 3rd parameter should be flags, not a file descriptor. * 003-fail2ban-gamin-socket-close-on-exec-no-leak.diff Add code that marks the Gamin monitor file descriptor with FD_CLOEXEC flags. Avoid leaking file descriptors to processes spawned when handling fail2ban actions (ex: iptables). --- File descriptors in action process before patches: dr-x------ 2 root root 0 . dr-xr-xr-x 8 root root 0 .. lr-x------ 1 root root 64 0 -> /dev/null <== OK l-wx------ 1 root root 64 1 -> /tmp/test.log <== used by test action lrwx------ 1 root root 64 2 -> /dev/null <== OK lrwx------ 1 root root 64 3 -> socket:[116361] <== NOK (fail2ban.sock leak) lr-x------ 1 root root 64 4 -> /proc/20090/fd <== used by test action l-wx------ 1 root root 64 5 -> /var/log/fail2ban.log <== OK lrwx------ 1 root root 64 6 -> socket:[115608] <== NOK (gamin sock leak) File descriptors in action process after patches: dr-x------ 2 root root 0 . dr-xr-xr-x 8 root root 0 .. lr-x------ 1 root root 64 0 -> /dev/null <== OK l-wx------ 1 root root 64 1 -> /tmp/test.log <== used by test action lrwx------ 1 root root 64 2 -> /dev/null <== OK lr-x------ 1 root root 64 3 -> /proc/18284/fd <== used by test action l-wx------ 1 root root 64 5 -> /var/log/fail2ban.log <== OK --- server/asyncserver.py | 16 +++++++++++++++- server/filter.py | 3 ++- server/filtergamin.py | 5 ++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/server/asyncserver.py b/server/asyncserver.py index 66b2b53f..35167a6b 100644 --- a/server/asyncserver.py +++ b/server/asyncserver.py @@ -29,7 +29,7 @@ __license__ = "GPL" from pickle import dumps, loads, HIGHEST_PROTOCOL from common import helpers -import asyncore, asynchat, socket, os, logging, sys, traceback +import asyncore, asynchat, socket, os, logging, sys, traceback, fcntl # Gets the instance of the logger. logSys = logging.getLogger("fail2ban.server") @@ -107,6 +107,7 @@ class AsyncServer(asyncore.dispatcher): except TypeError: logSys.warning("Type error") return + AsyncServer.__markCloseOnExec(conn) # Creates an instance of the handler class to handle the # request/response on the incoming connection. RequestHandler(conn, self.__transmitter) @@ -134,6 +135,7 @@ class AsyncServer(asyncore.dispatcher): self.bind(sock) except Exception: raise AsyncServerException("Unable to bind socket %s" % self.__sock) + AsyncServer.__markCloseOnExec(self.socket) self.listen(1) # Sets the init flag. self.__init = True @@ -159,6 +161,18 @@ class AsyncServer(asyncore.dispatcher): os.remove(self.__sock) logSys.debug("Socket shutdown") + ## + # Marks socket as close-on-exec to avoid leaking file descriptors when + # running actions involving command execution. + + # @param sock: socket file. + + #@staticmethod + def __markCloseOnExec(sock): + fd = sock.fileno() + flags = fcntl.fcntl(fd, fcntl.F_GETFD) + fcntl.fcntl(fd, fcntl.F_SETFD, flags|fcntl.FD_CLOEXEC) + __markCloseOnExec = staticmethod(__markCloseOnExec) ## # AsyncServerException is used to wrap communication exceptions. diff --git a/server/filter.py b/server/filter.py index f842c269..88a5d861 100644 --- a/server/filter.py +++ b/server/filter.py @@ -554,7 +554,8 @@ class FileContainer: self.__handler = open(self.__filename) # Set the file descriptor to be FD_CLOEXEC fd = self.__handler.fileno() - fcntl.fcntl(fd, fcntl.F_SETFD, fd | fcntl.FD_CLOEXEC) + flags = fcntl.fcntl(fd, fcntl.F_GETFD) + fcntl.fcntl(fd, fcntl.F_SETFD, flags | fcntl.FD_CLOEXEC) firstLine = self.__handler.readline() # Computes the MD5 of the first line. myHash = md5sum(firstLine).digest() diff --git a/server/filtergamin.py b/server/filtergamin.py index cff5aa54..196396c5 100644 --- a/server/filtergamin.py +++ b/server/filtergamin.py @@ -27,7 +27,7 @@ from failmanager import FailManagerEmpty from filter import FileFilter from mytime import MyTime -import time, logging, gamin +import time, logging, gamin, fcntl # Gets the instance of the logger. logSys = logging.getLogger("fail2ban.filter") @@ -52,6 +52,9 @@ class FilterGamin(FileFilter): self.__modified = False # Gamin monitor self.monitor = gamin.WatchMonitor() + fd = self.monitor.get_fd() + flags = fcntl.fcntl(fd, fcntl.F_GETFD) + fcntl.fcntl(fd, fcntl.F_SETFD, flags|fcntl.FD_CLOEXEC) logSys.debug("Created FilterGamin") From ffe48741e333baac89c63d3e5944020baf16acff Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Sat, 13 Apr 2013 22:20:57 -0400 Subject: [PATCH 3/9] DOC: thanks @kwirk for spotting the typos in exception message --- server/transmitter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/transmitter.py b/server/transmitter.py index dc121f93..b54fbb93 100644 --- a/server/transmitter.py +++ b/server/transmitter.py @@ -123,7 +123,7 @@ class Transmitter: elif command[2] == "off": self.__server.setIdleJail(name, False) else: - raise Exception("Invalid idle option, must be 'yes' or 'no'") + raise Exception("Invalid idle option, must be 'on' or 'off'") return self.__server.getIdleJail(name) # Filter elif command[1] == "addignoreip": From 28e9acf86a954f87ab5a89e25ed805594b6c5cd4 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 14 Apr 2013 15:55:18 +0100 Subject: [PATCH 4/9] TST: no cover additions to server, primarily daemon creation --- server/server.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/server.py b/server/server.py index a0824f1d..5f1a52c1 100644 --- a/server/server.py +++ b/server/server.py @@ -66,7 +66,7 @@ class Server: # First set the mask to only allow access to owner os.umask(0077) - if self.__daemon: + if self.__daemon: # pragma: no cover logSys.info("Starting in daemon mode") ret = self.__createDaemon() if ret: @@ -379,7 +379,7 @@ class Server: try: handler.flush() handler.close() - except (ValueError, KeyError): + except (ValueError, KeyError): # pragma: no cover if sys.version_info >= (2,6): raise # is known to be thrown after logging was shutdown once @@ -404,7 +404,7 @@ class Server: finally: self.__loggingLock.release() - def __createDaemon(self): + def __createDaemon(self): # pragma: no cover """ Detach a process from the controlling terminal and run it in the background as a daemon. From d259e903a3cc9f284db389824c6446ada64dce93 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 14 Apr 2013 15:56:14 +0100 Subject: [PATCH 5/9] TST: Coverage for coveralls.io should only be run on success --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 8da3e0bb..2d091754 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,5 +14,5 @@ install: script: - if [[ $TRAVIS_PYTHON_VERSION == 2.7 ]]; then export PYTHONPATH="$PYTHONPATH:/usr/share/pyshared:/usr/lib/pyshared/python2.7"; fi - if [[ $TRAVIS_PYTHON_VERSION == 2.7 ]]; then coverage run --rcfile=.travis_coveragerc fail2ban-testcases; else python ./fail2ban-testcases; fi -after_script: +after_success: - if [[ $TRAVIS_PYTHON_VERSION == 2.7 ]]; then coveralls; fi From 3d6791fe3e20b9125b999ead87f6dbc91aa24486 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 14 Apr 2013 15:57:37 +0100 Subject: [PATCH 6/9] ENH: Minor change to action for consistency of execStart/Stop --- server/action.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/action.py b/server/action.py index 5fde3ae1..ce9651e8 100644 --- a/server/action.py +++ b/server/action.py @@ -297,10 +297,8 @@ class Action: if not Action.executeCmd(checkCmd): logSys.error("Invariant check failed. Trying to restore a sane" + " environment") - stopCmd = Action.replaceTag(self.__actionStop, self.__cInfo) - Action.executeCmd(stopCmd) - startCmd = Action.replaceTag(self.__actionStart, self.__cInfo) - Action.executeCmd(startCmd) + self.execActionStop() + self.execActionStart() if not Action.executeCmd(checkCmd): logSys.fatal("Unable to restore environment") return False From 4c4b60f4b4c5b959b36ca5fe5636bc60e0aa057c Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 14 Apr 2013 15:58:35 +0100 Subject: [PATCH 7/9] TST: Add tag replace and escape test for actions --- testcases/actiontestcase.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/testcases/actiontestcase.py b/testcases/actiontestcase.py index b8292c27..50ae2816 100644 --- a/testcases/actiontestcase.py +++ b/testcases/actiontestcase.py @@ -61,6 +61,23 @@ class ExecuteAction(unittest.TestCase): def _is_logged(self, s): return s in self._log.getvalue() + def testReplaceTag(self): + aInfo = { + 'HOST': "192.0.2.0", + 'ABC': "123", + 'xyz': "890", + } + self.assertEqual( + self.__action.replaceTag("Text text", aInfo), + "Text 192.0.2.0 text") + self.assertEqual( + self.__action.replaceTag("Text text ABC", aInfo), + "Text 890 text 123 ABC") + self.assertEqual( + self.__action.replaceTag("", + {'matches': "some >char< should \< be[ escap}ed&"}), + r"some \>char\< should \\\< be\[ escap\}ed\&") + def testExecuteActionBan(self): self.__action.setActionStart("touch /tmp/fail2ban.test") self.__action.setActionStop("rm -f /tmp/fail2ban.test") From 94956bee84a0ceb0895fe24a863b87fa1590fae6 Mon Sep 17 00:00:00 2001 From: Steven Hiscocks Date: Sun, 14 Apr 2013 15:59:05 +0100 Subject: [PATCH 8/9] TST: test all valid loglevels in server testcases --- testcases/servertestcase.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/testcases/servertestcase.py b/testcases/servertestcase.py index ffb057a9..1b6eb8d3 100644 --- a/testcases/servertestcase.py +++ b/testcases/servertestcase.py @@ -504,7 +504,9 @@ class TransmitterLogging(TransmitterBase): def testLogLevel(self): self.setGetTest("loglevel", "4", 4) + self.setGetTest("loglevel", "3", 3) self.setGetTest("loglevel", "2", 2) + self.setGetTest("loglevel", "1", 1) self.setGetTest("loglevel", "-1", -1) self.setGetTest("loglevel", "0", 0) self.setGetTestNOK("loglevel", "Bird") From b8e823bd4ef335833893869fa246ca124307acb4 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 16 Apr 2013 23:44:38 -0400 Subject: [PATCH 9/9] DOC: initiated changelog (but not juice left to actually fill it up ;-)) --- ChangeLog | 13 +++++++++++++ DEVELOP | 4 ++++ 2 files changed, 17 insertions(+) diff --git a/ChangeLog b/ChangeLog index eabd6e81..12678936 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,19 @@ Fail2Ban (version 0.8.8) 2012/12/06 ================================================================================ +ver. 0.8.9 (2013/04/XXX) - wanna-be-stable +---------- + +This release incorporates 144 (XXX) non-merge commits from 14 +contributors (sorted by number of commits): Yaroslav Halchenko, Daniel +Black, Steven Hiscocks, ArndRa, hamilton5, pigsyn, Erwan Ben Souiden, +Michael Gebetsroither, Orion Poplawski, Artur Penttinen, sebres, +Nicolas Collignon, Pascal Borreli, blotus: + +- Fixes: +- New features: +- Enhancements: + ver. 0.8.8 (2012/12/06) - stable ---------- - Fixes: diff --git a/DEVELOP b/DEVELOP index 623aee12..a31bf04a 100644 --- a/DEVELOP +++ b/DEVELOP @@ -253,6 +253,10 @@ Releasing # Add/finalize the corresponding entry in the ChangeLog + To generate a list of committers use e.g. + + git shortlog -sn 0.8.8.. | sed -e 's,^[ 0-9\t]*,,g' | tr '\n' '\|' | sed -e 's:|:, :g' + # Update man pages (cd man ; ./generate-man )