From 3f67061aa358bf27460d1e0688c32417369c8a76 Mon Sep 17 00:00:00 2001
From: Julian Rother <julianr@fsmpi.rwth-aachen.de>
Date: Thu, 29 Jul 2021 21:00:33 +0200
Subject: [PATCH] Limit url parameter-based redirects to local urls
---
uffd/__init__.py | 3 ++-
uffd/invite/templates/invite/use.html | 4 ++--
uffd/mfa/views.py | 7 ++++---
uffd/oauth2/views.py | 11 ++++++-----
uffd/secure_redirect.py | 7 +++++++
uffd/session/views.py | 11 ++++++-----
uffd/templates/base.html | 2 +-
7 files changed, 28 insertions(+), 17 deletions(-)
create mode 100644 uffd/secure_redirect.py
diff --git a/uffd/__init__.py b/uffd/__init__.py
index 989ddab8..293b47a1 100644
--- a/uffd/__init__.py
+++ b/uffd/__init__.py
@@ -17,6 +17,7 @@ from uffd.database import db, SQLAlchemyJSON
import uffd.ldap
from uffd.template_helper import register_template_helper
from uffd.navbar import setup_navbar
+from uffd.secure_redirect import secure_local_redirect
# pylint: enable=wrong-import-position
def load_config_file(app, cfg_name, silent=False):
@@ -97,7 +98,7 @@ def create_app(test_config=None): # pylint: disable=too-many-locals
@app.route('/lang', methods=['POST'])
def setlang(): #pylint: disable=unused-variable
- resp = redirect(request.values.get('ref', '/'))
+ resp = secure_local_redirect(request.values.get('ref', '/'))
if 'lang' in request.values:
resp.set_cookie('language', request.values['lang'])
return resp
diff --git a/uffd/invite/templates/invite/use.html b/uffd/invite/templates/invite/use.html
index 4d5b53ad..c852f3db 100644
--- a/uffd/invite/templates/invite/use.html
+++ b/uffd/invite/templates/invite/use.html
@@ -33,7 +33,7 @@
<form method="POST" action="{{ url_for("invite.grant", token=invite.token) }}" class="mb-2">
<button type="submit" class="btn btn-primary btn-block">Add the roles to your account now</button>
</form>
- <a href="{{ url_for("session.logout", ref=url_for("session.login", ref=request.url)) }}" class="btn btn-secondary btn-block">Logout and switch to a different account</a>
+ <a href="{{ url_for("session.logout", ref=url_for("session.login", ref=request.full_path)) }}" class="btn btn-secondary btn-block">Logout and switch to a different account</a>
{% endif %}
{% if invite.allow_signup %}
<a href="{{ url_for("session.logout", ref=url_for("invite.signup_start", token=invite.token)) }}" class="btn btn-secondary btn-block">Logout to register a new account</a>
@@ -43,7 +43,7 @@
<a href="{{ url_for("invite.signup_start", token=invite.token) }}" class="btn btn-primary btn-block">Register a new account</a>
{% endif %}
{% if invite.roles %}
- <a href="{{ url_for("session.login", ref=request.url) }}" class="btn btn-primary btn-block">Login and add the roles to your account</a>
+ <a href="{{ url_for("session.login", ref=request.full_path) }}" class="btn btn-primary btn-block">Login and add the roles to your account</a>
{% endif %}
{% endif %}
</div>
diff --git a/uffd/mfa/views.py b/uffd/mfa/views.py
index 133df968..287b4ab0 100644
--- a/uffd/mfa/views.py
+++ b/uffd/mfa/views.py
@@ -10,6 +10,7 @@ from uffd.mfa.models import MFAMethod, TOTPMethod, WebauthnMethod, RecoveryCodeM
from uffd.session.views import login_required, login_required_pre_mfa, set_request_user
from uffd.user.models import User
from uffd.csrf import csrf_protect
+from uffd.secure_redirect import secure_local_redirect
from uffd.ratelimit import Ratelimit, format_delay
bp = Blueprint('mfa', __name__, template_folder='templates', url_prefix='/mfa/')
@@ -213,7 +214,7 @@ def auth():
session['user_mfa'] = True
set_request_user()
if session.get('user_mfa'):
- return redirect(request.values.get('ref', url_for('index')))
+ return secure_local_redirect(request.values.get('ref', url_for('index')))
return render_template('mfa/auth.html', ref=request.values.get('ref'))
@bp.route('/auth', methods=['POST'])
@@ -227,7 +228,7 @@ def auth_finish():
if method.verify(request.form['code']):
session['user_mfa'] = True
set_request_user()
- return redirect(request.values.get('ref', url_for('index')))
+ return secure_local_redirect(request.values.get('ref', url_for('index')))
for method in request.user_pre_mfa.mfa_recovery_codes:
if method.verify(request.form['code']):
db.session.delete(method)
@@ -240,7 +241,7 @@ def auth_finish():
if len(request.user_pre_mfa.mfa_recovery_codes) <= 5:
flash(_('You only have a few recovery codes remaining. Make sure to generate new ones before they run out.'))
return redirect(url_for('mfa.setup'))
- return redirect(request.values.get('ref', url_for('index')))
+ return secure_local_redirect(request.values.get('ref', url_for('index')))
mfa_ratelimit.log(request.user_pre_mfa.dn)
flash(_('Two-factor authentication failed'))
return redirect(url_for('mfa.auth', ref=request.values.get('ref')))
diff --git a/uffd/oauth2/views.py b/uffd/oauth2/views.py
index 30957deb..ed39aecd 100644
--- a/uffd/oauth2/views.py
+++ b/uffd/oauth2/views.py
@@ -8,6 +8,7 @@ from sqlalchemy.exc import IntegrityError
from uffd.ratelimit import host_ratelimit, format_delay
from uffd.database import db
+from uffd.secure_redirect import secure_local_redirect
from uffd.session.models import DeviceLoginConfirmation
from .models import OAuth2Client, OAuth2Grant, OAuth2Token, OAuth2DeviceLoginInitiation
@@ -91,7 +92,7 @@ def authorize(*args, **kwargs): # pylint: disable=unused-argument
host_delay = host_ratelimit.get_delay()
if host_delay:
flash('We received too many requests from your ip address/network! Please wait at least %s.'%format_delay(host_delay))
- return redirect(url_for('session.login', ref=request.url, devicelogin=True))
+ return redirect(url_for('session.login', ref=request.full_path, devicelogin=True))
host_ratelimit.log()
initiation = OAuth2DeviceLoginInitiation(oauth2_client_id=client.client_id)
db.session.add(initiation)
@@ -102,7 +103,7 @@ def authorize(*args, **kwargs): # pylint: disable=unused-argument
return redirect(url_for('session.login', ref=request.values['ref'], devicelogin=True))
session['devicelogin_id'] = initiation.id
session['devicelogin_secret'] = initiation.secret
- return redirect(url_for('session.devicelogin', ref=request.url))
+ return redirect(url_for('session.devicelogin', ref=request.full_path))
elif 'devicelogin_id' in session and 'devicelogin_secret' in session and 'devicelogin_confirmation' in session:
initiation = OAuth2DeviceLoginInitiation.query.filter_by(id=session['devicelogin_id'], secret=session['devicelogin_secret'],
oauth2_client_id=client.client_id).one_or_none()
@@ -112,12 +113,12 @@ def authorize(*args, **kwargs): # pylint: disable=unused-argument
del session['devicelogin_confirmation']
if not initiation or initiation.expired or not confirmation:
flash('Device login failed')
- return redirect(url_for('session.login', ref=request.url, devicelogin=True))
+ return redirect(url_for('session.login', ref=request.full_path, devicelogin=True))
request.oauth2_user = confirmation.user
db.session.delete(initiation)
db.session.commit()
else:
- return redirect(url_for('session.login', ref=request.url, devicelogin=True))
+ return redirect(url_for('session.login', ref=request.full_path, devicelogin=True))
# Here we would normally ask the user, if he wants to give the requesting
# service access to his data. Since we only have trusted services (the
# clients defined in the server config), we don't ask for consent.
@@ -163,7 +164,7 @@ def inject_logout_params(endpoint, values):
@bp.route('/logout')
def logout():
if not request.values.get('client_ids'):
- return redirect(request.values.get('ref', '/'))
+ return secure_local_redirect(request.values.get('ref', '/'))
client_ids = request.values['client_ids'].split(',')
clients = [OAuth2Client.from_id(client_id) for client_id in client_ids]
return render_template('oauth2/logout.html', clients=clients)
diff --git a/uffd/secure_redirect.py b/uffd/secure_redirect.py
new file mode 100644
index 00000000..69fa67cc
--- /dev/null
+++ b/uffd/secure_redirect.py
@@ -0,0 +1,7 @@
+from flask import redirect
+
+def secure_local_redirect(target):
+ # Reject URLs that include a scheme or host part
+ if not target.startswith('/') or target.startswith('//'):
+ target = '/'
+ return redirect(target)
diff --git a/uffd/session/views.py b/uffd/session/views.py
index 8d095a2c..b558b274 100644
--- a/uffd/session/views.py
+++ b/uffd/session/views.py
@@ -7,6 +7,7 @@ from flask_babel import gettext as _
from uffd.database import db
from uffd.csrf import csrf_protect
+from uffd.secure_redirect import secure_local_redirect
from uffd.user.models import User
from uffd.ldap import ldap, test_user_bind, LDAPInvalidDnError, LDAPBindError, LDAPPasswordIsMandatoryError
from uffd.ratelimit import Ratelimit, host_ratelimit, format_delay
@@ -111,7 +112,7 @@ def login_required_pre_mfa(no_redirect=False):
if no_redirect:
abort(403)
flash(_('You need to login first'))
- return redirect(url_for('session.login', ref=request.url))
+ return redirect(url_for('session.login', ref=request.full_path))
return func(*args, **kwargs)
return decorator
return wrapper
@@ -122,9 +123,9 @@ def login_required(group=None):
def decorator(*args, **kwargs):
if not request.user_pre_mfa:
flash(_('You need to login first'))
- return redirect(url_for('session.login', ref=request.url))
+ return redirect(url_for('session.login', ref=request.full_path))
if not request.user:
- return redirect(url_for('mfa.auth', ref=request.url))
+ return redirect(url_for('mfa.auth', ref=request.full_path))
if not request.user.is_in_group(group):
flash(_('Access denied'))
return redirect(url_for('index'))
@@ -135,7 +136,7 @@ def login_required(group=None):
@bp.route("/login/device/start")
def devicelogin_start():
session['devicelogin_started'] = True
- return redirect(request.values['ref'])
+ return secure_local_redirect(request.values['ref'])
@bp.route("/login/device")
def devicelogin():
@@ -160,7 +161,7 @@ def devicelogin_submit():
flash('Invalid confirmation code')
return render_template('session/devicelogin.html', ref=request.values.get('ref'), initiation=initiation)
session['devicelogin_confirmation'] = confirmation.id
- return redirect(request.values['ref'])
+ return secure_local_redirect(request.values['ref'])
@bp.route("/device")
@login_required()
diff --git a/uffd/templates/base.html b/uffd/templates/base.html
index f07351bc..58ef8d72 100644
--- a/uffd/templates/base.html
+++ b/uffd/templates/base.html
@@ -74,7 +74,7 @@
{% if config['LANGUAGES']|length > 1 %}
<li class="nav-item">
<form class="language-switch py-2 pr-1" method="POST" style="margin-left: -5px;" action="{{ url_for('setlang') }}">
- <input type="hidden" name="ref" value="{{ request.url }}">
+ <input type="hidden" name="ref" value="{{ request.full_path }}">
<select name="lang" class="bg-dark" style="border: 0px; color: rgba(255, 255, 255, 0.5);" onchange="$('.language-switch').submit()">
{% for identifier, name in config['LANGUAGES'].items() %}
<option value="{{ identifier }}" {{ 'selected' if identifier == get_locale() }}>{{ name }}</option>
--
GitLab