A (hopeful) fix for possible open-redirect. (#489)

While this is only an issue if the application sets the Werkzeug response variable:
autocorrect_location_header = False - it none the less poses a small security concern.

pyupgrade and black changed again .. sigh...
pin read the docs sphinx versions.

Closes: #486

Author: jwag956

.pre-commit-config.yaml

@@ -4,7 +4,7 @@ default_language_version:
     python: python3.8
 repos:
 -   repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v3.4.0
+    rev: v4.0.1
     hooks:
     -   id: trailing-whitespace
     -   id: end-of-file-fixer
@@ -14,16 +14,16 @@ repos:
     -   id: check-merge-conflict
     -   id: fix-byte-order-marker
 -   repo: https://github.com/asottile/pyupgrade
-    rev: v2.14.0
+    rev: v2.19.0
     hooks:
     -   id: pyupgrade
         args: [--py36-plus]
 -   repo: https://github.com/psf/black
-    rev: 21.5b0
+    rev: 21.5b1
     hooks:
     -   id: black
 -   repo: https://gitlab.com/pycqa/flake8
-    rev: 3.9.1
+    rev: 3.9.2
     hooks:
       - id: flake8
         additional_dependencies:

docs/configuration.rst

@@ -221,14 +221,59 @@ These configuration keys are used globally across all features.
 .. py:data:: SECURITY_REDIRECT_ALLOW_SUBDOMAINS
 
     If ``True`` then subdomains (and the root domain) of the top-level host set
-    by Flask's ``SERVER_NAME`` configuration will be allowed as post-login redirect targets.
+    by Flask's ``SERVER_NAME`` configuration will be allowed as post-view redirect targets.
     This is beneficial if you wish to place your authentiation on one subdomain and
     authenticated content on another, for example ``auth.domain.tld`` and ``app.domain.tld``.
 
     Default: ``False``.
 
     .. versionadded:: 4.0.0
 
+.. 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.
+
+    Default: ``None``
+
+    .. versionadded:: 4.0.2
+
+.. 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,}"``
+
+    .. versionadded:: 4.0.2
+
 .. py:data:: SECURITY_CSRF_PROTECT_MECHANISMS
 
     Authentication mechanisms that require CSRF protection.
@@ -606,13 +651,17 @@ Login/Logout
 
     Default: ``"/verify"``
 
+    .. versionadded:: 3.4.0
+
 
 .. py:data:: SECURITY_VERIFY_TEMPLATE
 
     Specifies the path to the template for the verify password page.
 
     Default: ``"security/verify.html"``.
 
+    .. versionadded:: 3.4.0
+
 .. py:data:: SECURITY_POST_VERIFY_URL
 
     Specifies the default view to redirect to after a user successfully re-authenticates either via
@@ -622,6 +671,8 @@ Login/Logout
 
     Default: ``None``.
 
+    .. versionadded:: 3.4.0
+
 Registerable
 ------------
 .. py:data:: SECURITY_REGISTERABLE

examples/fsqlalchemy1/tests/test_utils.py

@@ -41,5 +41,5 @@ def create_fake_user(user_cls, email="[email protected]", userid=1, roles=None):
 
 def create_fake_role(role_cls, name, permissions=None):
     if permissions:
-        permissions = ",".join([p.strip() for p in permissions.split(",")])
+        permissions = ",".join(p.strip() for p in permissions.split(","))
     return role_cls(name=name, permissions=permissions)

flask_security/core.py

@@ -7,11 +7,12 @@
     :copyright: (c) 2012 by Matt Wright.
     :copyright: (c) 2017 by CERN.
     :copyright: (c) 2017 by ETH Zurich, Swiss Data Science Center.
-    :copyright: (c) 2019-2020 by J. Christopher Wagner (jwag).
+    :copyright: (c) 2019-2021 by J. Christopher Wagner (jwag).
     :license: MIT, see LICENSE for more details.
 """
 
 from datetime import datetime, timedelta
+import re
 import warnings
 
 import pkg_resources
@@ -156,6 +157,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,}",
     "FORGOT_PASSWORD_TEMPLATE": "security/forgot_password.html",
     "LOGIN_USER_TEMPLATE": "security/login_user.html",
     "REGISTER_USER_TEMPLATE": "security/register_user.html",
@@ -593,6 +596,8 @@ def _get_state(app, datastore, anonymous_user=None, **kwargs):
             _unauthz_handler=default_unauthz_handler,
         )
     )
+    if "redirect_validate_re" in kwargs:
+        kwargs["_redirect_validate_re"] = re.compile(kwargs["redirect_validate_re"])
 
     if "login_manager" not in kwargs:
         kwargs["login_manager"] = _get_login_manager(app, anonymous_user)

flask_security/datastore.py

@@ -319,7 +319,7 @@ def create_role(self, **kwargs):
                 perms = ",".join(perms)
             elif isinstance(perms, str):
                 # squash spaces.
-                perms = ",".join([p.strip() for p in perms.split(",")])
+                perms = ",".join(p.strip() for p in perms.split(","))
             kwargs["permissions"] = perms
 
         role = self.role_model(**kwargs)

flask_security/decorators.py

@@ -504,7 +504,7 @@ def create_post():
     def wrapper(fn):
         @wraps(fn)
         def decorated_view(*args, **kwargs):
-            perm = Permission(*[RoleNeed(role) for role in roles])
+            perm = Permission(*(RoleNeed(role) for role in roles))
             if perm.can():
                 return fn(*args, **kwargs)
             if _security._unauthorized_callback:
@@ -578,7 +578,7 @@ def create_post():
     def wrapper(fn):
         @wraps(fn)
         def decorated_view(*args, **kwargs):
-            perm = Permission(*[FsPermNeed(fsperm) for fsperm in fsperms])
+            perm = Permission(*(FsPermNeed(fsperm) for fsperm in fsperms))
             if perm.can():
                 return fn(*args, **kwargs)
             if _security._unauthorized_callback:

flask_security/utils.py

@@ -498,6 +498,34 @@ def url_for_security(endpoint, **values):
 
 
 def validate_redirect_url(url):
+    """Validate that the URL for redirect is relative.
+    Allowing an absolute redirect is a security issue - a so-called open-redirect.
+    Note that by default Werkzeug will always take this URL and make it relative
+    when setting the Location header - but that behavior can be overridden.
+
+    The complexity here is that urlsplit() does pretty well, but browsers even today
+    May 2021 are very lenient in what they accept as URLs - for example:
+        next=\\\\github.com
+        next=%5C%5C%5Cgithub.com
+        next=/////github.com
+        next=%20\\\\github.com
+        next=%20///github.com
+        next=%20//github.com
+        next=%19////github.com - i.e. browser will strip control chars
+        next=%E2%80%8A///github.com - doesn't redirect! That is a unicode thin space.
+
+    All will result in a null netloc and scheme from urlsplit - however many browsers
+    will gladly strip off uninteresting characters and convert backslashes to forward
+    slashes - and the cases above will actually cause a redirect to github.com
+    Sigh.
+
+    Some articles claim that a relative url has to start with a '/' - but that isn't
+    strictly true. From: https://datatracker.ietf.org/doc/html/rfc3986#section-5
+    a relative path can start with a "//", "/", a non-colon, or be empty. So it seems
+    that all the above URLs are valid.
+    By the time we get the URL, it has been unencoded - so we can't really determine
+    if it is 'valid' since it appears that '/'s can appear in the URL if escaped.
+    """
     if url is None or url.strip() == "":
         return False
     url_next = urlsplit(url)
@@ -515,6 +543,9 @@ def validate_redirect_url(url):
             return True
         else:
             return False
+    if config_value("REDIRECT_VALIDATE_MODE") == "regex":
+        matcher = _security._redirect_validate_re.match(url)
+        return matcher is None
     return True
 
 

requirements/docs.txt

@@ -1,3 +1,3 @@
-Pallets-Sphinx-Themes
-Sphinx
-sphinx-issues
+Pallets-Sphinx-Themes~=2.0
+Sphinx~=4.0
+sphinx-issues~=1.2

tests/test_misc.py

@@ -58,6 +58,7 @@
     hash_data,
     send_mail,
     uia_phone_mapper,
+    validate_redirect_url,
     verify_hash,
 )
 
@@ -1034,3 +1035,19 @@ def test_post_security_with_application_root_and_views(app, sqlalchemy_datastore
     response = client.get("/logout")
     assert response.status_code == 302
     assert response.headers["Location"] == "http://localhost/post_logout"
+
+
+@pytest.mark.settings(redirect_validate_mode="regex")
+def test_validate_redirect(app, sqlalchemy_datastore):
+    """
+    Test various possible URLs that urlsplit() shows as relative but
+    many browsers will interpret as absolute - and this have a
+    open-redirect vulnerability. Note this vulnerability only
+    is viable if the application sets autocorrect_location_header = False
+    """
+    init_app_with_options(app, sqlalchemy_datastore)
+    with app.test_request_context("http://localhost:5001/login"):
+        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("//github.com")  # this is normal urlsplit

tests/view_scaffold.py

@@ -159,6 +159,14 @@ def create_user():
             add_user(user_datastore, test_acct, "password", ["admin"])
             print("Created User: {} with password {}".format(test_acct, "password"))
 
+    @app.after_request
+    def allow_absolute_redirect(r):
+        # This is JUST to test odd possible redirects that look relative but are
+        # interpreted by browsers as absolute.
+        # DON'T SET THIS IN YOUR APPLICATION!
+        r.autocorrect_location_header = False
+        return r
+
     @user_registered.connect_via(app)
     def on_user_registered(myapp, user, confirm_token, **extra):
         flash(f"To confirm {user.email} - go to /confirm/{confirm_token}")