Improve ratelimit implementation

README.md

@@ -134,6 +134,7 @@ This next release focus on two-factor-authentication as a measure to increase se
 * Enforce validation on fullname, username and email
 * Limit incorrect attempts to change the user's password to prevent brute force attacks #225 [CVE-2022-3273](https://nvd.nist.gov/vuln/detail/CVE-2022-3273)
 * Enforce password policy new password cannot be set as new password [CVE-2022-3376](https://nvd.nist.gov/vuln/detail/CVE-2022-3376)
+* Enforce better rate limit on login, mfa, password change and API [CVE-2022-3439](https://nvd.nist.gov/vuln/detail/CVE-2022-3439) [CVE-2022-3456](https://nvd.nist.gov/vuln/detail/CVE-2022-3456)
 
 Breaking changes:
 

doc/configuration.md

@@ -273,7 +273,7 @@ attacks and authenticated users to avoid Denial Of Service attack.
 
 | Option | Description | Example |
 | --- | --- | --- |
-| rate-limit | maximum number of requests per minute that can be made by an IP address for an unauthenticated connection. When this limit is reached, an HTTP 429 message is returned to the user. This security measure is used to limit brute force attacks on the login page and the RESTful API. | 10 |
+| rate-limit | maximum number of requests per hour that can be made on sensitive endpoints. When this limit is reached, an HTTP 429 message is returned to the user or the user is logged out. This security measure is used to limit brute force attacks on the login page and the RESTful API. | 20 |
 | rate-limit-dir | location where to store rate-limit information. When undefined, data is kept in memory. | /var/lib/rdiffweb/session |
 
 ## Custom user's password length limits

rdiffweb/controller/api.py

@@ -64,9 +64,14 @@ def _checkpassword(realm, username, password):
             return True
         # Disable password authentication for MFA
         if userobj.mfa == UserObject.ENABLED_MFA:
+            cherrypy.tools.ratelimit.hit()
             return False
     # Otherwise validate username password
-    return any(cherrypy.engine.publish('login', username, password))
+    valid = any(cherrypy.engine.publish('login', username, password))
+    if not valid:
+        # When invalid, we need to increase the rate limit.
+        cherrypy.tools.ratelimit.hit()
+    return valid
 
 
 class ApiCurrentUser(Controller):
@@ -99,9 +104,9 @@ def default(self):
 @cherrypy.tools.auth_basic(realm='rdiffweb', checkpassword=_checkpassword, priority=70)
 @cherrypy.tools.auth_form(on=False)
 @cherrypy.tools.auth_mfa(on=False)
-@cherrypy.tools.sessions(on=False)
 @cherrypy.tools.i18n(on=False)
-@cherrypy.tools.ratelimit()
+@cherrypy.tools.ratelimit(scope='rdiffweb-api', hit=0, priority=69)
+@cherrypy.tools.sessions(on=False)
 class ApiPage(Controller):
     """
     This class provide a restful API to access some of the rdiffweb resources.

rdiffweb/controller/form.py

@@ -19,8 +19,6 @@
 from markupsafe import Markup
 from wtforms.form import Form
 
-SUBMIT_METHODS = {'POST', 'PUT', 'PATCH', 'DELETE'}
-
 
 class _ProxyFormdata:
     """
@@ -65,7 +63,7 @@ def is_submitted(self):
         Consider the form submitted if there is an active request and
         the method is ``POST``, ``PUT``, ``PATCH``, or ``DELETE``.
         """
-        return cherrypy.request.method in SUBMIT_METHODS
+        return cherrypy.request.method == 'POST'
 
     def validate_on_submit(self):
         """

rdiffweb/controller/page_admin_session.py

@@ -30,10 +30,6 @@ class RevokeSessionForm(CherryForm):
     action = StringField(validators=[validators.regexp('delete')])
     number = IntegerField(validators=[validators.data_required()])
 
-    @property
-    def app(self):
-        return cherrypy.request.app
-
 
 @cherrypy.tools.is_admin()
 class AdminSessionPage(Controller):

rdiffweb/controller/page_login.py

@@ -62,7 +62,7 @@ class LoginPage(Controller):
 
     @cherrypy.expose()
     @cherrypy.tools.auth_mfa(on=False)
-    @cherrypy.tools.ratelimit()
+    @cherrypy.tools.ratelimit(methods=['POST'])
     def index(self, **kwargs):
         """
         Called by auth_form to generate the /login/ page.

rdiffweb/controller/page_mfa.py

@@ -70,7 +70,7 @@ def validate(self, extra_validators=None):
 
 class MfaPage(Controller):
     @cherrypy.expose()
-    @cherrypy.tools.ratelimit()
+    @cherrypy.tools.ratelimit(methods=['POST'])
     def index(self, **kwargs):
         form = MfaForm()
 

rdiffweb/controller/page_pref_general.py

@@ -29,10 +29,6 @@
 from rdiffweb.core.model import UserObject
 from rdiffweb.tools.i18n import gettext_lazy as _
 
-# Maximum number of password change attempt before logout
-CHANGE_PASSWORD_MAX_ATTEMPT = 5
-CHANGE_PASSWORD_ATTEMPTS = 'change_password_attempts'
-
 
 class UserProfileForm(CherryForm):
     action = HiddenField(default='set_profile_info')
@@ -99,23 +95,8 @@ def populate_obj(self, user):
         # Check if current password is "valid" if Not, rate limit the
         # number of attempts and logout user after too many invalid attempts.
         if not user.validate_password(self.current.data):
-            cherrypy.session[CHANGE_PASSWORD_ATTEMPTS] = cherrypy.session.get(CHANGE_PASSWORD_ATTEMPTS, 0) + 1
-            attempts = cherrypy.session[CHANGE_PASSWORD_ATTEMPTS]
-            if attempts >= CHANGE_PASSWORD_MAX_ATTEMPT:
-                cherrypy.session.clear()
-                cherrypy.session.regenerate()
-                flash(
-                    _("You were logged out because you entered the wrong password too many times."),
-                    level='warning',
-                )
-                raise cherrypy.HTTPRedirect('/login/')
             self.current.errors = [_("Wrong current password.")]
             return False
-
-        # Clear number of attempts
-        if CHANGE_PASSWORD_ATTEMPTS in cherrypy.session:
-            del cherrypy.session[CHANGE_PASSWORD_ATTEMPTS]
-
         try:
             user.set_password(self.new.data)
             return True
@@ -151,6 +132,7 @@ class PagePrefsGeneral(Controller):
     """
 
     @cherrypy.expose
+    @cherrypy.tools.ratelimit(methods=['POST'], logout=True)
     def default(self, **kwargs):
         # Process the parameters.
         profile_form = UserProfileForm(obj=self.app.currentuser)

rdiffweb/controller/page_pref_session.py

@@ -30,10 +30,6 @@ class RevokeSessionForm(CherryForm):
     action = StringField(validators=[validators.regexp('delete')])
     number = IntegerField(validators=[validators.data_required()])
 
-    @property
-    def app(self):
-        return cherrypy.request.app
-
 
 class PagePrefSession(Controller):
     @cherrypy.expose

rdiffweb/controller/tests/test_api.py

@@ -124,6 +124,14 @@ class APIRatelimitTest(rdiffweb.test.WebCase):
     }
 
     def test_login_ratelimit(self):
-        for i in range(0, 6):
-            self.getPage('/api/')
+        # Given invalid credentials sent to API
+        headers = [("Authorization", "Basic " + b64encode(b"admin:invalid").decode('ascii'))]
+        for i in range(1, 5):
+            self.getPage('/api/', headers=headers)
+            self.assertStatus(401)
+        # Then the 6th request is refused
+        self.getPage('/api/', headers=headers)
+        self.assertStatus(429)
+        # Next request is also refused event if credentials are valid.
+        self.getPage('/api/', headers=[("Authorization", "Basic " + b64encode(b"admin:admin123").decode('ascii'))])
         self.assertStatus(429)

rdiffweb/controller/tests/test_page_login.py

@@ -19,9 +19,9 @@
 
 @author: Patrik Dufresne
 """
+import os
 
-
-from parameterized import parameterized
+from parameterized import parameterized, parameterized_class
 
 import rdiffweb.test
 from rdiffweb.core.model import DbSession, SessionObject, UserObject
@@ -212,67 +212,80 @@ def test_getpage_default(self):
         self.assertInBody('HEADER-NAME')
 
 
+@parameterized_class(
+    [
+        {"default_config": {'rate-limit': 5}},
+        {"default_config": {'rate-limit': 5, 'rate-limit-dir': '/tmp'}},
+    ]
+)
 class LoginPageRateLimitTest(rdiffweb.test.WebCase):
-
-    default_config = {
-        'rate-limit': 5,
-    }
+    def setUp(self):
+        if os.path.isfile('/tmp/ratelimit-127.0.0.1'):
+            os.unlink('/tmp/ratelimit-127.0.0.1')
+        if os.path.isfile('/tmp/ratelimit-127.0.0.1.-login'):
+            os.unlink('/tmp/ratelimit-127.0.0.1.-login')
+        return super().setUp()
 
     def test_login_ratelimit(self):
         # Given an unauthenticate
-        # When requesting multple time the login page
-        for i in range(0, 6):
-            self.getPage('/login/')
+        # When requesting multiple time the login page
+        for i in range(1, 5):
+            self.getPage('/login/', method='POST', body={'login': 'invalid', 'password': 'invalid'})
+            self.assertStatus(200)
         # Then a 429 error (too many request) is return
+        self.getPage('/login/', method='POST', body={'login': 'invalid', 'password': 'invalid'})
         self.assertStatus(429)
 
 
-class LoginPageRateLimitWithSessionDirTest(rdiffweb.test.WebCase):
+class LoginPageRateLimitTest2(rdiffweb.test.WebCase):
 
-    default_config = {
-        'rate-limit-dir': '/tmp',
-        'rate-limit': 5,
-    }
+    default_config = {'rate-limit': 5}
 
-    def test_login_ratelimit(self):
+    def test_login_ratelimit_forwarded_for(self):
         # Given an unauthenticate
-        # When requesting multple time the login page
-        for i in range(0, 6):
-            self.getPage('/login/')
-        # Then a 429 error (too many request) is return
+        # When requesting multiple time the login page with different `X-Forwarded-For`
+        for i in range(1, 5):
+            self.getPage(
+                '/login/',
+                headers=[('X-Forwarded-For', '127.0.0.%s' % i)],
+                method='POST',
+                body={'login': 'invalid', 'password': 'invalid'},
+            )
+            self.assertStatus(200)
+        # Then original IP get blocked
+        self.getPage(
+            '/login/',
+            headers=[('X-Forwarded-For', '127.0.0.%s' % i)],
+            method='POST',
+            body={'login': 'invalid', 'password': 'invalid'},
+        )
         self.assertStatus(429)
 
 
-class LoginPageRateLimitTestWithXForwardedFor(rdiffweb.test.WebCase):
+class LoginPageRateLimitTest3(rdiffweb.test.WebCase):
+    default_config = {'rate-limit': 5}
 
-    default_config = {
-        'rate-limit': 5,
-    }
-
-    def test_login_ratelimit(self):
+    def test_login_ratelimit_real_ip(self):
         # Given an unauthenticate
-        # When requesting multple time the login page
-        for i in range(0, 6):
-            self.getPage('/login/', headers=[('X-Forwarded-For', '127.0.0.%s' % i)])
-        # Then a 429 error (too many request) is return
+        # When requesting multiple time the login page with different `X-Real-IP`
+        for i in range(1, 5):
+            self.getPage(
+                '/login/',
+                headers=[('X-Real-IP', '127.0.0.128')],
+                method='POST',
+                body={'login': 'invalid', 'password': 'invalid'},
+            )
+            self.assertStatus(200)
+        # Then the X-Real-IP get blocked
+        self.getPage(
+            '/login/',
+            headers=[('X-Real-IP', '127.0.0.128')],
+            method='POST',
+            body={'login': 'invalid', 'password': 'invalid'},
+        )
         self.assertStatus(429)
 
 
-class LoginPageRateLimitTestWithXRealIP(rdiffweb.test.WebCase):
-
-    default_config = {
-        'rate-limit': 5,
-    }
-
-    def test_login_ratelimit(self):
-        # Given an unauthenticate
-        # When requesting multple time the login page
-        for i in range(0, 6):
-            self.getPage('/login/', headers=[('X-Real-IP', '127.0.0.%s' % i)])
-        # Then a 200 is return.
-        self.assertStatus(200)
-
-
 class LogoutPageTest(rdiffweb.test.WebCase):
     def test_getpage_without_login(self):
         # Given an unauthenticated user

rdiffweb/controller/tests/test_page_prefs_general.py

@@ -203,21 +203,6 @@ def test_change_password_with_too_long(self):
         self._set_password(self.PASSWORD, new_password, new_password)
         self.assertInBody("Password must have between 8 and 128 characters.")
 
-    def test_change_password_too_many_attemps(self):
-        # When udating user's password with wrong current password 5 times
-        for _i in range(1, 5):
-            self._set_password('wrong', "pr3j5Dwi", "pr3j5Dwi")
-            self.assertStatus(200)
-            self.assertInBody("Wrong current password.")
-        # Then user session is cleared and user is redirect to login page
-        self._set_password('wrong', "pr3j5Dwi", "pr3j5Dwi")
-        self.assertStatus(303)
-        self.assertHeaderItemValue('Location', self.baseurl + '/login/')
-        # Then a warning message is displayed on login page
-        self.getPage('/login/')
-        self.assertStatus(200)
-        self.assertInBody('You were logged out because you entered the wrong password too many times.')
-
     def test_change_password_with_same_value(self):
         # Given a user with a password
         self._set_password(self.PASSWORD, "pr3j5Dwi", "pr3j5Dwi")
@@ -256,3 +241,31 @@ def test_update_repos(self):
         # Then the list is free of inexisting repos.
         userobj.expire()
         self.assertEqual(['broker-repo', 'testcases'], sorted([r.name for r in userobj.repo_objs]))
+
+
+class PagePrefGeneralRateLimitTest(rdiffweb.test.WebCase):
+    login = True
+
+    default_config = {'rate-limit': 5}
+
+    def test_change_password_too_many_attemps(self):
+        # When udating user's password with wrong current password 5 times
+        for _i in range(1, 5):
+            self.getPage(
+                '/prefs/general',
+                method='POST',
+                body={'action': 'set_password', 'current': 'wrong', 'new': 'pr3j5Dwi', 'confirm': 'pr3j5Dwi'},
+            )
+            self.assertStatus(200)
+            self.assertInBody("Wrong current password.")
+        # Then user session is cleared and user is redirect to login page
+        self.getPage(
+            '/prefs/general',
+            method='POST',
+            body={'action': 'set_password', 'current': 'wrong', 'new': 'pr3j5Dwi', 'confirm': 'pr3j5Dwi'},
+        )
+        self.assertStatus(303)
+        self.assertHeaderItemValue('Location', self.baseurl + '/')
+        # Then a warning message is displayed on login page
+        self.getPage('/login/')
+        self.assertStatus(200)

rdiffweb/core/config.py

@@ -404,8 +404,8 @@ def get_parser():
         '--rate-limit',
         metavar='LIMIT',
         type=int,
-        default=30,
-        help='maximum number of requests per minute that can be made by an IP address for an unauthenticated connection. When this limit is reached, an HTTP 429 message is returned to the user. This security measure is used to limit brute force attacks on the login page and the RESTful API.',
+        default=20,
+        help='maximum number of requests per hour that can be made on sensitive endpoints. When this limit is reached, an HTTP 429 message is returned to the user or the user is logged out. This security measure is used to limit brute force attacks on the login page and the RESTful API.',
     )
 
     parser.add(

rdiffweb/rdw_app.py

@@ -209,8 +209,8 @@ def __init__(self, cfg):
                 'tools.sessions.persistent': False,  # auth_form should update this.
                 'tools.auth_form.timeout': cfg.session_persistent_timeout,  # minutes
                 'tools.ratelimit.debug': cfg.debug,
-                'tools.ratelimit.delay': 60,
-                'tools.ratelimit.anonymous_limit': cfg.rate_limit,
+                'tools.ratelimit.delay': 3600,
+                'tools.ratelimit.limit': cfg.rate_limit,
                 'tools.ratelimit.storage_class': rate_limit_storage_class,
                 'tools.ratelimit.storage_path': cfg.rate_limit_dir,
             },