From f870c52398a55b9b5189932dd8caa24efb4bc1e1 Mon Sep 17 00:00:00 2001 From: Matt Molyneaux Date: Wed, 17 Jun 2020 21:57:49 +0100 Subject: [PATCH] Merge pull request from GHSA-6r3c-8xf3-ggrr * Fix directory traversal bug First try for the fix. Sanitization is done always on the url path. Removed _convert_file_to_url() * Restore use of _convert_file_to_url This is now done for all backends, assuming that SENDFILE_URL is set Also don't keep converting from Path object to str and back again * Fix example application so it works again * Update docs Co-authored-by: Gianluca Pacchiella --- .gitignore | 1 + django_sendfile/backends/_internalredirect.py | 16 ---- django_sendfile/backends/mod_wsgi.py | 2 +- django_sendfile/backends/nginx.py | 5 +- django_sendfile/backends/simple.py | 20 +++-- django_sendfile/tests.py | 79 ++++++++++++++++--- django_sendfile/utils.py | 60 ++++++++++++-- docs/backends.rst | 14 +--- docs/getting-started.rst | 11 ++- .../protected_downloads/download/models.py | 2 +- .../templates/download/download_list.html | 0 examples/protected_downloads/download/urls.py | 5 +- .../protected_downloads/download/views.py | 13 ++- examples/protected_downloads/settings.py | 21 ++++- 14 files changed, 178 insertions(+), 71 deletions(-) delete mode 100644 django_sendfile/backends/_internalredirect.py rename examples/protected_downloads/{ => download}/templates/download/download_list.html (100%) diff --git a/.gitignore b/.gitignore index b74ec6f..17dd736 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,4 @@ .tox /examples/protected_downloads/htmlcov /examples/protected_downloads/.coverage +/examples/protected_downloads/protected diff --git a/django_sendfile/backends/_internalredirect.py b/django_sendfile/backends/_internalredirect.py deleted file mode 100644 index 24bced6..0000000 --- a/django_sendfile/backends/_internalredirect.py +++ /dev/null @@ -1,16 +0,0 @@ -from urllib.parse import quote -import os.path - -from django.conf import settings - - -def _convert_file_to_url(filename): - relpath = os.path.relpath(filename, settings.SENDFILE_ROOT) - - url = [settings.SENDFILE_URL] - - while relpath: - relpath, head = os.path.split(relpath) - url.insert(1, head) - - return quote('/'.join(url)) diff --git a/django_sendfile/backends/mod_wsgi.py b/django_sendfile/backends/mod_wsgi.py index 7fcd104..07ba3f1 100644 --- a/django_sendfile/backends/mod_wsgi.py +++ b/django_sendfile/backends/mod_wsgi.py @@ -2,7 +2,7 @@ from django.http import HttpResponse -from ._internalredirect import _convert_file_to_url +from ..utils import _convert_file_to_url def sendfile(request, filename, **kwargs): diff --git a/django_sendfile/backends/nginx.py b/django_sendfile/backends/nginx.py index 29dc348..8764309 100644 --- a/django_sendfile/backends/nginx.py +++ b/django_sendfile/backends/nginx.py @@ -2,12 +2,11 @@ from django.http import HttpResponse -from ._internalredirect import _convert_file_to_url +from ..utils import _convert_file_to_url def sendfile(request, filename, **kwargs): response = HttpResponse() - url = _convert_file_to_url(filename) - response['X-Accel-Redirect'] = url.encode('utf-8') + response['X-Accel-Redirect'] = _convert_file_to_url(filename) return response diff --git a/django_sendfile/backends/simple.py b/django_sendfile/backends/simple.py index 11bc02c..0549b20 100644 --- a/django_sendfile/backends/simple.py +++ b/django_sendfile/backends/simple.py @@ -1,25 +1,29 @@ from email.utils import mktime_tz, parsedate_tz -import os import re -import stat from django.core.files.base import File from django.http import HttpResponse, HttpResponseNotModified from django.utils.http import http_date -def sendfile(request, filename, **kwargs): - # Respect the If-Modified-Since header. - statobj = os.stat(filename) +def sendfile(request, filepath, **kwargs): + '''Use the SENDFILE_ROOT value composed with the path arrived as argument + to build an absolute path with which resolve and return the file contents. + + If the path points to a file out of the root directory (should cover both + situations with '..' and symlinks) then a 404 is raised. + ''' + statobj = filepath.stat() + # Respect the If-Modified-Since header. if not was_modified_since(request.META.get('HTTP_IF_MODIFIED_SINCE'), - statobj[stat.ST_MTIME], statobj[stat.ST_SIZE]): + statobj.st_mtime, statobj.st_size): return HttpResponseNotModified() - with File(open(filename, 'rb')) as f: + with File(filepath.open('rb')) as f: response = HttpResponse(f.chunks()) - response["Last-Modified"] = http_date(statobj[stat.ST_MTIME]) + response["Last-Modified"] = http_date(statobj.st_mtime) return response diff --git a/django_sendfile/tests.py b/django_sendfile/tests.py index f95b915..71c6279 100644 --- a/django_sendfile/tests.py +++ b/django_sendfile/tests.py @@ -6,6 +6,7 @@ import shutil from django.conf import settings +from django.core.exceptions import ImproperlyConfigured from django.http import Http404, HttpRequest, HttpResponse from django.test import TestCase from django.utils.encoding import smart_str @@ -25,12 +26,22 @@ class TempFileTestCase(TestCase): def setUp(self): super(TempFileTestCase, self).setUp() self.TEMP_FILE_ROOT = mkdtemp() + self.setSendfileRoot(self.TEMP_FILE_ROOT) def tearDown(self): super(TempFileTestCase, self).tearDown() if os.path.exists(self.TEMP_FILE_ROOT): shutil.rmtree(self.TEMP_FILE_ROOT) + def setSendfileBackend(self, backend): + '''set the backend clearing the cache''' + settings.SENDFILE_BACKEND = backend + _get_sendfile.cache_clear() + + def setSendfileRoot(self, path): + '''set the backend clearing the cache''' + settings.SENDFILE_ROOT = path + def ensure_file(self, filename): path = os.path.join(self.TEMP_FILE_ROOT, filename) if not os.path.exists(path): @@ -43,12 +54,21 @@ class TestSendfile(TempFileTestCase): def setUp(self): super(TestSendfile, self).setUp() # set ourselves to be the sendfile backend - settings.SENDFILE_BACKEND = 'django_sendfile.tests' - _get_sendfile.cache_clear() + self.setSendfileBackend('django_sendfile.tests') def _get_readme(self): return self.ensure_file('testfile.txt') + def test_backend_is_none(self): + self.setSendfileBackend(None) + with self.assertRaises(ImproperlyConfigured): + real_sendfile(HttpRequest(), "notafile.txt") + + def test_root_is_none(self): + self.setSendfileRoot(None) + with self.assertRaises(ImproperlyConfigured): + real_sendfile(HttpRequest(), "notafile.txt") + def test_404(self): try: real_sendfile(HttpRequest(), 'fhdsjfhjk.txt') @@ -102,12 +122,33 @@ def test_attachment_filename_unicode(self): response['Content-Disposition']) +class TestSimpleSendfileBackend(TempFileTestCase): + + def setUp(self): + super().setUp() + self.setSendfileBackend('django_sendfile.backends.simple') + + def test_correct_file(self): + filepath = self.ensure_file('readme.txt') + response = real_sendfile(HttpRequest(), filepath) + self.assertTrue(response is not None) + + def test_containing_unicode(self): + filepath = self.ensure_file(u'péter_là_gueule.txt') + response = real_sendfile(HttpRequest(), filepath) + self.assertTrue(response is not None) + + def test_sensible_file_access_in_simplesendfile(self): + filepath = self.ensure_file('../../../etc/passwd') + with self.assertRaises(Http404): + real_sendfile(HttpRequest(), filepath) + + class TestXSendfileBackend(TempFileTestCase): def setUp(self): super(TestXSendfileBackend, self).setUp() - settings.SENDFILE_BACKEND = 'django_sendfile.backends.xsendfile' - _get_sendfile.cache_clear() + self.setSendfileBackend('django_sendfile.backends.xsendfile') def test_correct_file_in_xsendfile_header(self): filepath = self.ensure_file('readme.txt') @@ -126,21 +167,30 @@ class TestNginxBackend(TempFileTestCase): def setUp(self): super(TestNginxBackend, self).setUp() - settings.SENDFILE_BACKEND = 'django_sendfile.backends.nginx' - settings.SENDFILE_ROOT = self.TEMP_FILE_ROOT + self.setSendfileBackend('django_sendfile.backends.nginx') settings.SENDFILE_URL = '/private' - _get_sendfile.cache_clear() + + def test_sendfile_url_not_set(self): + settings.SENDFILE_URL = None + filepath = self.ensure_file('readme.txt') + response = real_sendfile(HttpRequest(), filepath) + self.assertTrue(response is not None) + self.assertEqual(response.content, b'') + self.assertEqual(os.path.join(self.TEMP_FILE_ROOT, 'readme.txt'), + response['X-Accel-Redirect']) def test_correct_url_in_xaccelredirect_header(self): filepath = self.ensure_file('readme.txt') response = real_sendfile(HttpRequest(), filepath) self.assertTrue(response is not None) + self.assertEqual(response.content, b'') self.assertEqual('/private/readme.txt', response['X-Accel-Redirect']) def test_xaccelredirect_header_containing_unicode(self): filepath = self.ensure_file(u'péter_là_gueule.txt') response = real_sendfile(HttpRequest(), filepath) self.assertTrue(response is not None) + self.assertEqual(response.content, b'') self.assertEqual('/private/péter_là_gueule.txt', unquote(response['X-Accel-Redirect'])) @@ -148,19 +198,28 @@ class TestModWsgiBackend(TempFileTestCase): def setUp(self): super(TestModWsgiBackend, self).setUp() - settings.SENDFILE_BACKEND = 'django_sendfile.backends.mod_wsgi' - settings.SENDFILE_ROOT = self.TEMP_FILE_ROOT + self.setSendfileBackend('django_sendfile.backends.mod_wsgi') settings.SENDFILE_URL = '/private' - _get_sendfile.cache_clear() + + def test_sendfile_url_not_set(self): + settings.SENDFILE_URL = None + filepath = self.ensure_file('readme.txt') + response = real_sendfile(HttpRequest(), filepath) + self.assertTrue(response is not None) + self.assertEqual(response.content, b'') + self.assertEqual(os.path.join(self.TEMP_FILE_ROOT, 'readme.txt'), + response['Location']) def test_correct_url_in_location_header(self): filepath = self.ensure_file('readme.txt') response = real_sendfile(HttpRequest(), filepath) self.assertTrue(response is not None) + self.assertEqual(response.content, b'') self.assertEqual('/private/readme.txt', response['Location']) def test_location_header_containing_unicode(self): filepath = self.ensure_file(u'péter_là_gueule.txt') response = real_sendfile(HttpRequest(), filepath) self.assertTrue(response is not None) + self.assertEqual(response.content, b'') self.assertEqual('/private/péter_là_gueule.txt', unquote(response['Location'])) diff --git a/django_sendfile/utils.py b/django_sendfile/utils.py index 25d117a..e5579f4 100644 --- a/django_sendfile/utils.py +++ b/django_sendfile/utils.py @@ -1,7 +1,9 @@ from functools import lru_cache from importlib import import_module from mimetypes import guess_type -import os.path +from pathlib import Path, PurePath +from urllib.parse import quote +import logging import unicodedata from django.conf import settings @@ -10,6 +12,8 @@ from django.utils.encoding import force_str from django.utils.http import urlquote +logger = logging.getLogger(__name__) + @lru_cache(maxsize=None) def _get_sendfile(): @@ -20,6 +24,45 @@ def _get_sendfile(): return module.sendfile +def _convert_file_to_url(path): + try: + url_root = PurePath(getattr(settings, "SENDFILE_URL", None)) + except TypeError: + return path + + path_root = PurePath(settings.SENDFILE_ROOT) + path_obj = PurePath(path) + + relpath = path_obj.relative_to(path_root) + # Python 3.5: Path.resolve() has no `strict` kwarg, so use pathmod from an + # already instantiated Path object + url = relpath._flavour.pathmod.normpath(str(url_root / relpath)) + + return quote(str(url)) + + +def _sanitize_path(filepath): + try: + path_root = Path(getattr(settings, 'SENDFILE_ROOT', None)) + except TypeError: + raise ImproperlyConfigured('You must specify a value for SENDFILE_ROOT') + + filepath_obj = Path(filepath) + + # get absolute path + # Python 3.5: Path.resolve() has no `strict` kwarg, so use pathmod from an + # already instantiated Path object + filepath_abs = Path(filepath_obj._flavour.pathmod.normpath(str(path_root / filepath_obj))) + + # if filepath_abs is not relative to path_root, relative_to throws an error + try: + filepath_abs.relative_to(path_root) + except ValueError: + raise Http404('{} wrt {} is impossible'.format(filepath_abs, path_root)) + + return filepath_abs + + def sendfile(request, filename, attachment=False, attachment_filename=None, mimetype=None, encoding=None): """ @@ -41,25 +84,28 @@ def sendfile(request, filename, attachment=False, attachment_filename=None, If neither ``mimetype`` or ``encoding`` are specified, then they will be guessed via the filename (using the standard Python mimetypes module) """ + filepath_obj = _sanitize_path(filename) + logger.debug('filename \'%s\' requested "\ + "-> filepath \'%s\' obtained', filename, filepath_obj) _sendfile = _get_sendfile() - if not os.path.exists(filename): - raise Http404('"%s" does not exist' % filename) + if not filepath_obj.exists(): + raise Http404('"%s" does not exist' % filepath_obj) - guessed_mimetype, guessed_encoding = guess_type(filename) + guessed_mimetype, guessed_encoding = guess_type(str(filepath_obj)) if mimetype is None: if guessed_mimetype: mimetype = guessed_mimetype else: mimetype = 'application/octet-stream' - response = _sendfile(request, filename, mimetype=mimetype) + response = _sendfile(request, filepath_obj, mimetype=mimetype) # Suggest to view (inline) or download (attachment) the file parts = ['attachment' if attachment else 'inline'] if attachment_filename is None: - attachment_filename = os.path.basename(filename) + attachment_filename = filepath_obj.name if attachment_filename: attachment_filename = force_str(attachment_filename) @@ -73,7 +119,7 @@ def sendfile(request, filename, attachment=False, attachment_filename=None, response['Content-Disposition'] = '; '.join(parts) - response['Content-length'] = os.path.getsize(filename) + response['Content-length'] = filepath_obj.stat().st_size response['Content-Type'] = mimetype if not encoding: diff --git a/docs/backends.rst b/docs/backends.rst index 9d6b0ac..7c77a94 100644 --- a/docs/backends.rst +++ b/docs/backends.rst @@ -43,10 +43,8 @@ embedded mode. It requires a bit more work to get it to do the same job as xsendfile though. However some may find it easier to setup, as they don't need to compile and install mod_xsendfile_. -Firstly there are two more Django settings: +Firstly there one more Django setting that needs to be given: -* ``SENDFILE_ROOT`` - this is a directoy where all files that will be used with - sendfile must be located * ``SENDFILE_URL`` - internal URL prefix for all files served via sendfile These settings are needed as this backend makes mod_wsgi_ send an internal @@ -93,10 +91,8 @@ Nginx backend :py:mod:`django_sendfile.backends.nginx` -As with the mod_wsgi backend you need to set two extra settings: +As with the mod_wsgi backend you need to set an extra settings: -* ``SENDFILE_ROOT`` - this is a directory where all files that will be used with - sendfile must be located * ``SENDFILE_URL`` - internal URL prefix for all files served via sendfile You then need to configure Nginx to only allow internal access to the files you @@ -141,12 +137,6 @@ configure mod_xsendfile_, but that should be as simple as: In your virtualhost file/conf file. -As with the mod_wsgi backend you need to set two extra settings: - -* ``SENDFILE_ROOT`` - this is a directory where all files that will be used with - sendfile must be located -* ``SENDFILE_URL`` - internal URL prefix for all files served via sendfile - .. _mod_xsendfile: https://tn123.org/mod_xsendfile/ .. _Apache: http://httpd.apache.org/ diff --git a/docs/getting-started.rst b/docs/getting-started.rst index 8bd3c5e..a9b5ec4 100644 --- a/docs/getting-started.rst +++ b/docs/getting-started.rst @@ -17,9 +17,14 @@ And then add ``django_sendfile`` to ``INSTALLED_APPS`` in your settings module. It is not strictly nessessary to have django_sendfile in ``INSTALLED_APPS``, but this may change in future. -You will also need to select a backend via ``SENDFILE_BACKEND`` in your -settings module. Additionally, you may need to set ``SENDFILE_URL`` and -``SENDFILE_ROOT``. See the :doc:`backends` documentation for more details. + +You will need to have the following set in your settings module: + +* ``SENDFILE_BACKEND`` - the dotted module notation of the backend you wish to use +* ``SENDFILE_ROOT`` - the directory you wish to serve files from + +Additionally, you may need to set ``SENDFILE_URL`` . See the :doc:`backends` +documentation for more details. Use In Views diff --git a/examples/protected_downloads/download/models.py b/examples/protected_downloads/download/models.py index 912e4d7..709fc4b 100644 --- a/examples/protected_downloads/download/models.py +++ b/examples/protected_downloads/download/models.py @@ -20,7 +20,7 @@ def __unicode__(self): return self.title def get_absolute_url(self): - return reverse('download', [self.pk], {}) + return reverse('download', kwargs={"download_id": self.pk}) class Meta: app_label = "download" diff --git a/examples/protected_downloads/templates/download/download_list.html b/examples/protected_downloads/download/templates/download/download_list.html similarity index 100% rename from examples/protected_downloads/templates/download/download_list.html rename to examples/protected_downloads/download/templates/download/download_list.html diff --git a/examples/protected_downloads/download/urls.py b/examples/protected_downloads/download/urls.py index 1af6826..2ce8a63 100644 --- a/examples/protected_downloads/download/urls.py +++ b/examples/protected_downloads/download/urls.py @@ -1,8 +1,9 @@ from django.conf import urls -from .views import download, download_list +from .views import direct_download, download, download_list urlpatterns = [ - urls.url(r'^$', download_list), urls.url(r'(?P\d+)/$', download, name='download'), + urls.url(r'^$', download_list), + urls.url(r'direct/(?P.*)$', direct_download, name='direct_download'), ] diff --git a/examples/protected_downloads/download/views.py b/examples/protected_downloads/download/views.py index a06f929..e3f671c 100644 --- a/examples/protected_downloads/download/views.py +++ b/examples/protected_downloads/download/views.py @@ -2,7 +2,6 @@ from django.db.models import Q from django.http import HttpResponseForbidden from django.shortcuts import get_object_or_404, render -from django.template import RequestContext from django_sendfile import sendfile @@ -25,10 +24,16 @@ def _auth_download(request, download): def download_list(request): downloads = Download.objects.all() - if request.user.is_authenticated(): + if request.user.is_authenticated: downloads = downloads.filter(Q(is_public=True) | Q(users=request.user)) else: downloads = downloads.filter(is_public=True) return render(request, 'download/download_list.html', - {'download_list': downloads}, - context_instance=RequestContext(request)) + {'download_list': downloads}) + + +def direct_download(request, filename=None): + if not filename: + filename = request.GET.get("filename") + + return sendfile(request, filename) diff --git a/examples/protected_downloads/settings.py b/examples/protected_downloads/settings.py index 147b4a2..29d4322 100644 --- a/examples/protected_downloads/settings.py +++ b/examples/protected_downloads/settings.py @@ -19,6 +19,21 @@ } } +LOGGING = { + 'version': 1, + 'disable_existing_loggers': False, + 'handlers': { + 'console': { + 'class': 'logging.StreamHandler', + }, + }, + 'loggers': { + 'django_sendfile': { + 'handlers': ['console'], + 'level': os.getenv('DJANGO_LOG_LEVEL', 'INFO'), + }, + }, +} # Local time zone for this installation. Choices can be found here: # http://en.wikipedia.org/wiki/List_of_tz_zones_by_name # although not all choices may be available on all operating systems. @@ -69,12 +84,10 @@ "django.contrib.auth.context_processors.auth", "django.contrib.messages.context_processors.messages", ], - 'loaders': [ - ( + 'loaders': ( 'django.template.loaders.filesystem.Loader', 'django.template.loaders.app_directories.Loader', - ), - ], + ), 'debug': DEBUG, }, }]