From df2fd66a7f4e8ffd36e8678697a8a4f76760dc54 Mon Sep 17 00:00:00 2001 From: "Maarten A. Breddels" Date: Thu, 11 Jul 2024 13:34:04 +0200 Subject: [PATCH] fix: CVE followup, do not serve relative files We still served files above the directory of intent. We now test all endpoint, using both starlette and flask to make sure this will not happen again. --- solara/server/cdn_helper.py | 20 +++-- solara/server/flask.py | 7 +- solara/server/starlette.py | 7 +- solara/server/utils.py | 7 ++ tests/integration/apps/not-allowed | 1 + tests/integration/apps/public/test.txt | 1 + tests/integration/apps/secure/app.py | 4 + tests/integration/server_test.py | 100 +++++++++++++++++++++++++ 8 files changed, 133 insertions(+), 14 deletions(-) create mode 100644 tests/integration/apps/not-allowed create mode 100644 tests/integration/apps/public/test.txt create mode 100644 tests/integration/apps/secure/app.py diff --git a/solara/server/cdn_helper.py b/solara/server/cdn_helper.py index 614cc18ea..69c085a14 100644 --- a/solara/server/cdn_helper.py +++ b/solara/server/cdn_helper.py @@ -1,9 +1,9 @@ import logging -import os import pathlib import shutil import requests +from solara.server.utils import path_is_child_of import solara.settings @@ -14,6 +14,9 @@ def put_in_cache(base_cache_dir: pathlib.Path, path, data: bytes): cache_path = base_cache_dir / path + if not path_is_child_of(cache_path, base_cache_dir): + raise PermissionError("Trying to write outside of cache directory") + pathlib.Path(cache_path.parent).mkdir(parents=True, exist_ok=True) try: logger.info("Writing cache file: %s", cache_path) @@ -27,16 +30,8 @@ def get_from_cache(base_cache_dir: pathlib.Path, path): # Make sure cache_path is a subdirectory of base_cache_dir # so we don't accidentally read files from the parent directory # which is a security risk. - # We use os.path.normpath() because we do not want to follow symlinks - # in editable installs, since some packages are symlinked - if not os.path.normpath(cache_path).startswith(os.path.normpath(base_cache_dir)): - logger.warning( - "Trying to read from outside of cache directory: %s is not a subdir of %s (%s - %s)", - cache_path, - base_cache_dir, - os.path.normpath(cache_path), - os.path.normpath(base_cache_dir), - ) + if not path_is_child_of(cache_path, base_cache_dir): + logger.warning("Trying to read from outside of cache directory: %s is not a subdir of %s", cache_path, base_cache_dir) raise PermissionError("Trying to read from outside of cache directory") try: @@ -74,6 +69,9 @@ def get_path(base_cache_dir: pathlib.Path, path) -> pathlib.Path: store_path = path if len(parts) != 1 else pathlib.Path(path) / "__main.js" cache_path = base_cache_dir / store_path + if not path_is_child_of(cache_path, base_cache_dir): + raise PermissionError("Trying to read from outside of cache directory") + if cache_path.exists(): # before d7eba856f100d5c3c64f4eec22c62390f084cb40 on windows, we could # accidentally write to the cache directory, so we need to check if we still diff --git a/solara/server/flask.py b/solara/server/flask.py index 6b659a55c..d8f48e2be 100644 --- a/solara/server/flask.py +++ b/solara/server/flask.py @@ -199,7 +199,7 @@ def nbext(dir, filename): for directory in server.nbextensions_directories: file = directory / dir / filename if file.exists(): - return send_from_directory(directory / dir, filename) + return send_from_directory(directory, dir + os.path.sep + filename) return flask.Response("not found", status=404) @@ -217,7 +217,10 @@ def cdn(path): if not allowed(): abort(401) cache_directory = settings.assets.proxy_cache_dir - content = cdn_helper.get_data(Path(cache_directory), path) + try: + content = cdn_helper.get_data(Path(cache_directory), path) + except PermissionError: + return flask.Response("not found", status=404) mime = mimetypes.guess_type(path) return flask.Response(content, mimetype=mime[0]) diff --git a/solara/server/starlette.py b/solara/server/starlette.py index 299d3a9a3..a62c4a1d1 100644 --- a/solara/server/starlette.py +++ b/solara/server/starlette.py @@ -3,6 +3,7 @@ import logging import math import os +from pathlib import Path import sys import threading import typing @@ -14,6 +15,8 @@ import uvicorn.server import websockets.legacy.http +from solara.server.utils import path_is_child_of + try: import solara_enterprise @@ -405,6 +408,9 @@ def lookup_path(self, path: str) -> typing.Tuple[str, typing.Optional[os.stat_re original_path = os.path.join(directory, path) full_path = os.path.realpath(original_path) directory = os.path.realpath(directory) + # return early if someone tries to access a file outside of the directory + if not path_is_child_of(Path(original_path), Path(directory)): + return "", None try: return full_path, os.stat(full_path) except (FileNotFoundError, NotADirectoryError): @@ -449,7 +455,6 @@ def lookup_path(self, path: str) -> typing.Tuple[str, typing.Optional[os.stat_re full_path = str(get_path(settings.assets.proxy_cache_dir, path)) except Exception: return "", None - return full_path, os.stat(full_path) diff --git a/solara/server/utils.py b/solara/server/utils.py index 42f1a3d47..2f718942f 100644 --- a/solara/server/utils.py +++ b/solara/server/utils.py @@ -1,6 +1,7 @@ import contextlib import logging import os +from pathlib import Path import pdb import traceback @@ -16,6 +17,12 @@ def start_error(title, msg, exception: Exception = None): os._exit(-1) +def path_is_child_of(path: Path, parent: Path) -> bool: + # We use os.path.normpath() because we do not want to follow symlinks + # in editable installs, since some packages are symlinked + return os.path.normpath(path).startswith(os.path.normpath(parent)) + + @contextlib.contextmanager def pdb_guard(): from . import settings diff --git a/tests/integration/apps/not-allowed b/tests/integration/apps/not-allowed new file mode 100644 index 000000000..d3730bda0 --- /dev/null +++ b/tests/integration/apps/not-allowed @@ -0,0 +1 @@ +not accessible diff --git a/tests/integration/apps/public/test.txt b/tests/integration/apps/public/test.txt new file mode 100644 index 000000000..9daeafb98 --- /dev/null +++ b/tests/integration/apps/public/test.txt @@ -0,0 +1 @@ +test diff --git a/tests/integration/apps/secure/app.py b/tests/integration/apps/secure/app.py new file mode 100644 index 000000000..68986fc0d --- /dev/null +++ b/tests/integration/apps/secure/app.py @@ -0,0 +1,4 @@ +import solara + + +page = solara.Button("Click me") diff --git a/tests/integration/server_test.py b/tests/integration/server_test.py index 93a332070..5126f1274 100644 --- a/tests/integration/server_test.py +++ b/tests/integration/server_test.py @@ -5,8 +5,11 @@ import playwright.sync_api import pytest import reacton.ipywidgets as w +import requests import solara +import solara.server.server +from solara.server import settings HERE = Path(__file__).parent @@ -211,3 +214,100 @@ def test_kernel_asyncio(browser: playwright.sync_api.Browser, solara_server, sol context1.close() context2.close() solara.server.settings.kernel.threaded = threaded + + +def test_cdn_secure(solara_server, solara_app, extra_include_path): + cdn_url = solara_server.base_url + "/_solara/cdn" + assert solara.settings.assets.proxy + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = cdn_url + "/vue-grid-layout@1.0.2/dist/vue-grid-layout.min.js" + response = requests.get(url) + assert response.status_code == 200 + # create a file in /share/solara + test_file = settings.assets.proxy_cache_dir.parent / "not-allowed" + test_file.write_text("test") + url = cdn_url + "/..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + +def test_nbextension_secure(solara_server, solara_app, extra_include_path): + nbextensions_url = solara_server.base_url + "/static/nbextensions" + nbextensions_directories = [k for k in solara.server.server.nbextensions_directories if k.exists()] + assert nbextensions_directories, "we should at least test one directory" + nbextensions_directory = nbextensions_directories[0] + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = nbextensions_url + "/jupyter-vuetify/nodeps.js" + response = requests.get(url) + assert response.status_code == 200 + test_file = nbextensions_directory.parent / "not-allowed" + test_file.write_text("test") + url = nbextensions_url + "/..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + url = nbextensions_url + "/foo/..%2f..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + +def test_assets_secure(solara_server, solara_app, extra_include_path): + assets_url = solara_server.base_url + "/static/assets" + assets_directory = solara.server.server.solara_static.parent / "assets" + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = assets_url + "/theme.js" + response = requests.get(url) + assert response.status_code == 200 + test_file = assets_directory.parent / "__init__.py" + assert test_file.exists() + url = assets_url + "/..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404 + + url = assets_url + "/foo/..%2f..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404 + + +def test_public_secure(solara_server, solara_app, extra_include_path): + public_url = solara_server.base_url + "/static/public" + + with solara_app(str(HERE / "apps/secure/app.py")): + apps = list(solara.server.app.apps.values()) + assert len(apps) == 1 + app = apps[0] + public_directory = app.directory.parent / "public" + url = public_url + "/test.txt" + response = requests.get(url) + assert response.status_code == 200 + test_file = public_directory.parent / "not-allowed" + assert test_file.exists() + url = public_url + "/..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + url = public_url + "/foo/..%2f..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + +def test_static_secure(solara_server, solara_app, extra_include_path): + static_url = solara_server.base_url + "/static" + static_directory = solara.server.server.solara_static + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = static_url + "/main.js" + response = requests.get(url) + assert response.status_code == 200 + test_file = static_directory.parent / "__init__.py" + assert test_file.exists() + url = static_url + "/..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404 + + url = static_url + "/foo/..%2f..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404