From b2ea97b479f860c5b8b3f945d841957daa1758f4 Mon Sep 17 00:00:00 2001 From: claravox Date: Mon, 11 Dec 2023 13:26:44 +0100 Subject: [PATCH 1/4] YDA-5546: harden static loader --- yoda_eus/app.py | 17 +++---- yoda_eus/tests/test_unit.py | 91 +++++++++++++++++++++++++++++++++++-- yoda_eus/util.py | 43 ++++++++++++++++++ 3 files changed, 136 insertions(+), 15 deletions(-) create mode 100644 yoda_eus/util.py diff --git a/yoda_eus/app.py b/yoda_eus/app.py index 5960706..8030c4c 100644 --- a/yoda_eus/app.py +++ b/yoda_eus/app.py @@ -7,7 +7,7 @@ import urllib.parse from datetime import datetime from os import path -from typing import Any, Dict +from typing import Any, Dict, Optional import bcrypt from flask import abort, Flask, jsonify, make_response, render_template, request, Response, send_from_directory @@ -17,6 +17,7 @@ from jinja2 import ChoiceLoader, FileSystemLoader from yoda_eus.mail import is_email_valid, send_email_template_if_needed from yoda_eus.password_complexity import check_password_complexity +from yoda_eus.util import get_validated_static_path db = SQLAlchemy() @@ -578,7 +579,7 @@ def check_api_secret() -> Response: abort(make_response(jsonify({'status': 'error', 'message': 'EUS secret header not present or does not match.'}), 403)) @app.before_request - def static_loader() -> Response: + def static_loader() -> Optional[Response]: """ Static files handling - recognisable through '/assets/' Override requested static file if present in user_static_area @@ -589,15 +590,9 @@ def static_loader() -> Response: :returns: Static file """ - if request.full_path.split('/')[1] == 'assets': - user_static_area = path.join(app.config.get('YODA_THEME_PATH'), app.config.get('YODA_THEME')) - asset_dir, asset_name = path.split(request.path) - static_dir = asset_dir.replace('/assets', user_static_area + '/static') - user_static_filename = path.join(static_dir, asset_name) - - if not path.exists(user_static_filename): - static_dir = asset_dir.replace('/assets', '/var/www/yoda/static') - + result = get_validated_static_path(request.full_path, request.path, app.config.get('YODA_THEME_PATH'), app.config.get('YODA_THEME')) + if result is not None: + static_dir, asset_name = result return send_from_directory(static_dir, asset_name) @ app.url_defaults diff --git a/yoda_eus/tests/test_unit.py b/yoda_eus/tests/test_unit.py index a01a310..0624148 100644 --- a/yoda_eus/tests/test_unit.py +++ b/yoda_eus/tests/test_unit.py @@ -1,14 +1,15 @@ -__copyright__ = 'Copyright (c) 2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__copyright__ = "Copyright (c) 2023, Utrecht University" +__license__ = "GPLv3, see LICENSE" import string +from unittest.mock import patch from yoda_eus.mail import is_email_valid from yoda_eus.password_complexity import check_password_complexity +from yoda_eus.util import get_validated_static_path class TestMain: - def test_password_validation_ok(self): result = check_password_complexity("Test123456789!") assert len(result) == 0 @@ -50,10 +51,92 @@ def test_password_validation_multiple(self): assert len(result) == 3 assert "Password is too short: it needs to be at least 10 characters." in result assert "Password needs to contain at least one digit." in result - assert "Password needs to contain at least one punctuation character ({})".format(string.punctuation) in result + assert ( + "Password needs to contain at least one punctuation character ({})".format( + string.punctuation + ) + in result + ) def is_email_valid_yes(self): assert is_email_valid("yoda@uu.nl") def is_email_valid_no(self): assert not is_email_valid("this is not a valid email address") + + def exists_return_value(self, pathname): + """ Mock path.exists function. True if path does not contain "theme" and "uu" """ + return not ("theme" in pathname and "uu" in pathname) + + @patch("os.path.exists") + def test_static_loader_valid_path(self, mock_exists): + mock_exists.side_effect = self.exists_return_value + # uu theme + static_dir, asset_name = get_validated_static_path( + "/assets/img/logo.svg?wekr", + "/assets/img/logo.svg", + "/var/www/yoda/themes", + "uu", + ) + assert static_dir == "/var/www/yoda/static/img" + assert asset_name == "logo.svg" + # other theme + static_dir, asset_name = get_validated_static_path( + "/assets/img/logo.svg?wekr", + "/assets/img/logo.svg", + "/var/www/yoda/themes", + "wur", + ) + assert static_dir == "/var/www/yoda/themes/wur/static/img" + assert asset_name == "logo.svg" + + @patch("os.path.exists") + def test_static_loader_invalid_path(self, mock_exists): + mock_exists.side_effect = self.exists_return_value + # Too short + assert ( + get_validated_static_path("/?sawerw", "/", "/var/www/yoda/themes", "uu") + is None + ) + # Path traversal attack + assert ( + get_validated_static_path( + "/assets/../../../../etc/passwd?werwrwr", + "/assets/../../../../etc/passwd", + "/var/www/yoda/themes", + "uu", + ) + is None + ) + # non-printable characters + full_path = "/assets/" + chr(13) + "img/logo.svg?werwer" + path = "/assets/" + chr(13) + "img/logo.svg" + assert ( + get_validated_static_path(full_path, path, "/var/www/yoda/themes", "uu") + is None + ) + assert ( + get_validated_static_path(full_path, path, "/var/www/yoda/themes", "wur") + is None + ) + # non-printable characters in asset name + full_path = "/assets/img/l" + chr(13) + "ogo.svg?werwer" + path = "/assets/img/l" + chr(13) + "ogo.svg" + assert ( + get_validated_static_path(full_path, path, "/var/www/yoda/themes", "uu") + is None + ) + assert ( + get_validated_static_path(full_path, path, "/var/www/yoda/themes", "wur") + is None + ) + # .. in file name + assert ( + get_validated_static_path( + "/assets/img/lo..go.svg?sklaerw", + "/assets/img/lo..go.svg?sklaerw", + "/var/www/yoda/themes", + "uu", + ) + is None + ) diff --git a/yoda_eus/util.py b/yoda_eus/util.py new file mode 100644 index 0000000..71b0d8b --- /dev/null +++ b/yoda_eus/util.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python3 + +__copyright__ = 'Copyright (c) 2021-2023, Utrecht University' +__license__ = 'GPLv3, see LICENSE' + +from typing import Optional, Tuple +from os import path +from re import fullmatch +from werkzeug.security import safe_join +from werkzeug.utils import secure_filename + + +def get_validated_static_path(full_path, request_path, yoda_theme_path, yoda_theme) -> Optional[Tuple[str, str]]: + """ + Static files handling - recognisable through '/assets/' + Confirms that input path is valid and return corresponding static path + + :param full_path: Full path of request + :param request_path: Short path of request + :param yoda_theme_path: Path to the yoda themes + :param yoda_theme: Name of the chosen theme + + :returns: Tuple of static directory and filename for correct path, None for incorrect path + """ + parts = full_path.split('/') + + if len(parts) > 2 and fullmatch('[ -~]*', full_path) is not None and parts[1] == 'assets': + parts = parts[2:-1] + user_static_area = path.join(yoda_theme_path, yoda_theme) + _, asset_name = path.split(request_path) + # Confirm that asset_name is safe + if asset_name != secure_filename(asset_name): + return + + static_dir = safe_join(user_static_area + '/static', *parts) + if not static_dir: + return + user_static_filename = path.join(static_dir, asset_name) + + if not path.exists(user_static_filename): + static_dir = safe_join('/var/www/yoda/static', *parts) + + return static_dir, asset_name From f36a6c8d72cee2b0d7b3aa4a3885665dc30c6c37 Mon Sep 17 00:00:00 2001 From: claravox Date: Mon, 11 Dec 2023 13:46:17 +0100 Subject: [PATCH 2/4] Linting, copying yoda-portal linting style --- setup.cfg | 2 +- yoda_eus/app.py | 25 ++++++++++++++++++++----- yoda_eus/util.py | 3 ++- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/setup.cfg b/setup.cfg index 66c2fe2..5a52900 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,5 +1,5 @@ [flake8] -ignore=E501 +ignore=E221,E241,E402,W503,W605,F403,F405 import-order-style=smarkets strictness=short docstring_style=sphinx diff --git a/yoda_eus/app.py b/yoda_eus/app.py index 8030c4c..3974cb0 100644 --- a/yoda_eus/app.py +++ b/yoda_eus/app.py @@ -351,7 +351,10 @@ def process_forgot_password() -> Response: return response if (not is_email_valid(username) and app.config.get("MAIL_ONLY_TO_VALID_ADDRESS").lower() == "true"): - errors = {"errors": ["Unable to send password reset email, because your user name ('{}') is not a valid email address.".format(username)]} + errors = { + "errors": ["Unable to send password reset email, " + "because your user name ('{}') is not a valid email address.".format(username)] + } response = make_response(render_template('forgot-password.html', **errors)) response.status_code = 404 return response @@ -558,10 +561,14 @@ def refuse_api_requests() -> Response: The EUS presents two web interfaces (vhosts) on two different TCP ports. One of these does not have the API available, so that API access can be restricted on a TCP level (e.g. in firewalls). - If this instance does not have the API enabled, access to API functions is intercepted and blocked by this function. + If this instance does not have the API enabled, access to API functions is intercepted + and blocked by this function. """ if request.path.startswith("/api/"): - abort(make_response(jsonify({'status': 'error', 'message': 'The EUS API has been disabled on this interface.'}), 403)) + abort(make_response(jsonify({ + 'status': 'error', + 'message': 'The EUS API has been disabled on this interface.' + }), 403)) @app.before_request def check_api_secret() -> Response: @@ -576,7 +583,10 @@ def check_api_secret() -> Response: elif secret_header in request.headers and request.headers[secret_header] == app.config.get("API_SECRET"): return else: - abort(make_response(jsonify({'status': 'error', 'message': 'EUS secret header not present or does not match.'}), 403)) + abort(make_response(jsonify({ + 'status': 'error', + 'message': 'EUS secret header not present or does not match.' + }), 403)) @app.before_request def static_loader() -> Optional[Response]: @@ -590,7 +600,12 @@ def static_loader() -> Optional[Response]: :returns: Static file """ - result = get_validated_static_path(request.full_path, request.path, app.config.get('YODA_THEME_PATH'), app.config.get('YODA_THEME')) + result = get_validated_static_path( + request.full_path, + request.path, + app.config.get('YODA_THEME_PATH'), + app.config.get('YODA_THEME') + ) if result is not None: static_dir, asset_name = result return send_from_directory(static_dir, asset_name) diff --git a/yoda_eus/util.py b/yoda_eus/util.py index 71b0d8b..19393a7 100644 --- a/yoda_eus/util.py +++ b/yoda_eus/util.py @@ -3,9 +3,10 @@ __copyright__ = 'Copyright (c) 2021-2023, Utrecht University' __license__ = 'GPLv3, see LICENSE' -from typing import Optional, Tuple from os import path from re import fullmatch +from typing import Optional, Tuple + from werkzeug.security import safe_join from werkzeug.utils import secure_filename From 7b02b41a8d4b6f9b9cef9977dad9f2915a09b1b6 Mon Sep 17 00:00:00 2001 From: claravox Date: Mon, 11 Dec 2023 14:12:41 +0100 Subject: [PATCH 3/4] mypy type formatting, linting --- yoda_eus/app.py | 2 ++ yoda_eus/util.py | 32 ++++++++++++++++++++------------ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/yoda_eus/app.py b/yoda_eus/app.py index 3974cb0..1cc84f5 100644 --- a/yoda_eus/app.py +++ b/yoda_eus/app.py @@ -610,6 +610,8 @@ def static_loader() -> Optional[Response]: static_dir, asset_name = result return send_from_directory(static_dir, asset_name) + return None + @ app.url_defaults def add_cache_buster(endpoint: str, values: Dict[str, str]) -> None: """Add cache buster to asset (static) URLs.""" diff --git a/yoda_eus/util.py b/yoda_eus/util.py index 19393a7..65ff776 100644 --- a/yoda_eus/util.py +++ b/yoda_eus/util.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 -__copyright__ = 'Copyright (c) 2021-2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__copyright__ = "Copyright (c) 2021-2023, Utrecht University" +__license__ = "GPLv3, see LICENSE" from os import path from re import fullmatch @@ -11,34 +11,42 @@ from werkzeug.utils import secure_filename -def get_validated_static_path(full_path, request_path, yoda_theme_path, yoda_theme) -> Optional[Tuple[str, str]]: +def get_validated_static_path( + full_path: str, request_path: str, yoda_theme_path: str, yoda_theme: str +) -> Optional[Tuple[str, str]]: """ Static files handling - recognisable through '/assets/' Confirms that input path is valid and return corresponding static path - :param full_path: Full path of request - :param request_path: Short path of request + :param full_path: Full path of request + :param request_path: Short path of request :param yoda_theme_path: Path to the yoda themes - :param yoda_theme: Name of the chosen theme + :param yoda_theme: Name of the chosen theme :returns: Tuple of static directory and filename for correct path, None for incorrect path """ - parts = full_path.split('/') + parts = full_path.split("/") - if len(parts) > 2 and fullmatch('[ -~]*', full_path) is not None and parts[1] == 'assets': + if ( + len(parts) > 2 + and fullmatch("[ -~]*", full_path) is not None + and parts[1] == "assets" + ): parts = parts[2:-1] user_static_area = path.join(yoda_theme_path, yoda_theme) _, asset_name = path.split(request_path) # Confirm that asset_name is safe if asset_name != secure_filename(asset_name): - return + return None - static_dir = safe_join(user_static_area + '/static', *parts) + static_dir = safe_join(user_static_area + "/static", *parts) if not static_dir: - return + return None user_static_filename = path.join(static_dir, asset_name) if not path.exists(user_static_filename): - static_dir = safe_join('/var/www/yoda/static', *parts) + static_dir = safe_join("/var/www/yoda/static", *parts) return static_dir, asset_name + + return None From 7a1cc1ab378ab970163bc4144e1ffff684be4063 Mon Sep 17 00:00:00 2001 From: Lazlo Westerhof Date: Tue, 12 Dec 2023 15:38:48 +0100 Subject: [PATCH 4/4] Update copyright --- yoda_eus/app.py | 2 +- yoda_eus/mail.py | 2 +- yoda_eus/password_complexity.py | 2 +- yoda_eus/tests/test_integration.py | 2 +- yoda_eus/tests/test_unit.py | 4 ++-- yoda_eus/util.py | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/yoda_eus/app.py b/yoda_eus/app.py index 1cc84f5..2590b62 100644 --- a/yoda_eus/app.py +++ b/yoda_eus/app.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 __copyright__ = 'Copyright (c) 2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__license__ = 'GPLv3, see LICENSE' import secrets import urllib.parse diff --git a/yoda_eus/mail.py b/yoda_eus/mail.py index 991ef8f..13208b3 100644 --- a/yoda_eus/mail.py +++ b/yoda_eus/mail.py @@ -1,5 +1,5 @@ __copyright__ = 'Copyright (c) 2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__license__ = 'GPLv3, see LICENSE' import email import os diff --git a/yoda_eus/password_complexity.py b/yoda_eus/password_complexity.py index 2ab51b6..87a0b59 100644 --- a/yoda_eus/password_complexity.py +++ b/yoda_eus/password_complexity.py @@ -1,5 +1,5 @@ __copyright__ = 'Copyright (c) 2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__license__ = 'GPLv3, see LICENSE' import string from typing import List diff --git a/yoda_eus/tests/test_integration.py b/yoda_eus/tests/test_integration.py index 69abcc9..3009d96 100644 --- a/yoda_eus/tests/test_integration.py +++ b/yoda_eus/tests/test_integration.py @@ -1,5 +1,5 @@ __copyright__ = 'Copyright (c) 2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__license__ = 'GPLv3, see LICENSE' import base64 diff --git a/yoda_eus/tests/test_unit.py b/yoda_eus/tests/test_unit.py index 0624148..e0045c8 100644 --- a/yoda_eus/tests/test_unit.py +++ b/yoda_eus/tests/test_unit.py @@ -1,5 +1,5 @@ -__copyright__ = "Copyright (c) 2023, Utrecht University" -__license__ = "GPLv3, see LICENSE" +__copyright__ = 'Copyright (c) 2023, Utrecht University' +__license__ = 'GPLv3, see LICENSE' import string from unittest.mock import patch diff --git a/yoda_eus/util.py b/yoda_eus/util.py index 65ff776..36d8912 100644 --- a/yoda_eus/util.py +++ b/yoda_eus/util.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 -__copyright__ = "Copyright (c) 2021-2023, Utrecht University" -__license__ = "GPLv3, see LICENSE" +__copyright__ = 'Copyright (c) 2021-2023, Utrecht University' +__license__ = 'GPLv3, see LICENSE' from os import path from re import fullmatch