From 4fc21f0450da807a28dc9e40f1c03e632037a3b1 Mon Sep 17 00:00:00 2001 From: claravox Date: Tue, 12 Dec 2023 15:39:52 +0100 Subject: [PATCH] YDA-5546: harden static loader Co-authored-by: Lazlo Westerhof --- setup.cfg | 2 +- yoda_eus/app.py | 44 +++++++++------ yoda_eus/mail.py | 2 +- yoda_eus/password_complexity.py | 2 +- yoda_eus/tests/test_integration.py | 2 +- yoda_eus/tests/test_unit.py | 89 +++++++++++++++++++++++++++++- yoda_eus/util.py | 52 +++++++++++++++++ 7 files changed, 170 insertions(+), 23 deletions(-) create mode 100644 yoda_eus/util.py 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 5960706..2590b62 100644 --- a/yoda_eus/app.py +++ b/yoda_eus/app.py @@ -1,13 +1,13 @@ #!/usr/bin/env python3 __copyright__ = 'Copyright (c) 2023, Utrecht University' -__license__ = 'GPLv3, see LICENSE' +__license__ = 'GPLv3, see LICENSE' import secrets 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() @@ -350,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 @@ -557,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: @@ -575,10 +583,13 @@ 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() -> Response: + def static_loader() -> Optional[Response]: """ Static files handling - recognisable through '/assets/' Override requested static file if present in user_static_area @@ -589,17 +600,18 @@ 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) + 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/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 a01a310..e0045c8 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' +__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..36d8912 --- /dev/null +++ b/yoda_eus/util.py @@ -0,0 +1,52 @@ +#!/usr/bin/env python3 + +__copyright__ = 'Copyright (c) 2021-2023, Utrecht University' +__license__ = 'GPLv3, see LICENSE' + +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 + + +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 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 None + + static_dir = safe_join(user_static_area + "/static", *parts) + if not static_dir: + 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) + + return static_dir, asset_name + + return None