From 889386b4a6e5d1eda52fec3dc1aa6ce043d3b59b Mon Sep 17 00:00:00 2001 From: Michael Wyraz Date: Fri, 6 Dec 2019 09:35:21 +0100 Subject: [PATCH 1/6] Limiter implementation --- core/admin/mailu/internal/__init__.py | 11 +----- core/admin/mailu/internal/views/auth.py | 11 +++--- core/admin/mailu/limiter.py | 45 +++++++++++++++++++++++++ core/admin/mailu/utils.py | 9 ++--- core/admin/requirements.txt | 2 +- 5 files changed, 57 insertions(+), 21 deletions(-) create mode 100644 core/admin/mailu/limiter.py diff --git a/core/admin/mailu/internal/__init__.py b/core/admin/mailu/internal/__init__.py index cf0ea3f7..95f2f782 100644 --- a/core/admin/mailu/internal/__init__.py +++ b/core/admin/mailu/internal/__init__.py @@ -1,4 +1,4 @@ -from flask_limiter import RateLimitExceeded +from mailu.limiter import RateLimitExceeded from mailu import utils from flask import current_app as app @@ -20,13 +20,4 @@ def rate_limit_handler(e): return response -@utils.limiter.request_filter -def whitelist_webmail(): - try: - return flask.request.headers["Client-Ip"] ==\ - app.config["HOST_WEBMAIL"] - except: - return False - - from mailu.internal.views import * diff --git a/core/admin/mailu/internal/views/auth.py b/core/admin/mailu/internal/views/auth.py index 83a63953..af1c552f 100644 --- a/core/admin/mailu/internal/views/auth.py +++ b/core/admin/mailu/internal/views/auth.py @@ -7,18 +7,21 @@ import flask_login import base64 + @internal.route("/auth/email") -@utils.limiter.limit( - lambda: app.config["AUTH_RATELIMIT"], - lambda: flask.request.headers["Client-Ip"] -) def nginx_authentication(): """ Main authentication endpoint for Nginx email server """ + utils.limiter.check(flask.request.headers["Client-Ip"]) headers = nginx.handle_authentication(flask.request.headers) response = flask.Response() for key, value in headers.items(): response.headers[key] = str(value) + if ("Auth-Status" in headers) and (headers["Auth-Status"]=="OK"): + utils.limiter.reset(flask.request.headers["Client-Ip"]) + else: + utils.limiter.hit(flask.request.headers["Client-Ip"]) + return response diff --git a/core/admin/mailu/limiter.py b/core/admin/mailu/limiter.py new file mode 100644 index 00000000..4df78d27 --- /dev/null +++ b/core/admin/mailu/limiter.py @@ -0,0 +1,45 @@ +import limits +import limits.storage +import limits.strategies +import ipaddress + +class RateLimitExceeded(Exception): + pass + +class Limiter: + + def __init__(self): + self.storage = None + self.limiter = None + self.rate = None + self.subnet = None + + def init_app(self, app): + self.storage = limits.storage.storage_from_string(app.config["RATELIMIT_STORAGE_URL"]) + self.limiter = limits.strategies.MovingWindowRateLimiter(self.storage) + self.rate = limits.parse(app.config["AUTH_RATELIMIT"]) + self.subnet = ipaddress.ip_network(app.config["SUBNET"]) + + def check(self,clientip): + # TODO: activate this code if we have limits at webmail level + #if ipaddress.ip_address(clientip) in self.subnet: + # # no limits for internal requests (e.g. from webmail) + # return + if not self.limiter.test(self.rate,"client-ip",clientip): + raise RateLimitExceeded() + + def hit(self,clientip): + # TODO: activate this code if we have limits at webmail level + #if ipaddress.ip_address(clientip) in self.subnet: + # # no limits for internal requests (e.g. from webmail) + # return + if not self.limiter.hit(self.rate,"client-ip",clientip): + raise RateLimitExceeded() + + def reset(self,clientip): + # TODO: activate this code if we have limits at webmail level + #if ipaddress.ip_address(clientip) in self.subnet: + # # no limits for internal requests (e.g. from webmail) + # return + # limit reset is not supported by the rate limit library + pass diff --git a/core/admin/mailu/utils.py b/core/admin/mailu/utils.py index b11b1689..df23b8e7 100644 --- a/core/admin/mailu/utils.py +++ b/core/admin/mailu/utils.py @@ -1,11 +1,10 @@ -from mailu import models +from mailu import models, limiter import flask import flask_login import flask_script import flask_migrate import flask_babel -import flask_limiter from werkzeug.contrib import fixers @@ -20,10 +19,8 @@ def handle_needs_login(): flask.url_for('ui.login', next=flask.request.endpoint) ) - -# Request rate limitation -limiter = flask_limiter.Limiter(key_func=lambda: current_user.username) - +# Rate limiter +limiter = limiter.Limiter() # Application translation babel = flask_babel.Babel() diff --git a/core/admin/requirements.txt b/core/admin/requirements.txt index c68130db..bc6e772f 100644 --- a/core/admin/requirements.txt +++ b/core/admin/requirements.txt @@ -7,7 +7,7 @@ Flask-migrate Flask-script Flask-wtf Flask-debugtoolbar -Flask-limiter +limits redis WTForms-Components socrate From c079f2ac4abfae3a5840dce7e66e30047d32f21f Mon Sep 17 00:00:00 2001 From: Michael Wyraz Date: Fri, 6 Dec 2019 09:38:25 +0100 Subject: [PATCH 2/6] Changelog --- towncrier/newsfragments/1278.fix | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 towncrier/newsfragments/1278.fix diff --git a/towncrier/newsfragments/1278.fix b/towncrier/newsfragments/1278.fix new file mode 100644 index 00000000..9abf893c --- /dev/null +++ b/towncrier/newsfragments/1278.fix @@ -0,0 +1,2 @@ + +Ratelimit counts up on failed auth only now From bee80b5c64951747096c09ab68707bf764e50b56 Mon Sep 17 00:00:00 2001 From: Michael Wyraz Date: Fri, 6 Dec 2019 11:02:21 +0100 Subject: [PATCH 3/6] Remove rate limit reset --- core/admin/mailu/internal/views/auth.py | 4 +--- core/admin/mailu/limiter.py | 8 -------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/core/admin/mailu/internal/views/auth.py b/core/admin/mailu/internal/views/auth.py index af1c552f..b1f37d17 100644 --- a/core/admin/mailu/internal/views/auth.py +++ b/core/admin/mailu/internal/views/auth.py @@ -17,9 +17,7 @@ def nginx_authentication(): response = flask.Response() for key, value in headers.items(): response.headers[key] = str(value) - if ("Auth-Status" in headers) and (headers["Auth-Status"]=="OK"): - utils.limiter.reset(flask.request.headers["Client-Ip"]) - else: + if ("Auth-Status" not in headers) or (headers["Auth-Status"]!="OK"): utils.limiter.hit(flask.request.headers["Client-Ip"]) return response diff --git a/core/admin/mailu/limiter.py b/core/admin/mailu/limiter.py index 4df78d27..fd0b138b 100644 --- a/core/admin/mailu/limiter.py +++ b/core/admin/mailu/limiter.py @@ -35,11 +35,3 @@ class Limiter: # return if not self.limiter.hit(self.rate,"client-ip",clientip): raise RateLimitExceeded() - - def reset(self,clientip): - # TODO: activate this code if we have limits at webmail level - #if ipaddress.ip_address(clientip) in self.subnet: - # # no limits for internal requests (e.g. from webmail) - # return - # limit reset is not supported by the rate limit library - pass From a7f787f91462b2fc12839b00bd22b680553ec9f2 Mon Sep 17 00:00:00 2001 From: Michael Wyraz Date: Mon, 16 Dec 2019 18:46:17 +0100 Subject: [PATCH 4/6] Make rate limit for subnet (webmail) configurable --- core/admin/mailu/configuration.py | 1 + core/admin/mailu/limiter.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/core/admin/mailu/configuration.py b/core/admin/mailu/configuration.py index c7c695f1..7dcd7c3a 100644 --- a/core/admin/mailu/configuration.py +++ b/core/admin/mailu/configuration.py @@ -32,6 +32,7 @@ DEFAULT_CONFIG = { 'POSTMASTER': 'postmaster', 'TLS_FLAVOR': 'cert', 'AUTH_RATELIMIT': '10/minute;1000/hour', + 'AUTH_RATELIMIT_SUBNET': True, 'DISABLE_STATISTICS': False, # Mail settings 'DMARC_RUA': None, diff --git a/core/admin/mailu/limiter.py b/core/admin/mailu/limiter.py index fd0b138b..3fe4d94b 100644 --- a/core/admin/mailu/limiter.py +++ b/core/admin/mailu/limiter.py @@ -13,25 +13,25 @@ class Limiter: self.limiter = None self.rate = None self.subnet = None + self.rate_limit_subnet = True def init_app(self, app): self.storage = limits.storage.storage_from_string(app.config["RATELIMIT_STORAGE_URL"]) self.limiter = limits.strategies.MovingWindowRateLimiter(self.storage) self.rate = limits.parse(app.config["AUTH_RATELIMIT"]) + self.rate_limit_subnet = str(app.config["AUTH_RATELIMIT_SUBNET"])!='False' self.subnet = ipaddress.ip_network(app.config["SUBNET"]) def check(self,clientip): - # TODO: activate this code if we have limits at webmail level - #if ipaddress.ip_address(clientip) in self.subnet: - # # no limits for internal requests (e.g. from webmail) - # return + # disable limits for internal requests (e.g. from webmail)? + if rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet: + return if not self.limiter.test(self.rate,"client-ip",clientip): raise RateLimitExceeded() def hit(self,clientip): - # TODO: activate this code if we have limits at webmail level - #if ipaddress.ip_address(clientip) in self.subnet: - # # no limits for internal requests (e.g. from webmail) - # return + # disable limits for internal requests (e.g. from webmail)? + if rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet: + return if not self.limiter.hit(self.rate,"client-ip",clientip): raise RateLimitExceeded() From 70f797dbd9742c811c83f95cd839f7032129a8ae Mon Sep 17 00:00:00 2001 From: Michael Wyraz Date: Mon, 16 Dec 2019 18:47:21 +0100 Subject: [PATCH 5/6] Don't raise rate limit exception on hit(), only on check() --- core/admin/mailu/limiter.py | 3 +-- docs/configuration.rst | 10 ++++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/core/admin/mailu/limiter.py b/core/admin/mailu/limiter.py index 3fe4d94b..5a1b10fc 100644 --- a/core/admin/mailu/limiter.py +++ b/core/admin/mailu/limiter.py @@ -33,5 +33,4 @@ class Limiter: # disable limits for internal requests (e.g. from webmail)? if rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet: return - if not self.limiter.hit(self.rate,"client-ip",clientip): - raise RateLimitExceeded() + self.limiter.hit(self.rate,"client-ip",clientip) diff --git a/docs/configuration.rst b/docs/configuration.rst index 386fa41c..cda6becb 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -38,8 +38,14 @@ recommended to setup a generic value and later configure a mail alias for that address. The ``AUTH_RATELIMIT`` holds a security setting for fighting attackers that -try to guess user passwords. The value is the limit of requests that a single -IP address can perform against IMAP, POP and SMTP authentication endpoints. +try to guess user passwords. The value is the limit of failed authentication attempts +that a single IP address can perform against IMAP, POP and SMTP authentication endpoints. + +If ``AUTH_RATELIMIT_SUBNET`` is ``True`` (which is the default), the ``AUTH_RATELIMIT`` +rules does also apply to auth requests coming from ``SUBNET``, especially for the webmail. +If you disable this, ensure that the rate limit on the webmail is enforced in a different +way (e.g. roundcube plug-in), otherwise an attacker can simply bypass the limit using webmail. + The ``TLS_FLAVOR`` sets how Mailu handles TLS connections. Setting this value to ``notls`` will cause Mailu not to server any web content! More on :ref:`tls_flavor`. From 7688caa784bf3090c477d96495ba44515750591b Mon Sep 17 00:00:00 2001 From: micw Date: Sun, 5 Jan 2020 19:44:06 +0100 Subject: [PATCH 6/6] Add missing self. --- core/admin/mailu/limiter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/admin/mailu/limiter.py b/core/admin/mailu/limiter.py index 5a1b10fc..f7819c31 100644 --- a/core/admin/mailu/limiter.py +++ b/core/admin/mailu/limiter.py @@ -24,13 +24,13 @@ class Limiter: def check(self,clientip): # disable limits for internal requests (e.g. from webmail)? - if rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet: + if self.rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet: return if not self.limiter.test(self.rate,"client-ip",clientip): raise RateLimitExceeded() def hit(self,clientip): # disable limits for internal requests (e.g. from webmail)? - if rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet: + if self.rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet: return self.limiter.hit(self.rate,"client-ip",clientip)