Skip to content

Commit

Permalink
fix: CVE followup, do not serve relative files
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
maartenbreddels committed Jul 11, 2024
1 parent 681f69b commit 86646f5
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 14 deletions.
20 changes: 9 additions & 11 deletions solara/server/cdn_helper.py
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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)
Expand All @@ -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:
Expand Down Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions solara/server/flask.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)


Expand All @@ -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])

Expand Down
7 changes: 6 additions & 1 deletion solara/server/starlette.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
import math
import os
from pathlib import Path
import sys
import threading
import typing
Expand All @@ -14,6 +15,8 @@
import uvicorn.server
import websockets.legacy.http

from solara.server.utils import path_is_child_of

try:
import solara_enterprise

Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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)


Expand Down
7 changes: 7 additions & 0 deletions solara/server/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import contextlib
import logging
import os
from pathlib import Path
import pdb
import traceback

Expand All @@ -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
Expand Down
100 changes: 100 additions & 0 deletions tests/integration/server_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 + "/[email protected]/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

0 comments on commit 86646f5

Please sign in to comment.