From 8c38907016ceb9d0a366ac9fa026e3f3ffbf6b44 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Fri, 13 Jul 2012 20:19:30 +0100 Subject: [PATCH 001/544] Misconfigured DNS should not ban *successful* ssh logins Noticed while looking at the source (to see the point of ssh-ddos). POSSIBLE BREAK-IN ATTEMPT - sounds scary? But keep reading the message. It's not a login failure. It's a warning about reverse-DNS. The login can still succeed, and if it _does_ fail, that will be logged as normal. Jul 9 05:43:00 brick sshd[18971]: Address 200.41.233.234 maps to host234.advance.com. ar, but this does not map back to the address - POSSIBLE BREAK-IN ATTEMPT! Jul 9 05:43:00 brick sshd[18971]: Invalid user html from 200.41.233.234 The problem (in my mind) is that some users are stuck with bad dns. The warning won't stop them from logging in. I'm pretty sure they can't even see it. But when they exceed a threshold number of logins - which could be all successful logins - fail2ban will trigger. fail2ban shouldn't adding additional checks to successful logins - it goes against the name fail2ban :) - the first X "POSSIBLE BREAK-IN ATTEMPT"s would be permitted anyway - if you want to ban bad DNS, the right way is PARANOID in /etc/hosts.deny I've checked the source of OpenSSH, and this will only affect the reverse-DNS error. (I won't be offended if you want to check for yourself though ;) $ grep -r -h -C1 'ATTEMPT' openssh-5.5p1/ logit("reverse mapping checking getaddrinfo for %.700s " "[%s] failed - POSSIBLE BREAK-IN ATTEMPT!", name, ntop); return xstrdup(ntop); -- logit("Address %.100s maps to %.600s, but this does not " "map back to the address - POSSIBLE BREAK-IN ATTEMPT!", ntop, name); $ --- config/filter.d/sshd.conf | 1 - 1 file changed, 1 deletion(-) diff --git a/config/filter.d/sshd.conf b/config/filter.d/sshd.conf index e838cecc..4a3db7cc 100644 --- a/config/filter.d/sshd.conf +++ b/config/filter.d/sshd.conf @@ -32,7 +32,6 @@ failregex = ^%(__prefix_line)s(?:error: PAM: )?Authentication failure for .* fro ^%(__prefix_line)sUser .+ from not allowed because listed in DenyUsers\s*$ ^%(__prefix_line)sauthentication failure; logname=\S* uid=\S* euid=\S* tty=\S* ruser=\S* rhost=(?:\s+user=.*)?\s*$ ^%(__prefix_line)srefused connect from \S+ \(\)\s*$ - ^%(__prefix_line)sAddress .* POSSIBLE BREAK-IN ATTEMPT!*\s*$ ^%(__prefix_line)sUser .+ from not allowed because none of user's groups are listed in AllowGroups\s*$ # Option: ignoreregex From 9510619b7b89f9adb3ad44f3f5377d3abd43bcbf Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Thu, 1 Nov 2012 15:24:32 -0400 Subject: [PATCH 002/544] ENH: minor -- print out why skipping a backend while testing --- fail2ban-testcases | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fail2ban-testcases b/fail2ban-testcases index 0160781d..ea95e9c5 100755 --- a/fail2ban-testcases +++ b/fail2ban-testcases @@ -142,14 +142,14 @@ filters = [FilterPoll] # always available try: from server.filtergamin import FilterGamin filters.append(FilterGamin) -except: - pass +except Exception, e: + print "I: Skipping gamin backend testing. Got exception '%s'" % e try: from server.filterpyinotify import FilterPyinotify filters.append(FilterPyinotify) -except: - pass +except Exception, e: + print "I: Skipping pyinotify backend testing. Got exception '%s'" % e for Filter_ in filters: tests.addTest(unittest.makeSuite( From 5becaf8ef2b8cd37d65de8079ce38d5e2c05be6d Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Thu, 1 Nov 2012 15:34:20 -0400 Subject: [PATCH 003/544] BF: (python 2.[45]) store backends names in a list to use .index later on (Closes gh-83) .index() got into tuple's API only in 2.6 --- fail2ban-testcases | 1 + server/jail.py | 4 +++- testcases/filtertestcase.py | 8 ++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/fail2ban-testcases b/fail2ban-testcases index ea95e9c5..cc616c0e 100755 --- a/fail2ban-testcases +++ b/fail2ban-testcases @@ -125,6 +125,7 @@ tests.addTest(unittest.makeSuite(filtertestcase.LogFile)) tests.addTest(unittest.makeSuite(filtertestcase.LogFileMonitor)) tests.addTest(unittest.makeSuite(filtertestcase.GetFailures)) tests.addTest(unittest.makeSuite(filtertestcase.DNSUtilsTests)) +tests.addTest(unittest.makeSuite(filtertestcase.JailTests)) # DateDetector tests.addTest(unittest.makeSuite(datedetectortestcase.DateDetectorTest)) diff --git a/server/jail.py b/server/jail.py index 4ab83db4..dee64e7f 100644 --- a/server/jail.py +++ b/server/jail.py @@ -33,7 +33,9 @@ logSys = logging.getLogger("fail2ban.jail") class Jail: #Known backends. Each backend should have corresponding __initBackend method - _BACKENDS = ('pyinotify', 'gamin', 'polling') + # yoh: stored in a list instead of a tuple since only + # list had .index until 2.6 + _BACKENDS = ['pyinotify', 'gamin', 'polling'] def __init__(self, name, backend = "auto"): self.__name = name diff --git a/testcases/filtertestcase.py b/testcases/filtertestcase.py index eade537f..c10fa78d 100644 --- a/testcases/filtertestcase.py +++ b/testcases/filtertestcase.py @@ -28,6 +28,7 @@ import sys import time import tempfile +from server.jail import Jail from server.filterpoll import FilterPoll from server.filter import FileFilter, DNSUtils from server.failmanager import FailManager @@ -626,3 +627,10 @@ class DNSUtilsTests(unittest.TestCase): self.assertEqual(res, ['192.0.43.10']) else: self.assertEqual(res, []) + +class JailTests(unittest.TestCase): + + def testSetBackend_gh83(self): + # smoke test + jail = Jail('test', backend='polling') # Must not fail to initiate + From 09355663f7a3c0409e08efdebf98b1bbf47d1d9c Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 5 Nov 2012 16:54:19 -0500 Subject: [PATCH 004/544] BF: (python 2.4) -- access to staticmethod should go via Class TODO: get away from using all those staticmethods in f2b --- server/action.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/action.py b/server/action.py index 387c115c..bd750337 100644 --- a/server/action.py +++ b/server/action.py @@ -255,7 +255,7 @@ class Action: if tag == 'matches': # That one needs to be escaped since its content is # out of our control - value = escapeTag(value) + value = Action.escapeTag(value) string = string.replace('<' + tag + '>', value) # New line string = string.replace("
", '\n') From 8e64c281dd178e29a6ef50928f4552757043e05b Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 5 Nov 2012 20:09:15 -0500 Subject: [PATCH 005/544] BF: in code we should use MyTime wrapper instead of time module directly to allow for some tests to work correctly --- server/filter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/filter.py b/server/filter.py index d69f02aa..f71a0a06 100644 --- a/server/filter.py +++ b/server/filter.py @@ -220,7 +220,7 @@ class Filter(JailThread): # to enable banip fail2ban-client BAN command def addBannedIP(self, ip): - unixTime = time.time() + unixTime = MyTime.time() for i in xrange(self.failManager.getMaxRetry()): self.failManager.addFailure(FailTicket(ip, unixTime)) From 6288ec27577cb5ddeab9151b5a72c3803cf1041c Mon Sep 17 00:00:00 2001 From: David Engeset Date: Mon, 5 Nov 2012 15:07:33 -0800 Subject: [PATCH 006/544] Added in command option to unban and IP, just like using 'banip'. Command looks like: fail2ban-client set unbanip --- server/actions.py | 13 +++++++++++++ server/banmanager.py | 26 +++++++++++++++++++++++++- server/server.py | 3 +++ server/transmitter.py | 3 +++ 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/server/actions.py b/server/actions.py index f936de18..79783395 100644 --- a/server/actions.py +++ b/server/actions.py @@ -120,6 +120,19 @@ class Actions(JailThread): def getBanTime(self): return self.__banManager.getBanTime() + ## + # Remove a banned IP now, rather than waiting for it to expire, even if set to never expire. + # + # @return the IP string or 'None' if not unbanned. + def removeBannedIP(self, ip): + # Find the ticket with the IP. + ticket = self.__banManager.getTicketByIP(ip) + if ticket != False: + # Unban the IP. + self.__unBan(ticket) + return ip + return 'None' + ## # Main loop. # diff --git a/server/banmanager.py b/server/banmanager.py index 214916b7..e9c81214 100644 --- a/server/banmanager.py +++ b/server/banmanager.py @@ -208,7 +208,7 @@ class BanManager: return unBanList finally: self.__lock.release() - + ## # Flush the ban list. # @@ -223,3 +223,27 @@ class BanManager: return uBList finally: self.__lock.release() + + ## + # Gets the ticket for the specified IP. + # + # @return the ticket for the IP or False. + def getTicketByIP(self, ip): + try: + ipticket = False + self.__lock.acquire() + + # Find the ticket the IP goes with. + for ticket in self.__banList: + if ticket.getIP() == ip: + ipticket = ticket + break + + unBanList = [ipticket] + # Remove the ticket from the ban list. + self.__banList = [ticket for ticket in self.__banList + if ticket not in unBanList] + + return ipticket + finally: + self.__lock.release() diff --git a/server/server.py b/server/server.py index b734f82a..d9532be2 100644 --- a/server/server.py +++ b/server/server.py @@ -241,6 +241,9 @@ class Server: def setBanIP(self, name, value): return self.__jails.getFilter(name).addBannedIP(value) + def setUnbanIP(self, name, value): + return self.__jails.getAction(name).removeBannedIP(value) + def getBanTime(self, name): return self.__jails.getAction(name).getBanTime() diff --git a/server/transmitter.py b/server/transmitter.py index a618a1a1..23b609a1 100644 --- a/server/transmitter.py +++ b/server/transmitter.py @@ -175,6 +175,9 @@ class Transmitter: elif command[1] == "banip": value = command[2] return self.__server.setBanIP(name,value) + elif command[1] == "unbanip": + value = command[2] + return self.__server.setUnbanIP(name,value) elif command[1] == "addaction": value = command[2] self.__server.addAction(name, value) From f14c7ae401376ad984dde151cf7b3fd6cf5081a1 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 5 Nov 2012 20:37:06 -0500 Subject: [PATCH 007/544] ENH: refactored previous commit to make it more Pythonic (With prev commit closes gh-86, gh-81) --- server/actions.py | 2 +- server/banmanager.py | 18 ++++++------------ 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/server/actions.py b/server/actions.py index 79783395..3461e672 100644 --- a/server/actions.py +++ b/server/actions.py @@ -127,7 +127,7 @@ class Actions(JailThread): def removeBannedIP(self, ip): # Find the ticket with the IP. ticket = self.__banManager.getTicketByIP(ip) - if ticket != False: + if ticket is not None: # Unban the IP. self.__unBan(ticket) return ip diff --git a/server/banmanager.py b/server/banmanager.py index e9c81214..596dc4e9 100644 --- a/server/banmanager.py +++ b/server/banmanager.py @@ -230,20 +230,14 @@ class BanManager: # @return the ticket for the IP or False. def getTicketByIP(self, ip): try: - ipticket = False self.__lock.acquire() - # Find the ticket the IP goes with. - for ticket in self.__banList: + # Find the ticket the IP goes with and return it + for i, ticket in enumerate(self.__banList): if ticket.getIP() == ip: - ipticket = ticket - break - - unBanList = [ipticket] - # Remove the ticket from the ban list. - self.__banList = [ticket for ticket in self.__banList - if ticket not in unBanList] - - return ipticket + # Return the ticket after removing (popping) + # if from the ban list. + return self.__banList.pop(i) finally: self.__lock.release() + return None # if none found From 2d672d1c81ae5bdeaf9e5a03f8574f9b397925c7 Mon Sep 17 00:00:00 2001 From: David Engeset Date: Mon, 5 Nov 2012 14:02:11 -0800 Subject: [PATCH 008/544] Added in while loop to process the Fail Manager after the requested banned IP was added to its queue. This solves the issue of needing to touch the log file that is being monitored to get the IP to be banned accordingly. Added in import of FailManagerEmpty exception class. --- server/filter.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server/filter.py b/server/filter.py index f71a0a06..b37e37e6 100644 --- a/server/filter.py +++ b/server/filter.py @@ -27,6 +27,7 @@ __date__ = "$Date$" __copyright__ = "Copyright (c) 2004 Cyril Jaquier" __license__ = "GPL" +from failmanager import FailManagerEmpty from failmanager import FailManager from ticket import FailTicket from jailthread import JailThread @@ -224,6 +225,14 @@ class Filter(JailThread): for i in xrange(self.failManager.getMaxRetry()): self.failManager.addFailure(FailTicket(ip, unixTime)) + # Perform the banning of the IP now. + try: + while True: + ticket = self.failManager.toBan() + self.jail.putFailTicket(ticket) + except FailManagerEmpty: + self.failManager.cleanup(MyTime.time()) + return ip ## From b773ed617bf73f01ec4757d37caf63df30760b3c Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 5 Nov 2012 21:12:03 -0500 Subject: [PATCH 009/544] DOC: minor "fixes" in DEVELOP --- DEVELOP | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/DEVELOP b/DEVELOP index 025b58b4..7a6a7b9b 100644 --- a/DEVELOP +++ b/DEVELOP @@ -58,18 +58,20 @@ one):: RF-Note just a note which might be useful to address while doing RF JailThread -> Filter -> FileFilter -> {FilterPoll, FilterPyinotify, ...} - | | * FileContainer - | + FailManager - | + DateDetector - \- -> Actions - * Actions - + BanManager - + | * FileContainer + + FailManager + + DateDetector + + Jail (provided in __init__) which contains this Filter + (used for passing tickets from FailManager to Jail's __queue) Server + Jails * Jail - + Filter + + Filter (in __filter) * tickets (in __queue) + + Actions (in __action) + * Action + + BanManager + failmanager.py ~~~~~~~~~~~~~~ @@ -147,7 +149,7 @@ one way or another provide except FailManagerEmpty: self.failManager.cleanup(MyTime.time()) -thus channeling "ban tickets" from their failManager to a +thus channeling "ban tickets" from their failManager to the corresponding jail. action.py From 1e12c220e6828f9dd8d4530eb9a3abe843d7155e Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 5 Nov 2012 21:22:33 -0500 Subject: [PATCH 010/544] minor: added a note on now "negative" log entries on "POSSIBLE BREAK-IN ATTEMPT" --- testcases/files/logs/sshd | 1 + 1 file changed, 1 insertion(+) diff --git a/testcases/files/logs/sshd b/testcases/files/logs/sshd index 639aaf0b..8e6c1273 100644 --- a/testcases/files/logs/sshd +++ b/testcases/files/logs/sshd @@ -22,6 +22,7 @@ Feb 25 14:34:11 belka sshd[31607]: User root from ferrari.inescn.pt not allowed Nov 11 23:33:27 Server sshd[5174]: refused connect from _U2FsdGVkX19P3BCJmFBHhjLza8BcMH06WCUVwttMHpE=_@::ffff:218.249.210.161 (::ffff:218.249.210.161) #7 added exclamation mark to BREAK-IN +# Now should be a negative since we decided not to catch those Oct 15 19:51:35 server sshd[7592]: Address 1.2.3.4 maps to 1234.bbbbbb.com, but this does not map back to the address - POSSIBLE BREAK-IN ATTEMPT Oct 15 19:51:35 server sshd[7592]: Address 1.2.3.4 maps to 1234.bbbbbb.com, but this does not map back to the address - POSSIBLE BREAK-IN ATTEMPT! From f52ba9923a1ad44274792f08e9d4ad0ce8dd4c3e Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 5 Nov 2012 21:30:07 -0500 Subject: [PATCH 011/544] ENH: downgrade "already banned" from WARN to INFO level (Closes gh-79) Most of the time it is a benign latency effect so nothing to warn about. --- server/actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/actions.py b/server/actions.py index 3461e672..94344dce 100644 --- a/server/actions.py +++ b/server/actions.py @@ -181,7 +181,7 @@ class Actions(JailThread): action.execActionBan(aInfo) return True else: - logSys.warn("[%s] %s already banned" % (self.jail.getName(), + logSys.info("[%s] %s already banned" % (self.jail.getName(), str(aInfo["ip"]))) return False From 63237a785ec6bfa00f5e3ec0117c86297bc5b4b4 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 5 Nov 2012 21:50:49 -0500 Subject: [PATCH 012/544] DOC: forgotten --help entry for " unban " --- common/protocol.py | 1 + 1 file changed, 1 insertion(+) diff --git a/common/protocol.py b/common/protocol.py index db2b07eb..2f8ffa6c 100644 --- a/common/protocol.py +++ b/common/protocol.py @@ -64,6 +64,7 @@ protocol = [ ["set bantime