From a0d44d31e05426ebfdcbe689440e91525e4fc8fa Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Fri, 1 Nov 2024 01:44:47 -0700 Subject: [PATCH 1/4] =?UTF-8?q?Don=E2=80=99t=20read=20redirect=20target=20?= =?UTF-8?q?from=20query=20string=20in=202FA=20flow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As far as I know, the `urllib.parse`-based filtering is safe, but there’s no legitimate way for the `GET` handler to receive a `referer` – the entire handler is only there in case someone manually goes to their browser’s address bar and loads the current address while in the middle of 2FA, and possibly shouldn’t exist at all. --- weasyl/controllers/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/weasyl/controllers/user.py b/weasyl/controllers/user.py index 635ad8197..b94c12b09 100644 --- a/weasyl/controllers/user.py +++ b/weasyl/controllers/user.py @@ -108,7 +108,7 @@ def signin_2fa_auth_get_(request): login.signout(request) raise WeasylError('TwoFactorAuthenticationAuthenticationTimeout') else: - ref = request.params["referer"] if "referer" in request.params else "/" + ref = "/" return Response(define.webpage( request.userid, "etc/signin_2fa_auth.html", From 8076fcebb0d7e59cc8aa309f6910bbd328f33b59 Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Fri, 1 Nov 2024 01:51:11 -0700 Subject: [PATCH 2/4] Never read a `referer` from anywhere other than the request body Just applying normal good practice to a key place; as far as I know, nothing was exploitable. --- weasyl/controllers/settings.py | 4 ++-- weasyl/controllers/user.py | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/weasyl/controllers/settings.py b/weasyl/controllers/settings.py index d705cc6a2..22bcaf7ac 100644 --- a/weasyl/controllers/settings.py +++ b/weasyl/controllers/settings.py @@ -798,10 +798,10 @@ def manage_alias_post_(request): @token_checked @disallow_api def sfw_toggle_(request): - form = request.web_input(redirect="/") + redirect = request.POST.get("redirect", "/") currentstate = request.cookies.get('sfwmode', "nsfw") newstate = "sfw" if currentstate == "nsfw" else "nsfw" - response = HTTPSeeOther(location=form.redirect) + response = HTTPSeeOther(location=redirect) response.set_cookie("sfwmode", newstate, max_age=60 * 60 * 24 * 365) return response diff --git a/weasyl/controllers/user.py b/weasyl/controllers/user.py index b94c12b09..c2e419a8a 100644 --- a/weasyl/controllers/user.py +++ b/weasyl/controllers/user.py @@ -35,13 +35,13 @@ def signin_get_(request): @guest_required @token_checked def signin_post_(request): - form = request.web_input(username="", password="", referer="", sfwmode="nsfw") - form.referer = form.referer or '/' + form = request.web_input(username="", password="", sfwmode="nsfw") + referer = request.POST.get("referer") or "/" logid, logerror = login.authenticate_bcrypt(form.username, form.password, request=request, ip_address=request.client_addr, user_agent=request.user_agent) if logid and logerror is None: - response = HTTPSeeOther(location=form.referer) + response = HTTPSeeOther(location=referer) response.set_cookie('WZL', request.weasyl_session.sessionid, max_age=60 * 60 * 24 * 365, secure=request.scheme == 'https', httponly=True) if form.sfwmode == "sfw": @@ -72,7 +72,7 @@ def signin_post_(request): response = Response(define.webpage( request.userid, "etc/signin_2fa_auth.html", - [define.get_display_name(logid), form.referer, remaining_recovery_codes, None], + [define.get_display_name(logid), referer, remaining_recovery_codes, None], title="Sign In - 2FA" )) response.set_cookie('WZL', sess.sessionid, max_age=60 * 60 * 24 * 365, @@ -81,7 +81,7 @@ def signin_post_(request): response.set_cookie("sfwmode", "sfw", max_age=31536000) return response elif logerror == "invalid": - return Response(define.webpage(request.userid, "etc/signin.html", [True, form.referer])) + return Response(define.webpage(request.userid, "etc/signin.html", [True, referer])) elif logerror == "banned": message = moderation.get_ban_message(logid) return Response(define.errorpage(request.userid, message)) @@ -136,13 +136,13 @@ def signin_2fa_auth_post_(request): # 2FA passed, so login and cleanup. login.signout(request) login.signin(request, tfa_userid, ip_address=request.client_addr, user_agent=request.user_agent) - ref = request.params["referer"] or "/" # User is out of recovery codes, so force-deactivate 2FA if two_factor_auth.get_number_of_recovery_codes(tfa_userid) == 0: two_factor_auth.force_deactivate(tfa_userid) raise WeasylError('TwoFactorAuthenticationZeroRecoveryCodesRemaining', links=[["2FA Dashboard", "/control/2fa/status"], ["Return to the Home Page", "/"]]) # Return to the target page, removing the scheme and domain per urlsplit. + ref = request.POST["referer"] or "/" urlparts = urlsplit(ref) response = HTTPSeeOther(location=urlunsplit(['', '', urlparts[2], urlparts[3], urlparts[4]])) response.set_cookie('WZL', request.weasyl_session.sessionid, max_age=60 * 60 * 24 * 365, @@ -159,11 +159,11 @@ def signin_2fa_auth_post_(request): sess.additional_data['2fa_pwd_auth_attempts'] += 1 flag_modified(sess, 'additional_data') tx.add(sess) - # 2FA failed; redirect to 2FA input page & inform user that authentication failed. + # 2FA failed; respond with 2FA input page & inform user that authentication failed. return Response(define.webpage( request.userid, "etc/signin_2fa_auth.html", - [define.get_display_name(tfa_userid), request.params["referer"], two_factor_auth.get_number_of_recovery_codes(tfa_userid), + [define.get_display_name(tfa_userid), request.POST["referer"], two_factor_auth.get_number_of_recovery_codes(tfa_userid), "2fa"], title="Sign In - 2FA")) From cf479a762a921c62010cf5c5fe244d97367e1734 Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Fri, 1 Nov 2024 02:15:33 -0700 Subject: [PATCH 3/4] Assert that all redirects back are internal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit instead of filtering just on the 2FA endpoint – for consistency. --- weasyl/controllers/settings.py | 3 ++- weasyl/controllers/user.py | 8 +++----- weasyl/forms.py | 13 +++++++++++++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/weasyl/controllers/settings.py b/weasyl/controllers/settings.py index 22bcaf7ac..d400b6dc2 100644 --- a/weasyl/controllers/settings.py +++ b/weasyl/controllers/settings.py @@ -8,6 +8,7 @@ from weasyl.controllers.decorators import disallow_api, login_required, token_checked from weasyl.error import WeasylError +from weasyl.forms import checked_redirect from weasyl import ( api, avatar, banner, blocktag, collection, commishinfo, define, emailer, folder, followuser, frienduser, ignoreuser, @@ -802,6 +803,6 @@ def sfw_toggle_(request): currentstate = request.cookies.get('sfwmode', "nsfw") newstate = "sfw" if currentstate == "nsfw" else "nsfw" - response = HTTPSeeOther(location=redirect) + response = HTTPSeeOther(location=checked_redirect(redirect)) response.set_cookie("sfwmode", newstate, max_age=60 * 60 * 24 * 365) return response diff --git a/weasyl/controllers/user.py b/weasyl/controllers/user.py index c2e419a8a..a583bd588 100644 --- a/weasyl/controllers/user.py +++ b/weasyl/controllers/user.py @@ -1,5 +1,3 @@ -from urllib.parse import urlsplit, urlunsplit - import arrow from pyramid.httpexceptions import HTTPSeeOther from pyramid.response import Response @@ -22,6 +20,7 @@ token_checked, ) from weasyl.error import WeasylError +from weasyl.forms import checked_redirect from weasyl.sessions import create_session @@ -41,7 +40,7 @@ def signin_post_(request): logid, logerror = login.authenticate_bcrypt(form.username, form.password, request=request, ip_address=request.client_addr, user_agent=request.user_agent) if logid and logerror is None: - response = HTTPSeeOther(location=referer) + response = HTTPSeeOther(location=checked_redirect(referer)) response.set_cookie('WZL', request.weasyl_session.sessionid, max_age=60 * 60 * 24 * 365, secure=request.scheme == 'https', httponly=True) if form.sfwmode == "sfw": @@ -143,8 +142,7 @@ def signin_2fa_auth_post_(request): links=[["2FA Dashboard", "/control/2fa/status"], ["Return to the Home Page", "/"]]) # Return to the target page, removing the scheme and domain per urlsplit. ref = request.POST["referer"] or "/" - urlparts = urlsplit(ref) - response = HTTPSeeOther(location=urlunsplit(['', '', urlparts[2], urlparts[3], urlparts[4]])) + response = HTTPSeeOther(location=checked_redirect(ref)) response.set_cookie('WZL', request.weasyl_session.sessionid, max_age=60 * 60 * 24 * 365, secure=request.scheme == 'https', httponly=True) return response diff --git a/weasyl/forms.py b/weasyl/forms.py index 726e9d112..def7e96d7 100644 --- a/weasyl/forms.py +++ b/weasyl/forms.py @@ -1,4 +1,5 @@ from typing import Iterable, TypeVar +from urllib.parse import urlsplit from weasyl.error import WeasylError @@ -29,3 +30,15 @@ def only(iterable: Iterable[_T]) -> _T: return x raise WeasylError("Unexpected") + + +def checked_redirect(path: str) -> str: + """ + Returns the passed URL, asserting that it’s a valid place to redirect back to within the same application (e.g. after logging in). + """ + split_result = urlsplit(path) + + if split_result.scheme or split_result.netloc: + raise WeasylError("Unexpected", level="warning") + + return path From 6f7cc231a62a31d63e036fa501f37cca3ed36620 Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Sat, 2 Nov 2024 01:27:14 -0700 Subject: [PATCH 4/4] Always construct absolute URL for `Location` from return path parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Of course it was exploitable! This is “fixed” in WebOb 1.8.8, but it’s not really WebOb’s responsibility to turn `Location: //…` into an absolute URL where the value is treated as a path-absolute URL. In fact, since RFC 7231 (the latest HTTP/1.1 specification), `Location` doesn’t need to be absolute; WebOb shouldn’t touch it at all. Treating the value differently from clients is bound to cause confusion (e.g. when we move away from WebOb, would this have been a problem waiting to happen?). WebOb’s current fix is to alter the prefix to `/%2f`, which is *semantically different*. WebOb 1.8.8 also checks for the specific prefix `"//"`, but it still uses `urljoin`, which will happily cause exactly the same problem with `" //evil.example"` (substitute tab for space on older patches of Python, e.g. 3.10 before 3.10.12 (CVE-2023-24329)). That’s not an issue for what’s probably the most common way to have this open redirect vulnerability – what we did, echoing the request path – but, again, it’s not WebOb’s issue in general. Finally, we should probably just redirect to slash-normalized URLs globally before even getting to the Python layer, but… another time. --- weasyl/controllers/settings.py | 3 +-- weasyl/controllers/user.py | 5 ++--- weasyl/define.py | 7 +++++++ weasyl/forms.py | 13 ------------- weasyl/test/web/test_redirect.py | 22 ++++++++++++++++++++++ 5 files changed, 32 insertions(+), 18 deletions(-) create mode 100644 weasyl/test/web/test_redirect.py diff --git a/weasyl/controllers/settings.py b/weasyl/controllers/settings.py index d400b6dc2..898acb532 100644 --- a/weasyl/controllers/settings.py +++ b/weasyl/controllers/settings.py @@ -8,7 +8,6 @@ from weasyl.controllers.decorators import disallow_api, login_required, token_checked from weasyl.error import WeasylError -from weasyl.forms import checked_redirect from weasyl import ( api, avatar, banner, blocktag, collection, commishinfo, define, emailer, folder, followuser, frienduser, ignoreuser, @@ -803,6 +802,6 @@ def sfw_toggle_(request): currentstate = request.cookies.get('sfwmode', "nsfw") newstate = "sfw" if currentstate == "nsfw" else "nsfw" - response = HTTPSeeOther(location=checked_redirect(redirect)) + response = HTTPSeeOther(location=define.path_redirect(redirect)) response.set_cookie("sfwmode", newstate, max_age=60 * 60 * 24 * 365) return response diff --git a/weasyl/controllers/user.py b/weasyl/controllers/user.py index a583bd588..456e02980 100644 --- a/weasyl/controllers/user.py +++ b/weasyl/controllers/user.py @@ -20,7 +20,6 @@ token_checked, ) from weasyl.error import WeasylError -from weasyl.forms import checked_redirect from weasyl.sessions import create_session @@ -40,7 +39,7 @@ def signin_post_(request): logid, logerror = login.authenticate_bcrypt(form.username, form.password, request=request, ip_address=request.client_addr, user_agent=request.user_agent) if logid and logerror is None: - response = HTTPSeeOther(location=checked_redirect(referer)) + response = HTTPSeeOther(location=define.path_redirect(referer)) response.set_cookie('WZL', request.weasyl_session.sessionid, max_age=60 * 60 * 24 * 365, secure=request.scheme == 'https', httponly=True) if form.sfwmode == "sfw": @@ -142,7 +141,7 @@ def signin_2fa_auth_post_(request): links=[["2FA Dashboard", "/control/2fa/status"], ["Return to the Home Page", "/"]]) # Return to the target page, removing the scheme and domain per urlsplit. ref = request.POST["referer"] or "/" - response = HTTPSeeOther(location=checked_redirect(ref)) + response = HTTPSeeOther(location=define.path_redirect(ref)) response.set_cookie('WZL', request.weasyl_session.sessionid, max_age=60 * 60 * 24 * 365, secure=request.scheme == 'https', httponly=True) return response diff --git a/weasyl/define.py b/weasyl/define.py index 1e1a3964f..98c85293e 100644 --- a/weasyl/define.py +++ b/weasyl/define.py @@ -240,6 +240,13 @@ def is_csrf_valid(request): return request.headers.get('origin') == _ORIGIN +def path_redirect(path_qs: str) -> str: + """ + Return an absolute URL for an internal redirect within the application’s origin. + """ + return _ORIGIN + path_qs + + @region.cache_on_arguments(namespace='v3') def _get_all_config(userid): """ diff --git a/weasyl/forms.py b/weasyl/forms.py index def7e96d7..726e9d112 100644 --- a/weasyl/forms.py +++ b/weasyl/forms.py @@ -1,5 +1,4 @@ from typing import Iterable, TypeVar -from urllib.parse import urlsplit from weasyl.error import WeasylError @@ -30,15 +29,3 @@ def only(iterable: Iterable[_T]) -> _T: return x raise WeasylError("Unexpected") - - -def checked_redirect(path: str) -> str: - """ - Returns the passed URL, asserting that it’s a valid place to redirect back to within the same application (e.g. after logging in). - """ - split_result = urlsplit(path) - - if split_result.scheme or split_result.netloc: - raise WeasylError("Unexpected", level="warning") - - return path diff --git a/weasyl/test/web/test_redirect.py b/weasyl/test/web/test_redirect.py new file mode 100644 index 000000000..0d22bebf0 --- /dev/null +++ b/weasyl/test/web/test_redirect.py @@ -0,0 +1,22 @@ +import pytest + +from weasyl import define +from weasyl.test import db_utils + + +@pytest.mark.usefixtures("db", "cache") +def test_sfw_toggle(app, monkeypatch): + monkeypatch.setattr(define, "_ORIGIN", "http://localhost") + + user = db_utils.create_user() + app.set_cookie(*db_utils.create_session(user).split("=", 1)) + + resp = app.post("/control/sfwtoggle", { + "redirect": "/~referer", + }, status=303) + assert resp.headers["location"] == "http://localhost/~referer", "SFW toggle should redirect back to original page" + + resp = app.post("/control/sfwtoggle", { + "redirect": "//evil.example/", + }, status=303) + assert resp.headers["location"] == "http://localhost//evil.example/", "SFW toggle shouldn’t act as an open redirect"