Address all review remarks

Author: codingjoe

README.md

@@ -29,10 +29,16 @@ license](https://img.shields.io/badge/license-MIT-blue.svg)](https://raw.githubu
 sequenceDiagram
     autonumber
     actor Browser
+    participant S3
+    participant Middleware
+    Browser->>Django: GET form view
+    activate Django
+    Django->>Browser: RESPONSE w/ presigned POST URL & signed middleware key
+    deactivate Django
     Browser->>S3: POST large file
     activate S3
     S3->>Browser: RESPONSE AWS S3 key
-    Browser->>Middleware: POST AWS S3 key
+    Browser->>Middleware: POST AWS S3 key (signed)
     activate Middleware
     Middleware->>S3: GET AWS S3 key
     S3->>Middleware: RESPONSE large file promise

SECURITY.md

@@ -0,0 +1,48 @@
+# Security Policy
+
+## Security Considerations
+
+The wake of CVE-2022-24840 revealed the importance to document security considerations.
+The following attack vectors have been considered during development. Should there be
+a possible vector or consideration missing, please contact the maintainers, as described
+below.
+
+We use [pre-signed POST URLs](s3-pre-signed-url) to upload files to AWS S3.
+[Django's internal signer](django-signing) is used to sign the upload path and validate
+it before fetching files from S3.
+
+Please note, that Django's signer uses the `SECRET_KEY`, rotating the key will void all
+signatures.
+
+[s3-pre-signed-url]: https://boto3.amazonaws.com/v1/documentation/api/latest/guide/s3-presigned-urls.html
+[django-signing]: https://docs.djangoproject.com/en/stable/topics/signing/
+
+### Upload of malicious files
+
+AWS S3 supports MIME type detection and content-type enforcement.
+You can limit the upload of malicious files via the MIME type [accept][accept].
+However, this is not a security measure, and you should always validate files before
+processing them.
+
+[accept]: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/accept
+
+### Request file injection
+
+Tho files can always be included in a request, CVE-2022-24840 revealed that we need
+to consider people injecting any files that reside on your S3 bucket. However, we do
+presign the upload location and validate it before fetching files from S3.
+
+### Path traversal & timing attacks
+
+We fetch files from your S3 bucket. This behavior could be used to brute force valid
+file names. We mitigate this by signing the allowed upload path and validating it.
+The upload path is unique for each file input and request. Therefore, an attacker can
+not escape and access any files but the one uploaded by the attacker.
+
+## Reporting a Vulnerability
+
+NEVER open an issue or discussion to report a vulnerability. Please contact one of the
+maintainers of the project either via email or Telegram:
+
+* Email: [johannes@maron.family](mailto:johannes@maron.family)
+* Telegram: [@codingjoe](https://t.me/codingjoe)

s3file/forms.py

@@ -4,10 +4,10 @@
 import uuid
 
 from django.conf import settings
-from django.core import signing
 from django.utils.functional import cached_property
 from storages.utils import safe_join
 
+from s3file.middleware import S3FileMiddleware
 from s3file.storages import storage
 
 logger = logging.getLogger("s3file")
@@ -50,11 +50,10 @@ def build_attrs(self, *args, **kwargs):
             "data-fields-%s" % key: value for key, value in response["fields"].items()
         }
         defaults["data-url"] = response["url"]
-        signer = signing.Signer(
-            salt=f"{S3FileInputMixin.__module__}.{S3FileInputMixin.__name__}"
+        # we sign upload location, and will only accept files within the same folder
+        defaults["data-s3f-signature"] = S3FileMiddleware.sign_s3_key_prefix(
+            self.upload_folder
         )
-        print(self.upload_folder)
-        defaults["data-s3f-signature"] = signer.signature(self.upload_folder)
         defaults.update(attrs)
 
         try:

s3file/middleware.py

@@ -6,7 +6,6 @@
 from django.utils.crypto import constant_time_compare
 
 from . import views
-from .forms import S3FileInputMixin
 from .storages import local_dev, storage
 
 logger = logging.getLogger("s3file")
@@ -38,21 +37,18 @@ def __call__(self, request):
 
         return self.get_response(request)
 
-    @staticmethod
-    def get_files_from_storage(paths, signature):
+    @classmethod
+    def get_files_from_storage(cls, paths, signature):
         """Return S3 file where the name does not include the path."""
         try:
             location = storage.aws_location
         except AttributeError:
             location = storage.location
-        signer = signing.Signer(
-            salt=f"{S3FileInputMixin.__module__}.{S3FileInputMixin.__name__}"
-        )
         for path in paths:
             path = pathlib.PurePosixPath(path)
-            print(path)
-            print(signer.signature(path.parent), signature)
-            if not constant_time_compare(signer.signature(path.parent), signature):
+            if not constant_time_compare(
+                cls.sign_s3_key_prefix(path.parent), signature
+            ):
                 raise PermissionDenied("Illegal signature!")
             try:
                 relative_path = str(path.relative_to(location))
@@ -67,3 +63,12 @@ def get_files_from_storage(paths, signature):
                 yield f
             except (OSError, ValueError):
                 logger.exception("File not found: %s", path)
+
+    @classmethod
+    def sign_s3_key_prefix(cls, path):
+        """
+        Signature to validate the S3 keys passed the middleware before fetching files.
+
+        Return a base64-encoded HMAC-SHA256 of the upload folder aka the S3 key-prefix.
+        """
+        return signing.Signer(salt="s3file.middleware.S3FileMiddleware").signature(path)

s3file/views.py

@@ -2,7 +2,6 @@
 import hashlib
 import hmac
 import logging
-from pathlib import Path
 
 from django import http
 from django.conf import settings

tests/conftest.py

@@ -25,7 +25,7 @@ def driver():
 
 @pytest.fixture
 def freeze_upload_folder(monkeypatch):
-    """Freeze datetime and UUID."""
+    """Freeze the upload folder which by default contains a random UUID v4."""
     upload_folder = Path(storage.aws_location) / "tmp" / "s3file"
     monkeypatch.setattr(
         "s3file.forms.S3FileInputMixin.upload_folder",

tests/test_forms.py

@@ -38,7 +38,7 @@ def test_value_from_datadict(self, freeze_upload_folder, client, upload_file):
             reverse("upload"),
             {
                 "file": f"custom/location/{uploaded_file}",
-                "file-s3f-signature": "m94qBxBsnMIuIICiY133kX18KkllSPMVbhGAdAwNn1A",
+                "file-s3f-signature": "FxQXie3wnVnCUFqGzFZ8DCFKAXFA3bnQ8tE96U11o80",
                 "s3file": "file",
             },
         )
@@ -88,7 +88,7 @@ def test_build_attr(self, freeze_upload_folder):
         }
         assert (
             ClearableFileInput().build_attrs({})["data-s3f-signature"]
-            == "tFV9nGZlq9WX1I5Sotit18z1f4C_3lPnj33_zo4LZRc"
+            == "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI"
         )
         assert ClearableFileInput().build_attrs({})["class"] == "s3file"
         assert (

tests/test_middleware.py

@@ -1,7 +1,7 @@
 import os
 
 import pytest
-from django.core.exceptions import PermissionDenied, SuspiciousFileOperation
+from django.core.exceptions import PermissionDenied
 from django.core.files.base import ContentFile
 from django.core.files.uploadedfile import SimpleUploadedFile
 
@@ -17,7 +17,7 @@ def test_get_files_from_storage(self, freeze_upload_folder):
         )
         files = S3FileMiddleware.get_files_from_storage(
             [os.path.join(storage.aws_location, name)],
-            "tFV9nGZlq9WX1I5Sotit18z1f4C_3lPnj33_zo4LZRc",
+            "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI",
         )
         file = next(files)
         assert file.read() == content
@@ -35,7 +35,7 @@ def test_process_request(self, freeze_upload_folder, rf):
             data={
                 "file": "custom/location/tmp/s3file/s3_file.txt",
                 "s3file": "file",
-                "file-s3f-signature": "tFV9nGZlq9WX1I5Sotit18z1f4C_3lPnj33_zo4LZRc",
+                "file-s3f-signature": "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI",
             },
         )
         S3FileMiddleware(lambda x: None)(request)
@@ -49,7 +49,7 @@ def test_process_request__location_escape(self, freeze_upload_folder, rf):
             data={
                 "file": "custom/location/secrets/passwords.txt",
                 "s3file": "file",
-                "file-s3f-signature": "tFV9nGZlq9WX1I5Sotit18z1f4C_3lPnj33_zo4LZRc",
+                "file-s3f-signature": "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI",
             },
         )
         with pytest.raises(PermissionDenied) as e:
@@ -66,8 +66,8 @@ def test_process_request__multiple_files(self, freeze_upload_folder, rf):
                     "custom/location/tmp/s3file/s3_file.txt",
                     "custom/location/tmp/s3file/s3_other_file.txt",
                 ],
-                "file-s3f-signature": "tFV9nGZlq9WX1I5Sotit18z1f4C_3lPnj33_zo4LZRc",
-                "other_file-s3f-signature": "tFV9nGZlq9WX1I5Sotit18z1f4C_3lPnj33_zo4LZRc",
+                "file-s3f-signature": "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI",
+                "other_file-s3f-signature": "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI",
                 "s3file": ["file", "other_file"],
             },
         )
@@ -88,9 +88,9 @@ def test_process_request__no_location(self, freeze_upload_folder, rf, settings):
         request = rf.post(
             "/",
             data={
-                "file": f"tmp/s3file/s3_file.txt",
+                "file": "tmp/s3file/s3_file.txt",
                 "s3file": "file",
-                "file-s3f-signature": "scjzm3N8njBQIVSGEhOchtM0TkGyb2U6OXGLVlRUZhY",
+                "file-s3f-signature": "pJYaM4x7RzLDLVXWuphK2dMqqc0oLr_jZFasfGU7BhU",
             },
         )
         S3FileMiddleware(lambda x: None)(request)
@@ -103,7 +103,7 @@ def test_process_request__no_file(self, freeze_upload_folder, rf, caplog):
             data={
                 "file": "custom/location/tmp/s3file/does_not_exist.txt",
                 "s3file": "file",
-                "file-s3f-signature": "tFV9nGZlq9WX1I5Sotit18z1f4C_3lPnj33_zo4LZRc",
+                "file-s3f-signature": "VRIPlI1LCjUh1EtplrgxQrG8gSAaIwT48mMRlwaCytI",
             },
         )
         S3FileMiddleware(lambda x: None)(request)
@@ -119,6 +119,7 @@ def test_process_request__no_signature(self, rf, caplog):
         )
         with pytest.raises(PermissionDenied) as e:
             S3FileMiddleware(lambda x: None)(request)
+        assert "No signature provided." in str(e.value)
 
     def test_process_request__wrong_signature(self, rf, caplog):
         request = rf.post(
@@ -131,3 +132,10 @@ def test_process_request__wrong_signature(self, rf, caplog):
         )
         with pytest.raises(PermissionDenied) as e:
             S3FileMiddleware(lambda x: None)(request)
+        assert "Illegal signature!" in str(e.value)
+
+    def test_sign_s3_key_prefix(self, rf):
+        assert (
+            S3FileMiddleware.sign_s3_key_prefix("test/test")
+            == "a8KINhIf1IpSD5sgdXE4wEQodZorq_8CmwkqZ5V6nr4"
+        )