From 39667ff6f7f8a5803368c4b75ab2a11c8c8e81d3 Mon Sep 17 00:00:00 2001 From: Nicolas Collignon Date: Tue, 2 Apr 2013 19:11:59 +0200 Subject: [PATCH] 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")