fix: Restrict upload of binary or unknown file types by default (#1507)

* Fix #1377

* Bump to 3.2.0

* fix: Deny upload of binary or unknown file types

* Update tests

* Update validation.rst

* Update docs/validation.rst

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

* Add test that binary uploads fail by default

* Add migration info

---------

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

CHANGELOG.rst

@@ -754,7 +754,7 @@ CHANGELOG
 
 
 0.5.4a1
-=======
+========
 
 * Adds description field.
 

README.rst

@@ -49,6 +49,26 @@ Documentation
 Please head over to the separate `documentation <https://django-filer.readthedocs.io/en/latest/index.html>`_
 for all the details on how to install, configure and use django-filer.
 
+Upgrading
+=========
+
+Version 3.3
+-----------
+
+django-filer version 3 contains a change in security policy for file uploads.
+**By default, binary file or files of unknown type are not allowed to be uploaded.**
+To allow upload of binary files in your project, add
+
+.. code-block:: python
+
+    FILER_REMOVE_FILE_VALIDATORS = [
+        "application/octet-stream",
+    ]
+
+to your project's settings. Be aware that binary files always are a security risk.
+See the documentation for more information on how to configure file upload validators,
+e.g., running files through a virus checker.
+
 
 .. |pypi| image:: https://badge.fury.io/py/django-filer.svg
     :target: http://badge.fury.io/py/django-filer

docs/validation.rst

@@ -54,9 +54,9 @@ files with the mime type ``image/svg+xml``. Those files are dangerous since
 they are executed by a browser without any warnings.
 
 Validation hooks do not restrict the upload of other executable files
-(like ``*.exe`` or shell scripts). Those are not automatically executed
+(like ``*.exe`` or shell scripts). **Those are not automatically executed
 by the browser but still present a point of attack, if a user saves them
-to disk and executes them locally.
+to disk and executes them locally.**
 
 You can release validation restrictions by setting
 ``FILER_REMOVE_FILE_VALIDATORS`` to a list of mime types to be removed from
@@ -111,7 +111,7 @@ This just rejects any file for upload. By default this happens for HTML files
 
 This validator rejects any SVG file that contains the bytes ``<script`` or
 ``javascript:``. This probably is a too strict criteria, since those bytes
-might be part of a legitimate say string. The above code is a simplification
+might be part of a legitimate string. The above code is a simplification
 the actual code also checks for occurrences of event attribute like
 ``onclick="..."``.
 
@@ -144,10 +144,11 @@ a malicious file unknowingly.
     FILER_REMOVE_FILE_VALIDATORS = [
         "text/html",
         "image/svg+xml",
+        "application/octet-stream",
     ]
 
-No HTML upload and restricted SVG upload
-........................................
+No HTML upload and restricted SVG upload, no binary or unknown file upload
+...........................................................................
 
 This is the default setting. It will deny any SVG file that might contain
 Javascript. It is prone to false positives (i.e. files being rejected that
@@ -176,6 +177,8 @@ in the user's browser.
         "image/svg+xml": ["filer.validation.deny"],
     }
 
+(Still not binary or unknown file upload)
+
 Experimental SVG sanitization
 .............................
 
@@ -259,3 +262,38 @@ You can use it to distinguish validation for certain user groups if needed.
 
 If you distinguish validation by the mime type, remember to register the
 validator function for all relevant mime types.
+
+
+Checking uploads for viruses using ClamAV
+-----------------------------------------
+
+If you have ClamAV installed and use `django-clamd <https://github.com/vstoykov/django-clamd>`_
+you can add a validator that checks for viruses in uploaded files.
+
+.. code-block:: python
+
+    FILER_REMOVE_FILE_VALIDATORS = ["application/octet-stream"]
+    FILER_ADD_FILE_VALIDATORS = {
+        "application/octet-stream": ["my_validator_app.validators.validate_octet_stream"],
+    }
+
+
+.. code-block:: python
+
+    def validate_octet_stream(file_name: str, file: typing.IO, owner: User, mime_type: str) -> None:
+        """Octet streams are binary files without a specific mime type. They are run through
+        a virus check."""
+        try:
+            from django_clamd.validators import validate_file_infection
+
+            validate_file_infection(file)
+        except (ModuleNotFoundError, ImportError):
+            raise FileValidationError(
+                _('File "{file_name}": Virus check for binary/unknown file not available').format(file_name=file_name)
+            )
+
+.. note::
+
+    Virus-checked files still might contain executable code. While the code is not
+    executed by the browser, a user might still download the file and execute it
+    manually.

filer/settings.py

@@ -292,6 +292,7 @@ def update_server_settings(settings, defaults, s, t):
 FILE_VALIDATORS = {
     "text/html": ["filer.validation.deny_html"],
     "image/svg+xml": ["filer.validation.validate_svg"],
+    "application/octet-stream": ["filer.validation.deny"],
 }
 
 remove_mime_types = getattr(settings, "FILER_REMOVE_FILE_VALIDATORS", [])

tests/test_admin.py

@@ -3,6 +3,7 @@
 
 import django
 import django.core.files
+from django.apps import apps
 from django.conf import settings
 from django.contrib import admin
 from django.contrib.admin import helpers
@@ -484,6 +485,10 @@ def test_filer_upload_file_no_folder(self, extra_headers={}):
             self.assertEqual(stored_image.mime_type, 'image/jpeg')
 
     def test_filer_upload_binary_data(self, extra_headers={}):
+        config = apps.get_app_config("filer")
+
+        validators = config.FILE_VALIDATORS  # Remember the validators
+        config.FILE_VALIDATORS = {}  # Remove deny for application/octet-stream
         self.assertEqual(File.objects.count(), 0)
         with open(self.binary_filename, 'rb') as fh:
             file_obj = django.core.files.File(fh)
@@ -494,12 +499,29 @@ def test_filer_upload_binary_data(self, extra_headers={}):
                 'jsessionid': self.client.session.session_key
             }
             self.client.post(url, post_data, **extra_headers)
+            config.FILE_VALIDATORS = validators  # Reset validators
+
             self.assertEqual(Image.objects.count(), 0)
             self.assertEqual(File.objects.count(), 1)
             stored_file = File.objects.first()
             self.assertEqual(stored_file.original_filename, self.binary_name)
             self.assertEqual(stored_file.mime_type, 'application/octet-stream')
 
+    def test_filer_upload_binary_data_fails_by_default(self, extra_headers={}):
+        self.assertEqual(File.objects.count(), 0)
+        with open(self.binary_filename, 'rb') as fh:
+            file_obj = django.core.files.File(fh)
+            url = reverse('admin:filer-ajax_upload')
+            post_data = {
+                'Filename': self.binary_name,
+                'Filedata': file_obj,
+                'jsessionid': self.client.session.session_key
+            }
+            self.client.post(url, post_data, **extra_headers)
+
+            self.assertEqual(Image.objects.count(), 0)
+            self.assertEqual(File.objects.count(), 0)
+
     def test_filer_ajax_upload_file(self):
         self.assertEqual(Image.objects.count(), 0)
         folder = Folder.objects.create(name='foo')