From 8b5abc4d4db9926a3d76b34b8b03255effb5e712 Mon Sep 17 00:00:00 2001 From: Chris Wagner Date: Thu, 28 Dec 2023 09:02:15 -0800 Subject: [PATCH] Fix (again) possible open redirect vulnerability. (#897) Improve the regex (thanks Brandon Elliot) to catch more crafted relative paths that browsers convert to absolute. Add the "absolute" option to SECURITY_REDIRECT_VALIDATE_MODE which restores Werkzeug prior behavior of converting all Location header values into absolute paths rather than relative (autocorrect_location_header=True). This is the backport to 5.3 --- .pre-commit-config.yaml | 4 +-- CHANGES.rst | 10 +++++++ docs/configuration.rst | 41 ++++++++------------------- docs/patterns.rst | 54 ++++++++++++++++++++++++++++++++++++ flask_security/__init__.py | 2 +- flask_security/core.py | 30 ++++++++++++++++---- tests/test_common.py | 5 ++-- tests/test_confirmable.py | 3 +- tests/test_misc.py | 39 +++++++++++++++++++++++++- tests/test_two_factor.py | 12 +++++--- tests/test_unified_signin.py | 9 ++++-- tests/test_utils.py | 11 ++++++++ 12 files changed, 170 insertions(+), 50 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d1625fe3..f607f5c8 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,7 +19,7 @@ repos: - id: pyupgrade args: [--py38-plus] - repo: https://github.com/psf/black - rev: 23.10.0 + rev: 23.12.1 hooks: - id: black - repo: https://github.com/pycqa/flake8 @@ -30,7 +30,7 @@ repos: - flake8-bugbear - flake8-implicit-str-concat - repo: https://github.com/Riverside-Healthcare/djLint - rev: v1.34.0 + rev: v1.34.1 hooks: - id: djlint-jinja files: "\\.html" diff --git a/CHANGES.rst b/CHANGES.rst index 1a800071..c7112607 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -3,6 +3,16 @@ Flask-Security Changelog Here you can see the full list of changes between each Flask-Security release. +Version 5.3.3 +------------- + +Released December 29, 2023 + +Fixes ++++++ +- (:issue:`893`) Once again work on open-redirect vulnerability - this time due to newer Werkzeug. + Addresses: CVE-2023-49438 + Version 5.3.2 ------------- diff --git a/docs/configuration.rst b/docs/configuration.rst index f363c889..5922ddd0 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -279,46 +279,27 @@ These configuration keys are used globally across all features. .. py:data:: SECURITY_REDIRECT_VALIDATE_MODE - These 2 configuration options attempt to solve a open-redirect vulnerability - that can be exploited if an application sets the Werkzeug response option - ``autocorrect_location_header = False`` (it is ``True`` by default). - For numerous views (e.g. /login) Flask-Security allows callers to specify - a redirect upon successful completion (via the ?next parameter). So it is - possible for a user to be tricked into logging in to a legitimate site - and then redirected to a malicious site. Flask-Security attempts to - verify that redirects are always relative to overcome this security concern. - FS uses the standard Python library urlsplit() to parse the URL and verify - that the ``netloc`` hasn't been altered. - However, many browsers actually accept URLs that should be considered - relative and perform various stripping and conversion that can cause them - to be interpreted as absolute. A trivial example of this is: - - .. line-block:: - /login?next=%20///github.com - - This will pass the urlsplit() test that it is relative - but many browsers - will simply strip off the space and interpret it as an absolute URL! - With the default configuration of Werkzeug this isn't an issue since it by - default modifies the Location Header to with the request ``netloc``. However - if the application sets the Werkzeug response option - ``autocorrect_location_header = False`` this will allow a redirect outside of - the application. - - Setting this to ``"regex"`` will force the URL to be matched using the - pattern specified below. If a match occurs the URL is considered 'absolute' - and will be rejected. + Defines how Flask-Security will attempt to mitigate an open redirect + vulnerability w.r.t. client supplied `next` parameters. + Please see :ref:`redirect_topic` for a complete discussion. + + Current options include `"absolute"` and `"regex"`. A list is allowed. - Default: ``None`` + + Default: ``["absolute"]`` .. versionadded:: 4.0.2 + .. versionchanged:: 5.3.3 + Default is now `"absolute"` and now takes a list. + .. py:data:: SECURITY_REDIRECT_VALIDATE_RE This regex handles known patterns that can be exploited. Basically, don't allow control characters or white-space followed by slashes (or back slashes). - Default: ``r"^/{4,}|\\{3,}|[\s\000-\037][/\\]{2,}"`` + Default: ``r"^/{4,}|\\{3,}|[\s\000-\037][/\\]{2,}(?![/\\])|[/\\]([^/\\]|/[^/\\])*[/\\].*"`` .. versionadded:: 4.0.2 diff --git a/docs/patterns.rst b/docs/patterns.rst index 5544230e..9db13f47 100644 --- a/docs/patterns.rst +++ b/docs/patterns.rst @@ -93,6 +93,60 @@ can be protected by requiring a 'fresh' or recent authentication. Flask-Security Flask-Security itself uses this as part of securing the :ref:`unified-sign-in`, :ref:`two-factor`, and :ref:`webauthn` setup endpoints. +.. _redirect_topic: + +Open Redirect Exposure +~~~~~~~~~~~~~~~~~~~~~~~ +Flask-Security, accepts a ``next=xx`` parameter (either +as a query param OR in the POSTed form) which it will use when completing an operation +which results in a redirection. If a malicious user/ +application can inject an arbitrary ``next`` parameter which redirects to an external +location, this results in a security vulnerability called an `open redirect`. +The following endpoints accept a ``next`` parameter:: + +- .login ("/login") +- .logout ("/logout") +- .register ("/register") +- .verify ("/verify") +- .two_factor_token_validation ("/tf-validate") +- .wan_verify_response ("/wan-verify") +- .wan_signin_response ("/wan-signin") +- .us_signin ("/us-signin") +- .us_verify ("/us-verify") + + +Flask-Security attempts to verify that redirects are always relative. +FS uses the standard Python library urlsplit() to parse the URL and verify +that the ``netloc`` hasn't been altered. +However, many browsers actually accept URLs that should be considered +relative and perform various stripping and conversions that can cause them +to be interpreted as absolute. A trivial example of this is: + +.. line-block:: + /login?next=%20///github.com + +This will pass the urlsplit() test that it is relative - but many browsers +will simply strip off the space and interpret it as an absolute URL! + +Prior to Werkzeug 2.1, Werkzeug set the response configuration variable +``autocorrect_location_header = True`` which forced the response `Location` +header to always be an absolute path - thus effectively squashing any open +redirect possibility. However since 2.1 it is now `False`. + +Flask Security offers +2 mitigations for this via the :py:data:`SECURITY_REDIRECT_VALIDATE_MODE` and +:py:data:`SECURITY_REDIRECT_VALIDATE_RE` configuration variables. + +- The first mode - `"absolute"`, which is the default, is to once again set Werkzeug's ``autocorrect_location_header`` + to ``True``. Please note that this is set JUST for Flask-Security's blueprint - not all requests. +- With the second mode - `"regex"` - FS uses a regular expression to validate all ``next`` parameters to make sure + they will be interpreted as `relative`. Be aware that the default regular + expression is based on in-the-field testing and it is quite possible that there + are other crafted relative URLs that could escape detection. + +:py:data:`SECURITY_REDIRECT_VALIDATE_MODE` actually takes a list - so both +mechanisms can be specified. + .. _pass_validation_topic: Password Validation and Complexity diff --git a/flask_security/__init__.py b/flask_security/__init__.py index a3732b86..7c1e8141 100644 --- a/flask_security/__init__.py +++ b/flask_security/__init__.py @@ -134,4 +134,4 @@ ) from .webauthn_util import WebauthnUtil -__version__ = "5.3.2" +__version__ = "5.3.3" diff --git a/flask_security/core.py b/flask_security/core.py index b460cd86..eb3afa3a 100644 --- a/flask_security/core.py +++ b/flask_security/core.py @@ -196,8 +196,8 @@ "REDIRECT_HOST": None, "REDIRECT_BEHAVIOR": None, "REDIRECT_ALLOW_SUBDOMAINS": False, - "REDIRECT_VALIDATE_MODE": None, - "REDIRECT_VALIDATE_RE": r"^/{4,}|\\{3,}|[\s\000-\037][/\\]{2,}", + "REDIRECT_VALIDATE_MODE": ["absolute"], + "REDIRECT_VALIDATE_RE": r"^/{4,}|\\{3,}|[\s\000-\037][/\\]{2,}(?![/\\])|[/\\]([^/\\]|/[^/\\])*[/\\].*", # noqa: E501 "FORGOT_PASSWORD_TEMPLATE": "security/forgot_password.html", "LOGIN_USER_TEMPLATE": "security/login_user.html", "REGISTER_USER_TEMPLATE": "security/register_user.html", @@ -1432,10 +1432,6 @@ def init_app( self._username_util = self.username_util_cls(app) self._webauthn_util = self.webauthn_util_cls(app) self._mf_recovery_codes_util = self.mf_recovery_codes_util_cls(app) - rvre = cv("REDIRECT_VALIDATE_RE", app=app) - if rvre: - self._redirect_validate_re = re.compile(rvre) - self.remember_token_serializer = _get_serializer(app, "remember") self.login_serializer = _get_serializer(app, "login") self.reset_serializer = _get_serializer(app, "reset") @@ -1522,10 +1518,32 @@ def init_app( if cv("OAUTH_ENABLE", app=app): self.oauthglue = OAuthGlue(app, self._oauth) + redirect_validate_mode = cv("REDIRECT_VALIDATE_MODE", app=app) or [] + if redirect_validate_mode: + # should be "regex" or "absolute" or a list of those + if not isinstance(redirect_validate_mode, list): + redirect_validate_mode = [redirect_validate_mode] + if all([m in ["regex", "absolute"] for m in redirect_validate_mode]): + if "regex" in redirect_validate_mode: + rvre = cv("REDIRECT_VALIDATE_RE", app=app) + if rvre: + self._redirect_validate_re = re.compile(rvre) + else: + raise ValueError("Must specify REDIRECT_VALIDATE_RE") + else: + raise ValueError("Invalid value(s) for REDIRECT_VALIDATE_MODE") + + def autocorrect(r): + # By setting this (Werkzeug) avoids any open-redirect issues. + r.autocorrect_location_header = True + return r + # register our blueprint/endpoints bp = None if self.register_blueprint: bp = create_blueprint(app, self, __name__) + if "absolute" in redirect_validate_mode: + bp.after_request(autocorrect) self.two_factor_plugins.create_blueprint(app, bp, self) if self.oauthglue: self.oauthglue._create_blueprint(app, bp) diff --git a/tests/test_common.py b/tests/test_common.py index 3fe2c170..a3feaab2 100644 --- a/tests/test_common.py +++ b/tests/test_common.py @@ -22,6 +22,7 @@ from tests.test_utils import ( authenticate, capture_flashes, + check_location, get_auth_token_version_3x, get_form_action, get_num_queries, @@ -183,13 +184,13 @@ def test_login_form_username(client): @pytest.mark.settings(username_enable=True, username_required=True) -def test_login_form_username_required(client): +def test_login_form_username_required(app, client): # If username required - we should still be able to login with email alone # given default user_identity_attributes response = client.post( "/login", data=dict(email="matt@lp.com", password="password") ) - assert response.location == "/" + assert check_location(app, response.location, "/") @pytest.mark.confirmable() diff --git a/tests/test_confirmable.py b/tests/test_confirmable.py index f8e1391e..efa4cb7a 100644 --- a/tests/test_confirmable.py +++ b/tests/test_confirmable.py @@ -22,6 +22,7 @@ authenticate, capture_flashes, capture_registrations, + check_location, is_authenticated, logout, populate_data, @@ -469,7 +470,7 @@ def test_auto_login(app, client, get_message): token = registrations[0]["confirm_token"] response = client.get("/confirm/" + token, follow_redirects=False) - assert "/postlogin" == response.location + assert check_location(app, response.location, "/postlogin") assert is_authenticated(client, get_message) diff --git a/tests/test_misc.py b/tests/test_misc.py index ff6603b3..d1b20a05 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -25,6 +25,7 @@ authenticate, capture_flashes, capture_reset_password_requests, + check_location, check_xlation, get_csrf_token, init_app_with_options, @@ -1136,7 +1137,7 @@ def test_verify_fresh(app, client, get_message): response = client.post( verify_url, data=dict(password="password"), follow_redirects=False ) - assert response.location == "http://localhost/fresh" + assert check_location(app, response.location, "/fresh") # should be fine now response = client.get("/fresh", follow_redirects=True) @@ -1320,6 +1321,27 @@ def test_post_security_with_application_root_and_views(app, sqlalchemy_datastore assert "/post_logout" in response.location +def test_invalid_redirect_config(app, sqlalchemy_datastore, get_message): + with pytest.raises(ValueError): + init_app_with_options( + app, + sqlalchemy_datastore, + **{"SECURITY_REDIRECT_VALIDATE_MODE": ["regex", "bogus"]}, + ) + + +def test_invalid_redirect_re(app, sqlalchemy_datastore, get_message): + with pytest.raises(ValueError): + init_app_with_options( + app, + sqlalchemy_datastore, + **{ + "SECURITY_REDIRECT_VALIDATE_MODE": ["regex"], + "SECURITY_REDIRECT_VALIDATE_RE": None, + }, + ) + + @pytest.mark.settings(redirect_validate_mode="regex") def test_validate_redirect(app, sqlalchemy_datastore): """ @@ -1333,9 +1355,24 @@ def test_validate_redirect(app, sqlalchemy_datastore): assert not validate_redirect_url("\\\\\\github.com") assert not validate_redirect_url(" //github.com") assert not validate_redirect_url("\t//github.com") + assert not validate_redirect_url(r"/\github.com") + assert not validate_redirect_url(r"\/github.com") assert not validate_redirect_url("//github.com") # this is normal urlsplit +@pytest.mark.settings(post_login_view="\\\\\\github.com") +def test_validate_redirect_default(app, client): + """ + Test various possible URLs that urlsplit() shows as relative but + many browsers will interpret as absolute - and thus have a + open-redirect vulnerability. This tests the default configuration for + Flask-Security, Flask, Werkzeug + """ + authenticate(client) + response = client.get("/login", follow_redirects=False) + assert response.location.startswith("http://localhost") + + def test_kwargs(): import warnings diff --git a/tests/test_two_factor.py b/tests/test_two_factor.py index 5defdaca..57413fc9 100644 --- a/tests/test_two_factor.py +++ b/tests/test_two_factor.py @@ -26,6 +26,7 @@ authenticate, capture_flashes, capture_send_code_requests, + check_location, get_form_action, get_session, is_authenticated, @@ -811,7 +812,7 @@ def on_identity_changed(app, identity): # Validate token - this should complete 2FA setup response = client.post("/tf-validate", data=dict(code=code), follow_redirects=True) assert b"You successfully changed" in response.data - assert response.history[0].location == "/tf-setup" + assert check_location(app, response.history[0].location, "/tf-setup") # Upon completion, session cookie shouldnt have any two factor stuff in it. session = get_session(response) @@ -1291,7 +1292,9 @@ def test_verify(app, client, get_message): # Test setup when re-authenticate required authenticate(client) response = client.get("tf-setup", follow_redirects=False) - assert "/verify?next=http://localhost/tf-setup" in response.location + assert check_location( + app, response.location, "/verify?next=http://localhost/tf-setup" + ) logout(client) # Now try again - follow redirects to get to verify form @@ -1318,7 +1321,8 @@ def test_verify(app, client, get_message): follow_redirects=False, ) assert response.status_code == 302 - assert response.location == "http://localhost/tf-setup" + assert check_location(app, response.location, "/tf-setup") + assert get_message("REAUTHENTICATION_SUCCESSFUL") == flashes[0]["message"].encode( "utf-8" ) @@ -1443,4 +1447,4 @@ def test_post_setup_redirect(app, client): # Validate token - this should complete 2FA setup response = client.post("/tf-validate", data=dict(code=code), follow_redirects=False) - assert response.location == "/post_setup_view" + assert check_location(app, response.location, "/post_setup_view") diff --git a/tests/test_unified_signin.py b/tests/test_unified_signin.py index 81866239..53a4f7dc 100644 --- a/tests/test_unified_signin.py +++ b/tests/test_unified_signin.py @@ -25,6 +25,7 @@ authenticate, capture_flashes, capture_reset_password_requests, + check_location, get_form_action, is_authenticated, logout, @@ -455,7 +456,7 @@ def test_us_passwordless_confirm_json(app, client, get_message): matcher = re.findall(r"\w+:.*", outbox[0].body, re.IGNORECASE) link = matcher[0].split(":", 1)[1] response = client.get(link, headers=headers, follow_redirects=False) - assert response.location == "/login" + assert check_location(app, response.location, "/login") # should be able to authenticate now. response = client.post( @@ -986,7 +987,9 @@ def test_verify(app, client, get_message): us_authenticate(client) response = client.get("us-setup", follow_redirects=False) verify_url = response.location - assert "/us-verify?next=http://localhost/us-setup" in verify_url + assert check_location( + app, response.location, "/us-verify?next=http://localhost/us-setup" + ) logout(client) us_authenticate(client) @@ -1017,7 +1020,7 @@ def test_verify(app, client, get_message): code = sms_sender.messages[0].split()[-1].strip(".") response = client.post(verify_url, data=dict(passcode=code), follow_redirects=False) - assert response.location == "http://localhost/us-setup" + assert check_location(app, response.location, "/us-setup") def test_verify_json(app, client, get_message): diff --git a/tests/test_utils.py b/tests/test_utils.py index e1587359..9e36aa2b 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -27,6 +27,7 @@ us_security_token_sent, ) from flask_security.utils import hash_data, hash_password +from flask_security.utils import config_value as cv from itsdangerous import BadSignature, SignatureExpired, URLSafeTimedSerializer from werkzeug.http import parse_cookie @@ -63,6 +64,16 @@ def is_authenticated(client, get_message): raise ValueError("Failed to figure out if authenticated") +def check_location(app, location, expected_base): + # verify response location. This can be absolute or relative based + # on configuration + redirect_validate_mode = cv("REDIRECT_VALIDATE_MODE", app=app) or [] + if "absolute" in redirect_validate_mode: + return location == f"http://localhost{expected_base}" + else: + return location == expected_base + + def verify_token(client_nc, token, status=None): # Use passed auth token in API that requires auth and verify status. # Pass in a client_nc to get valid results.