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 <[email protected]>

Author: moggers87

.gitignore

@@ -13,3 +13,4 @@
 .tox
 /examples/protected_downloads/htmlcov
 /examples/protected_downloads/.coverage
+/examples/protected_downloads/protected

django_sendfile/backends/_internalredirect.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):

django_sendfile/backends/mod_wsgi.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

django_sendfile/backends/nginx.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
 
 

django_sendfile/backends/simple.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,41 +167,59 @@ 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']))
 
 
 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']))

django_sendfile/tests.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:

django_sendfile/utils.py

@@ -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/