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

@@ -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"

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
 -------------
 

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
 

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

flask_security/__init__.py

@@ -134,4 +134,4 @@
 )
 from .webauthn_util import WebauthnUtil
 
-__version__ = "5.3.2"
+__version__ = "5.3.3"

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)

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()

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)
 
 

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
 

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")